| Message ID | 20240823035116.21590-4-rongqianfeng@vivo.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: Use devm_clk_get_enabled() helpers | expand |
在 2024/8/23 23:33, Andy Shevchenko 写道: > [Some people who received this message don't often get email from andriy.shevchenko@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Fri, Aug 23, 2024 at 11:51:15AM +0800, Rong Qianfeng wrote: >> 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, no more special handling needed here, remove the goto >> label "err:". > ... > >> ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", >> &clk_freq); > (side note: this driver should use i2c_timings and respective I2C core > APIs instead of this) Sorry, I didn't fully understand what you meant, it's my problem. I guess you are suggesting to use an API like i2c_parse_fw_timings() to get the clock-frequency? Best Regards, Qianfeng > >> if (ret) { >> dev_err(&pdev->dev, "clock-frequency not specified in DT\n"); >> - goto err; >> + return ret; > While at it, > > return dev_err_probe(...); > >> } >> i2c->speed = clk_freq / 1000; > (side note: this should be HZ_PER_KHZ from units.h) > >> if (i2c->speed == 0) { >> ret = -EINVAL; >> dev_err(&pdev->dev, "clock-frequency minimum is 1000\n"); >> - goto err; >> + return ret; > return dev_err_probe(...); > >> } > ... > >> ret = platform_get_irq(pdev, 0); >> if (ret < 0) >> - goto err; >> + return ret; >> i2c->irq = ret; > I would add a blank line here. > >> ret = devm_request_irq(&pdev->dev, i2c->irq, jz4780_i2c_irq, 0, >> dev_name(&pdev->dev), i2c); >> if (ret) >> - goto err; >> + return ret; > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Aug 26, 2024 at 11:03:20AM +0800, Rong Qianfeng wrote: > 在 2024/8/23 23:33, Andy Shevchenko 写道: > > On Fri, Aug 23, 2024 at 11:51:15AM +0800, Rong Qianfeng wrote: ... > > > ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", > > > &clk_freq); > > (side note: this driver should use i2c_timings and respective I2C core > > APIs instead of this) > Sorry, I didn't fully understand what you meant, it's my problem. I guess > you are suggesting to use an API like i2c_parse_fw_timings() to get the > clock-frequency? Yes.
在 2024/8/26 18:33, Andy Shevchenko 写道: > On Mon, Aug 26, 2024 at 11:03:20AM +0800, Rong Qianfeng wrote: >> 在 2024/8/23 23:33, Andy Shevchenko 写道: >>> On Fri, Aug 23, 2024 at 11:51:15AM +0800, Rong Qianfeng wrote: > ... > >>>> ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", >>>> &clk_freq); >>> (side note: this driver should use i2c_timings and respective I2C core >>> APIs instead of this) >> Sorry, I didn't fully understand what you meant, it's my problem. I guess >> you are suggesting to use an API like i2c_parse_fw_timings() to get the >> clock-frequency? > Yes. Very good point, Thanks for letting me know about these advanced APIs. I think we may need some other patch series to discuss how to implement it, because there are many places in the i2c that need such modifications, and these modifications are not suitable in the current patch series. >
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, no more special handling needed here, remove the goto label "err:". Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com> Acked-by: Paul Cercueil <paul@crapouillou.net> --- drivers/i2c/busses/i2c-jz4780.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)