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? 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? > > 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(). > + * > + * @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. 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 > + * @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? > + > + 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. 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. 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. If it all sounds fuzzy, let me think a bit about it and come back with a more clear suggestion. > + > + if (unlikely(!genpd || !genpd->opp_to_performance_state)) > + return 0; > + > + genpd_lock(genpd); .... so in the end, this is the only lock that should be needed. > + state = genpd->opp_to_performance_state(genpd, opp); > + genpd_unlock(genpd); > + > + return state; > +} > +EXPORT_SYMBOL_GPL(genpd_opp_to_performance_state); > + > /** > * of_genpd_opp_to_performance_state- Gets performance state of device's > * power domain corresponding to a DT node's "required-opps" property. > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 776c546d581a..e03f300f7468 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -233,6 +233,8 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent, > struct generic_pm_domain *of_genpd_remove_last(struct device_node *np); > int of_genpd_parse_idle_states(struct device_node *dn, > struct genpd_power_state **states, int *n); > +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev, > + struct dev_pm_opp *opp); > unsigned int of_genpd_opp_to_performance_state(struct device *dev, > struct device_node *np); > > @@ -274,6 +276,12 @@ static inline int of_genpd_parse_idle_states(struct > device_node *dn, > return -ENODEV; > } > > +static inline unsigned int > +genpd_opp_to_performance_state(struct device *genpd_dev, struct dev_pm_opp > *opp) > +{ > + return 0; > +} > + > static inline unsigned int > of_genpd_opp_to_performance_state(struct device *dev, > struct device_node *np) > -- > 2.18.0.rc1.242.g61856ae69a2c > Besides the comments above, I am all in for the approach as such. Kind regards Uffe
