[PATCH V5 0/4] clk: new APIs to handle all available clocks

[PATCH V5 0/4] clk: new APIs to handle all available clocks

[Dong Aisheng]

This patch series is a continue of discussion from here,
https://patchwork.kernel.org/patch/9986293/
that some users may want to handle all available clocks from device
tree without need to know the detailed clock information likes clock
numbers and names. This is useful in writing some generic drivers to
handle clock part.

Note:
This patch series is tested on MX6Q SDB cpufreq driver with a minor change
to switch to use clk_bulk_get_all.
But patch 4 only test compiling. Hopefully someone could help test
the function.

v3->v4:
 * improve 'devres->clks = *clks' according to Stephen's suggestion
v2->v3:
 * address all comments from Stephen
 * fix build warnings on other architectures.
v1->v2:
 * add clk_bulk_{get|put}_all() which only supports DT platform currently
 * remove _all variants and the wrapper struct clk_bulk
 * make of_clk_bulk_get and of_clk_bulk_get_all private until someone
   proves they need it because they don't have a struct device pointer.

Dong Aisheng (4):
  clk: bulk: add of_clk_bulk_get()
  clk: add new APIs to operate on all available clocks
  clk: add managed version of clk_bulk_get_all
  video: simplefb: switch to use clk_bulk API to simplify clock
    operations

 drivers/clk/clk-bulk.c         | 80 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-devres.c       | 24 +++++++++++++
 drivers/video/fbdev/simplefb.c | 68 ++++++++---------------------------
 include/linux/clk.h            | 65 +++++++++++++++++++++++++++++++++-
 4 files changed, 182 insertions(+), 55 deletions(-)

-- 
2.7.4

[PATCH V5 1/4] clk: bulk: add of_clk_bulk_get()

[Dong Aisheng]

'clock-names' property is optional in DT, so of_clk_bulk_get() is
introduced here to handle this for DT users without 'clock-names'
specified. Later clk_bulk_get_all() will be implemented on top of
it and this API will be kept private until someone proves they need
it because they don't have a struct device pointer.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Russell King <linux@arm.linux.org.uk>
Reported-by: Shawn Guo <shawnguo@kernel.org>
Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * no changes
v2->v3:
 * remove #if define condition
 * remove EXPORT_SYMBOL
---
 drivers/clk/clk-bulk.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c
