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

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'.

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/clk/clk-devres.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 24 ++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

Quoting Cristian Ciocaltea (2024-09-26 03:43:20)
> 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'.
> 
> 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/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,

I'm not super excited about the one letter difference in the function
name as that's likely to lead to confusion for someone.

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

Instead of duplicating can you make devm_clk_bulk_get_all_enable() use
the devm_clk_bulk_get_all_enabled() function but not return the number
of clks all in this patch? It will make the diff much more readable that
way.

On 10/17/24 10:45 PM, Stephen Boyd wrote:
> Quoting Cristian Ciocaltea (2024-09-26 03:43:20)
>> 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'.

[...]

> Instead of duplicating can you make devm_clk_bulk_get_all_enable() use
> the devm_clk_bulk_get_all_enabled() function but not return the number
> of clks all in this patch? It will make the diff much more readable that
> way.

Done in v3 [1].

Thanks for reviewing,
Cristian

[1] https://lore.kernel.org/all/20241018-clk_bulk_ena_fix-v3-0-57e8bb82460c@collabora.com/