Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar <devarsht@ti.com>]

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 ?

Regards
Devarsh