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@xxxxxxxxx>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> 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
