[PATCH v5 0/3] Provide devm_clk_bulk_get_all_enabled() helper

[PATCH v5 0/3] 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>
---
Changes in v5:
- Rebased series onto next-20241217
- Removed the patch introducing the new helper - merged in v6.13-rc1
- Updated last patch to fix a rebase conflict
- Link to v4: https://lore.kernel.org/r/20241019-clk_bulk_ena_fix-v4-0-57f108f64e70@collabora.com

Changes in v4:
- Dropped usage of gotos in the new helper implementation to further
  minimize the diff (Stephen)
- Link to v3: https://lore.kernel.org/r/20241018-clk_bulk_ena_fix-v3-0-57e8bb82460c@collabora.com

Changes in v3:
- Made devm_clk_bulk_get_all_enable() use the new helper, as suggested
  by Stephen to improve diff readability
- Rebased series onto next-20241017
- Link to v2: https://lore.kernel.org/r/20240926-clk_bulk_ena_fix-v2-0-9c767510fbb5@collabora.com

Changes in v2:
- Dropped references to 'broken' API in commit descriptions, per Mani's
  suggestion
- Added R-b tags from Angelo and Mani
- Link to v1: https://lore.kernel.org/r/20240914-clk_bulk_ena_fix-v1-0-ce3537585c06@collabora.com

---
Cristian Ciocaltea (3):
      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/pci/controller/dwc/pci-exynos.c | 2 +-
 drivers/soc/mediatek/mtk-pmic-wrap.c    | 4 ++--
 include/linux/clk.h                     | 9 ---------
 3 files changed, 3 insertions(+), 12 deletions(-)
---
base-commit: fdb298fa865b0136f7be842e6c2e6310dede421a
change-id: 20240912-clk_bulk_ena_fix-16ba77358ddf

[PATCH v5 1/3] soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()

[Cristian Ciocaltea]

The helper devm_clk_bulk_get_all_enable() missed to return the number of
clocks stored in the clk_bulk_data table referenced by the clks
argument and, therefore, will be dropped.

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

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
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 9fdc0ef79202618d0bd0188d0bf53152285c6c51..0bcd8582637550c90c1c6df619077b7df7bb0048 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.47.0

Re: [PATCH v5 1/3] soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()

[AngeloGioacchino Del Regno]

Il 17/12/24 22:41, Cristian Ciocaltea ha scritto:
> The helper devm_clk_bulk_get_all_enable() missed to return the number of
> clocks stored in the clk_bulk_data table referenced by the clks
> argument and, therefore, will be dropped.
> 
> 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
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>


Stephen, sorry for missing the discussion about needing an ack on older versions -
Cristian just pointed that out to me.

Anyway, if you want to take this one with patch [3/3] I'm fine with that, so

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


Cheers!

Re: [PATCH v5 1/3] soc: mediatek: pwrap: Switch to devm_clk_bulk_get_all_enabled()

[Stephen Boyd]

Quoting Cristian Ciocaltea (2024-12-17 13:41:51)
> The helper devm_clk_bulk_get_all_enable() missed to return the number of
> clocks stored in the clk_bulk_data table referenced by the clks
> argument and, therefore, will be dropped.
> 
> 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
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

Applied to clk-next

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

[Cristian Ciocaltea]

The helper devm_clk_bulk_get_all_enable() missed to return the number of
clocks stored in the clk_bulk_data table referenced by the clks
argument and, therefore, will be dropped.

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

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
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 6a830166d37fff5bbbc35aaa7cc1b67b03e6ec3b..ace736b025b1b7d4cdcbd51d2e9d99af29f21149 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.47.0

Re: [PATCH v5 2/3] PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()

[Cristian Ciocaltea]

Hi Bjorn,

On 12/17/24 11:41 PM, Cristian Ciocaltea wrote:
> The helper devm_clk_bulk_get_all_enable() missed to return the number of
> clocks stored in the clk_bulk_data table referenced by the clks
> argument and, therefore, will be dropped.
> 
> 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
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

In case you missed the previous requests, we need your ack on this patch
so that Stephen can apply the entire series to the clk tree and drop the
obsolete helper.

Thanks,
Cristian

Re: [PATCH v5 2/3] PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()

[Krzysztof Wilczyński]

Hello,

> > The helper devm_clk_bulk_get_all_enable() missed to return the number of
> > clocks stored in the clk_bulk_data table referenced by the clks
> > argument and, therefore, will be dropped.
> > 
> > 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
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> 
> In case you missed the previous requests, we need your ack on this patch
> so that Stephen can apply the entire series to the clk tree and drop the
> obsolete helper.

Please, take the following:

  Acked-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Should be sufficient.

Also, sorry for keeping you both waiting.

	Krzysztof

Re: [PATCH v5 2/3] PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()

[Cristian Ciocaltea]

On 12/23/24 4:18 PM, Krzysztof Wilczyński wrote:
> Hello,
> 
>>> The helper devm_clk_bulk_get_all_enable() missed to return the number of
>>> clocks stored in the clk_bulk_data table referenced by the clks
>>> argument and, therefore, will be dropped.
>>>
>>> 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
>>>
>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>
>> In case you missed the previous requests, we need your ack on this patch
>> so that Stephen can apply the entire series to the clk tree and drop the
>> obsolete helper.
> 
> Please, take the following:
> 
>   Acked-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
> 
> Should be sufficient.
> 
> Also, sorry for keeping you both waiting.

Thanks Krzysztof, no worries!

Regards,
Cristian

Re: [PATCH v5 2/3] PCI: exynos: Switch to devm_clk_bulk_get_all_enabled()

[Stephen Boyd]

Quoting Cristian Ciocaltea (2024-12-17 13:41:52)
> The helper devm_clk_bulk_get_all_enable() missed to return the number of
> clocks stored in the clk_bulk_data table referenced by the clks
> argument and, therefore, will be dropped.
> 
> 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
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

Applied to clk-next

[PATCH v5 3/3] 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.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 include/linux/clk.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1dcee6d701e439f35effd86ca3ee3b47a9839bd0..b607482ca77e987b9344c38f25ebb5c8d35c1d39 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -1138,15 +1138,6 @@ static inline void clk_restore_context(void) {}
 
 #endif
 
-/* Deprecated. Use devm_clk_bulk_get_all_enabled() */
-static inline int __must_check
-devm_clk_bulk_get_all_enable(struct device *dev, struct clk_bulk_data **clks)
-{
-	int ret = devm_clk_bulk_get_all_enabled(dev, clks);
-
-	return ret > 0 ? 0 : ret;
-}
-
 /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
 static inline int clk_prepare_enable(struct clk *clk)
 {

-- 
2.47.0

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

[Stephen Boyd]

Quoting Cristian Ciocaltea (2024-12-17 13:41:53)
> 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.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

Applied to clk-next

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

[Cristian Ciocaltea]

On 12/17/24 11:41 PM, 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.
> 
> 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>
> ---
> Changes in v5:
> - Rebased series onto next-20241217
> - Removed the patch introducing the new helper - merged in v6.13-rc1
> - Updated last patch to fix a rebase conflict
> - Link to v4: https://lore.kernel.org/r/20241019-clk_bulk_ena_fix-v4-0-57f108f64e70@collabora.com

@Angelo, Matthias, Bjorn:

Could you please provide an ack for the first two patches so that
Stephen can apply them to the clk tree along with the last one removing
the obsolete helper - there are no other users of it as of next-20241217.

Thanks,
Cristian