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
