| Message ID | 1520596498-4801-3-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Shimoda-san, On Fri, Mar 9, 2018 at 12:54 PM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > This patch adds suspend/resume support for Renesas PWM driver. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Thanks for your patch! > --- a/drivers/pwm/pwm-rcar.c > +++ b/drivers/pwm/pwm-rcar.c > @@ -254,11 +254,38 @@ static int rcar_pwm_remove(struct platform_device *pdev) > }; > MODULE_DEVICE_TABLE(of, rcar_pwm_of_table); > > +#ifdef CONFIG_PM_SLEEP > +static int rcar_pwm_suspend(struct device *dev) > +{ > + pm_runtime_put(dev); > + > + return 0; > +} > + > +static int rcar_pwm_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev); > + struct pwm_chip *chip = &rcar_pwm->chip; > + struct pwm_device *pwm = &chip->pwms[0]; > + > + pm_runtime_get_sync(dev); This enables the module clock unconditionally, so you can restore the register values below... > + rcar_pwm_config(chip, pwm, pwm->state.duty_cycle, pwm->state.period); > + if (pwm_is_enabled(pwm)) > + rcar_pwm_enable(chip, pwm); ... but shouldn't you disable the clock again if the PWM is not in use, like below? else pm_runtime_put(dev); Likewise, I think the call to "pm_runtime_put(dev)" in rcar_pwm_suspend() should be protected by a pwm_is_enabled(pwm) check. > + return 0; > +} 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
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Friday, March 9, 2018 9:53 PM > > Hi Shimoda-san, > > On Fri, Mar 9, 2018 at 12:54 PM, Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > This patch adds suspend/resume support for Renesas PWM driver. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Thanks for your patch! Thank you for your review! > > --- a/drivers/pwm/pwm-rcar.c > > +++ b/drivers/pwm/pwm-rcar.c > > @@ -254,11 +254,38 @@ static int rcar_pwm_remove(struct platform_device *pdev) > > }; > > MODULE_DEVICE_TABLE(of, rcar_pwm_of_table); > > > > +#ifdef CONFIG_PM_SLEEP > > +static int rcar_pwm_suspend(struct device *dev) > > +{ > > + pm_runtime_put(dev); > > + > > + return 0; > > +} > > + > > +static int rcar_pwm_resume(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev); > > + struct pwm_chip *chip = &rcar_pwm->chip; > > + struct pwm_device *pwm = &chip->pwms[0]; > > + > > + pm_runtime_get_sync(dev); > > This enables the module clock unconditionally, so you can restore the > register values below... > > > + rcar_pwm_config(chip, pwm, pwm->state.duty_cycle, pwm->state.period); > > + if (pwm_is_enabled(pwm)) > > + rcar_pwm_enable(chip, pwm); > > ... but shouldn't you disable the clock again if the PWM is not in use, > like below? > > else > pm_runtime_put(dev); Thank you for the pointing out. I realize that this suspend/resume function should check PWMF_REQUESTED condition because this driver enables the clock after the .request(rcar_pwm_request) was called. So, I will fix this patch in v2. Also, reducing power point of view, this driver should call not pm_runtime_get_sync/put() in _request/_free(). But, I would like to implement such code as a next step :) Best regards, Yoshihiro Shimoda > Likewise, I think the call to "pm_runtime_put(dev)" in rcar_pwm_suspend() > should be protected by a pwm_is_enabled(pwm) check. > > > + return 0; > > +} > > 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
Hi Shimoda-san, On Tue, Mar 13, 2018 at 8:04 AM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: >> From: Geert Uytterhoeven, Sent: Friday, March 9, 2018 9:53 PM >> On Fri, Mar 9, 2018 at 12:54 PM, Yoshihiro Shimoda >> <yoshihiro.shimoda.uh@renesas.com> wrote: >> > This patch adds suspend/resume support for Renesas PWM driver. >> > >> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >> >> Thanks for your patch! > > Thank you for your review! > >> > --- a/drivers/pwm/pwm-rcar.c >> > +++ b/drivers/pwm/pwm-rcar.c >> > @@ -254,11 +254,38 @@ static int rcar_pwm_remove(struct platform_device *pdev) >> > }; >> > MODULE_DEVICE_TABLE(of, rcar_pwm_of_table); >> > >> > +#ifdef CONFIG_PM_SLEEP >> > +static int rcar_pwm_suspend(struct device *dev) >> > +{ >> > + pm_runtime_put(dev); >> > + >> > + return 0; >> > +} >> > + >> > +static int rcar_pwm_resume(struct device *dev) >> > +{ >> > + struct platform_device *pdev = to_platform_device(dev); >> > + struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev); >> > + struct pwm_chip *chip = &rcar_pwm->chip; >> > + struct pwm_device *pwm = &chip->pwms[0]; >> > + >> > + pm_runtime_get_sync(dev); >> >> This enables the module clock unconditionally, so you can restore the >> register values below... >> >> > + rcar_pwm_config(chip, pwm, pwm->state.duty_cycle, pwm->state.period); >> > + if (pwm_is_enabled(pwm)) >> > + rcar_pwm_enable(chip, pwm); >> >> ... but shouldn't you disable the clock again if the PWM is not in use, >> like below? >> >> else >> pm_runtime_put(dev); > > Thank you for the pointing out. > I realize that this suspend/resume function should check PWMF_REQUESTED condition > because this driver enables the clock after the .request(rcar_pwm_request) was called. > So, I will fix this patch in v2. OK. > Also, reducing power point of view, this driver should call not pm_runtime_get_sync/put() > in _request/_free(). But, I would like to implement such code as a next step :) That makes sense. I was already wondering whether those are the best locations to call the pm_runtime_*() functions, but I'm not sufficiently familiar with the PWM subsystem. 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
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c index b942010..f62045c 100644 --- a/drivers/pwm/pwm-rcar.c +++ b/drivers/pwm/pwm-rcar.c @@ -254,11 +254,38 @@ static int rcar_pwm_remove(struct platform_device *pdev) }; MODULE_DEVICE_TABLE(of, rcar_pwm_of_table); +#ifdef CONFIG_PM_SLEEP +static int rcar_pwm_suspend(struct device *dev) +{ + pm_runtime_put(dev); + + return 0; +} + +static int rcar_pwm_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev); + struct pwm_chip *chip = &rcar_pwm->chip; + struct pwm_device *pwm = &chip->pwms[0]; + + pm_runtime_get_sync(dev); + + rcar_pwm_config(chip, pwm, pwm->state.duty_cycle, pwm->state.period); + if (pwm_is_enabled(pwm)) + rcar_pwm_enable(chip, pwm); + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ +static SIMPLE_DEV_PM_OPS(rcar_pwm_pm_ops, rcar_pwm_suspend, rcar_pwm_resume); + static struct platform_driver rcar_pwm_driver = { .probe = rcar_pwm_probe, .remove = rcar_pwm_remove, .driver = { .name = "pwm-rcar", + .pm = &rcar_pwm_pm_ops, .of_match_table = of_match_ptr(rcar_pwm_of_table), } };
This patch adds suspend/resume support for Renesas PWM driver. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pwm/pwm-rcar.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)