| Message ID | 20201012135517.19468-3-frank@allwinnertech.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce devm_pm_opp_set_* API | expand |
On 12-10-20, 21:55, Frank Lee wrote: > From: Yangtao Li <tiny.windzz@gmail.com> > > Add devres wrapper for dev_pm_opp_set_prop_name() to simplify driver > code. > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > --- > drivers/opp/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/linux/pm_opp.h | 6 ++++++ > 2 files changed, 45 insertions(+) On a second thought I am looking at dropping this one as you haven't added any users yet and I am afraid it will stay unused.
On Wed, Oct 28, 2020 at 6:29 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 12-10-20, 21:55, Frank Lee wrote: > > From: Yangtao Li <tiny.windzz@gmail.com> > > > > Add devres wrapper for dev_pm_opp_set_prop_name() to simplify driver > > code. > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > > --- > > drivers/opp/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > > include/linux/pm_opp.h | 6 ++++++ > > 2 files changed, 45 insertions(+) > > On a second thought I am looking at dropping this one as you haven't > added any users yet and I am afraid it will stay unused. Now it looks like that dev_pm_opp_set_prop_name() is used relatively less. Maybe we can wait until a caller, and then pick up the patch. MBR, Yangtao
On 28-10-20, 19:02, Frank Lee wrote: > On Wed, Oct 28, 2020 at 6:29 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 12-10-20, 21:55, Frank Lee wrote: > > > From: Yangtao Li <tiny.windzz@gmail.com> > > > > > > Add devres wrapper for dev_pm_opp_set_prop_name() to simplify driver > > > code. > > > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > > > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > > > --- > > > drivers/opp/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > > > include/linux/pm_opp.h | 6 ++++++ > > > 2 files changed, 45 insertions(+) > > > > On a second thought I am looking at dropping this one as you haven't > > added any users yet and I am afraid it will stay unused. > > Now it looks like that dev_pm_opp_set_prop_name() is used relatively less. > Maybe we can wait until a caller, and then pick up the patch. I am even wondering if we should be adding any of the devm_* helpers for now to be honest. Even for the other one we have only one user. Them major user of the OPP core is the CPU subsystem and it is never going to use these devm_* helpers as the CPU device doesn't get bound to a driver, it is rather a fake platform device which gets the cpufreq drivers probed. So the only users of these devm_* helpers is going to be non-CPU devices. Considering that we have only one user right now, it may be better to just fix it instead of adding any of the devm_* helpers.
On Wed, Oct 28, 2020 at 10:46 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 28-10-20, 19:02, Frank Lee wrote: > > On Wed, Oct 28, 2020 at 6:29 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 12-10-20, 21:55, Frank Lee wrote: > > > > From: Yangtao Li <tiny.windzz@gmail.com> > > > > > > > > Add devres wrapper for dev_pm_opp_set_prop_name() to simplify driver > > > > code. > > > > > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> > > > > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > > > > --- > > > > drivers/opp/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > > > > include/linux/pm_opp.h | 6 ++++++ > > > > 2 files changed, 45 insertions(+) > > > > > > On a second thought I am looking at dropping this one as you haven't > > > added any users yet and I am afraid it will stay unused. > > > > Now it looks like that dev_pm_opp_set_prop_name() is used relatively less. > > Maybe we can wait until a caller, and then pick up the patch. > > I am even wondering if we should be adding any of the devm_* helpers > for now to be honest. Even for the other one we have only one user. > Them major user of the OPP core is the CPU subsystem and it is never > going to use these devm_* helpers as the CPU device doesn't get bound > to a driver, it is rather a fake platform device which gets the > cpufreq drivers probed. So the only users of these devm_* helpers is > going to be non-CPU devices. Considering that we have only one user > right now, it may be better to just fix it instead of adding any of > the devm_* helpers. GPU is also a relatively large number of opp consumers. Most of the time, the dev_pm_opp_set_* functions will only be set once. If don't need the driver to dynamically manage and release the opp, it may be OK？ Yangtao
On 30-10-20, 19:19, Frank Lee wrote: > GPU is also a relatively large number of opp consumers. I was talking about the number of files or locations from which this routine (the devm_* variant) is going to get called. And it is one right now. And I don't see if any of the other callers are going to use it for now. > Most of the time, the dev_pm_opp_set_* functions will only be set once. Right. > If don't need the driver to dynamically manage and release the opp, it > may be OK？ Every call to dev_pm_opp_set_supported_hw() increases the ref count of the OPP table and if it isn't balanced with a call to dev_pm_opp_put_supported_hw(), then the OPP table will never get freed. So if the driver is a module and ends up creating an OPP table every time, then this will lead to leakage. The best way to fix this is by calling dev_pm_opp_put_supported_hw() from the right place and then we are good.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index b38852dde578..3263fa8302c9 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1721,6 +1721,45 @@ void dev_pm_opp_put_prop_name(struct opp_table *opp_table) } EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name); +static void devm_pm_opp_put_prop_name(struct device *dev, void *res) +{ + dev_pm_opp_put_prop_name(*(struct opp_table **)res); +} + +/** + * devm_pm_opp_set_prop_name() - Set prop-extn name + * @dev: Device for which the prop-name has to be set. + * @name: name to postfix to properties. + * + * This is required only for the V2 bindings, and it enables a platform to + * specify the extn to be used for certain property names. The properties to + * which the extension will apply are opp-microvolt and opp-microamp. OPP core + * should postfix the property name with -<name> while looking for them. + * + * The opp_table structure will be freed after the device is destroyed. + */ +struct opp_table *devm_pm_opp_set_prop_name(struct device *dev, + const char *name) +{ + struct opp_table **ptr, *opp_table; + + ptr = devres_alloc(devm_pm_opp_put_prop_name, + sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + opp_table = dev_pm_opp_set_prop_name(dev, name); + if (!IS_ERR(opp_table)) { + *ptr = opp_table; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return opp_table; +} +EXPORT_SYMBOL(devm_pm_opp_set_prop_name); + static int _allocate_set_opp_data(struct opp_table *opp_table) { struct dev_pm_set_opp_data *data; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 901920e29c54..1708485200dd 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -143,6 +143,7 @@ void dev_pm_opp_put_supported_hw(struct opp_table *opp_table); struct opp_table *devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count); struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name); void dev_pm_opp_put_prop_name(struct opp_table *opp_table); +struct opp_table *devm_pm_opp_set_prop_name(struct device *dev, const char *name); struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); void dev_pm_opp_put_regulators(struct opp_table *opp_table); struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name); @@ -321,6 +322,11 @@ static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, con static inline void dev_pm_opp_put_prop_name(struct opp_table *opp_table) {} +static inline struct opp_table *devm_pm_opp_set_prop_name(struct device *dev, const char *name) +{ + return ERR_PTR(-ENOTSUPP); +} + static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) { return ERR_PTR(-ENOTSUPP);