[PATCH 0/4] Provide devm_clk_bulk_get_all_enabled() helper

[PATCH 0/4] Provide devm_clk_bulk_get_all_enabled() helper

[Cristian Ciocaltea]

Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
clocks") added devm_clk_bulk_get_all_enable() function, but missed to
return the number of clocks stored in the clk_bulk_data table referenced
by the clks argument.

That is required in case there is a need to iterate these clocks later,
therefore I couldn't see any use case of this parameter and should have
been simply removed from the function declaration.

The first patch in the series provides devm_clk_bulk_get_all_enabled()
variant, which is consistent with devm_clk_bulk_get_all() in terms of
the returned value:

 > 0 if one or more clocks have been stored
 = 0 if there are no clocks
 < 0 if an error occurred

Moreover, the naming is consistent with devm_clk_get_enabled(), i.e. use
the past form of 'enable'.

The next two patches switch existing users of devm_clk_get_enable() to
the new helper - there were only two, as of next-20240913.

The last patch drops the now obsolete devm_clk_bulk_get_all_enable()
helper.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (4):
      clk: Provide devm_clk_bulk_get_all_enabled() helper
      soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()
      PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()
      clk: Drop obsolete devm_clk_bulk_get_all_enable() helper

 drivers/clk/clk-devres.c                | 30 ++++++++++++++++--------------
 drivers/pci/controller/dwc/pci-exynos.c |  2 +-
 drivers/soc/mediatek/mtk-pmic-wrap.c    |  4 ++--
 include/linux/clk.h                     | 12 +++++++-----
 4 files changed, 26 insertions(+), 22 deletions(-)
---
base-commit: 5acd9952f95fb4b7da6d09a3be39195a80845eb6
change-id: 20240912-clk_bulk_ena_fix-16ba77358ddf

[PATCH 1/4] clk: Provide devm_clk_bulk_get_all_enabled() helper

[Cristian Ciocaltea]

Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
clocks") added devm_clk_bulk_get_all_enable() function, but missed to
return the number of clocks stored in the clk_bulk_data table referenced
by the clks argument.  Without knowing the number, it's not possible to
iterate these clocks when needed, hence the argument is useless and
could have been simply removed.

Introduce devm_clk_bulk_get_all_enabled() variant, which is consistent
with devm_clk_bulk_get_all() in terms of the returned value:

 > 0 if one or more clocks have been stored
 = 0 if there are no clocks
 < 0 if an error occurred

Moreover, the naming is consistent with devm_clk_get_enabled(), i.e. use
the past form of 'enable'.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/clk/clk-devres.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 24 ++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 82ae1f26e634..4203aaaa7544 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -250,6 +250,40 @@ int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
 
