diff mbox series

[v3,3/4] i2c: jz4780: Use devm_clk_get_enabled() helpers

Message ID 20240823035116.21590-4-rongqianfeng@vivo.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series i2c: Use devm_clk_get_enabled() helpers | expand

Commit Message

Rong Qianfeng Aug. 23, 2024, 3:51 a.m. UTC
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(-)

Comments

Andy Shevchenko Aug. 23, 2024, 3:33 p.m. UTC | #1
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)

>  	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;
Rong Qianfeng Aug. 26, 2024, 3:03 a.m. UTC | #2
在 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
>
>
Andy Shevchenko Aug. 26, 2024, 10:33 a.m. UTC | #3
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.
Rong Qianfeng Aug. 26, 2024, 11:56 a.m. UTC | #4
在 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 mbox series

Patch

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