index 6904ed6..4460ac5 100644
--- a/drivers/clk/clk-bulk.c
+++ b/drivers/clk/clk-bulk.c
@@ -19,6 +19,35 @@
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/of.h>
+
+static int __must_check of_clk_bulk_get(struct device_node *np, int num_clks,
+					struct clk_bulk_data *clks)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < num_clks; i++)
+		clks[i].clk = NULL;
+
+	for (i = 0; i < num_clks; i++) {
+		clks[i].clk = of_clk_get(np, i);
+		if (IS_ERR(clks[i].clk)) {
+			ret = PTR_ERR(clks[i].clk);
+			pr_err("%pOF: Failed to get clk index: %d ret: %d\n",
+			       np, i, ret);
+			clks[i].clk = NULL;
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	clk_bulk_put(i, clks);
+
+	return ret;
+}
 
 void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
 {
-- 
2.7.4

[PATCH V5 2/4] clk: add new APIs to operate on all available clocks

[Dong Aisheng]

This patch introduces of_clk_bulk_get_all and clk_bulk_x_all APIs
to users who just want to handle all available clocks from device tree
without need to know the detailed clock information likes clock numbers
and names. This is useful in writing some generic drivers to handle clock
part.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * no changes
v2->v3:
 * remove #if define condition
 * use kmalloc_array
v1->v2:
 * make of_clk_bulk_get_all private
 * add clk_bulk_get/put_all
---
 drivers/clk/clk-bulk.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h    | 42 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c
index 4460ac5..6a7118d 100644
--- a/drivers/clk/clk-bulk.c
+++ b/drivers/clk/clk-bulk.c
@@ -17,9 +17,11 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 
 static int __must_check of_clk_bulk_get(struct device_node *np, int num_clks,
 					struct clk_bulk_data *clks)
@@ -49,6 +51,32 @@ static int __must_check of_clk_bulk_get(struct device_node *np, int num_clks,
 	return ret;
 }
 
+static int __must_check of_clk_bulk_get_all(struct device_node *np,
+					    struct clk_bulk_data **clks)
+{
+	struct clk_bulk_data *clk_bulk;
+	int num_clks;
+	int ret;
+
+	num_clks = of_clk_get_parent_count(np);
+	if (!num_clks)
+		return 0;
+
+	clk_bulk = kmalloc_array(num_clks, sizeof(*clk_bulk), GFP_KERNEL);
+	if (!clk_bulk)
+		return -ENOMEM;
+
+	ret = of_clk_bulk_get(np, num_clks, clk_bulk);
+	if (ret) {
+		kfree(clk_bulk);
+		return ret;
+	}
+
+	*clks = clk_bulk;
+
+	return num_clks;
+}
+
 void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
 {
 	while (--num_clks >= 0) {
@@ -88,6 +116,29 @@ int __must_check clk_bulk_get(struct device *dev, int num_clks,
 }
 EXPORT_SYMBOL(clk_bulk_get);
 
+void clk_bulk_put_all(int num_clks, struct clk_bulk_data *clks)
+{
+	if (IS_ERR_OR_NULL(clks))
+		return;
+
+	clk_bulk_put(num_clks, clks);
+
+	kfree(clks);
+}
+EXPORT_SYMBOL(clk_bulk_put_all);
+
+int __must_check clk_bulk_get_all(struct device *dev,
+				  struct clk_bulk_data **clks)
+{
+	struct device_node *np = dev_of_node(dev);
+
+	if (!np)
+		return 0;
+
+	return of_clk_bulk_get_all(np, clks);
+}
+EXPORT_SYMBOL(clk_bulk_get_all);
+
 #ifdef CONFIG_HAVE_CLK_PREPARE
 
 /**
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 4f750c4..e9433c7 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -312,7 +312,26 @@ struct clk *clk_get(struct device *dev, const char *id);
  */
 int __must_check clk_bulk_get(struct device *dev, int num_clks,
 			      struct clk_bulk_data *clks);
-
+/**
+ * clk_bulk_get_all - lookup and obtain all available references to clock
+ *		      producer.
+ * @dev: device for clock "consumer"
+ * @clks: pointer to the clk_bulk_data table of consumer
+ *
+ * This helper function allows drivers to get all clk consumers in one
+ * operation. If any of the clk cannot be acquired then any clks
+ * that were obtained will be freed before returning to the caller.
+ *
+ * Returns a positive value for the number of clocks obtained while the
+ * clock references are stored in the clk_bulk_data table in @clks field.
+ * Returns 0 if there're none and a negative value if something failed.
+ *
+ * Drivers must assume that the clock source is not enabled.
+ *
+ * clk_bulk_get should not be called from within interrupt context.
+ */
+int __must_check clk_bulk_get_all(struct device *dev,
+				  struct clk_bulk_data **clks);
 /**
  * devm_clk_bulk_get - managed get multiple clk consumers
  * @dev: device for clock "consumer"
@@ -488,6 +507,19 @@ void clk_put(struct clk *clk);
 void clk_bulk_put(int num_clks, struct clk_bulk_data *clks);
 
 /**
+ * clk_bulk_put_all - "free" all the clock source
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table of consumer
+ *
+ * Note: drivers must ensure that all clk_bulk_enable calls made on this
+ * clock source are balanced by clk_bulk_disable calls prior to calling
+ * this function.
+ *
+ * clk_bulk_put_all should not be called from within interrupt context.
+ */
+void clk_bulk_put_all(int num_clks, struct clk_bulk_data *clks);
+
+/**
  * devm_clk_put	- "free" a managed clock source
  * @dev: device used to acquire the clock
  * @clk: clock source acquired with devm_clk_get()
@@ -642,6 +674,12 @@ static inline int __must_check clk_bulk_get(struct device *dev, int num_clks,
 	return 0;
 }
 
+static inline int __must_check clk_bulk_get_all(struct device *dev,
+					 struct clk_bulk_data **clks)
+{
+	return 0;
+}
+
 static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 {
 	return NULL;
@@ -663,6 +701,8 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void clk_bulk_put(int num_clks, struct clk_bulk_data *clks) {}
 
+static inline void clk_bulk_put_all(int num_clks, struct clk_bulk_data *clks) {}
+
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
 
-- 
2.7.4

[PATCH V5 3/4] clk: add managed version of clk_bulk_get_all

[Dong Aisheng]

This patch introduces the managed version of clk_bulk_get_all.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * improve 'devres->clks = *clks' according to Stephen's suggestion
v2->v3:
 * a minor fix of build warning on PowerPC platform.
v1->v2:
 * new patch
---
 drivers/clk/clk-devres.c | 24 ++++++++++++++++++++++++
 include/linux/clk.h      | 23 +++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index d854e26..12c8745 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -70,6 +70,30 @@ int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
 }
 EXPORT_SYMBOL_GPL(devm_clk_bulk_get);
 
+int __must_check devm_clk_bulk_get_all(struct device *dev,
+				       struct clk_bulk_data **clks)
+{
+	struct clk_bulk_devres *devres;
+	int ret;
+
+	devres = devres_alloc(devm_clk_bulk_release,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	ret = clk_bulk_get_all(dev, &devres->clks);
+	if (ret > 0) {
+		*clks = devres->clks;
+		devres->num_clks = ret;
+		devres_add(dev, devres);
+	} else {
+		devres_free(devres);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
+
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
 	struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index e9433c7..c705271 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -346,6 +346,22 @@ int __must_check clk_bulk_get_all(struct device *dev,
  */
 int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
 				   struct clk_bulk_data *clks);
+/**
+ * devm_clk_bulk_get_all - managed get multiple clk consumers
+ * @dev: device for clock "consumer"
+ * @clks: pointer to the clk_bulk_data table of consumer
+ *
+ * Returns a positive value for the number of clocks obtained while the
+ * clock references are stored in the clk_bulk_data table in @clks field.
+ * Returns 0 if there're none and a negative value if something failed.
+ *
+ * This helper function allows drivers to get several clk
+ * consumers in one operation with management, the clks will
+ * automatically be freed when the device is unbound.
+ */
+
+int __must_check devm_clk_bulk_get_all(struct device *dev,
+				       struct clk_bulk_data **clks);
 
 /**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
@@ -691,6 +707,13 @@ static inline int __must_check devm_clk_bulk_get(struct device *dev, int num_clk
 	return 0;
 }
 
+static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
+						     struct clk_bulk_data **clks)
+{
+
+	return 0;
+}
+
 static inline struct clk *devm_get_clk_from_child(struct device *dev,
 				struct device_node *np, const char *con_id)
 {
-- 
2.7.4

[PATCH V5 4/4] video: simplefb: switch to use clk_bulk API to simplify clock operations

[Dong Aisheng]

Switching to use clk_bulk API to simplify clock operations.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v4->v5:
 * fix wrong setting of par->clks_enabled
v3->v4:
 * no changes
v2->v3:
 * fix a build warning on x86 platform due to a wrong
   of the prototype of simplefb_clocks_enable
v1->v2:
 * switch to clk_bulk_get_all from of_clk_bulk_get_all
---
 drivers/video/fbdev/simplefb.c | 68 +++++++++---------------------------------
 1 file changed, 14 insertions(+), 54 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 9a9d748..8d1fbd6 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -182,7 +182,7 @@ struct simplefb_par {
 #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
 	bool clks_enabled;
 	unsigned int clk_count;
-	struct clk **clks;
+	struct clk_bulk_data *clks;
 #endif
 #if defined CONFIG_OF && defined CONFIG_REGULATOR
 	bool regulators_enabled;
@@ -214,37 +214,13 @@ static int simplefb_clocks_get(struct simplefb_par *par,
 			       struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct clk *clock;
-	int i;
 
 	if (dev_get_platdata(&pdev->dev) || !np)
 		return 0;
 
-	par->clk_count = of_clk_get_parent_count(np);
-	if (!par->clk_count)
-		return 0;
-
-	par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
-	if (!par->clks)
-		return -ENOMEM;
-
-	for (i = 0; i < par->clk_count; i++) {
-		clock = of_clk_get(np, i);
-		if (IS_ERR(clock)) {
-			if (PTR_ERR(clock) = -EPROBE_DEFER) {
-				while (--i >= 0) {
-					if (par->clks[i])
-						clk_put(par->clks[i]);
-				}
-				kfree(par->clks);
-				return -EPROBE_DEFER;
-			}
-			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
-				__func__, i, PTR_ERR(clock));
-			continue;
-		}
-		par->clks[i] = clock;
-	}
+	par->clk_count = clk_bulk_get_all(&pdev->dev, &par->clks);
+	if ((par->clk_count < 0) && (par->clk_count = -EPROBE_DEFER))
+		return -EPROBE_DEFER;
 
 	return 0;
 }
@@ -252,39 +228,23 @@ static int simplefb_clocks_get(struct simplefb_par *par,
 static void simplefb_clocks_enable(struct simplefb_par *par,
 				   struct platform_device *pdev)
 {
-	int i, ret;
+	int ret;
 
-	for (i = 0; i < par->clk_count; i++) {
-		if (par->clks[i]) {
-			ret = clk_prepare_enable(par->clks[i]);
-			if (ret) {
-				dev_err(&pdev->dev,
-					"%s: failed to enable clock %d: %d\n",
-					__func__, i, ret);
-				clk_put(par->clks[i]);
-				par->clks[i] = NULL;
-			}
-		}
+	ret = clk_bulk_prepare_enable(par->clk_count, par->clks);
+	if (ret) {
+		par->clks_enabled = false;
+		dev_warn(&pdev->dev, "failed to enable clocks\n");
+	} else {
+		par->clks_enabled = true;
 	}
-	par->clks_enabled = true;
 }
 
 static void simplefb_clocks_destroy(struct simplefb_par *par)
 {
-	int i;
-
-	if (!par->clks)
-		return;
-
-	for (i = 0; i < par->clk_count; i++) {
-		if (par->clks[i]) {
-			if (par->clks_enabled)
-				clk_disable_unprepare(par->clks[i]);
-			clk_put(par->clks[i]);
-		}
-	}
+	if (par->clks_enabled)
+		clk_bulk_disable_unprepare(par->clk_count, par->clks);
 
-	kfree(par->clks);
+	clk_bulk_put_all(par->clk_count, par->clks);
 }
 #else
 static int simplefb_clocks_get(struct simplefb_par *par,
-- 
2.7.4

[PATCH V5 4/4] video: simplefb: switch to use clk_bulk API to simplify clock operations

[Dong Aisheng]

Switching to use clk_bulk API to simplify clock operations.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-fbdev at vger.kernel.org
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v4->v5:
 * fix wrong setting of par->clks_enabled
v3->v4:
 * no changes
v2->v3:
 * fix a build warning on x86 platform due to a wrong
   of the prototype of simplefb_clocks_enable
v1->v2:
 * switch to clk_bulk_get_all from of_clk_bulk_get_all
---
 drivers/video/fbdev/simplefb.c | 68 +++++++++---------------------------------
 1 file changed, 14 insertions(+), 54 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 9a9d748..8d1fbd6 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -182,7 +182,7 @@ struct simplefb_par {
 #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
 	bool clks_enabled;
 	unsigned int clk_count;
-	struct clk **clks;
+	struct clk_bulk_data *clks;
 #endif
 #if defined CONFIG_OF && defined CONFIG_REGULATOR
 	bool regulators_enabled;
@@ -214,37 +214,13 @@ static int simplefb_clocks_get(struct simplefb_par *par,
 			       struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct clk *clock;
-	int i;
 
 	if (dev_get_platdata(&pdev->dev) || !np)
 		return 0;
 
-	par->clk_count = of_clk_get_parent_count(np);
-	if (!par->clk_count)
-		return 0;
-
-	par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
-	if (!par->clks)
-		return -ENOMEM;
-
-	for (i = 0; i < par->clk_count; i++) {
-		clock = of_clk_get(np, i);
-		if (IS_ERR(clock)) {
-			if (PTR_ERR(clock) == -EPROBE_DEFER) {
-				while (--i >= 0) {
-					if (par->clks[i])
-						clk_put(par->clks[i]);
-				}
-				kfree(par->clks);
-				return -EPROBE_DEFER;
-			}
-			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
-				__func__, i, PTR_ERR(clock));
-			continue;
-		}
-		par->clks[i] = clock;
-	}
+	par->clk_count = clk_bulk_get_all(&pdev->dev, &par->clks);
+	if ((par->clk_count < 0) && (par->clk_count == -EPROBE_DEFER))
+		return -EPROBE_DEFER;
 
 	return 0;
 }
@@ -252,39 +228,23 @@ static int simplefb_clocks_get(struct simplefb_par *par,
 static void simplefb_clocks_enable(struct simplefb_par *par,
 				   struct platform_device *pdev)
 {
-	int i, ret;
+	int ret;
 
-	for (i = 0; i < par->clk_count; i++) {
-		if (par->clks[i]) {
-			ret = clk_prepare_enable(par->clks[i]);
-			if (ret) {
-				dev_err(&pdev->dev,
-					"%s: failed to enable clock %d: %d\n",
-					__func__, i, ret);
-				clk_put(par->clks[i]);
-				par->clks[i] = NULL;
-			}
-		}
+	ret = clk_bulk_prepare_enable(par->clk_count, par->clks);
+	if (ret) {
+		par->clks_enabled = false;
+		dev_warn(&pdev->dev, "failed to enable clocks\n");
+	} else {
+		par->clks_enabled = true;
 	}
-	par->clks_enabled = true;
 }
 
 static void simplefb_clocks_destroy(struct simplefb_par *par)
 {
-	int i;
-
-	if (!par->clks)
-		return;
-
-	for (i = 0; i < par->clk_count; i++) {
-		if (par->clks[i]) {
-			if (par->clks_enabled)
-				clk_disable_unprepare(par->clks[i]);
-			clk_put(par->clks[i]);
-		}
-	}
+	if (par->clks_enabled)
+		clk_bulk_disable_unprepare(par->clk_count, par->clks);
 
-	kfree(par->clks);
+	clk_bulk_put_all(par->clk_count, par->clks);
 }
 #else
 static int simplefb_clocks_get(struct simplefb_par *par,
-- 
2.7.4

Re: [PATCH V5 4/4] video: simplefb: switch to use clk_bulk API to simplify clock operations

[Hans de Goede]

Hi,

On 29-08-18 16:41, Dong Aisheng wrote:
> Switching to use clk_bulk API to simplify clock operations.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v4->v5:
>   * fix wrong setting of par->clks_enabled
> v3->v4:
>   * no changes
> v2->v3:
>   * fix a build warning on x86 platform due to a wrong
>     of the prototype of simplefb_clocks_enable
> v1->v2:
>   * switch to clk_bulk_get_all from of_clk_bulk_get_all
> ---
>   drivers/video/fbdev/simplefb.c | 68 +++++++++---------------------------------
>   1 file changed, 14 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 9a9d748..8d1fbd6 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -182,7 +182,7 @@ struct simplefb_par {
>   #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
>   	bool clks_enabled;
>   	unsigned int clk_count;
> -	struct clk **clks;
> +	struct clk_bulk_data *clks;
>   #endif
>   #if defined CONFIG_OF && defined CONFIG_REGULATOR
>   	bool regulators_enabled;
> @@ -214,37 +214,13 @@ static int simplefb_clocks_get(struct simplefb_par *par,
>   			       struct platform_device *pdev)
>   {
>   	struct device_node *np = pdev->dev.of_node;
> -	struct clk *clock;
> -	int i;
>   
>   	if (dev_get_platdata(&pdev->dev) || !np)
>   		return 0;
>   
> -	par->clk_count = of_clk_get_parent_count(np);
> -	if (!par->clk_count)
> -		return 0;
> -
> -	par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
> -	if (!par->clks)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < par->clk_count; i++) {
> -		clock = of_clk_get(np, i);
> -		if (IS_ERR(clock)) {
> -			if (PTR_ERR(clock) = -EPROBE_DEFER) {
> -				while (--i >= 0) {
> -					if (par->clks[i])
> -						clk_put(par->clks[i]);
> -				}
> -				kfree(par->clks);
> -				return -EPROBE_DEFER;
> -			}
> -			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> -				__func__, i, PTR_ERR(clock));
> -			continue;
> -		}
> -		par->clks[i] = clock;
> -	}
> +	par->clk_count = clk_bulk_get_all(&pdev->dev, &par->clks);
> +	if ((par->clk_count < 0) && (par->clk_count = -EPROBE_DEFER))
> +		return -EPROBE_DEFER;

I just noticed this now, but clk_count is unsigned so it will never
be < 0, please make it signed.

Also there is no need to check for < 0 just (par->clk_count = -EPROBE_DEFER)
is enough (if that is true < 0 also always is true).

>   
>   	return 0;
>   }
> @@ -252,39 +228,23 @@ static int simplefb_clocks_get(struct simplefb_par *par,
>   static void simplefb_clocks_enable(struct simplefb_par *par,
>   				   struct platform_device *pdev)
>   {
> -	int i, ret;
> +	int ret;
>   
> -	for (i = 0; i < par->clk_count; i++) {
> -		if (par->clks[i]) {
> -			ret = clk_prepare_enable(par->clks[i]);
> -			if (ret) {
> -				dev_err(&pdev->dev,
> -					"%s: failed to enable clock %d: %d\n",
> -					__func__, i, ret);
> -				clk_put(par->clks[i]);
> -				par->clks[i] = NULL;
> -			}
> -		}
> +	ret = clk_bulk_prepare_enable(par->clk_count, par->clks);

Hmm, what happens if par->clk_count < 0 (another <0 value then
-EPROBEDEFER) ?

> +	if (ret) {
> +		par->clks_enabled = false;

No need to set false here.

> +		dev_warn(&pdev->dev, "failed to enable clocks\n");
> +	} else {
> +		par->clks_enabled = true;
>   	}
> -	par->clks_enabled = true;
>   }
>   
>   static void simplefb_clocks_destroy(struct simplefb_par *par)
>   {
> -	int i;
> -
> -	if (!par->clks)
> -		return;
> -
> -	for (i = 0; i < par->clk_count; i++) {
> -		if (par->clks[i]) {
> -			if (par->clks_enabled)
> -				clk_disable_unprepare(par->clks[i]);
> -			clk_put(par->clks[i]);
> -		}
> -	}
> +	if (par->clks_enabled)
> +		clk_bulk_disable_unprepare(par->clk_count, par->clks);
>   
> -	kfree(par->clks);
> +	clk_bulk_put_all(par->clk_count, par->clks);

Again what about par->clk_count < 0 ?

Regards,

Hans

[PATCH V5 4/4] video: simplefb: switch to use clk_bulk API to simplify clock operations

[Hans de Goede]

Hi,

On 29-08-18 16:41, Dong Aisheng wrote:
> Switching to use clk_bulk API to simplify clock operations.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: linux-fbdev at vger.kernel.org
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v4->v5:
>   * fix wrong setting of par->clks_enabled
> v3->v4:
>   * no changes
> v2->v3:
>   * fix a build warning on x86 platform due to a wrong
>     of the prototype of simplefb_clocks_enable
> v1->v2:
>   * switch to clk_bulk_get_all from of_clk_bulk_get_all
> ---
>   drivers/video/fbdev/simplefb.c | 68 +++++++++---------------------------------
>   1 file changed, 14 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 9a9d748..8d1fbd6 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -182,7 +182,7 @@ struct simplefb_par {
>   #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
>   	bool clks_enabled;
>   	unsigned int clk_count;
> -	struct clk **clks;
> +	struct clk_bulk_data *clks;
>   #endif
>   #if defined CONFIG_OF && defined CONFIG_REGULATOR
>   	bool regulators_enabled;
> @@ -214,37 +214,13 @@ static int simplefb_clocks_get(struct simplefb_par *par,
>   			       struct platform_device *pdev)
>   {
>   	struct device_node *np = pdev->dev.of_node;
> -	struct clk *clock;
> -	int i;
>   
>   	if (dev_get_platdata(&pdev->dev) || !np)
>   		return 0;
>   
> -	par->clk_count = of_clk_get_parent_count(np);
> -	if (!par->clk_count)
> -		return 0;
> -
> -	par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
> -	if (!par->clks)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < par->clk_count; i++) {
> -		clock = of_clk_get(np, i);
> -		if (IS_ERR(clock)) {
> -			if (PTR_ERR(clock) == -EPROBE_DEFER) {
> -				while (--i >= 0) {
> -					if (par->clks[i])
> -						clk_put(par->clks[i]);
> -				}
> -				kfree(par->clks);
> -				return -EPROBE_DEFER;
> -			}
> -			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> -				__func__, i, PTR_ERR(clock));
> -			continue;
> -		}
> -		par->clks[i] = clock;
> -	}
> +	par->clk_count = clk_bulk_get_all(&pdev->dev, &par->clks);
> +	if ((par->clk_count < 0) && (par->clk_count == -EPROBE_DEFER))
> +		return -EPROBE_DEFER;

I just noticed this now, but clk_count is unsigned so it will never
be < 0, please make it signed.

Also there is no need to check for < 0 just (par->clk_count == -EPROBE_DEFER)
is enough (if that is true < 0 also always is true).

>   
>   	return 0;
>   }
> @@ -252,39 +228,23 @@ static int simplefb_clocks_get(struct simplefb_par *par,
>   static void simplefb_clocks_enable(struct simplefb_par *par,
>   				   struct platform_device *pdev)
>   {
> -	int i, ret;
> +	int ret;
>   
> -	for (i = 0; i < par->clk_count; i++) {
> -		if (par->clks[i]) {
> -			ret = clk_prepare_enable(par->clks[i]);
> -			if (ret) {
> -				dev_err(&pdev->dev,
> -					"%s: failed to enable clock %d: %d\n",
> -					__func__, i, ret);
> -				clk_put(par->clks[i]);
> -				par->clks[i] = NULL;
> -			}
> -		}
> +	ret = clk_bulk_prepare_enable(par->clk_count, par->clks);

Hmm, what happens if par->clk_count < 0 (another <0 value then
-EPROBEDEFER) ?

> +	if (ret) {
> +		par->clks_enabled = false;

No need to set false here.

> +		dev_warn(&pdev->dev, "failed to enable clocks\n");
> +	} else {
> +		par->clks_enabled = true;
>   	}
> -	par->clks_enabled = true;
>   }
>   
>   static void simplefb_clocks_destroy(struct simplefb_par *par)
>   {
> -	int i;
> -
> -	if (!par->clks)
> -		return;
> -
> -	for (i = 0; i < par->clk_count; i++) {
> -		if (par->clks[i]) {
> -			if (par->clks_enabled)
> -				clk_disable_unprepare(par->clks[i]);
> -			clk_put(par->clks[i]);
> -		}
> -	}
> +	if (par->clks_enabled)
> +		clk_bulk_disable_unprepare(par->clk_count, par->clks);
>   
> -	kfree(par->clks);
> +	clk_bulk_put_all(par->clk_count, par->clks);

Again what about par->clk_count < 0 ?

Regards,

Hans

RE: [PATCH V5 4/4] video: simplefb: switch to use clk_bulk API to simplify clock operations

[A.s. Dong]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBIYW5zIGRlIEdvZWRlIFttYWls
dG86aGRlZ29lZGVAcmVkaGF0LmNvbV0NCj4gU2VudDogV2VkbmVzZGF5LCBBdWd1c3QgMjksIDIw
MTggMTE6MjIgUE0NCg0KWy4uLl0NCg0KPiBPbiAyOS0wOC0xOCAxNjo0MSwgRG9uZyBBaXNoZW5n
IHdyb3RlOg0KPiA+IFN3aXRjaGluZyB0byB1c2UgY2xrX2J1bGsgQVBJIHRvIHNpbXBsaWZ5IGNs
b2NrIG9wZXJhdGlvbnMuDQo+ID4NCj4gPiBDYzogSGFucyBkZSBHb2VkZSA8aGRlZ29lZGVAcmVk
aGF0LmNvbT4NCj4gPiBDYzogQmFydGxvbWllaiBab2xuaWVya2lld2ljeiA8Yi56b2xuaWVya2ll
QHNhbXN1bmcuY29tPg0KPiA+IENjOiBsaW51eC1mYmRldkB2Z2VyLmtlcm5lbC5vcmcNCj4gPiBD
YzogTWFzYWhpcm8gWWFtYWRhIDx5YW1hZGEubWFzYWhpcm9Ac29jaW9uZXh0LmNvbT4NCj4gPiBD
YzogU3RlcGhlbiBCb3lkIDxzYm95ZEBjb2RlYXVyb3JhLm9yZz4NCj4gPiBUZXN0ZWQtYnk6IFRo
b3IgVGhheWVyIDx0aG9yLnRoYXllckBsaW51eC5pbnRlbC5jb20+DQo+ID4gU2lnbmVkLW9mZi1i
eTogRG9uZyBBaXNoZW5nIDxhaXNoZW5nLmRvbmdAbnhwLmNvbT4NCj4gPiAtLS0NCj4gPiB2NC0+
djU6DQo+ID4gICAqIGZpeCB3cm9uZyBzZXR0aW5nIG9mIHBhci0+Y2xrc19lbmFibGVkDQo+ID4g
djMtPnY0Og0KPiA+ICAgKiBubyBjaGFuZ2VzDQo+ID4gdjItPnYzOg0KPiA+ICAgKiBmaXggYSBi
dWlsZCB3YXJuaW5nIG9uIHg4NiBwbGF0Zm9ybSBkdWUgdG8gYSB3cm9uZw0KPiA+ICAgICBvZiB0
aGUgcHJvdG90eXBlIG9mIHNpbXBsZWZiX2Nsb2Nrc19lbmFibGUNCj4gPiB2MS0+djI6DQo+ID4g
ICAqIHN3aXRjaCB0byBjbGtfYnVsa19nZXRfYWxsIGZyb20gb2ZfY2xrX2J1bGtfZ2V0X2FsbA0K
PiA+IC0tLQ0KPiA+ICAgZHJpdmVycy92aWRlby9mYmRldi9zaW1wbGVmYi5jIHwgNjggKysrKysr
KysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4gICAxIGZpbGUgY2hhbmdl
ZCwgMTQgaW5zZXJ0aW9ucygrKSwgNTQgZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZmIC0tZ2l0
IGEvZHJpdmVycy92aWRlby9mYmRldi9zaW1wbGVmYi5jDQo+ID4gYi9kcml2ZXJzL3ZpZGVvL2Zi
ZGV2L3NpbXBsZWZiLmMgaW5kZXggOWE5ZDc0OC4uOGQxZmJkNiAxMDA2NDQNCj4gPiAtLS0gYS9k
cml2ZXJzL3ZpZGVvL2ZiZGV2L3NpbXBsZWZiLmMNCj4gPiArKysgYi9kcml2ZXJzL3ZpZGVvL2Zi
ZGV2L3NpbXBsZWZiLmMNCj4gPiBAQCAtMTgyLDcgKzE4Miw3IEBAIHN0cnVjdCBzaW1wbGVmYl9w
YXIgew0KPiA+ICAgI2lmIGRlZmluZWQgQ09ORklHX09GICYmIGRlZmluZWQgQ09ORklHX0NPTU1P
Tl9DTEsNCj4gPiAgIAlib29sIGNsa3NfZW5hYmxlZDsNCj4gPiAgIAl1bnNpZ25lZCBpbnQgY2xr
X2NvdW50Ow0KPiA+IC0Jc3RydWN0IGNsayAqKmNsa3M7DQo+ID4gKwlzdHJ1Y3QgY2xrX2J1bGtf
ZGF0YSAqY2xrczsNCj4gPiAgICNlbmRpZg0KPiA+ICAgI2lmIGRlZmluZWQgQ09ORklHX09GICYm
IGRlZmluZWQgQ09ORklHX1JFR1VMQVRPUg0KPiA+ICAgCWJvb2wgcmVndWxhdG9yc19lbmFibGVk
Ow0KPiA+IEBAIC0yMTQsMzcgKzIxNCwxMyBAQCBzdGF0aWMgaW50IHNpbXBsZWZiX2Nsb2Nrc19n
ZXQoc3RydWN0IHNpbXBsZWZiX3Bhcg0KPiAqcGFyLA0KPiA+ICAgCQkJICAgICAgIHN0cnVjdCBw
bGF0Zm9ybV9kZXZpY2UgKnBkZXYpDQo+ID4gICB7DQo+ID4gICAJc3RydWN0IGRldmljZV9ub2Rl
ICpucCA9IHBkZXYtPmRldi5vZl9ub2RlOw0KPiA+IC0Jc3RydWN0IGNsayAqY2xvY2s7DQo+ID4g
LQlpbnQgaTsNCj4gPg0KPiA+ICAgCWlmIChkZXZfZ2V0X3BsYXRkYXRhKCZwZGV2LT5kZXYpIHx8
ICFucCkNCj4gPiAgIAkJcmV0dXJuIDA7DQo+ID4NCj4gPiAtCXBhci0+Y2xrX2NvdW50ID0gb2Zf
Y2xrX2dldF9wYXJlbnRfY291bnQobnApOw0KPiA+IC0JaWYgKCFwYXItPmNsa19jb3VudCkNCj4g
PiAtCQlyZXR1cm4gMDsNCj4gPiAtDQo+ID4gLQlwYXItPmNsa3MgPSBrY2FsbG9jKHBhci0+Y2xr
X2NvdW50LCBzaXplb2Yoc3RydWN0IGNsayAqKSwgR0ZQX0tFUk5FTCk7DQo+ID4gLQlpZiAoIXBh
ci0+Y2xrcykNCj4gPiAtCQlyZXR1cm4gLUVOT01FTTsNCj4gPiAtDQo+ID4gLQlmb3IgKGkgPSAw
OyBpIDwgcGFyLT5jbGtfY291bnQ7IGkrKykgew0KPiA+IC0JCWNsb2NrID0gb2ZfY2xrX2dldChu
cCwgaSk7DQo+ID4gLQkJaWYgKElTX0VSUihjbG9jaykpIHsNCj4gPiAtCQkJaWYgKFBUUl9FUlIo
Y2xvY2spID09IC1FUFJPQkVfREVGRVIpIHsNCj4gPiAtCQkJCXdoaWxlICgtLWkgPj0gMCkgew0K
PiA+IC0JCQkJCWlmIChwYXItPmNsa3NbaV0pDQo+ID4gLQkJCQkJCWNsa19wdXQocGFyLT5jbGtz
W2ldKTsNCj4gPiAtCQkJCX0NCj4gPiAtCQkJCWtmcmVlKHBhci0+Y2xrcyk7DQo+ID4gLQkJCQly
ZXR1cm4gLUVQUk9CRV9ERUZFUjsNCj4gPiAtCQkJfQ0KPiA+IC0JCQlkZXZfZXJyKCZwZGV2LT5k
ZXYsICIlczogY2xvY2sgJWQgbm90IGZvdW5kOiAlbGRcbiIsDQo+ID4gLQkJCQlfX2Z1bmNfXywg
aSwgUFRSX0VSUihjbG9jaykpOw0KPiA+IC0JCQljb250aW51ZTsNCj4gPiAtCQl9DQo+ID4gLQkJ
cGFyLT5jbGtzW2ldID0gY2xvY2s7DQo+ID4gLQl9DQo+ID4gKwlwYXItPmNsa19jb3VudCA9IGNs
a19idWxrX2dldF9hbGwoJnBkZXYtPmRldiwgJnBhci0+Y2xrcyk7DQo+ID4gKwlpZiAoKHBhci0+
Y2xrX2NvdW50IDwgMCkgJiYgKHBhci0+Y2xrX2NvdW50ID09IC1FUFJPQkVfREVGRVIpKQ0KPiA+
ICsJCXJldHVybiAtRVBST0JFX0RFRkVSOw0KPiANCj4gSSBqdXN0IG5vdGljZWQgdGhpcyBub3cs
IGJ1dCBjbGtfY291bnQgaXMgdW5zaWduZWQgc28gaXQgd2lsbCBuZXZlciBiZSA8IDAsIHBsZWFz
ZQ0KPiBtYWtlIGl0IHNpZ25lZC4NCj4gDQoNCkdvb2QgZmluZGluZ3MuDQoNCj4gQWxzbyB0aGVy
ZSBpcyBubyBuZWVkIHRvIGNoZWNrIGZvciA8IDAganVzdCAocGFyLT5jbGtfY291bnQgPT0gLUVQ
Uk9CRV9ERUZFUikNCj4gaXMgZW5vdWdoIChpZiB0aGF0IGlzIHRydWUgPCAwIGFsc28gYWx3YXlz
IGlzIHRydWUpLg0KPiANCg0KWWVzLCB5b3UncmUgcmlnaHQuDQoNCj4gPg0KPiA+ICAgCXJldHVy
biAwOw0KPiA+ICAgfQ0KPiA+IEBAIC0yNTIsMzkgKzIyOCwyMyBAQCBzdGF0aWMgaW50IHNpbXBs
ZWZiX2Nsb2Nrc19nZXQoc3RydWN0IHNpbXBsZWZiX3Bhcg0KPiAqcGFyLA0KPiA+ICAgc3RhdGlj
IHZvaWQgc2ltcGxlZmJfY2xvY2tzX2VuYWJsZShzdHJ1Y3Qgc2ltcGxlZmJfcGFyICpwYXIsDQo+
ID4gICAJCQkJICAgc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikNCj4gPiAgIHsNCj4gPiAt
CWludCBpLCByZXQ7DQo+ID4gKwlpbnQgcmV0Ow0KPiA+DQo+ID4gLQlmb3IgKGkgPSAwOyBpIDwg
cGFyLT5jbGtfY291bnQ7IGkrKykgew0KPiA+IC0JCWlmIChwYXItPmNsa3NbaV0pIHsNCj4gPiAt
CQkJcmV0ID0gY2xrX3ByZXBhcmVfZW5hYmxlKHBhci0+Y2xrc1tpXSk7DQo+ID4gLQkJCWlmIChy
ZXQpIHsNCj4gPiAtCQkJCWRldl9lcnIoJnBkZXYtPmRldiwNCj4gPiAtCQkJCQkiJXM6IGZhaWxl
ZCB0byBlbmFibGUgY2xvY2sgJWQ6ICVkXG4iLA0KPiA+IC0JCQkJCV9fZnVuY19fLCBpLCByZXQp
Ow0KPiA+IC0JCQkJY2xrX3B1dChwYXItPmNsa3NbaV0pOw0KPiA+IC0JCQkJcGFyLT5jbGtzW2ld
ID0gTlVMTDsNCj4gPiAtCQkJfQ0KPiA+IC0JCX0NCj4gPiArCXJldCA9IGNsa19idWxrX3ByZXBh
cmVfZW5hYmxlKHBhci0+Y2xrX2NvdW50LCBwYXItPmNsa3MpOw0KPiANCj4gSG1tLCB3aGF0IGhh
cHBlbnMgaWYgcGFyLT5jbGtfY291bnQgPCAwIChhbm90aGVyIDwwIHZhbHVlIHRoZW4NCj4gLUVQ
Uk9CRURFRkVSKSA/DQo+IA0KDQpHb29kIGNhdGNoLiBJIHdpbGwgYWRkIGEgY2hlY2tpbmcgb2Yg
aXQuDQpkaWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9mYmRldi9zaW1wbGVmYi5jIGIvZHJpdmVy
cy92aWRlby9mYmRldi9zaW1wbGVmYi5jDQppbmRleCA0NzdlMDA4Li44OWZiMWU3IDEwMDY0NA0K
LS0tIGEvZHJpdmVycy92aWRlby9mYmRldi9zaW1wbGVmYi5jDQorKysgYi9kcml2ZXJzL3ZpZGVv
L2ZiZGV2L3NpbXBsZWZiLmMNCkBAIC0yMzAsNiArMjMwLDkgQEAgc3RhdGljIHZvaWQgc2ltcGxl
ZmJfY2xvY2tzX2VuYWJsZShzdHJ1Y3Qgc2ltcGxlZmJfcGFyICpwYXIsDQogew0KICAgICAgICBp
bnQgcmV0Ow0KIA0KKyAgICAgICBpZiAocGFyLT5jbGtfY291bnQgPD0gMCkNCisgICAgICAgICAg
ICAgICByZXR1cm47DQorDQogICAgICAgIHJldCA9IGNsa19idWxrX3ByZXBhcmVfZW5hYmxlKHBh
ci0+Y2xrX2NvdW50LCBwYXItPmNsa3MpOw0KICAgICAgICBpZiAocmV0KQ0KICAgICAgICAgICAg
ICAgIGRldl93YXJuKCZwZGV2LT5kZXYsICJmYWlsZWQgdG8gZW5hYmxlIGNsb2Nrc1xuIik7DQpA
QCAtMjM5LDYgKzI0Miw5IEBAIHN0YXRpYyB2b2lkIHNpbXBsZWZiX2Nsb2Nrc19lbmFibGUoc3Ry
dWN0IHNpbXBsZWZiX3BhciAqcGFyLA0KIA0KIHN0YXRpYyB2b2lkIHNpbXBsZWZiX2Nsb2Nrc19k
ZXN0cm95KHN0cnVjdCBzaW1wbGVmYl9wYXIgKnBhcikNCiB7DQorICAgICAgIGlmIChwYXItPmNs
a19jb3VudCA8PSAwKQ0KKyAgICAgICAgICAgICAgIHJldHVybjsNCisNCiAgICAgICAgaWYgKHBh
ci0+Y2xrc19lbmFibGVkKQ0KICAgICAgICAgICAgICAgIGNsa19idWxrX2Rpc2FibGVfdW5wcmVw
YXJlKHBhci0+Y2xrX2NvdW50LCBwYXItPmNsa3MpOw0KDQo+ID4gKwlpZiAocmV0KSB7DQo+ID4g
KwkJcGFyLT5jbGtzX2VuYWJsZWQgPSBmYWxzZTsNCj4gDQo+IE5vIG5lZWQgdG8gc2V0IGZhbHNl
IGhlcmUuDQo+IA0KDQpHb3QgaXQuIFRoZW4gbGV0J3MgcmVwbHkgb24gdGhlIGt6YWxsb2MNCg0K
PiA+ICsJCWRldl93YXJuKCZwZGV2LT5kZXYsICJmYWlsZWQgdG8gZW5hYmxlIGNsb2Nrc1xuIik7
DQo+ID4gKwl9IGVsc2Ugew0KPiA+ICsJCXBhci0+Y2xrc19lbmFibGVkID0gdHJ1ZTsNCj4gPiAg
IAl9DQo+ID4gLQlwYXItPmNsa3NfZW5hYmxlZCA9IHRydWU7DQo+ID4gICB9DQo+ID4NCj4gPiAg
IHN0YXRpYyB2b2lkIHNpbXBsZWZiX2Nsb2Nrc19kZXN0cm95KHN0cnVjdCBzaW1wbGVmYl9wYXIg
KnBhcikNCj4gPiAgIHsNCj4gPiAtCWludCBpOw0KPiA+IC0NCj4gPiAtCWlmICghcGFyLT5jbGtz
KQ0KPiA+IC0JCXJldHVybjsNCj4gPiAtDQo+ID4gLQlmb3IgKGkgPSAwOyBpIDwgcGFyLT5jbGtf
Y291bnQ7IGkrKykgew0KPiA+IC0JCWlmIChwYXItPmNsa3NbaV0pIHsNCj4gPiAtCQkJaWYgKHBh
ci0+Y2xrc19lbmFibGVkKQ0KPiA+IC0JCQkJY2xrX2Rpc2FibGVfdW5wcmVwYXJlKHBhci0+Y2xr
c1tpXSk7DQo+ID4gLQkJCWNsa19wdXQocGFyLT5jbGtzW2ldKTsNCj4gPiAtCQl9DQo+ID4gLQl9
DQo+ID4gKwlpZiAocGFyLT5jbGtzX2VuYWJsZWQpDQo+ID4gKwkJY2xrX2J1bGtfZGlzYWJsZV91
bnByZXBhcmUocGFyLT5jbGtfY291bnQsIHBhci0+Y2xrcyk7DQo+ID4NCj4gPiAtCWtmcmVlKHBh
ci0+Y2xrcyk7DQo+ID4gKwljbGtfYnVsa19wdXRfYWxsKHBhci0+Y2xrX2NvdW50LCBwYXItPmNs
a3MpOw0KPiANCj4gQWdhaW4gd2hhdCBhYm91dCBwYXItPmNsa19jb3VudCA8IDAgPw0KPiANCj4g
UmVnYXJkcywNCj4gDQo+IEhhbnMNCg0KV2lsbCByZXNlbmQgYSBuZXcgcGF0Y2ggc2VyaWVzLiBU
aGFua3MgZm9yIHRoZSBjYXJlZnVsbHkgcmV2aWV3Lg0KDQpSZWdhcmRzDQpEb25nIEFpc2hlbmcN
Cg0K

[PATCH V5 4/4] video: simplefb: switch to use clk_bulk API to simplify clock operations

[A.s. Dong]

> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede at redhat.com]
> Sent: Wednesday, August 29, 2018 11:22 PM

[...]

> On 29-08-18 16:41, Dong Aisheng wrote:
> > Switching to use clk_bulk API to simplify clock operations.
> >
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Cc: linux-fbdev at vger.kernel.org
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v4->v5:
> >   * fix wrong setting of par->clks_enabled
> > v3->v4:
> >   * no changes
> > v2->v3:
> >   * fix a build warning on x86 platform due to a wrong
> >     of the prototype of simplefb_clocks_enable
> > v1->v2:
> >   * switch to clk_bulk_get_all from of_clk_bulk_get_all
> > ---
> >   drivers/video/fbdev/simplefb.c | 68 +++++++++---------------------------------
> >   1 file changed, 14 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/simplefb.c
> > b/drivers/video/fbdev/simplefb.c index 9a9d748..8d1fbd6 100644
> > --- a/drivers/video/fbdev/simplefb.c
> > +++ b/drivers/video/fbdev/simplefb.c
> > @@ -182,7 +182,7 @@ struct simplefb_par {
> >   #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
> >   	bool clks_enabled;
> >   	unsigned int clk_count;
> > -	struct clk **clks;
> > +	struct clk_bulk_data *clks;
> >   #endif
> >   #if defined CONFIG_OF && defined CONFIG_REGULATOR
> >   	bool regulators_enabled;
> > @@ -214,37 +214,13 @@ static int simplefb_clocks_get(struct simplefb_par
> *par,
> >   			       struct platform_device *pdev)
> >   {
> >   	struct device_node *np = pdev->dev.of_node;
> > -	struct clk *clock;
> > -	int i;
> >
> >   	if (dev_get_platdata(&pdev->dev) || !np)
> >   		return 0;
> >
> > -	par->clk_count = of_clk_get_parent_count(np);
> > -	if (!par->clk_count)
> > -		return 0;
> > -
> > -	par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
> > -	if (!par->clks)
> > -		return -ENOMEM;
> > -
> > -	for (i = 0; i < par->clk_count; i++) {
> > -		clock = of_clk_get(np, i);
> > -		if (IS_ERR(clock)) {
> > -			if (PTR_ERR(clock) == -EPROBE_DEFER) {
> > -				while (--i >= 0) {
> > -					if (par->clks[i])
> > -						clk_put(par->clks[i]);
> > -				}
> > -				kfree(par->clks);
> > -				return -EPROBE_DEFER;
> > -			}
> > -			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> > -				__func__, i, PTR_ERR(clock));
> > -			continue;
> > -		}
> > -		par->clks[i] = clock;
> > -	}
> > +	par->clk_count = clk_bulk_get_all(&pdev->dev, &par->clks);
> > +	if ((par->clk_count < 0) && (par->clk_count == -EPROBE_DEFER))
> > +		return -EPROBE_DEFER;
> 
> I just noticed this now, but clk_count is unsigned so it will never be < 0, please
> make it signed.
> 

Good findings.

> Also there is no need to check for < 0 just (par->clk_count == -EPROBE_DEFER)
> is enough (if that is true < 0 also always is true).
> 

Yes, you're right.

> >
> >   	return 0;
> >   }
> > @@ -252,39 +228,23 @@ static int simplefb_clocks_get(struct simplefb_par
> *par,
> >   static void simplefb_clocks_enable(struct simplefb_par *par,
> >   				   struct platform_device *pdev)
> >   {
> > -	int i, ret;
> > +	int ret;
> >
> > -	for (i = 0; i < par->clk_count; i++) {
> > -		if (par->clks[i]) {
> > -			ret = clk_prepare_enable(par->clks[i]);
> > -			if (ret) {
> > -				dev_err(&pdev->dev,
> > -					"%s: failed to enable clock %d: %d\n",
> > -					__func__, i, ret);
> > -				clk_put(par->clks[i]);
> > -				par->clks[i] = NULL;
> > -			}
> > -		}
> > +	ret = clk_bulk_prepare_enable(par->clk_count, par->clks);
> 
> Hmm, what happens if par->clk_count < 0 (another <0 value then
> -EPROBEDEFER) ?
> 

Good catch. I will add a checking of it.
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 477e008..89fb1e7 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -230,6 +230,9 @@ static void simplefb_clocks_enable(struct simplefb_par *par,
 {
        int ret;
 
+       if (par->clk_count <= 0)
+               return;
+
        ret = clk_bulk_prepare_enable(par->clk_count, par->clks);
        if (ret)
                dev_warn(&pdev->dev, "failed to enable clocks\n");
@@ -239,6 +242,9 @@ static void simplefb_clocks_enable(struct simplefb_par *par,
 
 static void simplefb_clocks_destroy(struct simplefb_par *par)
 {
+       if (par->clk_count <= 0)
+               return;
+
        if (par->clks_enabled)
                clk_bulk_disable_unprepare(par->clk_count, par->clks);

> > +	if (ret) {
> > +		par->clks_enabled = false;
> 
> No need to set false here.
> 

Got it. Then let's reply on the kzalloc

> > +		dev_warn(&pdev->dev, "failed to enable clocks\n");
> > +	} else {
> > +		par->clks_enabled = true;
> >   	}
> > -	par->clks_enabled = true;
> >   }
> >
> >   static void simplefb_clocks_destroy(struct simplefb_par *par)
> >   {
> > -	int i;
> > -
> > -	if (!par->clks)
> > -		return;
> > -
> > -	for (i = 0; i < par->clk_count; i++) {
> > -		if (par->clks[i]) {
> > -			if (par->clks_enabled)
> > -				clk_disable_unprepare(par->clks[i]);
> > -			clk_put(par->clks[i]);
> > -		}
> > -	}
> > +	if (par->clks_enabled)
> > +		clk_bulk_disable_unprepare(par->clk_count, par->clks);
> >
> > -	kfree(par->clks);
> > +	clk_bulk_put_all(par->clk_count, par->clks);
> 
> Again what about par->clk_count < 0 ?
> 
> Regards,
> 
> Hans

Will resend a new patch series. Thanks for the carefully review.

Regards
Dong Aisheng