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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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