@@ -535,6 +535,71 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
}
}
+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;
+
+ if (state->polarity != pwm->state.polarity) {
+ if (!chip->ops->set_polarity) {
+ err = -EINVAL;
+ goto out_err;
+ }
+
+ /*
+ * Changing the polarity of a running PWM is only allowed when
+ * the PWM driver implements ->apply().
+ */
+ if (pwm->state.enabled) {
+ chip->ops->disable(chip, pwm);
+
+ /*
+ * Update pwm->state already here in case
+ * .set_polarity() or another callback depend on that.
+ */
+ pwm->state.enabled = false;
+ }
+
+ err = chip->ops->set_polarity(chip, pwm,
+ state->polarity);
+ if (err)
+ goto out_err;
+
+ pwm->state.polarity = state->polarity;
+ }
+
+ if (!state->enabled) {
+ if (pwm->state.enabled)
+ chip->ops->disable(chip, pwm);
+ return 0;
+ }
+
+ 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)
+ goto out_err;
+
+ pwm->state.period = state->period;
+ pwm->state.duty_cycle = state->duty_cycle;
+ }
+
+ if (state->enabled && !pwm->state.enabled) {
+ err = chip->ops->enable(chip, pwm);
+ if (err)
+ goto out_err;
+ }
+
+ return 0;
+
+out_err:
+ pwm->state = initial_state;
+ return err;
+}
+
/**
* pwm_apply_state() - atomically apply a new state to a PWM device
* @pwm: PWM device
@@ -544,6 +609,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
{
struct pwm_chip *chip;
int err;
+ int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state);
if (!pwm || !state || !state->period ||
state->duty_cycle > state->period)
@@ -557,70 +624,20 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
state->enabled == pwm->state.enabled)
return 0;
- if (chip->ops->apply) {
- err = chip->ops->apply(chip, pwm, state);
- if (err)
- return err;
-
- trace_pwm_apply(pwm, state);
-
- pwm->state = *state;
-
- /*
- * only do this after pwm->state was applied as some
- * implementations of .get_state depend on this
- */
- pwm_apply_state_debug(pwm, state);
- } else {
- /*
- * FIXME: restore the initial state in case of error.
- */
- if (state->polarity != pwm->state.polarity) {
- if (!chip->ops->set_polarity)
- return -EINVAL;
-
- /*
- * Changing the polarity of a running PWM is
- * only allowed when the PWM driver implements
- * ->apply().
- */
- if (pwm->state.enabled) {
- chip->ops->disable(chip, pwm);
- pwm->state.enabled = false;
- }
+ apply = chip->ops->apply ?: pwm_apply_legacy;
+ err = apply(chip, pwm, state);
+ if (err)
+ return err;
- err = chip->ops->set_polarity(chip, pwm,
- state->polarity);
- if (err)
- return err;
+ trace_pwm_apply(pwm, state);
- 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;
+ pwm->state = *state;
- pwm->state.duty_cycle = state->duty_cycle;
- pwm->state.period = state->period;
- }
-
- if (state->enabled != pwm->state.enabled) {
- if (state->enabled) {
- err = chip->ops->enable(chip, pwm);
- if (err)
- return err;
- } else {
- chip->ops->disable(chip, pwm);
- }
-
- pwm->state.enabled = state->enabled;
- }
- }
+ /*
+ * only do this after pwm->state was applied as some
+ * implementations of .get_state depend on this
+ */
+ pwm_apply_state_debug(pwm, state);
return 0;
}
Without this change it can happen that if changing the polarity succeeded but changing duty_cycle and period failed pwm->state contains a mixture between the old and the requested state. So remember the initial state before starting to modify the configuration and restore it when one of the required callback fails. Compared to the previous implementation .disable() (if necessary) is called earlier to prevent a glitch. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, this is my approach to address Thierry's review for my patch "pwm: Prevent a glitch in compat code" (https://lore.kernel.org/linux-pwm/20210308093600.25455-1-u.kleine-koenig@pengutronix.de). The problem that Thierry pointed out already exists in the status quo (but only if a callback failed) and with my patch it triggered more often. So here is a patch that fixes both problems (partial update of pwm->state and the glitch) in one go. Is it worth the effor to split this patch further? (e.g. 1) move legacy handling in a separate function, 2) address the partial update problem, 3) fix the glitch) Best regards Uwe drivers/pwm/core.c | 139 +++++++++++++++++++++++++-------------------- 1 file changed, 78 insertions(+), 61 deletions(-) base-commit: 64d7d074acd52e1bdff621f2cb86c0aae9bcef80