[PATCH v3 0/4] i2c: Use devm_clk_get_enabled() helpers

[PATCH v3 0/4] i2c: Use devm_clk_get_enabled() helpers

[Rong Qianfeng]

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 need to save clk pointer, drop sclk from struct
em_i2c_device.

-v3:
 *Add another patch to use dev_err_probe() in jz4780_i2c_probe().

-v2:
 *Add another patch to drop sclk from struct em_i2c_device.

Rong Qianfeng (4):
  i2c: emev2: Use devm_clk_get_enabled() helpers
  i2c: emev2: drop sclk from struct em_i2c_device
  i2c: jz4780: Use devm_clk_get_enabled() helpers
  i2c: jz4780: Use dev_err_probe()

 drivers/i2c/busses/i2c-emev2.c  | 23 +++++++----------------
 drivers/i2c/busses/i2c-jz4780.c | 33 +++++++++++----------------------
 2 files changed, 18 insertions(+), 38 deletions(-)

-- 
2.39.0

[PATCH v3 1/4] i2c: emev2: Use devm_clk_get_enabled() helpers

[Rong Qianfeng]

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().

Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com>
---
 drivers/i2c/busses/i2c-emev2.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 557409410445..20efe0b0cb85 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -373,14 +373,10 @@ static int em_i2c_probe(struct platform_device *pdev)
 
 	strscpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
 
-	priv->sclk = devm_clk_get(&pdev->dev, "sclk");
+	priv->sclk = devm_clk_get_enabled(&pdev->dev, "sclk");
 	if (IS_ERR(priv->sclk))
 		return PTR_ERR(priv->sclk);
 
-	ret = clk_prepare_enable(priv->sclk);
-	if (ret)
-		return ret;
-
 	priv->adap.timeout = msecs_to_jiffies(100);
 	priv->adap.retries = 5;
 	priv->adap.dev.parent = &pdev->dev;
@@ -397,26 +393,22 @@ static int em_i2c_probe(struct platform_device *pdev)
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_clk;
+		return ret;
 	priv->irq = ret;
 	ret = devm_request_irq(&pdev->dev, priv->irq, em_i2c_irq_handler, 0,
 				"em_i2c", priv);
 	if (ret)
-		goto err_clk;
+		return ret;
 
 	ret = i2c_add_adapter(&priv->adap);
 
 	if (ret)
-		goto err_clk;
+		return ret;
 
 	dev_info(&pdev->dev, "Added i2c controller %d, irq %d\n", priv->adap.nr,
 		 priv->irq);
 
 	return 0;
-
-err_clk:
-	clk_disable_unprepare(priv->sclk);
-	return ret;
 }
 
 static void em_i2c_remove(struct platform_device *dev)
@@ -424,7 +416,6 @@ static void em_i2c_remove(struct platform_device *dev)
 	struct em_i2c_device *priv = platform_get_drvdata(dev);
 
 	i2c_del_adapter(&priv->adap);
-	clk_disable_unprepare(priv->sclk);
 }
 
 static const struct of_device_id em_i2c_ids[] = {
-- 
2.39.0

Re: [PATCH v3 1/4] i2c: emev2: Use devm_clk_get_enabled() helpers

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:51:13AM +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().

...

>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
> -		goto err_clk;
> +		return ret;
>  	priv->irq = ret;

(1)

>  	ret = devm_request_irq(&pdev->dev, priv->irq, em_i2c_irq_handler, 0,
>  				"em_i2c", priv);
>  	if (ret)
> -		goto err_clk;
> +		return ret;
>  

>  	ret = i2c_add_adapter(&priv->adap);

>  

While at it, you may move this blank line to (1) above.

>  	if (ret)
> -		goto err_clk;
> +		return ret;


-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/4] i2c: emev2: drop sclk from struct em_i2c_device

[Rong Qianfeng]

For no need to save clk pointer, drop sclk from struct em_i2c_device.

Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/busses/i2c-emev2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 20efe0b0cb85..2a5d9d658246 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -67,7 +67,6 @@ struct em_i2c_device {
 	void __iomem *base;
 	struct i2c_adapter adap;
 	struct completion msg_done;
-	struct clk *sclk;
 	struct i2c_client *slave;
 	int irq;
 };
@@ -361,6 +360,7 @@ static const struct i2c_algorithm em_i2c_algo = {
 static int em_i2c_probe(struct platform_device *pdev)
 {
 	struct em_i2c_device *priv;
+	struct clk *sclk;
 	int ret;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -373,9 +373,9 @@ static int em_i2c_probe(struct platform_device *pdev)
 
 	strscpy(priv->adap.name, "EMEV2 I2C", sizeof(priv->adap.name));
 
-	priv->sclk = devm_clk_get_enabled(&pdev->dev, "sclk");
-	if (IS_ERR(priv->sclk))
-		return PTR_ERR(priv->sclk);
+	sclk = devm_clk_get_enabled(&pdev->dev, "sclk");
+	if (IS_ERR(sclk))
+		return PTR_ERR(sclk);
 
 	priv->adap.timeout = msecs_to_jiffies(100);
 	priv->adap.retries = 5;
-- 
2.39.0

Re: [PATCH v3 2/4] i2c: emev2: drop sclk from struct em_i2c_device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]

On Fri, Aug 23, 2024 at 11:51:14AM +0800, Rong Qianfeng wrote:
> For no need to save clk pointer, drop sclk from struct em_i2c_device.
> 
> Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Should be folded into patch 1 IMO.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] i2c: emev2: drop sclk from struct em_i2c_device

