Re: [PATCH 2/3] pwm: Prevent a glitch for legacy drivers

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

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

On Thu, Jul 01, 2021 at 09:29:26AM +0200, Uwe Kleine-König wrote:
> If a running PWM is reconfigured to disabled calling the ->config()
> callback before disabling the hardware might result in a glitch where
> the (maybe) new period and duty_cycle are visible on the output before
> disabling the hardware.
> 
> So handle disabling before calling ->config(). Also exit early in this case
> which is possible because period and duty_cycle don't matter for disabled PWMs.
> In return however ->config has to be called even if state->period ==
> pwm->state.period && state->duty_cycle != pwm->state.duty_cycle because setting
> these might have been skipped in the previous call.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3c72f8963073..20afe6d0bc5e 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -568,26 +568,33 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  		pwm->state.polarity = state->polarity;
>  	}
>  
> -	if (state->period != pwm->state.period ||
> -	    state->duty_cycle != pwm->state.duty_cycle) {
> -		err = chip->ops->config(pwm->chip, pwm,
> -					state->duty_cycle,
> -					state->period);
> -		if (err)
> -			return err;
> +	if (!state->enabled) {
> +		if (pwm->state.enabled)
> +			chip->ops->disable(chip, pwm);
>  
> -		pwm->state.period = state->period;
> -		pwm->state.duty_cycle = state->duty_cycle;
> +		return 0;
>  	}
>  
> -	if (state->enabled != pwm->state.enabled) {
> -		if (!pwm->state.enabled) {
> -			err = chip->ops->enable(chip, pwm);
> -			if (err)
> -				return err;
> -		} else {
> -			chip->ops->disable(chip, pwm);
> -		}
> +	/*
> +	 * We cannot skip calling ->config even if state->period ==
> +	 * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
> +	 * because we might have exited early in the last call to
> +	 * pwm_apply_state because of !state->enabled and so the two values in
> +	 * pwm->state might not be configured in hardware.
> +	 */
> +	err = chip->ops->config(pwm->chip, pwm,
> +				state->duty_cycle,
> +				state->period);
> +	if (err)
> +		return err;
> +
> +	pwm->state.period = state->period;
> +	pwm->state.duty_cycle = state->duty_cycle;
> +
> +	if (!pwm->state.enabled) {
> +		err = chip->ops->enable(chip, pwm);
> +		if (err)
> +			return err;
>  	}

I thought we might have discussed this, but I can't find any record of
it. How is this better than always configuring, whether the PWM is
disabled or not?

From an atomic point of view, the hardware state is expected to match
the PWM state that was passed to ->apply() after it returns. That means
that calling ->get_state() after ->apply() should return the same state
that was passed to ->apply(). With the above that's no longer true. It
doesn't actually matter, because legacy drivers don't support
->get_state(), but conceptually it's not mimicking the atomic API as
well as it could.

Thierry

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