On 02/05/2025 14:52, Devarsh Thakkar wrote:
Hi,
<snip>
It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
accessing am65x common region register space which has vid on idx0 which
we want to skip for am62l.
For dispc_plane_enable(), the caller uses
0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...
Yes above is correct, and I think that's how it is supposed to be.
No it's not. Both functions have "hw_plane" parameter, yet they require
a different value to be used even when referring to the same plane.
With a quick look at the code, changing the callers to pass the "old
style" hw_plane as the parameter to those irq functions, and the
functions internally get the hw_id, would solve most of the problems.
I don't follow above, hw_plane has 0 so it should not be used for
programming irq related functions which expect idx 1 as explained above.
We have various functions in tidss_dispc.c that have hw_plane as a
parameter. But the caller is supposed to know that for some functions
hw_plane is a plane index from 0, and for some it's the hw_id from
vid_info[].
There's still dispc_k3_set_irqenable() which manages 'main_disable' and
needs the hw_id, but maybe that's fine, even if a bit confusing.
I still feel there is no inherent bug here, but let me know if you want
me to put some debug prints or get register dump so that we can double
confirm.
I'm not saying there's a bug. I'm saying it's bad code and will cause
confusion and bugs in the future.
Ok I see what you mean to say.....although functionally it is working
fine but from readability point of view it is confusing since both
functions use same argument name i.e hw_plane in two different contexts.
In that case, I would propose to use hw_id as arg name for all
dispc_k3_vid* functions, will that be okay ?
I'd prefer to have all the dispc functions take the same kind of index.
Tomi
