On 12 October 2018 at 20:37, Ulf Hansson <[email protected]> wrote: > On 12 October 2018 at 13:11, Viresh Kumar <[email protected]> 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 <[email protected]> >> --- >> 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.