+int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
+					       struct clk_bulk_data **clks)
+{
+	struct clk_bulk_devres *devres;
+	int ret;
+
+	devres = devres_alloc(devm_clk_bulk_release_all_enable,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	ret = clk_bulk_get_all(dev, &devres->clks);
+	if (ret <= 0)
+		goto err_free_devres;
+
+	*clks = devres->clks;
+	devres->num_clks = ret;
+
+	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
+	if (ret) {
+		clk_bulk_put_all(devres->num_clks, devres->clks);
+		goto err_free_devres;
+	}
+
+	devres_add(dev, devres);
+
+	return devres->num_clks;
+
+err_free_devres:
+	devres_free(devres);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
+
 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 851a0f2cf42c..158c5072852e 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -511,6 +511,24 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
 int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
 					      struct clk_bulk_data **clks);
 
+/**
+ * devm_clk_bulk_get_all_enabled - Get and enable all clocks of the consumer (managed)
+ * @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 all clocks of the
+ * consumer and enables them in one operation with management.
+ * The clks will automatically be disabled and freed when the device
+ * is unbound.
+ */
+
+int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
+					       struct clk_bulk_data **clks);
+
 /**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -1040,6 +1058,12 @@ static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
 	return 0;
 }
 
+static inline int __must_check devm_clk_bulk_get_all_enabled(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.46.0

Re: [PATCH 1/4] clk: Provide devm_clk_bulk_get_all_enabled() helper

[AngeloGioacchino Del Regno]

Il 14/09/24 20:04, Cristian Ciocaltea ha scritto:
> Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
> clocks") added devm_clk_bulk_get_all_enable() function, but missed to
> return the number of clocks stored in the clk_bulk_data table referenced
> by the clks argument.  Without knowing the number, it's not possible to
> iterate these clocks when needed, hence the argument is useless and
> could have been simply removed.
> 
> Introduce devm_clk_bulk_get_all_enabled() variant, which is consistent
> with devm_clk_bulk_get_all() in terms of the returned value:
> 
>   > 0 if one or more clocks have been stored
>   = 0 if there are no clocks
>   < 0 if an error occurred
> 
> Moreover, the naming is consistent with devm_clk_get_enabled(), i.e. use
> the past form of 'enable'.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


> ---
>   drivers/clk/clk-devres.c | 34 ++++++++++++++++++++++++++++++++++
>   include/linux/clk.h      | 24 ++++++++++++++++++++++++
>   2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 82ae1f26e634..4203aaaa7544 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -250,6 +250,40 @@ int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
>   
> +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
> +					       struct clk_bulk_data **clks)
> +{
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release_all_enable,
> +			      sizeof(*devres), GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	ret = clk_bulk_get_all(dev, &devres->clks);
> +	if (ret <= 0)
> +		goto err_free_devres;
> +
> +	*clks = devres->clks;
> +	devres->num_clks = ret;
> +
> +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> +	if (ret) {
> +		clk_bulk_put_all(devres->num_clks, devres->clks);
> +		goto err_free_devres;
> +	}
> +
> +	devres_add(dev, devres);
> +
> +	return devres->num_clks;
> +
> +err_free_devres:
> +	devres_free(devres);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
> +
>   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 851a0f2cf42c..158c5072852e 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -511,6 +511,24 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
>   int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
>   					      struct clk_bulk_data **clks);
>   
> +/**
> + * devm_clk_bulk_get_all_enabled - Get and enable all clocks of the consumer (managed)
> + * @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 all clocks of the
> + * consumer and enables them in one operation with management.
> + * The clks will automatically be disabled and freed when the device
> + * is unbound.
> + */
> +
> +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
> +					       struct clk_bulk_data **clks);
> +
>   /**
>    * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>    * @dev: device for clock "consumer"
> @@ -1040,6 +1058,12 @@ static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
>   	return 0;
>   }
>   
> +static inline int __must_check devm_clk_bulk_get_all_enabled(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)
>   {
> 

[PATCH 2/4] soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()

[Cristian Ciocaltea]

The helper devm_clk_bulk_get_all_enable() is partially broken and will
be dropped since it missed to return the number of clocks stored in the
clk_bulk_data table referenced by the clks argument.

Use the newly introduced devm_clk_bulk_get_all_enabled() variant
instead, which is consistent with devm_clk_bulk_get_all() in terms of
the returned value:

 > 0 if one or more clocks have been stored
 = 0 if there are no clocks
 < 0 if an error occurred

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 9fdc0ef79202..0bcd85826375 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -2518,8 +2518,8 @@ static int pwrap_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = devm_clk_bulk_get_all_enable(wrp->dev, &clk);
-	if (ret)
+	ret = devm_clk_bulk_get_all_enabled(wrp->dev, &clk);
+	if (ret < 0)
 		return dev_err_probe(wrp->dev, ret,
 				     "failed to get clocks\n");
 

-- 
2.46.0

Re: [PATCH 2/4] soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()

[AngeloGioacchino Del Regno]

Il 14/09/24 20:04, Cristian Ciocaltea ha scritto:
> The helper devm_clk_bulk_get_all_enable() is partially broken and will
> be dropped since it missed to return the number of clocks stored in the
> clk_bulk_data table referenced by the clks argument.
> 
> Use the newly introduced devm_clk_bulk_get_all_enabled() variant
> instead, which is consistent with devm_clk_bulk_get_all() in terms of
> the returned value:
> 
>   > 0 if one or more clocks have been stored
>   = 0 if there are no clocks
>   < 0 if an error occurred
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   drivers/soc/mediatek/mtk-pmic-wrap.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 9fdc0ef79202..0bcd85826375 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -2518,8 +2518,8 @@ static int pwrap_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	ret = devm_clk_bulk_get_all_enable(wrp->dev, &clk);
> -	if (ret)
> +	ret = devm_clk_bulk_get_all_enabled(wrp->dev, &clk);
> +	if (ret < 0)
>   		return dev_err_probe(wrp->dev, ret,
>   				     "failed to get clocks\n");
>   
> 

[PATCH 3/4] PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()

[Cristian Ciocaltea]

The helper devm_clk_bulk_get_all_enable() is partially broken and will
be dropped since it missed to return the number of clocks stored in the
clk_bulk_data table referenced by the clks argument.

Use the newly introduced devm_clk_bulk_get_all_enabled() variant
instead, which is consistent with devm_clk_bulk_get_all() in terms of
the returned value:

 > 0 if one or more clocks have been stored
 = 0 if there are no clocks
 < 0 if an error occurred

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/pci/controller/dwc/pci-exynos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index fa45da28a218..056cebd4e0ba 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -300,7 +300,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(ep->elbi_base))
 		return PTR_ERR(ep->elbi_base);
 
-	ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
+	ret = devm_clk_bulk_get_all_enabled(dev, &ep->clks);
 	if (ret < 0)
 		return ret;
 

-- 
2.46.0

[PATCH 4/4] clk: Drop obsolete devm_clk_bulk_get_all_enable() helper

[Cristian Ciocaltea]

Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
clocks") added devm_clk_bulk_get_all_enable() function, but missed to
return the number of clocks stored in the clk_bulk_data table referenced
by the clks argument.  Without knowing the number, it's not possible to
iterate these clocks when needed, hence the argument is useless and
could have been simply removed.

A new helper devm_clk_bulk_get_all_enabled() has been introduced, which
is consistent with devm_clk_bulk_get_all() in terms of the returned
value.

Drop the obsolete function since all users switched to the new helper.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/clk/clk-devres.c | 32 --------------------------------
 include/linux/clk.h      | 22 ----------------------
 2 files changed, 54 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 4203aaaa7544..14a657f336fe 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -218,38 +218,6 @@ static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
 	clk_bulk_put_all(devres->num_clks, devres->clks);
 }
 
-int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
-					      struct clk_bulk_data **clks)
-{
-	struct clk_bulk_devres *devres;
-	int ret;
-
-	devres = devres_alloc(devm_clk_bulk_release_all_enable,
-			      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;
-	} else {
-		devres_free(devres);
-		return ret;
-	}
-
-	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
-	if (!ret) {
-		devres_add(dev, devres);
-	} else {
-		clk_bulk_put_all(devres->num_clks, devres->clks);
-		devres_free(devres);
-	}
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
-
 int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
 					       struct clk_bulk_data **clks)
 {
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 158c5072852e..b607482ca77e 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -495,22 +495,6 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
 int __must_check devm_clk_bulk_get_all(struct device *dev,
 				       struct clk_bulk_data **clks);
 
-/**
- * devm_clk_bulk_get_all_enable - Get and enable all clocks of the consumer (managed)
- * @dev: device for clock "consumer"
- * @clks: pointer to the clk_bulk_data table of consumer
- *
- * Returns success (0) or negative errno.
- *
- * This helper function allows drivers to get all clocks of the
- * consumer and enables them in one operation with management.
- * The clks will automatically be disabled and freed when the device
- * is unbound.
- */
-
-int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
-					      struct clk_bulk_data **clks);
-
 /**
  * devm_clk_bulk_get_all_enabled - Get and enable all clocks of the consumer (managed)
  * @dev: device for clock "consumer"
@@ -1052,12 +1036,6 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
 	return 0;
 }
 
-static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
-						struct clk_bulk_data **clks)
-{
-	return 0;
-}
-
 static inline int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
 						struct clk_bulk_data **clks)
 {

-- 
2.46.0

Re: [PATCH 4/4] clk: Drop obsolete devm_clk_bulk_get_all_enable() helper

[AngeloGioacchino Del Regno]

Il 14/09/24 20:04, Cristian Ciocaltea ha scritto:
> Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
> clocks") added devm_clk_bulk_get_all_enable() function, but missed to
> return the number of clocks stored in the clk_bulk_data table referenced
> by the clks argument.  Without knowing the number, it's not possible to
> iterate these clocks when needed, hence the argument is useless and
> could have been simply removed.
> 
> A new helper devm_clk_bulk_get_all_enabled() has been introduced, which
> is consistent with devm_clk_bulk_get_all() in terms of the returned
> value.
> 
> Drop the obsolete function since all users switched to the new helper.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

This should probably be done in two (three?) cycles if we want to avoid
immutable branches... but anyway, for the commit itself:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   drivers/clk/clk-devres.c | 32 --------------------------------
>   include/linux/clk.h      | 22 ----------------------
>   2 files changed, 54 deletions(-)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 4203aaaa7544..14a657f336fe 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -218,38 +218,6 @@ static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
>   	clk_bulk_put_all(devres->num_clks, devres->clks);
>   }
>   
> -int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> -					      struct clk_bulk_data **clks)
> -{
> -	struct clk_bulk_devres *devres;
> -	int ret;
> -
> -	devres = devres_alloc(devm_clk_bulk_release_all_enable,
> -			      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;
> -	} else {
> -		devres_free(devres);
> -		return ret;
> -	}
> -
> -	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> -	if (!ret) {
> -		devres_add(dev, devres);
> -	} else {
> -		clk_bulk_put_all(devres->num_clks, devres->clks);
> -		devres_free(devres);
> -	}
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> -
>   int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
>   					       struct clk_bulk_data **clks)
>   {
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 158c5072852e..b607482ca77e 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -495,22 +495,6 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
>   int __must_check devm_clk_bulk_get_all(struct device *dev,
>   				       struct clk_bulk_data **clks);
>   
> -/**
> - * devm_clk_bulk_get_all_enable - Get and enable all clocks of the consumer (managed)
> - * @dev: device for clock "consumer"
> - * @clks: pointer to the clk_bulk_data table of consumer
> - *
> - * Returns success (0) or negative errno.
> - *
> - * This helper function allows drivers to get all clocks of the
> - * consumer and enables them in one operation with management.
> - * The clks will automatically be disabled and freed when the device
> - * is unbound.
> - */
> -
> -int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> -					      struct clk_bulk_data **clks);
> -
>   /**
>    * devm_clk_bulk_get_all_enabled - Get and enable all clocks of the consumer (managed)
>    * @dev: device for clock "consumer"
> @@ -1052,12 +1036,6 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
>   	return 0;
>   }
>   
> -static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> -						struct clk_bulk_data **clks)
> -{
> -	return 0;
> -}
> -
>   static inline int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
>   						struct clk_bulk_data **clks)
>   {

Re: [PATCH 0/4] Provide devm_clk_bulk_get_all_enabled() helper

[Manivannan Sadhasivam]

On Sat, Sep 14, 2024 at 09:04:53PM +0300, Cristian Ciocaltea wrote:
> Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
> clocks") added devm_clk_bulk_get_all_enable() function, but missed to
> return the number of clocks stored in the clk_bulk_data table referenced
> by the clks argument.
> 
> That is required in case there is a need to iterate these clocks later,
> therefore I couldn't see any use case of this parameter and should have
> been simply removed from the function declaration.
> 

Is there an user that currerntly does this?

- Mani

> The first patch in the series provides devm_clk_bulk_get_all_enabled()
> variant, which is consistent with devm_clk_bulk_get_all() in terms of
> the returned value:
> 
>  > 0 if one or more clocks have been stored
>  = 0 if there are no clocks
>  < 0 if an error occurred
> 
> Moreover, the naming is consistent with devm_clk_get_enabled(), i.e. use
> the past form of 'enable'.
> 
> The next two patches switch existing users of devm_clk_get_enable() to
> the new helper - there were only two, as of next-20240913.
> 
> The last patch drops the now obsolete devm_clk_bulk_get_all_enable()
> helper.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (4):
>       clk: Provide devm_clk_bulk_get_all_enabled() helper
>       soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()
>       PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()
>       clk: Drop obsolete devm_clk_bulk_get_all_enable() helper
> 
>  drivers/clk/clk-devres.c                | 30 ++++++++++++++++--------------
>  drivers/pci/controller/dwc/pci-exynos.c |  2 +-
>  drivers/soc/mediatek/mtk-pmic-wrap.c    |  4 ++--
>  include/linux/clk.h                     | 12 +++++++-----
>  4 files changed, 26 insertions(+), 22 deletions(-)
> ---
> base-commit: 5acd9952f95fb4b7da6d09a3be39195a80845eb6
> change-id: 20240912-clk_bulk_ena_fix-16ba77358ddf
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/4] Provide devm_clk_bulk_get_all_enabled() helper

[AngeloGioacchino Del Regno]

Il 24/09/24 16:36, Manivannan Sadhasivam ha scritto:
> On Sat, Sep 14, 2024 at 09:04:53PM +0300, Cristian Ciocaltea wrote:
>> Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
>> clocks") added devm_clk_bulk_get_all_enable() function, but missed to
>> return the number of clocks stored in the clk_bulk_data table referenced
>> by the clks argument.
>>
>> That is required in case there is a need to iterate these clocks later,
>> therefore I couldn't see any use case of this parameter and should have
>> been simply removed from the function declaration.
>>
> 
> Is there an user that currerntly does this?
> 

Yes and the patch wasn't sent upstream yet, but anyway, regardless of that,
this series is fixing inconsistency with both naming and usage between the
clock (bulk) API functions, with that being the only function acting
different from the others, at best confusing people.

Cheers,
Angelo

> - Mani
> 
>> The first patch in the series provides devm_clk_bulk_get_all_enabled()
>> variant, which is consistent with devm_clk_bulk_get_all() in terms of
>> the returned value:
>>
>>   > 0 if one or more clocks have been stored
>>   = 0 if there are no clocks
>>   < 0 if an error occurred
>>
>> Moreover, the naming is consistent with devm_clk_get_enabled(), i.e. use
>> the past form of 'enable'.
>>
>> The next two patches switch existing users of devm_clk_get_enable() to
>> the new helper - there were only two, as of next-20240913.
>>
>> The last patch drops the now obsolete devm_clk_bulk_get_all_enable()
>> helper.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>> Cristian Ciocaltea (4):
>>        clk: Provide devm_clk_bulk_get_all_enabled() helper
>>        soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()
>>        PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()
>>        clk: Drop obsolete devm_clk_bulk_get_all_enable() helper
>>
>>   drivers/clk/clk-devres.c                | 30 ++++++++++++++++--------------
>>   drivers/pci/controller/dwc/pci-exynos.c |  2 +-
>>   drivers/soc/mediatek/mtk-pmic-wrap.c    |  4 ++--
>>   include/linux/clk.h                     | 12 +++++++-----
>>   4 files changed, 26 insertions(+), 22 deletions(-)
>> ---
>> base-commit: 5acd9952f95fb4b7da6d09a3be39195a80845eb6
>> change-id: 20240912-clk_bulk_ena_fix-16ba77358ddf
>>
> 

Re: [PATCH 0/4] Provide devm_clk_bulk_get_all_enabled() helper

[Manivannan Sadhasivam]

On Wed, Sep 25, 2024 at 09:52:44AM +0200, AngeloGioacchino Del Regno wrote:
> Il 24/09/24 16:36, Manivannan Sadhasivam ha scritto:
> > On Sat, Sep 14, 2024 at 09:04:53PM +0300, Cristian Ciocaltea wrote:
> > > Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
> > > clocks") added devm_clk_bulk_get_all_enable() function, but missed to
> > > return the number of clocks stored in the clk_bulk_data table referenced
> > > by the clks argument.
> > > 
> > > That is required in case there is a need to iterate these clocks later,
> > > therefore I couldn't see any use case of this parameter and should have
> > > been simply removed from the function declaration.
> > > 
> > 
> > Is there an user that currerntly does this?
> > 
> 
> Yes and the patch wasn't sent upstream yet, but anyway, regardless of that,
> this series is fixing inconsistency with both naming and usage between the
> clock (bulk) API functions, with that being the only function acting
> different from the others, at best confusing people.
> 

I agree with the 'inconsistency' part of the API, but I wouldn't call it as
*broken* as this series does. Please fix that and you can have my:

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Cheers,
> Angelo
> 
> > - Mani
> > 
> > > The first patch in the series provides devm_clk_bulk_get_all_enabled()
> > > variant, which is consistent with devm_clk_bulk_get_all() in terms of
> > > the returned value:
> > > 
> > >   > 0 if one or more clocks have been stored
> > >   = 0 if there are no clocks
> > >   < 0 if an error occurred
> > > 
> > > Moreover, the naming is consistent with devm_clk_get_enabled(), i.e. use
> > > the past form of 'enable'.
> > > 
> > > The next two patches switch existing users of devm_clk_get_enable() to
> > > the new helper - there were only two, as of next-20240913.
> > > 
> > > The last patch drops the now obsolete devm_clk_bulk_get_all_enable()
> > > helper.
> > > 
> > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > > ---
> > > Cristian Ciocaltea (4):
> > >        clk: Provide devm_clk_bulk_get_all_enabled() helper
> > >        soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()
> > >        PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()
> > >        clk: Drop obsolete devm_clk_bulk_get_all_enable() helper
> > > 
> > >   drivers/clk/clk-devres.c                | 30 ++++++++++++++++--------------
> > >   drivers/pci/controller/dwc/pci-exynos.c |  2 +-
> > >   drivers/soc/mediatek/mtk-pmic-wrap.c    |  4 ++--
> > >   include/linux/clk.h                     | 12 +++++++-----
> > >   4 files changed, 26 insertions(+), 22 deletions(-)
> > > ---
> > > base-commit: 5acd9952f95fb4b7da6d09a3be39195a80845eb6
> > > change-id: 20240912-clk_bulk_ena_fix-16ba77358ddf
> > > 
> > 
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/4] Provide devm_clk_bulk_get_all_enabled() helper

[Cristian Ciocaltea]

On 9/25/24 10:56 AM, Manivannan Sadhasivam wrote:
> On Wed, Sep 25, 2024 at 09:52:44AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 24/09/24 16:36, Manivannan Sadhasivam ha scritto:
>>> On Sat, Sep 14, 2024 at 09:04:53PM +0300, Cristian Ciocaltea wrote:
>>>> Commit 265b07df758a ("clk: Provide managed helper to get and enable bulk
>>>> clocks") added devm_clk_bulk_get_all_enable() function, but missed to
>>>> return the number of clocks stored in the clk_bulk_data table referenced
>>>> by the clks argument.
>>>>
>>>> That is required in case there is a need to iterate these clocks later,
>>>> therefore I couldn't see any use case of this parameter and should have
>>>> been simply removed from the function declaration.
>>>>
>>>
>>> Is there an user that currerntly does this?
>>>
>>
>> Yes and the patch wasn't sent upstream yet, but anyway, regardless of that,
>> this series is fixing inconsistency with both naming and usage between the
>> clock (bulk) API functions, with that being the only function acting
>> different from the others, at best confusing people.
>>
> 
> I agree with the 'inconsistency' part of the API, but I wouldn't call it as
> *broken* as this series does. Please fix that and you can have my:
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks Angelo and Mani for reviewing this.

I've submitted v2:

https://lore.kernel.org/lkml/20240926-clk_bulk_ena_fix-v2-0-9c767510fbb5@collabora.com/

Regards,
Cristian