[PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Wolfram Sang]

Resume failed because of uninitialized registers. Instead of adding a
resume callback, we simply initialize registers before every transfer.
This lightweight change is more robust and will keep us safe if we ever
need support for power domains or dynamic frequency changes.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Morimoto-san: you convinced me that putting all register settings to one place
is a nice thing to do. Here is the patch for that. However, having
pm_runtime_get/put in one place won't work, I am afraid. For I2C slave or
multi-master support, we need permanent-on handling. So, I think the current
pm_runtime handling is good in that regard. Can you agree?

 drivers/i2c/busses/i2c-rcar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 26f2ff22e97e55..66b84bf51bbfd0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -700,6 +700,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 	pm_runtime_get_sync(dev);
 
+	rcar_i2c_init(priv);
+
 	ret = rcar_i2c_bus_barrier(priv);
 	if (ret < 0)
 		goto out;
@@ -860,8 +862,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto out_pm_put;
 
-	rcar_i2c_init(priv);
-
 	/* Don't suspend when multi-master to keep arbitration working */
 	if (of_property_read_bool(dev->of_node, "multi-master"))
 		priv->flags |= ID_P_PM_BLOCKED;
-- 
2.11.0

Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Kuninori Morimoto]

Hi Wolfram

> Resume failed because of uninitialized registers. Instead of adding a
> resume callback, we simply initialize registers before every transfer.
> This lightweight change is more robust and will keep us safe if we ever
> need support for power domains or dynamic frequency changes.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Morimoto-san: you convinced me that putting all register settings to one place
> is a nice thing to do. Here is the patch for that. However, having
> pm_runtime_get/put in one place won't work, I am afraid. For I2C slave or
> multi-master support, we need permanent-on handling. So, I think the current
> pm_runtime handling is good in that regard. Can you agree?

Thanks.
I understand about I2C slave / multi-master case of pm_runtime_get/put.
But in probe case (after rcar_i2c_init() move), we can't do like below ?
this is more clear code for me.

-------------
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 26f2ff2..db7f4d1 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -855,7 +855,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	priv->dma_rx = priv->dma_tx = ERR_PTR(-EPROBE_DEFER);
 
 	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
 	ret = rcar_i2c_clock_calculate(priv, &i2c_t);
 	if (ret < 0)
 		goto out_pm_put;
@@ -863,11 +862,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	rcar_i2c_init(priv);
 
 	/* Don't suspend when multi-master to keep arbitration working */
-	if (of_property_read_bool(dev->of_node, "multi-master"))
+	if (of_property_read_bool(dev->of_node, "multi-master")) {
 		priv->flags |= ID_P_PM_BLOCKED;
-	else
-		pm_runtime_put(dev);
-
+		pm_runtime_get_sync(dev);
+	}
 
 	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(dev, irq, rcar_i2c_irq, 0, dev_name(dev), priv);

Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Kuninori Morimoto]

Hi Wolfram

> Resume failed because of uninitialized registers. Instead of adding a
> resume callback, we simply initialize registers before every transfer.
> This lightweight change is more robust and will keep us safe if we ever
> need support for power domains or dynamic frequency changes.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Morimoto-san: you convinced me that putting all register settings to one place
> is a nice thing to do. Here is the patch for that. However, having
> pm_runtime_get/put in one place won't work, I am afraid. For I2C slave or
> multi-master support, we need permanent-on handling. So, I think the current
> pm_runtime handling is good in that regard. Can you agree?

Thanks.
I understand about I2C slave / multi-master case of pm_runtime_get/put.
But in probe case (after rcar_i2c_init() move), we can't do like below ?
this is more clear code for me.

-------------
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 26f2ff2..db7f4d1 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -855,7 +855,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	priv->dma_rx = priv->dma_tx = ERR_PTR(-EPROBE_DEFER);
 
 	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
 	ret = rcar_i2c_clock_calculate(priv, &i2c_t);
 	if (ret < 0)
 		goto out_pm_put;
@@ -863,11 +862,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	rcar_i2c_init(priv);
 
 	/* Don't suspend when multi-master to keep arbitration working */
-	if (of_property_read_bool(dev->of_node, "multi-master"))
+	if (of_property_read_bool(dev->of_node, "multi-master")) {
 		priv->flags |= ID_P_PM_BLOCKED;
-	else
-		pm_runtime_put(dev);
-
+		pm_runtime_get_sync(dev);
+	}
 
 	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(dev, irq, rcar_i2c_irq, 0, dev_name(dev), priv);

Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Wolfram Sang]

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

Morimoto-san,

> I understand about I2C slave / multi-master case of pm_runtime_get/put.
> But in probe case (after rcar_i2c_init() move), we can't do like below ?
> this is more clear code for me.
> 
> -------------
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 26f2ff2..db7f4d1 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -855,7 +855,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>  	priv->dma_rx = priv->dma_tx = ERR_PTR(-EPROBE_DEFER);
>  
>  	pm_runtime_enable(dev);
> -	pm_runtime_get_sync(dev);
>  	ret = rcar_i2c_clock_calculate(priv, &i2c_t);

rcar_i2c_clock_calculate() uses clk_get_rate, so clk must be active.

Regards,

   Wolfram


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

Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Kuninori Morimoto]

Hi Wolfram

> > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> > index 26f2ff2..db7f4d1 100644
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
> > @@ -855,7 +855,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
> >  	priv->dma_rx = priv->dma_tx = ERR_PTR(-EPROBE_DEFER);
> >  
> >  	pm_runtime_enable(dev);
> > -	pm_runtime_get_sync(dev);
> >  	ret = rcar_i2c_clock_calculate(priv, &i2c_t);
> 
> rcar_i2c_clock_calculate() uses clk_get_rate, so clk must be active.

Ahh, OK, I understand.
Then,

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Best regards
---
Kuninori Morimoto

Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Kuninori Morimoto]

Hi Wolfram

> > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> > index 26f2ff2..db7f4d1 100644
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
> > @@ -855,7 +855,6 @@ static int rcar_i2c_probe(struct platform_device *pdev)
> >  	priv->dma_rx = priv->dma_tx = ERR_PTR(-EPROBE_DEFER);
> >  
> >  	pm_runtime_enable(dev);
> > -	pm_runtime_get_sync(dev);
> >  	ret = rcar_i2c_clock_calculate(priv, &i2c_t);
> 
> rcar_i2c_clock_calculate() uses clk_get_rate, so clk must be active.

Ahh, OK, I understand.
Then,

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Best regards
---
Kuninori Morimoto

Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer

[Wolfram Sang]

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

On Tue, Apr 18, 2017 at 08:38:35PM +0200, Wolfram Sang wrote:
> Resume failed because of uninitialized registers. Instead of adding a
> resume callback, we simply initialize registers before every transfer.
> This lightweight change is more robust and will keep us safe if we ever
> need support for power domains or dynamic frequency changes.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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