Re: [PATCH 3/3] pwm: Restore initial state if a legacy callback fails

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

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

On Thu, Jul 01, 2021 at 09:29:27AM +0200, Uwe Kleine-König wrote:
> It is not entirely accurate to go back to the initial state after e.g.
> .enable() failed, as .config() still modified the hardware, but this same
> inconsistency exists for drivers that implement .apply().
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 20afe6d0bc5e..6e30ef9b9b79 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -539,10 +539,8 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
>  	int err;
> +	struct pwm_state initial_state = pwm->state;
>  
> -	/*
> -	 * FIXME: restore the initial state in case of error.
> -	 */
>  	if (state->polarity != pwm->state.polarity) {
>  		if (!chip->ops->set_polarity)
>  			return -EINVAL;
> @@ -563,7 +561,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  		err = chip->ops->set_polarity(chip, pwm, state->polarity);
>  		if (err)
> -			return err;
> +			goto rollback;
>  
>  		pwm->state.polarity = state->polarity;
>  	}
> @@ -586,7 +584,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  				state->duty_cycle,
>  				state->period);
>  	if (err)
> -		return err;
> +		goto rollback;
>  
>  	pwm->state.period = state->period;
>  	pwm->state.duty_cycle = state->duty_cycle;
> @@ -594,10 +592,14 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (!pwm->state.enabled) {
>  		err = chip->ops->enable(chip, pwm);
>  		if (err)
> -			return err;
> +			goto rollback;
>  	}
>  
>  	return 0;
> +
> +rollback:
> +	pwm->state = initial_state;
> +	return err;
>  }

Can't we achieve the same thing by just removing all the updates to
pwm->state in pwm_apply_legacy()? Patch 1 in the series now does
pwm->state = *state for both the atomic and the legacy cases, so if
we don't update pwm->state explicitly in pwm_apply_legacy(), then
there should be no need to rollback, right?

What we currently do is a bit redundant anyway. pwm->state = *state
should be a no-op after pwm_apply_legacy().

Thierry

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