Hi Gaosheng
On Sat, Aug 03, 2024 at 02:52:31PM +0800, Gaosheng Cui wrote:
> Add the missing clk_disable_unprepare() before return in
> bt1_apb_request_clk().
>
> Signed-off-by: Gaosheng Cui <cuigaosheng1@xxxxxxxxxx>
> ---
> drivers/bus/bt1-apb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/bus/bt1-apb.c b/drivers/bus/bt1-apb.c
> index 595fb22b73e0..244f03988545 100644
> --- a/drivers/bus/bt1-apb.c
> +++ b/drivers/bus/bt1-apb.c
> @@ -210,12 +210,14 @@ static int bt1_apb_request_clk(struct bt1_apb *apb)
Thanks for the patch, but it's fully redundant.
> ret = devm_add_action_or_reset(apb->dev, bt1_apb_disable_clk, apb);
Please see the __devm_add_action_or_reset() method semantics. It
executes the passed "action" in case if the add-action procedure
fails.
> if (ret) {
> dev_err(apb->dev, "Can't add APB EHB clocks disable action\n");
> + clk_disable_unprepare(apb->pclk);
> return ret;
> }
>
> apb->rate = clk_get_rate(apb->pclk);
> if (!apb->rate) {
> dev_err(apb->dev, "Invalid clock rate\n");
> + clk_disable_unprepare(apb->pclk);
If the rate getting fails for some reason, then the action registered above
will be called in the framework of all the device-managed cleanups
executed for the probe() method.
So to speak there is no need in the change suggested by you here.
But what could be done is the devm_clk_get(), clk_prepare_enable() and
devm_add_action_or_reset() calls replacement with a single
devm_clk_get_enabled() invocation. That simplification will also cause
the bt1_apb_disable_clk() method removal.
A similar change can be applied to the bt1_axi_request_clk() method
in the drivers/bus/bt1-axi.c driver.
-Serge(y)
> return -EINVAL;
> }
>
> --
> 2.25.1
>
