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...@renesas.com> wrote:
> > This patch adds suspend/resume support for Renesas PWM driver.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@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 -- 
> ge...@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

Reply via email to