From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuninori Morimoto Subject: Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer Date: Tue, 18 Apr 2017 23:59:13 +0000 Message-ID: <87o9vtl19p.wl%kuninori.morimoto.gx@renesas.com> References: <20170418183835.13836-1-wsa+renesas@sang-engineering.com> Mime-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset="US-ASCII" Return-path: In-Reply-To: <20170418183835.13836-1-wsa+renesas@sang-engineering.com> Sender: linux-renesas-soc-owner@vger.kernel.org To: Wolfram Sang Cc: linux-i2c@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Kuninori Morimoto , Geert Uytterhoeven List-Id: linux-i2c@vger.kernel.org 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 > --- > > 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); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <87o9vtl19p.wl%kuninori.morimoto.gx@renesas.com> From: Kuninori Morimoto To: Wolfram Sang CC: , , Kuninori Morimoto , Geert Uytterhoeven Subject: Re: [PATCH] i2c: rcar: fix resume by always initializing registers before transfer In-Reply-To: <20170418183835.13836-1-wsa+renesas@sang-engineering.com> References: <20170418183835.13836-1-wsa+renesas@sang-engineering.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset="US-ASCII" Date: Tue, 18 Apr 2017 23:59:13 +0000 Sender: linux-i2c-owner@vger.kernel.org List-ID: 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 > --- > > 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);