[PATCH 0/5] Implement devm_of_clk_add_provider

[PATCH 0/5] Implement devm_of_clk_add_provider

[Ricardo Ribalda Delgado]

All Tull reported that there might be a great ammount of drivers with
imbalance on clk_add_provider. This is an issue for Device tree overlays
(and also a bug) https://lkml.org/lkml/2018/10/18/1103

This patchset implement a devm_ function of of_clk_add_provider, and
fixes 3 drivers.

Drivers like clk-gpio will be easily fixed with coccinelle if this set
is accepted. (I volunteer, I want to learn how to use it, just seen the
great presentations from Julia).

Ricardo Ribalda Delgado (5):
  clk: Refactor of_clk_add_provider and of_clk_add_hw_provider
  clk: Create devm_of_clk_add_provider
  clk: fixed-factor: Use devm_of_clk_add_provider
  clk: fixed-rate: Use devm_of_clk_add_provider
  clk: gpio: Use devm_of_clk_add_provider

 Documentation/driver-model/devres.txt |  1 +
 drivers/clk/clk-fixed-factor.c        | 14 ++++--
 drivers/clk/clk-fixed-rate.c          | 16 +++---
 drivers/clk/clk-gpio.c                |  2 +-
 drivers/clk/clk.c                     | 72 +++++++++++++++------------
 include/linux/clk-provider.h          | 11 ++++
 6 files changed, 73 insertions(+), 43 deletions(-)

-- 
2.19.1

[PATCH 1/5] clk: Refactor of_clk_add_provider and of_clk_add_hw_provider

[Ricardo Ribalda Delgado]

Both functions have almost the same functionality. Create a helper
function that is called by both functions.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk.c | 49 +++++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d31055ae6ec6..cd3117e66ab8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3723,15 +3723,11 @@ of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data)
 }
 EXPORT_SYMBOL_GPL(of_clk_hw_onecell_get);
 
