[PATCH] i2c: riic: Add Runtime PM support

[PATCH] i2c: riic: Add Runtime PM support

[Geert Uytterhoeven]

  - Replace explicit clock handling by Runtime PM calls,
  - Streamline Runtime PM handling in error paths,
  - Enable Runtime PM in .probe(),
  - Disable Runtime PM in .remove(),
  - Make sure the device is runtime-resumed when disabling interrupts in
    .remove().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
I don't think it's worth splitting off the last bug fix in a separate
(first) patch, using explicit clock handling, as that will be replaced
immediately by Runtime PM calls.
---
 drivers/i2c/busses/i2c-riic.c | 43 +++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index b75ff144b5704293..f31413fd9521ef9f 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -43,6 +43,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 
 #define RIIC_ICCR1	0x00
 #define RIIC_ICCR2	0x04
@@ -112,12 +113,10 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct riic_dev *riic = i2c_get_adapdata(adap);
 	unsigned long time_left;
-	int i, ret;
+	int i;
 	u8 start_bit;
 
-	ret = clk_prepare_enable(riic->clk);
-	if (ret)
-		return ret;
+	pm_runtime_get_sync(adap->dev.parent);
 
 	if (readb(riic->base + RIIC_ICCR2) & ICCR2_BBSY) {
 		riic->err = -EBUSY;
@@ -150,7 +149,7 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	}
 
  out:
-	clk_disable_unprepare(riic->clk);
+	pm_runtime_put(adap->dev.parent);
 
 	return riic->err ?: num;
 }
