| Message ID | 20241019-clk_bulk_ena_fix-v4-1-57f108f64e70@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Provide devm_clk_bulk_get_all_enabled() helper | expand |
Quoting Cristian Ciocaltea (2024-10-19 04:16:00) > 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'. > > To reduce code duplication and improve patch readability, make > devm_clk_bulk_get_all_enable() use the new helper, as suggested by > Stephen Boyd. > > 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> > --- I have minimized the diff further. Applied to clk-next (clk-devm). If the other patches can be merged through clk tree I'll need the PCI maintainers to ack, likely Bjorn? diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index 975fac29b27f..5368d92d9b39 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -218,15 +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) -{ - int ret = devm_clk_bulk_get_all_enabled(dev, clks); - - return ret > 0 ? 0 : 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) { @@ -239,23 +230,23 @@ int __must_check devm_clk_bulk_get_all_enabled(struct device *dev, return -ENOMEM; ret = clk_bulk_get_all(dev, &devres->clks); - if (ret <= 0) { + if (ret > 0) { + *clks = devres->clks; + devres->num_clks = ret; + } else { devres_free(devres); return ret; } - *clks = devres->clks; - devres->num_clks = ret; - ret = clk_bulk_prepare_enable(devres->num_clks, *clks); - if (ret) { + if (!ret) { + devres_add(dev, devres); + } else { clk_bulk_put_all(devres->num_clks, devres->clks); devres_free(devres); return ret; } - devres_add(dev, devres); - return devres->num_clks; } EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled); diff --git a/include/linux/clk.h b/include/linux/clk.h index 158c5072852e..1dcee6d701e4 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) { @@ -1160,6 +1138,15 @@ 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) {
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index 82ae1f26e634572b943d18b8d86267f0a69911a6..975fac29b27f3709e292531a114975c9108287db 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -220,6 +220,15 @@ static void devm_clk_bulk_release_all_enable(struct device *dev, void *res) 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; +} +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; @@ -230,25 +239,26 @@ int __must_check devm_clk_bulk_get_all_enable(struct device *dev, return -ENOMEM; ret = clk_bulk_get_all(dev, &devres->clks); - if (ret > 0) { - *clks = devres->clks; - devres->num_clks = ret; - } else { + if (ret <= 0) { devres_free(devres); return ret; } + *clks = devres->clks; + devres->num_clks = ret; + ret = clk_bulk_prepare_enable(devres->num_clks, *clks); - if (!ret) { - devres_add(dev, devres); - } else { + if (ret) { clk_bulk_put_all(devres->num_clks, devres->clks); devres_free(devres); + return ret; } - return ret; + devres_add(dev, devres); + + return devres->num_clks; } -EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable); +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled); static int devm_clk_match(struct device *dev, void *res, void *data) { diff --git a/include/linux/clk.h b/include/linux/clk.h index 851a0f2cf42c8c1bbada49d991bc185587942155..158c5072852e36c1583dc47ca7516fcdd928fe59 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) {
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'. To reduce code duplication and improve patch readability, make devm_clk_bulk_get_all_enable() use the new helper, as suggested by Stephen Boyd. 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 | 28 +++++++++++++++++++--------- include/linux/clk.h | 24 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 9 deletions(-)