Re: [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state()

[Viresh Kumar]

On 12 October 2018 at 20:37, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>> The OPP core currently stores the performance state in the consumer
>> device's OPP table, but that is going to change going forward and
>> performance state will rather be set directly in the genpd's OPP table.
>>
>> For that we need to get the performance state for genpd's device
>> structure instead of the consumer device's structure. Add a new helper
>> to do that.
>
> I guess what puzzles me a bit here is that we are using a struct
> device, while we actually should be talking about an OPP cookie
> instead, right?

The OPP cookie wouldn't get us to the platform specific conversion
handler, for that we need something from the genpd itself and so its
structure.

> So the "genpd's device structure" here is not the same as the virtual
> devices created by genpd to support multiple PM domains, right? Or is
> it?

You already found that I believe, it is genpd->dev.

>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> ---
>>  drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 ++++++++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 4b5714199490..2c82194d2a30 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -2508,6 +2508,45 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>>  }
>>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>>
>> +/**
>> + * genpd_opp_to_performance_state- Gets performance state of the genpd from its OPP node.
>
> Please rename to:
>
> pm_genpd_opp_to_perfromance_state().

Ok.

>> + *
>> + * @genpd_dev: Genpd's device for which the performance-state needs to be found.
>
> Maybe "genpd_dev" is the correct name to use here, as I understand
> it's actually the device representing the genpd. However, in other
> patches in this series you are also using "genpd_dev", while those
> instead corresponds to the virtual created devices by genpd.

Naming is a mess because I tried to follow the names you followed in
your multiple domain support. You used genpd_dev for the virtual device :)

> I would appreciate if we could make that more clear in the code.
>
> Maybe distinguish them as:
>
> genpd_dev
> genpd_virt_dev
> or just:
>
> dev
> virt_dev

Maybe, perhaps we should change domain.c with same naming for the internal
coding handling multiple domains as well ? I will send the patch for
that if you agree.

>> + * @opp: struct dev_pm_opp of the OPP for which we need to find performance
>> + *     state.
>> + *
>> + * Returns performance state encoded in the OPP of the genpd. This calls
>> + * platform specific genpd->opp_to_performance_state() callback to translate
>> + * power domain OPP to performance state.
>> + *
>> + * Returns performance state on success and 0 on failure.
>> + */
>> +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
>> +                                           struct dev_pm_opp *opp)
>> +{
>> +       struct generic_pm_domain *genpd = NULL, *temp;
>> +       int state;
>> +
>> +       lockdep_assert_held(&gpd_list_lock);
>
> What's this?

Don't we need to protect with a lock while traversing the below list?
Above is just a check to make sure lock is taken.

>> +
>> +       list_for_each_entry(temp, &gpd_list, gpd_list_node) {
>> +               if (&temp->dev == genpd_dev) {
>> +                       genpd = temp;
>> +                       break;
>> +               }
>> +       }
>
> I think we can do better than this.

I really want to :)

> We really don't want to walk the list of genpds while doing this. The
> caller should already know (if not now, we should fix it) that the
> struct device is used to represent a genpd.

Caller knows that genpd_dev here is genpd->dev really. But it doesn't
have pointer of the genpd itself.

> In other words, I am thinking using a container_of() or a finding a
> function pointer through the struct device, in any case, it would be
> better.

I am stupid. Container-of will work just fine I belive.