[Rong Qianfeng]

在 2024/8/26 17:09, Wolfram Sang 写道:
> On Fri, Aug 23, 2024 at 11:51:14AM +0800, Rong Qianfeng wrote:
>> For no need to save clk pointer, drop sclk from struct em_i2c_device.
>>
>> Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com>
>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Should be folded into patch 1 IMO.
Thanks for taking the time to reply.  Your comments can help me learn more.
I will try to do this in the next version.

---

Best Regards,
Qianfeng

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

[Rong Qianfeng]

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(-)

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

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

[Andy Shevchenko]

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;


-- 
With Best Regards,
Andy Shevchenko

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

[Rong Qianfeng]

在 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
>
>

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

[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.

-- 
With Best Regards,
Andy Shevchenko

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

[Rong Qianfeng]

在 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.
>

[PATCH v3 4/4] i2c: jz4780: Use dev_err_probe()

[Rong Qianfeng]

No more special handling needed here, so use dev_err_probe()
to simplify the code.

Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com>
---
 drivers/i2c/busses/i2c-jz4780.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-jz4780.c b/drivers/i2c/busses/i2c-jz4780.c
index f5362c5dfb50..0cb52a6d05b5 100644
--- a/drivers/i2c/busses/i2c-jz4780.c
+++ b/drivers/i2c/busses/i2c-jz4780.c
@@ -798,17 +798,15 @@ static int jz4780_i2c_probe(struct platform_device *pdev)
 
 	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");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+					"clock-frequency not specified in DT\n");
 
 	i2c->speed = clk_freq / 1000;
-	if (i2c->speed == 0) {
-		ret = -EINVAL;
-		dev_err(&pdev->dev, "clock-frequency minimum is 1000\n");
-		return ret;
-	}
+	if (i2c->speed == 0)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+					"clock-frequency minimum is 1000\n");
+
 	jz4780_i2c_set_speed(i2c);
 
 	dev_info(&pdev->dev, "Bus frequency is %d KHz\n", i2c->speed);
-- 
2.39.0

Re: [PATCH v3 4/4] i2c: jz4780: Use dev_err_probe()

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:51:16AM +0800, Rong Qianfeng wrote:
> No more special handling needed here, so use dev_err_probe()
> to simplify the code.

Ah, okay. But see below.

...

>  	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");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +					"clock-frequency not specified in DT\n");

Besides converting to the i2c_timings and respective APIs...

>  	i2c->speed = clk_freq / 1000;
> -	if (i2c->speed == 0) {
> -		ret = -EINVAL;
> -		dev_err(&pdev->dev, "clock-frequency minimum is 1000\n");
> -		return ret;
> -	}
> +	if (i2c->speed == 0)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +					"clock-frequency minimum is 1000\n");

...this makes sense to do with

	struct device *dev = &pdev->dev;
	...
		return dev_err_probe(dev, ...);

And continue with a patch to replace all those &pdev->dev with dev.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] i2c: jz4780: Use dev_err_probe()

[Rong Qianfeng]

在 2024/8/23 23:36, 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:16AM +0800, Rong Qianfeng wrote:
>> No more special handling needed here, so use dev_err_probe()
>> to simplify the code.
> Ah, okay. But see below.
>
> ...
>
>>        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");
>> -             return ret;
>> -     }
>> +     if (ret)
>> +             return dev_err_probe(&pdev->dev, ret,
>> +                                     "clock-frequency not specified in DT\n");
> Besides converting to the i2c_timings and respective APIs...
>
>>        i2c->speed = clk_freq / 1000;
>> -     if (i2c->speed == 0) {
>> -             ret = -EINVAL;
>> -             dev_err(&pdev->dev, "clock-frequency minimum is 1000\n");
>> -             return ret;
>> -     }
>> +     if (i2c->speed == 0)
>> +             return dev_err_probe(&pdev->dev, -EINVAL,
>> +                                     "clock-frequency minimum is 1000\n");
> ...this makes sense to do with
>
>          struct device *dev = &pdev->dev;
>          ...
>                  return dev_err_probe(dev, ...);
>
> And continue with a patch to replace all those &pdev->dev with dev.

Thanks very much for the detailed comments, I will modify all your 
suggestions in the next version.

Best Regards,
Qianfeng
>
> --
> With Best Regards,
> Andy Shevchenko
>
>