| Message ID | 1446146356-26166-1-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Wolfram, On Thu, Oct 29, 2015 at 8:19 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Reported-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for your patch! I suspect this bug was also the cause of the spurious failures of the R-Car Gen2 regulator quirk code I experienced sometimes during the last few days. > --- > drivers/i2c/busses/i2c-rcar.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 616433d387cdb2..ef26e933b1e9cb 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -612,7 +612,10 @@ static int rcar_i2c_probe(struct platform_device *pdev) > if (IS_ERR(priv->io)) > return PTR_ERR(priv->io); > > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > rcar_i2c_init(priv); > + pm_runtime_put(dev); > > irq = platform_get_irq(pdev, 0); > init_waitqueue_head(&priv->wait); > @@ -634,7 +637,6 @@ static int rcar_i2c_probe(struct platform_device *pdev) > return ret; pm_runtime_disable() in error path? > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Wolfram > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Reported-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-rcar.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 616433d387cdb2..ef26e933b1e9cb 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -612,7 +612,10 @@ static int rcar_i2c_probe(struct platform_device *pdev) > if (IS_ERR(priv->io)) > return PTR_ERR(priv->io); > > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > rcar_i2c_init(priv); > + pm_runtime_put(dev); Please correct me if I'm misunderstanding, but in my experience, above rcar_i2c_init() result can be removed if SoC has power-off feature, and if IP doesn't keep register value power-off case ? This case register doesn't keep myfunc_A's setting, because it calls pm_runtime_put() pm_runtime_get_sync() myfunc_A() pm_runtime_put() ... pm_runtime_get_sync() myfunc_B() pm_runtime_put() It should be this ? pm_runtime_get_sync() myfunc_A() myfunc_B() pm_runtime_put() -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Morimoto-san, > > + pm_runtime_enable(dev); > > + pm_runtime_get_sync(dev); > > rcar_i2c_init(priv); > > + pm_runtime_put(dev); > > Please correct me if I'm misunderstanding, but in my experience, > above rcar_i2c_init() result can be removed if SoC has power-off feature, > and if IP doesn't keep register value power-off case ? > This case register doesn't keep myfunc_A's setting, > because it calls pm_runtime_put() > > pm_runtime_get_sync() > myfunc_A() > pm_runtime_put() > ... > pm_runtime_get_sync() > myfunc_B() > pm_runtime_put() > > It should be this ? > > pm_runtime_get_sync() > myfunc_A() > myfunc_B() > pm_runtime_put() This is for power-off case, right? Wouldn't it be clearer then to add a resume function to pm_ops of the I2C driver which reinit the registers? Doing it before every transfer might be simpler but is also a bit implicit or subtle, I'd think. And for completeness: The above is only for the power-off case. In module-standby (MSTP bits), register values are always retained. Correct? Regards, Wolfram
Hi Wolfram > > pm_runtime_get_sync() > > myfunc_A() > > pm_runtime_put() > > ... > > pm_runtime_get_sync() > > myfunc_B() > > pm_runtime_put() > > > > It should be this ? > > > > pm_runtime_get_sync() > > myfunc_A() > > myfunc_B() > > pm_runtime_put() > > This is for power-off case, right? Wouldn't it be clearer then to add a > resume function to pm_ops of the I2C driver which reinit the registers? > Doing it before every transfer might be simpler but is also a bit > implicit or subtle, I'd think. > > And for completeness: The above is only for the power-off case. In > module-standby (MSTP bits), register values are always retained. > Correct? I'm not sure detail of pm_runtime, but it depends on power-domain support ? On SH-Mobile case, it automatically power-off:ed if all driver user was gone. This feature is not yet supported on R-Car case I think. So, both case are not problem at this point, but 1st case will be problem if power-domain was suppored. (If my memory was correct...) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On SH-Mobile case, it automatically power-off:ed if all driver > user was gone. It does that without calling some callback?
Hi Wolfram > > On SH-Mobile case, it automatically power-off:ed if all driver > > user was gone. > > It does that without calling some callback? Do you mean you can call init() in callback ? I don't care detail of design. But my concern was that register value might be disappear during many get/put if power-domain was implemented -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 616433d387cdb2..ef26e933b1e9cb 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -612,7 +612,10 @@ static int rcar_i2c_probe(struct platform_device *pdev) if (IS_ERR(priv->io)) return PTR_ERR(priv->io); + pm_runtime_enable(dev); + pm_runtime_get_sync(dev); rcar_i2c_init(priv); + pm_runtime_put(dev); irq = platform_get_irq(pdev, 0); init_waitqueue_head(&priv->wait); @@ -634,7 +637,6 @@ static int rcar_i2c_probe(struct platform_device *pdev) return ret; } - pm_runtime_enable(dev); platform_set_drvdata(pdev, priv); ret = i2c_add_numbered_adapter(adap);