-/**
- * of_clk_add_provider() - Register a clock provider for a node
- * @np: Device node pointer associated with clock provider
- * @clk_src_get: callback for decoding clock
- * @data: context pointer for @clk_src_get callback.
- */
-int of_clk_add_provider(struct device_node *np,
+static int __of_clk_add_provider(struct device_node *np,
 			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
 						   void *data),
+			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+						 void *data),
 			void *data)
 {
 	struct of_clk_provider *cp;
@@ -3744,11 +3740,12 @@ int of_clk_add_provider(struct device_node *np,
 	cp->node = of_node_get(np);
 	cp->data = data;
 	cp->get = clk_src_get;
+	cp->get_hw = get;
 
 	mutex_lock(&of_clk_mutex);
 	list_add(&cp->link, &of_clk_providers);
 	mutex_unlock(&of_clk_mutex);
-	pr_debug("Added clock from %pOF\n", np);
+	pr_debug("Added clock%s from %pOF\n", get?"_hw provider":"", np);
 
 	ret = of_clk_set_defaults(np, true);
 	if (ret < 0)
@@ -3756,6 +3753,20 @@ int of_clk_add_provider(struct device_node *np,
 
 	return ret;
 }
+
+/**
+ * of_clk_add_provider() - Register a clock provider for a node
+ * @np: Device node pointer associated with clock provider
+ * @clk_src_get: callback for decoding clock
+ * @data: context pointer for @clk_src_get callback.
+ */
+int of_clk_add_provider(struct device_node *np,
+		struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+					   void *data),
+		void *data)
+{
+	return  __of_clk_add_provider(np, clk_src_get, NULL, data);
+}
 EXPORT_SYMBOL_GPL(of_clk_add_provider);
 
 /**
@@ -3769,27 +3780,7 @@ int of_clk_add_hw_provider(struct device_node *np,
 						 void *data),
 			   void *data)
 {
-	struct of_clk_provider *cp;
-	int ret;
-
-	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
-	if (!cp)
-		return -ENOMEM;
-
-	cp->node = of_node_get(np);
-	cp->data = data;
-	cp->get_hw = get;
-
-	mutex_lock(&of_clk_mutex);
-	list_add(&cp->link, &of_clk_providers);
-	mutex_unlock(&of_clk_mutex);
-	pr_debug("Added clk_hw provider from %pOF\n", np);
-
-	ret = of_clk_set_defaults(np, true);
-	if (ret < 0)
-		of_clk_del_provider(np);
-
-	return ret;
+	return  __of_clk_add_provider(np, NULL, get, data);
 }
 EXPORT_SYMBOL_GPL(of_clk_add_hw_provider);
 
-- 
2.19.1

[PATCH 2/5] clk: Create devm_of_clk_add_provider

[Ricardo Ribalda Delgado]

Implement devm_ flavour of of_clk_add_provider. This will help fixing
of_node_get-put imbalances.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 Documentation/driver-model/devres.txt |  1 +
 drivers/clk/clk.c                     | 29 ++++++++++++++++++++++-----
 include/linux/clk-provider.h          | 11 ++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 43681ca0837f..a8e066d42355 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -237,6 +237,7 @@ CLOCK
   devm_clk_get()
   devm_clk_put()
   devm_clk_hw_register()
+  devm_of_clk_add_provider()
   devm_of_clk_add_hw_provider()
 
 DMA
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cd3117e66ab8..d315ffa6a32f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3789,10 +3789,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
 	of_clk_del_provider(*(struct device_node **)res);
 }
 
-int devm_of_clk_add_hw_provider(struct device *dev,
-			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
-					      void *data),
-			void *data)
+static int __devm_of_clk_add_provider(struct device *dev,
+		struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+					   void *data),
+		struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+				      void *data),
+		void *data)
 {
 	struct device_node **ptr, *np;
 	int ret;
@@ -3803,7 +3805,7 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 		return -ENOMEM;
 
 	np = dev->of_node;
-	ret = of_clk_add_hw_provider(np, get, data);
+	ret = __of_clk_add_provider(np, clk_src_get, get, data);
 	if (!ret) {
 		*ptr = np;
 		devres_add(dev, ptr);
@@ -3813,6 +3815,23 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 
 	return ret;
 }
+
+int devm_of_clk_add_provider(struct device *dev,
+		struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+					   void *data),
+		void *data)
+{
+	return __devm_of_clk_add_provider(dev, clk_src_get, NULL, data);
+}
+EXPORT_SYMBOL_GPL(devm_of_clk_add_provider);
+
+int devm_of_clk_add_hw_provider(struct device *dev,
+			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+					      void *data),
+			void *data)
+{
+	return __devm_of_clk_add_provider(dev, NULL, get, data);
+}
 EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);
 
 /**
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 08b1aa70a38d..bbd86fe656ef 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -905,6 +905,10 @@ int of_clk_add_hw_provider(struct device_node *np,
 			   struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 						 void *data),
 			   void *data);
+int devm_of_clk_add_provider(struct device *dev,
+		struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+					   void *data),
+		void *data);
 int devm_of_clk_add_hw_provider(struct device *dev,
 			   struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 						 void *data),
@@ -939,6 +943,13 @@ static inline int of_clk_add_hw_provider(struct device_node *np,
 {
 	return 0;
 }
+static inline int devm_of_clk_add_provider(struct device *dev,
+		struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+					   void *data),
+		void *data)
+{
+	return 0;
+}
 static inline int devm_of_clk_add_hw_provider(struct device *dev,
 			   struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 						 void *data),
-- 
2.19.1

[PATCH 3/5] clk: fixed-factor: Use devm_of_clk_add_provider

[Ricardo Ribalda Delgado]

Use devm_clk_add_provider. It will take care of clk un-registering.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk-fixed-factor.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 7df6b5b1e7ee..876d76522475 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -148,7 +148,8 @@ static const struct of_device_id set_rate_parent_matches[] = {
 	{ /* Sentinel */ },
 };
 
-static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
+static struct clk *_of_fixed_factor_clk_setup(struct device *dev,
+					      struct device_node *node)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;
@@ -187,7 +188,11 @@ static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
 		return clk;
 	}
 
