| Message ID | 20190320103003.20107-1-geert+renesas@glider.be |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: riic: Add Runtime PM support | expand |
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 >
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?
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?
> 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!
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>
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!
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; }
- 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(-)