Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 27/05/2023 07:02, Yassine Oudjana wrote:
> > Konrad pointed this out.
> >
> > Are you 100% sure you want to do this. We already have a way to count 
> > the # of power-domains in camss_configure_pd().
> >
> > Your series is now adding a dependency on power-domain-names.
> >
> > Is there a good reason to add that dependency ? If not, then lets just 
> > take the code from camss_configure_pd() and make it so that it can be 
> > used/reused here.
>
> Is there a good reason not to?I found that using the existing 
> index-based method would unnecessarily complicate things since an extra 
> layer of checks would be needed to differentiate between MSM8996 and 
> TITAN SoCs, since those have the TITAN GDSC at the same index where the 
> CAMSS GDSC is now added for MSM8996. The same checks will also have to 
> be repeated in error paths and during cleanup.
>
> I guessed the only reason we were still using this method for the 
> existing PDs was to remain compatible with old DT as Konrad mentioned, 
> and since this CAMSS PD is only added now, I thought it'd be a good 
> opportunity to introduce power-domain-names and simplify things a bit.

I think actually I agree with you but, I don't think you've gone far 
enough with this patch.

Now that I look at this code a bit more, it looks like we need to place 
the TITAN/CAMSS GDSC last in the list of power-domains or the magic 
indices won't work. So my suggestion to you to place the CAMSS_GDSC in 
the power-domain list wouldn't work, unless it was the last entry,..

Having magic indices doesn't make much sense to me. Aside from anything 
else we don't document or require that indexing behavior in our 
Documentation.

In fact, I'm wondering what is the use case of a vfe_lite on its own - 
without the TITAN_TOP GDSC switched on ? I'm looking at the block 
diagram of the clocks for the sm8250 the IFE_LITE is buried well inside 
of a series of other components..

The reverse OTOH holds. Full fat VFE can be collapsed individually, 
which is why they have their own GDSCs...

OK, we should get away from magic indices ASAP.

This is a good find, thank you for bringing it up.

Could you take a named pointer for the CAMSS/TITAN instead of an index ?

camss->genpd_camss_top * =
camss->genpd_vfe[] =

These have a very obvious meaning. We can read a top-level struct camss 
{} and immediately understand what is meant, whereas index = 0 doesn't 
mean anything and isn't obvious from the code anyway.

1. You're right we should introduce some kind of naming to
   break the bonds of magic indices.

   So lets do as you suggest and name the power-domains.

   However we should refactor the code to drop magic indices.

2. If and only if named power-domains are absent, fall back on
   legacy indexing. In this case we will assume legacy indexing
   assigns to our new named pointers.

3. New CAMSS dts will need to have named power-domains as a result.

---
bod