Hi Shimoda-san,
On Fri, Mar 9, 2018 at 12:54 PM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> This patch adds suspend/resume support for Renesas PWM driver.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
