Message ID | 20210701072927.328254-4-u.kleine-koenig@pengutronix.de |
---|---|
State | Accepted |
Headers | show |
Series | pwm: Some improvements for legacy drivers | expand |
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
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
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; } /**
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(-)