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
