[2/2] pwm: rcar: add suspend/resume support

Message ID 1520596498-4801-3-git-send-email-yoshihiro.shimoda.uh@renesas.com
State Superseded
Headers show
Series
  • pwm: rcar: Add suspend/resume support and cleanup for runtime_pm
Related show

Commit Message

Yoshihiro Shimoda March 9, 2018, 11:54 a.m.
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(+)

Comments

Geert Uytterhoeven March 9, 2018, 12:53 p.m. | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yoshihiro Shimoda March 13, 2018, 7:04 a.m. | #2
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
Geert Uytterhoeven March 13, 2018, 7:31 a.m. | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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),
 	}
 };