-	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (dev)
+		ret = devm_of_clk_add_provider(dev, of_clk_src_simple_get, clk);
+	else
+		ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
 	if (ret) {
 		clk_unregister(clk);
 		return ERR_PTR(ret);
@@ -201,7 +206,7 @@ static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
  */
 void __init of_fixed_factor_clk_setup(struct device_node *node)
 {
-	_of_fixed_factor_clk_setup(node);
+	_of_fixed_factor_clk_setup(NULL, node);
 }
 CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
 		of_fixed_factor_clk_setup);
@@ -210,7 +215,6 @@ static int of_fixed_factor_clk_remove(struct platform_device *pdev)
 {
 	struct clk *clk = platform_get_drvdata(pdev);
 
-	of_clk_del_provider(pdev->dev.of_node);
 	clk_unregister_fixed_factor(clk);
 
 	return 0;
@@ -224,7 +228,7 @@ static int of_fixed_factor_clk_probe(struct platform_device *pdev)
 	 * This function is not executed when of_fixed_factor_clk_setup
 	 * succeeded.
 	 */
-	clk = _of_fixed_factor_clk_setup(pdev->dev.of_node);
+	clk = _of_fixed_factor_clk_setup(&pdev->dev, pdev->dev.of_node);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-- 
2.19.1

[PATCH 4/5] clk: fixed-rate: Use devm_of_clk_add_provider

[Ricardo Ribalda Delgado]

Use devm_clk_add_provider. It will take care of clk un-registering.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk-fixed-rate.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 6d6475c32ee5..2678b7bd68ac 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -158,7 +158,8 @@ void clk_hw_unregister_fixed_rate(struct clk_hw *hw)
 EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_rate);
 
 #ifdef CONFIG_OF
