[PATCH 0/3] Introduce devm_pm_opp_set_* API

[PATCH 0/3] Introduce devm_pm_opp_set_* API

[Frank Lee]

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(-)

-- 
2.28.0

[PATCH 1/3] opp: Add devres wrapper for dev_pm_opp_set_supported_hw

[Frank Lee]

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))
 {
-- 
2.28.0

[PATCH 2/3] opp: Add devres wrapper for dev_pm_opp_set_prop_name

[Frank Lee]

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);
-- 
2.28.0

Re: [PATCH 2/3] opp: Add devres wrapper for dev_pm_opp_set_prop_name

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 2/3] opp: Add devres wrapper for dev_pm_opp_set_prop_name

[Frank Lee]

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

Re: [PATCH 2/3] opp: Add devres wrapper for dev_pm_opp_set_prop_name

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 2/3] opp: Add devres wrapper for dev_pm_opp_set_prop_name

[Frank Lee]

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

Re: [PATCH 2/3] opp: Add devres wrapper for dev_pm_opp_set_prop_name

[Viresh Kumar]

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.

-- 
viresh

[PATCH 3/3] drm/msm: Convert to devm_pm_opp_set_supported_hw

[Frank Lee]

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)
-- 
2.28.0

Re: [PATCH 3/3] drm/msm: Convert to devm_pm_opp_set_supported_hw

[Viresh Kumar]

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 ?

-- 
viresh

Re: [PATCH 0/3] Introduce devm_pm_opp_set_* API

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 0/3] Introduce devm_pm_opp_set_* API

[Viresh Kumar]

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.

-- 
viresh