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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

On Thu, Aug 19, 2021 at 03:28:31PM +0200, Thierry Reding wrote:
> 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?

No, this doesn't work because the implemention of the callbacks might
rely on ->state holding the result from a previous callback.
See for example drivers/pwm/pwm-lpc18xx-sct.c where .enable() calls
pwm_get_polarity().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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