@@ -281,20 +280,18 @@ static const struct i2c_algorithm riic_algo = {
 
 static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 {
-	int ret;
+	int ret = 0;
 	unsigned long rate;
 	int total_ticks, cks, brl, brh;
 
-	ret = clk_prepare_enable(riic->clk);
-	if (ret)
-		return ret;
+	pm_runtime_get_sync(riic->adapter.dev.parent);
 
 	if (t->bus_freq_hz > 400000) {
 		dev_err(&riic->adapter.dev,
 			"unsupported bus speed (%dHz). 400000 max\n",
 			t->bus_freq_hz);
-		clk_disable_unprepare(riic->clk);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	rate = clk_get_rate(riic->clk);
@@ -332,8 +329,8 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 	if (brl > (0x1F + 3)) {
 		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
 			(unsigned long)t->bus_freq_hz);
-		clk_disable_unprepare(riic->clk);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	brh = total_ticks - brl;
@@ -378,9 +375,9 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
-	clk_disable_unprepare(riic->clk);
-
-	return 0;
+out:
+	pm_runtime_put(riic->adapter.dev.parent);
+	return ret;
 }
 
 static struct riic_irq_desc riic_irqs[] = {
@@ -439,28 +436,36 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 	i2c_parse_fw_timings(&pdev->dev, &i2c_t, true);
 
+	pm_runtime_enable(&pdev->dev);
+
 	ret = riic_init_hw(riic, &i2c_t);
 	if (ret)
-		return ret;
-
+		goto out;
 
 	ret = i2c_add_adapter(adap);
 	if (ret)
-		return ret;
+		goto out;
 
 	platform_set_drvdata(pdev, riic);
 
 	dev_info(&pdev->dev, "registered with %dHz bus speed\n",
 		 i2c_t.bus_freq_hz);
 	return 0;
+
+out:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
 }
 
 static int riic_i2c_remove(struct platform_device *pdev)
 {
 	struct riic_dev *riic = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(&pdev->dev);
 	writeb(0, riic->base + RIIC_ICIER);
+	pm_runtime_put(&pdev->dev);
 	i2c_del_adapter(&riic->adapter);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] i2c: riic: Add Runtime PM support

[Niklas Söderlund]

Hi Geert,

Thanks for your work.

On 2019-03-20 11:30:03 +0100, Geert Uytterhoeven wrote:
>   - Replace explicit clock handling by Runtime PM calls,
>   - Streamline Runtime PM handling in error paths,
>   - Enable Runtime PM in .probe(),
>   - Disable Runtime PM in .remove(),
>   - Make sure the device is runtime-resumed when disabling interrupts in
>     .remove().

Thanks for this change, it was educational for me to review.

> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> I don't think it's worth splitting off the last bug fix in a separate
> (first) patch, using explicit clock handling, as that will be replaced
> immediately by Runtime PM calls.
> ---
>  drivers/i2c/busses/i2c-riic.c | 43 +++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index b75ff144b5704293..f31413fd9521ef9f 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -43,6 +43,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  
>  #define RIIC_ICCR1	0x00
>  #define RIIC_ICCR2	0x04
> @@ -112,12 +113,10 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  {
>  	struct riic_dev *riic = i2c_get_adapdata(adap);
>  	unsigned long time_left;
> -	int i, ret;
> +	int i;
>  	u8 start_bit;
>  
> -	ret = clk_prepare_enable(riic->clk);
> -	if (ret)
> -		return ret;
> +	pm_runtime_get_sync(adap->dev.parent);
>  
>  	if (readb(riic->base + RIIC_ICCR2) & ICCR2_BBSY) {
>  		riic->err = -EBUSY;
> @@ -150,7 +149,7 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	}
>  
>   out:
> -	clk_disable_unprepare(riic->clk);
> +	pm_runtime_put(adap->dev.parent);
>  
>  	return riic->err ?: num;
>  }
> @@ -281,20 +280,18 @@ static const struct i2c_algorithm riic_algo = {
>  
>  static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
>  {
> -	int ret;
> +	int ret = 0;
>  	unsigned long rate;
>  	int total_ticks, cks, brl, brh;
>  
> -	ret = clk_prepare_enable(riic->clk);
> -	if (ret)
> -		return ret;
> +	pm_runtime_get_sync(riic->adapter.dev.parent);
>  
>  	if (t->bus_freq_hz > 400000) {
>  		dev_err(&riic->adapter.dev,
>  			"unsupported bus speed (%dHz). 400000 max\n",
>  			t->bus_freq_hz);
> -		clk_disable_unprepare(riic->clk);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
>  	}
>  
>  	rate = clk_get_rate(riic->clk);
> @@ -332,8 +329,8 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
>  	if (brl > (0x1F + 3)) {
>  		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
>  			(unsigned long)t->bus_freq_hz);
> -		clk_disable_unprepare(riic->clk);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
>  	}
>  
>  	brh = total_ticks - brl;
> @@ -378,9 +375,9 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
>  
>  	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
>  
> -	clk_disable_unprepare(riic->clk);
> -
> -	return 0;
> +out:
> +	pm_runtime_put(riic->adapter.dev.parent);
> +	return ret;
>  }
>  
>  static struct riic_irq_desc riic_irqs[] = {
> @@ -439,28 +436,36 @@ static int riic_i2c_probe(struct platform_device *pdev)
>  
>  	i2c_parse_fw_timings(&pdev->dev, &i2c_t, true);
>  
> +	pm_runtime_enable(&pdev->dev);
> +
>  	ret = riic_init_hw(riic, &i2c_t);
>  	if (ret)
> -		return ret;
> -
> +		goto out;
>  
>  	ret = i2c_add_adapter(adap);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	platform_set_drvdata(pdev, riic);
>  
>  	dev_info(&pdev->dev, "registered with %dHz bus speed\n",
>  		 i2c_t.bus_freq_hz);
>  	return 0;
> +
> +out:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
>  }
>  
>  static int riic_i2c_remove(struct platform_device *pdev)
>  {
>  	struct riic_dev *riic = platform_get_drvdata(pdev);
>  
> +	pm_runtime_get_sync(&pdev->dev);
>  	writeb(0, riic->base + RIIC_ICIER);
> +	pm_runtime_put(&pdev->dev);
>  	i2c_del_adapter(&riic->adapter);
> +	pm_runtime_disable(&pdev->dev);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] i2c: riic: Add Runtime PM support

[Wolfram Sang]

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

On Wed, Mar 20, 2019 at 11:30:03AM +0100, Geert Uytterhoeven wrote:
>   - Replace explicit clock handling by Runtime PM calls,
>   - Streamline Runtime PM handling in error paths,
>   - Enable Runtime PM in .probe(),
>   - Disable Runtime PM in .remove(),
>   - Make sure the device is runtime-resumed when disabling interrupts in
>     .remove().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> I don't think it's worth splitting off the last bug fix in a separate
> (first) patch, using explicit clock handling, as that will be replaced
> immediately by Runtime PM calls.

Chris, what do you think about this patch?


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

RE: [PATCH] i2c: riic: Add Runtime PM support

[Chris Brandt]

From a functional standpoint, it is simply doing the same operation that it does today, just with different APIs, so that's fine.

I will go and test it to make sure it works on RZ/A1 and RZ/A2 and didn't break anything, and then reply.


Chris


> -----Original Message-----
> From: Wolfram Sang <wsa@the-dreams.de>
> Sent: Monday, April 15, 2019 8:17 AM
> To: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Chris Brandt <Chris.Brandt@renesas.com>; linux-i2c@vger.kernel.org;
> linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH] i2c: riic: Add Runtime PM support
> 
> On Wed, Mar 20, 2019 at 11:30:03AM +0100, Geert Uytterhoeven wrote:
> >   - Replace explicit clock handling by Runtime PM calls,
> >   - Streamline Runtime PM handling in error paths,
> >   - Enable Runtime PM in .probe(),
> >   - Disable Runtime PM in .remove(),
> >   - Make sure the device is runtime-resumed when disabling interrupts in
> >     .remove().
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > I don't think it's worth splitting off the last bug fix in a separate
> > (first) patch, using explicit clock handling, as that will be replaced
> > immediately by Runtime PM calls.
> 
> Chris, what do you think about this patch?

Re: [PATCH] i2c: riic: Add Runtime PM support

[Wolfram Sang]

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


> I will go and test it to make sure it works on RZ/A1 and RZ/A2 and
> didn't break anything, and then reply.

Cool, thanks!


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

RE: [PATCH] i2c: riic: Add Runtime PM support

[Chris Brandt]

On Mon, Apr 15, 2019, Wolfram Sang wrote:
> On Wed, Mar 20, 2019 at 11:30:03AM +0100, Geert Uytterhoeven wrote:
> >   - Replace explicit clock handling by Runtime PM calls,
> >   - Streamline Runtime PM handling in error paths,
> >   - Enable Runtime PM in .probe(),
> >   - Disable Runtime PM in .remove(),
> >   - Make sure the device is runtime-resumed when disabling interrupts in
> >     .remove().
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > I don't think it's worth splitting off the last bug fix in a separate
> > (first) patch, using explicit clock handling, as that will be replaced
> > immediately by Runtime PM calls.
> 
> Chris, what do you think about this patch?

Tested on RZ/A1H RSK and RZ/A2M EVB using an LCD touchscreen. No issues.

Tested-by: Chris Brandt <chris.brandt@renesas.com>

Re: [PATCH] i2c: riic: Add Runtime PM support

[Wolfram Sang]

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

On Wed, Mar 20, 2019 at 11:30:03AM +0100, Geert Uytterhoeven wrote:
>   - Replace explicit clock handling by Runtime PM calls,
>   - Streamline Runtime PM handling in error paths,
>   - Enable Runtime PM in .probe(),
>   - Disable Runtime PM in .remove(),
>   - Make sure the device is runtime-resumed when disabling interrupts in
>     .remove().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied to for-next, thanks!


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