-static struct clk *_of_fixed_clk_setup(struct device_node *node)
+static struct clk *_of_fixed_clk_setup(struct device *dev,
+				       struct device_node *node)
 {
 	struct clk *clk;
 	const char *clk_name = node->name;
@@ -173,12 +174,16 @@ static struct clk *_of_fixed_clk_setup(struct device_node *node)
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
-	clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
+	clk = clk_register_fixed_rate_with_accuracy(dev, clk_name, NULL,
 						    0, rate, accuracy);
 	if (IS_ERR(clk))
 		return clk;
 
-	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (dev)
+		ret = devm_of_clk_add_provider(dev, of_clk_src_simple_get, clk);
+	else
+		ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
 	if (ret) {
 		clk_unregister(clk);
 		return ERR_PTR(ret);
@@ -192,7 +197,7 @@ static struct clk *_of_fixed_clk_setup(struct device_node *node)
  */
 void __init of_fixed_clk_setup(struct device_node *node)
 {
-	_of_fixed_clk_setup(node);
+	_of_fixed_clk_setup(NULL, node);
 }
 CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
 
@@ -200,7 +205,6 @@ static int of_fixed_clk_remove(struct platform_device *pdev)
 {
 	struct clk *clk = platform_get_drvdata(pdev);
 
-	of_clk_del_provider(pdev->dev.of_node);
 	clk_unregister_fixed_rate(clk);
 
 	return 0;
@@ -214,7 +218,7 @@ static int of_fixed_clk_probe(struct platform_device *pdev)
 	 * This function is not executed when of_fixed_clk_setup
 	 * succeeded.
 	 */
-	clk = _of_fixed_clk_setup(pdev->dev.of_node);
+	clk = _of_fixed_clk_setup(&pdev->dev, pdev->dev.of_node);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-- 
2.19.1

[PATCH 5/5] clk: gpio: Use devm_of_clk_add_provider

[Ricardo Ribalda Delgado]

Use devm_clk_add_provider. It will take care of clk un-registering.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/clk/clk-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 40af4fbab4d2..6eedb4bd779c 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -252,7 +252,7 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	return devm_of_clk_add_provider(&pdev->dev, of_clk_src_simple_get, clk);
 }
 
 static const struct of_device_id gpio_clk_match_table[] = {
-- 
2.19.1

Re: [PATCH 0/5] Implement devm_of_clk_add_provider

[Stephen Boyd]

Quoting Ricardo Ribalda Delgado (2018-11-01 07:40:39)
> All Tull reported that there might be a great ammount of drivers with
> imbalance on clk_add_provider. This is an issue for Device tree overlays
> (and also a bug) https://lkml.org/lkml/2018/10/18/1103
> 
> This patchset implement a devm_ function of of_clk_add_provider, and
> fixes 3 drivers.
> 
> Drivers like clk-gpio will be easily fixed with coccinelle if this set
> is accepted. (I volunteer, I want to learn how to use it, just seen the
> great presentations from Julia).

We already have devm_of_clk_add_hw_provider(), so any instances of
of_clk_add_provider() should be replaced with that, instead of
propagating the usage of of_clk_add_provider() any further. I'll gladly
apply patches to convert drivers from struct clk based APIs to struct
clk_hw based APIs so that we can clearly split clk providers from clk
consumers. So if you're interested in working on some coccinelle script
for that it would be great!

Re: [PATCH 0/5] Implement devm_of_clk_add_provider

[Ricardo Ribalda Delgado]

Hi Stephen
On Fri, Nov 2, 2018 at 12:35 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Ricardo Ribalda Delgado (2018-11-01 07:40:39)
> > All Tull reported that there might be a great ammount of drivers with
> > imbalance on clk_add_provider. This is an issue for Device tree overlays
> > (and also a bug) https://lkml.org/lkml/2018/10/18/1103
> >
> > This patchset implement a devm_ function of of_clk_add_provider, and
> > fixes 3 drivers.
> >
> > Drivers like clk-gpio will be easily fixed with coccinelle if this set
> > is accepted. (I volunteer, I want to learn how to use it, just seen the
> > great presentations from Julia).
>
> We already have devm_of_clk_add_hw_provider(), so any instances of
> of_clk_add_provider() should be replaced with that, instead of
> propagating the usage of of_clk_add_provider() any further. I'll gladly
> apply patches to convert drivers from struct clk based APIs to struct
> clk_hw based APIs so that we can clearly split clk providers from clk
> consumers. So if you're interested in working on some coccinelle script
> for that it would be great!
>

Will look into that.
Can you take a look to 1/5 of this patchset? I believe that it is
valid even if we do not take 2-5.

Cheers

-- 
Ricardo Ribalda

Re: [PATCH 0/5] Implement devm_of_clk_add_provider

[Stephen Boyd]

Quoting Ricardo Ribalda Delgado (2018-11-01 23:54:50)
> Hi Stephen
> On Fri, Nov 2, 2018 at 12:35 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Ricardo Ribalda Delgado (2018-11-01 07:40:39)
> > > All Tull reported that there might be a great ammount of drivers with
> > > imbalance on clk_add_provider. This is an issue for Device tree overlays
> > > (and also a bug) https://lkml.org/lkml/2018/10/18/1103
> > >
> > > This patchset implement a devm_ function of of_clk_add_provider, and
> > > fixes 3 drivers.
> > >
> > > Drivers like clk-gpio will be easily fixed with coccinelle if this set
> > > is accepted. (I volunteer, I want to learn how to use it, just seen the
> > > great presentations from Julia).
> >
> > We already have devm_of_clk_add_hw_provider(), so any instances of
> > of_clk_add_provider() should be replaced with that, instead of
> > propagating the usage of of_clk_add_provider() any further. I'll gladly
> > apply patches to convert drivers from struct clk based APIs to struct
> > clk_hw based APIs so that we can clearly split clk providers from clk
> > consumers. So if you're interested in working on some coccinelle script
> > for that it would be great!
> >
> 
> Will look into that.
> Can you take a look to 1/5 of this patchset? I believe that it is
> valid even if we do not take 2-5.

Sure. Again, that patch is combining code that we eventually want to
delete, so while it is a nice 9 line reduction, it again goes in the
wrong direction by merging two functions together that we'll want to
unmerge later when one of the functions is removed. I'd rather see
effort put into converting all drivers than merging deprecated code.