Re: [PATCH v3 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

On Wed, Jan 09, 2019 at 05:19:06PM +0900, Yoshihiro Shimoda wrote:
> To remove legacy API related functions in the future, this patch
> uses "atomic" related function instead. No change in behavior.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-rcar.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index e3ab663..652a937 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -316,18 +316,16 @@ static int rcar_pwm_suspend(struct device *dev)
>  static int rcar_pwm_resume(struct device *dev)
>  {
>  	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
> +	struct pwm_state state;
>  
>  	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
>  		return 0;
>  
>  	pm_runtime_get_sync(dev);
>  
> -	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
> -			pwm->state.period);
> -	if (pwm_is_enabled(pwm))
> -		rcar_pwm_enable(pwm->chip, pwm);
> +	pwm_get_state(pwm, &state);
>  
> -	return 0;
> +	return rcar_pwm_apply(pwm->chip, pwm, &state);
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  static SIMPLE_DEV_PM_OPS(rcar_pwm_pm_ops, rcar_pwm_suspend, rcar_pwm_resume);

As was recently discussed elsewhere, it should really be up to the
consumer of PWMs to reinitialize the PWM on resume. However, this is a
preexisting condition, so let's do it in a follow-up.

Do you have any cases where you really rely on this? Resume ordering
does not guarantee that the PWM and consumer will be resumed in the
right order, so you may be enabling the PWM too soon, or too late, by
doing this in the PWM driver.

There are patches in the works to help with this using device links, but
I think even with those there will be cases where the consumer will want
to directly control when the PWM is restored, so you should consider
moving the driver away from implementing suspend/resume itself.

It'd be interesting to hear if there any cases where things break if you
simply remove these PM callbacks. If so we should investigate and fix
them so that these can be removed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]