Hi, this patchset introduce devm_pm_opp_set_prop_name() and devm_pm_opp_set_supported_hw().
Yangtao Li (3): opp: Add devres wrapper for dev_pm_opp_set_supported_hw opp: Add devres wrapper for dev_pm_opp_set_prop_name drm/msm: Convert to devm_pm_opp_set_supported_hw
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- drivers/opp/core.c | 80 +++++++++++++++++++++++++++ include/linux/pm_opp.h | 14 +++++ 3 files changed, 95 insertions(+), 1 deletion(-)
From: Yangtao Li tiny.windzz@gmail.com
Add devres wrapper for dev_pm_opp_set_supported_hw() 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 | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 ++++++++ 2 files changed, 49 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 3ca7543142bf..b38852dde578 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1625,6 +1625,47 @@ void dev_pm_opp_put_supported_hw(struct opp_table *opp_table) } EXPORT_SYMBOL_GPL(dev_pm_opp_put_supported_hw);
+static void devm_pm_opp_put_supported_hw(struct device *dev, void *res) +{ + dev_pm_opp_put_supported_hw(*(struct opp_table **)res); +} + +/** + * devm_pm_opp_set_supported_hw() - Set supported platforms + * @dev: Device for which supported-hw has to be set. + * @versions: Array of hierarchy of versions to match. + * @count: Number of elements in the array. + * + * This is required only for the V2 bindings, and it enables a platform to + * specify the hierarchy of versions it supports. OPP layer will then enable + * OPPs, which are available for those versions, based on its 'opp-supported-hw' + * property. + * + * The opp_table structure will be freed after the device is destroyed. + */ +struct opp_table *devm_pm_opp_set_supported_hw(struct device *dev, + const u32 *versions, + unsigned int count) +{ + struct opp_table **ptr, *opp_table; + + ptr = devres_alloc(devm_pm_opp_put_supported_hw, + sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + opp_table = dev_pm_opp_set_supported_hw(dev, versions, count); + 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_supported_hw); + /** * dev_pm_opp_set_prop_name() - Set prop-extn name * @dev: Device for which the prop-name has to be set. diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index dbb484524f82..901920e29c54 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -140,6 +140,7 @@ int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb
struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count); 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 *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); @@ -298,6 +299,13 @@ static inline struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
static inline void dev_pm_opp_put_supported_hw(struct opp_table *opp_table) {}
+static inline struct opp_table *devm_pm_opp_set_supported_hw(struct device *dev, + const u32 *versions, + unsigned int count) +{ + return ERR_PTR(-ENOTSUPP); +} + static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data)) {
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(+)
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);
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.
From: Yangtao Li tiny.windzz@gmail.com
Use the devm_pm_opp_set_supported_hw() to avoid memory leaks in some cases.
Signed-off-by: Yangtao Li tiny.windzz@gmail.com Signed-off-by: Yangtao Li frank@allwinnertech.com --- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 91726da82ed6..8d88f119a59f 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -1487,7 +1487,7 @@ static void check_speed_bin(struct device *dev) nvmem_cell_put(cell); }
- dev_pm_opp_set_supported_hw(dev, &val, 1); + devm_pm_opp_set_supported_hw(dev, &val, 1); }
struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
On 12-10-20, 21:55, Frank Lee wrote:
From: Yangtao Li tiny.windzz@gmail.com
Use the devm_pm_opp_set_supported_hw() to avoid memory leaks in some cases.
Signed-off-by: Yangtao Li tiny.windzz@gmail.com Signed-off-by: Yangtao Li frank@allwinnertech.com
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 91726da82ed6..8d88f119a59f 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -1487,7 +1487,7 @@ static void check_speed_bin(struct device *dev) nvmem_cell_put(cell); }
- dev_pm_opp_set_supported_hw(dev, &val, 1);
- devm_pm_opp_set_supported_hw(dev, &val, 1);
}
struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
Rob: Can I have your Ack to pick this patch up please ?
On 12-10-20, 21:55, Frank Lee wrote:
Hi, this patchset introduce devm_pm_opp_set_prop_name() and devm_pm_opp_set_supported_hw().
Yangtao Li (3): opp: Add devres wrapper for dev_pm_opp_set_supported_hw opp: Add devres wrapper for dev_pm_opp_set_prop_name drm/msm: Convert to devm_pm_opp_set_supported_hw
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- drivers/opp/core.c | 80 +++++++++++++++++++++++++++ include/linux/pm_opp.h | 14 +++++ 3 files changed, 95 insertions(+), 1 deletion(-)
Applied. Thanks.
On 28-10-20, 11:36, Viresh Kumar wrote:
On 12-10-20, 21:55, Frank Lee wrote:
Hi, this patchset introduce devm_pm_opp_set_prop_name() and devm_pm_opp_set_supported_hw().
Yangtao Li (3): opp: Add devres wrapper for dev_pm_opp_set_supported_hw opp: Add devres wrapper for dev_pm_opp_set_prop_name drm/msm: Convert to devm_pm_opp_set_supported_hw
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- drivers/opp/core.c | 80 +++++++++++++++++++++++++++ include/linux/pm_opp.h | 14 +++++ 3 files changed, 95 insertions(+), 1 deletion(-)
Applied. Thanks.
And I have dropped all of them based on the discussion we are having. Thanks.
dri-devel@lists.freedesktop.org
-
Frank Lee -
Frank Lee -
Viresh Kumar