| Message ID | 20240822025258.53263-3-rongqianfeng@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Use devm_clk_get_enabled() helpers | expand |
Hi Rong, Le jeudi 22 août 2024 à 10:52 +0800, Rong Qianfeng a écrit : > The devm_clk_get_enabled() helpers: > - call devm_clk_get() > - call clk_prepare_enable() and register what is needed in order > to > call clk_disable_unprepare() when needed, as a managed resource. > > This simplifies the code and avoids the calls to > clk_disable_unprepare(). > > While at it, remove the goto label "err:", and use its return value > to > return the error code. > > Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com> Acked-by: Paul Cercueil <paul@crapouillou.net> With a small suggestion below. > --- > drivers/i2c/busses/i2c-jz4780.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-jz4780.c > b/drivers/i2c/busses/i2c-jz4780.c > index 4aafdfab6305..f5362c5dfb50 100644 > --- a/drivers/i2c/busses/i2c-jz4780.c > +++ b/drivers/i2c/busses/i2c-jz4780.c > @@ -792,26 +792,22 @@ static int jz4780_i2c_probe(struct > platform_device *pdev) > > platform_set_drvdata(pdev, i2c); > > - i2c->clk = devm_clk_get(&pdev->dev, NULL); > + i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL); > if (IS_ERR(i2c->clk)) > return PTR_ERR(i2c->clk); > > - ret = clk_prepare_enable(i2c->clk); > - if (ret) > - return ret; > - > ret = of_property_read_u32(pdev->dev.of_node, "clock- > frequency", > &clk_freq); > if (ret) { > dev_err(&pdev->dev, "clock-frequency not specified > in DT\n"); > - goto err; > + return ret; > } Since there is no more special handling needed here, you could just: if (ret) return dev_err_probe(&pdev->dev, ret, "clock-frequency...\n"); And the same for the other error handling paths that you changed. I wouldn't request a V2 just for that though. Cheers, -Paul > > i2c->speed = clk_freq / 1000; > if (i2c->speed == 0) { > ret = -EINVAL; > dev_err(&pdev->dev, "clock-frequency minimum is > 1000\n"); > - goto err; > + return ret; > } > jz4780_i2c_set_speed(i2c); > > @@ -827,29 +823,24 @@ static int jz4780_i2c_probe(struct > platform_device *pdev) > > ret = platform_get_irq(pdev, 0); > if (ret < 0) > - goto err; > + return ret; > i2c->irq = ret; > ret = devm_request_irq(&pdev->dev, i2c->irq, jz4780_i2c_irq, > 0, > dev_name(&pdev->dev), i2c); > if (ret) > - goto err; > + return ret; > > ret = i2c_add_adapter(&i2c->adap); > if (ret < 0) > - goto err; > + return ret; > > return 0; > - > -err: > - clk_disable_unprepare(i2c->clk); > - return ret; > } > > static void jz4780_i2c_remove(struct platform_device *pdev) > { > struct jz4780_i2c *i2c = platform_get_drvdata(pdev); > > - clk_disable_unprepare(i2c->clk); > i2c_del_adapter(&i2c->adap); > } >
在 2024/8/22 17:35, Paul Cercueil 写道: > Hi Rong, > > Le jeudi 22 août 2024 à 10:52 +0800, Rong Qianfeng a écrit : >> The devm_clk_get_enabled() helpers: >> - call devm_clk_get() >> - call clk_prepare_enable() and register what is needed in order >> to >> call clk_disable_unprepare() when needed, as a managed resource. >> >> This simplifies the code and avoids the calls to >> clk_disable_unprepare(). >> >> While at it, remove the goto label "err:", and use its return value >> to >> return the error code. >> >> Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com> > Acked-by: Paul Cercueil <paul@crapouillou.net> > > With a small suggestion below. > >> --- >> drivers/i2c/busses/i2c-jz4780.c | 21 ++++++--------------- >> 1 file changed, 6 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-jz4780.c >> b/drivers/i2c/busses/i2c-jz4780.c >> index 4aafdfab6305..f5362c5dfb50 100644 >> --- a/drivers/i2c/busses/i2c-jz4780.c >> +++ b/drivers/i2c/busses/i2c-jz4780.c >> @@ -792,26 +792,22 @@ static int jz4780_i2c_probe(struct >> platform_device *pdev) >> >> platform_set_drvdata(pdev, i2c); >> >> - i2c->clk = devm_clk_get(&pdev->dev, NULL); >> + i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL); >> if (IS_ERR(i2c->clk)) >> return PTR_ERR(i2c->clk); >> >> - ret = clk_prepare_enable(i2c->clk); >> - if (ret) >> - return ret; >> - >> ret = of_property_read_u32(pdev->dev.of_node, "clock- >> frequency", >> &clk_freq); >> if (ret) { >> dev_err(&pdev->dev, "clock-frequency not specified >> in DT\n"); >> - goto err; >> + return ret; >> } > Since there is no more special handling needed here, you could just: > if (ret) > return dev_err_probe(&pdev->dev, ret, "clock-frequency...\n"); > > And the same for the other error handling paths that you changed. > > I wouldn't request a V2 just for that though. Thanks for taking the time Paul,highly appreciated. Cheers, -Qianfeng > > Cheers, > -Paul > >> >> i2c->speed = clk_freq / 1000; >> if (i2c->speed == 0) { >> ret = -EINVAL; >> dev_err(&pdev->dev, "clock-frequency minimum is >> 1000\n"); >> - goto err; >> + return ret; >> } >> jz4780_i2c_set_speed(i2c); >> >> @@ -827,29 +823,24 @@ static int jz4780_i2c_probe(struct >> platform_device *pdev) >> >> ret = platform_get_irq(pdev, 0); >> if (ret < 0) >> - goto err; >> + return ret; >> i2c->irq = ret; >> ret = devm_request_irq(&pdev->dev, i2c->irq, jz4780_i2c_irq, >> 0, >> dev_name(&pdev->dev), i2c); >> if (ret) >> - goto err; >> + return ret; >> >> ret = i2c_add_adapter(&i2c->adap); >> if (ret < 0) >> - goto err; >> + return ret; >> >> return 0; >> - >> -err: >> - clk_disable_unprepare(i2c->clk); >> - return ret; >> } >> >> static void jz4780_i2c_remove(struct platform_device *pdev) >> { >> struct jz4780_i2c *i2c = platform_get_drvdata(pdev); >> >> - clk_disable_unprepare(i2c->clk); >> i2c_del_adapter(&i2c->adap); >> } >>
diff --git a/drivers/i2c/busses/i2c-jz4780.c b/drivers/i2c/busses/i2c-jz4780.c index 4aafdfab6305..f5362c5dfb50 100644 --- a/drivers/i2c/busses/i2c-jz4780.c +++ b/drivers/i2c/busses/i2c-jz4780.c @@ -792,26 +792,22 @@ static int jz4780_i2c_probe(struct platform_device *pdev) platform_set_drvdata(pdev, i2c); - i2c->clk = devm_clk_get(&pdev->dev, NULL); + i2c->clk = devm_clk_get_enabled(&pdev->dev, NULL); if (IS_ERR(i2c->clk)) return PTR_ERR(i2c->clk); - ret = clk_prepare_enable(i2c->clk); - if (ret) - return ret; - ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &clk_freq); if (ret) { dev_err(&pdev->dev, "clock-frequency not specified in DT\n"); - goto err; + return ret; } i2c->speed = clk_freq / 1000; if (i2c->speed == 0) { ret = -EINVAL; dev_err(&pdev->dev, "clock-frequency minimum is 1000\n"); - goto err; + return ret; } jz4780_i2c_set_speed(i2c); @@ -827,29 +823,24 @@ static int jz4780_i2c_probe(struct platform_device *pdev) ret = platform_get_irq(pdev, 0); if (ret < 0) - goto err; + return ret; i2c->irq = ret; ret = devm_request_irq(&pdev->dev, i2c->irq, jz4780_i2c_irq, 0, dev_name(&pdev->dev), i2c); if (ret) - goto err; + return ret; ret = i2c_add_adapter(&i2c->adap); if (ret < 0) - goto err; + return ret; return 0; - -err: - clk_disable_unprepare(i2c->clk); - return ret; } static void jz4780_i2c_remove(struct platform_device *pdev) { struct jz4780_i2c *i2c = platform_get_drvdata(pdev); - clk_disable_unprepare(i2c->clk); i2c_del_adapter(&i2c->adap); }
The devm_clk_get_enabled() helpers: - call devm_clk_get() - call clk_prepare_enable() and register what is needed in order to call clk_disable_unprepare() when needed, as a managed resource. This simplifies the code and avoids the calls to clk_disable_unprepare(). While at it, remove the goto label "err:", and use its return value to return the error code. Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com> --- drivers/i2c/busses/i2c-jz4780.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)