Hi Uwe,
On Sat, 1 May 2021, Uwe Kleine-König wrote:
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@xxxxxxxxxxxxxx>
Thanks for your patch, which is now commit d7bff84fe7ed8c3b ("pwm:
Ensure for legacy drivers that pwm->state stays consistent") in
pwm/for-next.
This commit broke the backlight on the Atmark Techno Armadillo 800 EVA
board (arch/arm/boot/dts/r8a7740-armadillo800eva.dts), which now shows a
black screen. Reverting the commit fixes the problem.
Do you have an idea what is wrong, and how to fix it?
Anything I can do to investigate?
Thanks!
---
Hello,
just a small optimisation: At the end of pwm_apply_legacy()
state->enabled is known to be true, so simplify
if (state->enabled && !pwm->state.enabled) {
to
if (!pwm->state.enabled) {
Best regards
Uwe
drivers/pwm/core.c | 139 +++++++++++++++++++++++++--------------------
1 file changed, 78 insertions(+), 61 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c4d5c0667137..57105deafb55 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -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 (!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;
}
--
2.30.2
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
