lkml.org 
[lkml]   [2018]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state()
On 12 October 2018 at 20:37, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> 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@linaro.org>
>> ---
>> 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.

\
 
 \ /
  Last update: 2018-10-12 17:41    [W:0.066 / U:3.680 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site