Hi Shimoda-san,

On Tue, Mar 13, 2018 at 8:04 AM, Yoshihiro Shimoda
<yoshihiro.shimoda...@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...@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.

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 -- 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