* [PATCH] pwm: Ensure for legacy drivers that pwm->state stays consistent @ 2021-04-11 16:04 Uwe Kleine-König 2021-05-01 16:09 ` [PATCH v2] " Uwe Kleine-König 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2021-04-11 16:04 UTC (permalink / raw) To: Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel 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(-) diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index c4d5c0667137..3f216c59bb38 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 (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; } base-commit: 64d7d074acd52e1bdff621f2cb86c0aae9bcef80 -- 2.30.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-04-11 16:04 [PATCH] pwm: Ensure for legacy drivers that pwm->state stays consistent Uwe Kleine-König @ 2021-05-01 16:09 ` Uwe Kleine-König 2021-06-28 11:46 ` Thierry Reding ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Uwe Kleine-König @ 2021-05-01 16:09 UTC (permalink / raw) To: Thierry Reding, Lee Jones; +Cc: linux-pwm, kernel 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, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-05-01 16:09 ` [PATCH v2] " Uwe Kleine-König @ 2021-06-28 11:46 ` Thierry Reding 2021-06-28 17:24 ` Uwe Kleine-König 2021-06-29 19:44 ` Geert Uytterhoeven 2021-06-29 19:52 ` Geert Uytterhoeven 2 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2021-06-28 11:46 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Lee Jones, linux-pwm, kernel [-- Attachment #1: Type: text/plain, Size: 1115 bytes --] On Sat, May 01, 2021 at 06:09:43PM +0200, 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@pengutronix.de> > --- > 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(-) Applied, thanks. I made a few tiny changes such as dropping the apply function pointer, which didn't seem very useful. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-06-28 11:46 ` Thierry Reding @ 2021-06-28 17:24 ` Uwe Kleine-König 0 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2021-06-28 17:24 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-pwm, Lee Jones, kernel [-- Attachment #1: Type: text/plain, Size: 1557 bytes --] On Mon, Jun 28, 2021 at 01:46:06PM +0200, Thierry Reding wrote: > On Sat, May 01, 2021 at 06:09:43PM +0200, 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@pengutronix.de> > > --- > > 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(-) > > Applied, thanks. I made a few tiny changes such as dropping the apply > function pointer, which didn't seem very useful. I just checked what you did and I can live with your changes. However I think you should point out in the commit log what you actually changed. 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-05-01 16:09 ` [PATCH v2] " Uwe Kleine-König 2021-06-28 11:46 ` Thierry Reding @ 2021-06-29 19:44 ` Geert Uytterhoeven 2021-06-30 6:48 ` Uwe Kleine-König 2021-06-29 19:52 ` Geert Uytterhoeven 2 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2021-06-29 19:44 UTC (permalink / raw) To: Uwe Kleine-König Cc: Thierry Reding, Lee Jones, linux-pwm, kernel, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 6009 bytes --] 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@pengutronix.de> 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@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-06-29 19:44 ` Geert Uytterhoeven @ 2021-06-30 6:48 ` Uwe Kleine-König 2021-06-30 10:22 ` Geert Uytterhoeven 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2021-06-30 6:48 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-renesas-soc, linux-pwm, Thierry Reding, Lee Jones, kernel [-- Attachment #1: Type: text/plain, Size: 2057 bytes --] Hi Geert, On Tue, Jun 29, 2021 at 09:44:38PM +0200, Geert Uytterhoeven wrote: > 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@pengutronix.de> > > 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? I starred at the patch for some time now and couldn't find a problem. Looking at drivers/pwm/pwm-renesas-tpu.c I don't see something obvious. (The .set_polarity callback is faulty as I doesn't commit the request to hardware, but that shouldn't matter here.) I guess the first request the backlight driver emits is .period = 33333, .duty_cycle = 33333, .enabled = true, .polarity = PWM_POLARITY_INVERSED, which should result into the following driver calls (with and without the breaking commit): tpu_pwm_set_polarity(chip, pwm, PWM_POLARITY_INVERSED); tpu_pwm_config(chip, pwm, 33333, 33333); tpu_pwm_enable(chip, pwm); Can you confirm that? Feel free to contact me via irc if you have questions/insights. Thanks for your time to report the issue, 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-06-30 6:48 ` Uwe Kleine-König @ 2021-06-30 10:22 ` Geert Uytterhoeven 2021-06-30 16:21 ` Uwe Kleine-König 0 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2021-06-30 10:22 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linux-Renesas, Linux PWM List, Thierry Reding, Lee Jones, Sascha Hauer Hi Uwe, On Wed, Jun 30, 2021 at 8:48 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Tue, Jun 29, 2021 at 09:44:38PM +0200, Geert Uytterhoeven wrote: > > 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@pengutronix.de> > > > > 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? > > I starred at the patch for some time now and couldn't find a problem. > Looking at drivers/pwm/pwm-renesas-tpu.c I don't see something obvious. > (The .set_polarity callback is faulty as I doesn't commit the request to > hardware, but that shouldn't matter here.) > > I guess the first request the backlight driver emits is > > .period = 33333, > .duty_cycle = 33333, > .enabled = true, > .polarity = PWM_POLARITY_INVERSED, > > which should result into the following driver calls (with and without > the breaking commit): > > tpu_pwm_set_polarity(chip, pwm, PWM_POLARITY_INVERSED); > tpu_pwm_config(chip, pwm, 33333, 33333); > tpu_pwm_enable(chip, pwm); > > Can you confirm that? tpu_pwm_config() is no longer called: renesas-tpu-pwm e6600000.pwm: tpu_pwm_set_polarity:334: channel 2, polarity = 1 -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, duty_ns = 0, period_ns = 33333 -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, duty_ns = 33333, period_ns = 33333 renesas-tpu-pwm e6600000.pwm: tpu_pwm_enable:346: channel 2 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-06-30 10:22 ` Geert Uytterhoeven @ 2021-06-30 16:21 ` Uwe Kleine-König 2021-06-30 17:08 ` Geert Uytterhoeven 2021-06-30 17:18 ` Thierry Reding 0 siblings, 2 replies; 11+ messages in thread From: Uwe Kleine-König @ 2021-06-30 16:21 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linux-Renesas, Linux PWM List, Thierry Reding, Lee Jones, Sascha Hauer [-- Attachment #1: Type: text/plain, Size: 5666 bytes --] Hello, On Wed, Jun 30, 2021 at 12:22:22PM +0200, Geert Uytterhoeven wrote: > Hi Uwe, > > On Wed, Jun 30, 2021 at 8:48 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Tue, Jun 29, 2021 at 09:44:38PM +0200, Geert Uytterhoeven wrote: > > > 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@pengutronix.de> > > > > > > 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? > > > > I starred at the patch for some time now and couldn't find a problem. > > Looking at drivers/pwm/pwm-renesas-tpu.c I don't see something obvious. > > (The .set_polarity callback is faulty as I doesn't commit the request to > > hardware, but that shouldn't matter here.) > > > > I guess the first request the backlight driver emits is > > > > .period = 33333, > > .duty_cycle = 33333, > > .enabled = true, > > .polarity = PWM_POLARITY_INVERSED, > > > > which should result into the following driver calls (with and without > > the breaking commit): > > > > tpu_pwm_set_polarity(chip, pwm, PWM_POLARITY_INVERSED); > > tpu_pwm_config(chip, pwm, 33333, 33333); > > tpu_pwm_enable(chip, pwm); > > > > Can you confirm that? > > tpu_pwm_config() is no longer called: > > renesas-tpu-pwm e6600000.pwm: tpu_pwm_set_polarity:334: channel > 2, polarity = 1 > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, > duty_ns = 0, period_ns = 33333 > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, > duty_ns = 33333, period_ns = 33333 > renesas-tpu-pwm e6600000.pwm: tpu_pwm_enable:346: channel 2 OK, I see a problem (though this doesn't explain the display staying off directly after boot): After doing: pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = false, .polarity = ..}); .period and .duty_cycle are assumed to be set even though calling ->config was skipped because .enabled is false. So when pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = true, .polarity = ..}); is called next, ->config isn't called because the core assumes .duty_cycle and .period are already setup fine. So we either must not skip calling ->config when .enabled is false: diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index ab38627bcacd..f8a5a095a410 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -558,12 +558,8 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, pwm->state.polarity = state->polarity; } - if (!state->enabled) { - if (pwm->state.enabled) - chip->ops->disable(chip, pwm); - - return 0; - } + if (!state->enabled && pwm->state.enabled) + chip->ops->disable(chip, pwm); if (state->period != pwm->state.period || state->duty_cycle != pwm->state.duty_cycle) { @@ -577,7 +573,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, pwm->state.duty_cycle = state->duty_cycle; } - if (!pwm->state.enabled) { + if (state->enabled && !pwm->state.enabled) { err = chip->ops->enable(chip, pwm); if (err) goto rollback; or we have to call ->config unconditionally: diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index ab38627bcacd..05d7afe25b42 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -565,17 +565,21 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, return 0; } + /* + * We cannot skip this even if state->period == pwm->state.period && + * state->duty_cycle == pwm->state.duty_cycle because we might have + * exited early in the last call to pwm_apply_state because of + * !state->enabled and so the two values in pwm->state might not be + * configured in hardware. + */ + err = chip->ops->config(pwm->chip, pwm, + state->duty_cycle, + state->period); + if (err) + goto rollback; + + pwm->state.period = state->period; + pwm->state.duty_cycle = state->duty_cycle; - 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 rollback; - - pwm->state.period = state->period; - pwm->state.duty_cycle = state->duty_cycle; - } if (!pwm->state.enabled) { err = chip->ops->enable(chip, pwm); I slightly prefer the latter patch, but if this is indeed your problem both should fix it for you. Can you give that a try please? 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 --] ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-06-30 16:21 ` Uwe Kleine-König @ 2021-06-30 17:08 ` Geert Uytterhoeven 2021-06-30 17:18 ` Thierry Reding 1 sibling, 0 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2021-06-30 17:08 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linux-Renesas, Linux PWM List, Thierry Reding, Lee Jones, Sascha Hauer Hi Uwe, On Wed, Jun 30, 2021 at 6:21 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Wed, Jun 30, 2021 at 12:22:22PM +0200, Geert Uytterhoeven wrote: > > On Wed, Jun 30, 2021 at 8:48 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Tue, Jun 29, 2021 at 09:44:38PM +0200, Geert Uytterhoeven wrote: > > > > 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@pengutronix.de> > > > > > > > > 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? > > > > > > I starred at the patch for some time now and couldn't find a problem. > > > Looking at drivers/pwm/pwm-renesas-tpu.c I don't see something obvious. > > > (The .set_polarity callback is faulty as I doesn't commit the request to > > > hardware, but that shouldn't matter here.) > > > > > > I guess the first request the backlight driver emits is > > > > > > .period = 33333, > > > .duty_cycle = 33333, > > > .enabled = true, > > > .polarity = PWM_POLARITY_INVERSED, > > > > > > which should result into the following driver calls (with and without > > > the breaking commit): > > > > > > tpu_pwm_set_polarity(chip, pwm, PWM_POLARITY_INVERSED); > > > tpu_pwm_config(chip, pwm, 33333, 33333); > > > tpu_pwm_enable(chip, pwm); > > > > > > Can you confirm that? > > > > tpu_pwm_config() is no longer called: > > > > renesas-tpu-pwm e6600000.pwm: tpu_pwm_set_polarity:334: channel > > 2, polarity = 1 > > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, > > duty_ns = 0, period_ns = 33333 > > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, > > duty_ns = 33333, period_ns = 33333 > > renesas-tpu-pwm e6600000.pwm: tpu_pwm_enable:346: channel 2 > > OK, I see a problem (though this doesn't explain the display staying > off directly after boot): > > After doing: > > pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = false, .polarity = ..}); > > .period and .duty_cycle are assumed to be set even though calling > ->config was skipped because .enabled is false. > > So when > > pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = true, .polarity = ..}); > > is called next, ->config isn't called because the core assumes > .duty_cycle and .period are already setup fine. > > So we either must not skip calling ->config when .enabled is false: > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index ab38627bcacd..f8a5a095a410 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -558,12 +558,8 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, > pwm->state.polarity = state->polarity; > } > > - if (!state->enabled) { > - if (pwm->state.enabled) > - chip->ops->disable(chip, pwm); > - > - return 0; > - } > + if (!state->enabled && pwm->state.enabled) > + chip->ops->disable(chip, pwm); > > if (state->period != pwm->state.period || > state->duty_cycle != pwm->state.duty_cycle) { > @@ -577,7 +573,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, > pwm->state.duty_cycle = state->duty_cycle; > } > > - if (!pwm->state.enabled) { > + if (state->enabled && !pwm->state.enabled) { > err = chip->ops->enable(chip, pwm); > if (err) > goto rollback; Works. > or we have to call ->config unconditionally: > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index ab38627bcacd..05d7afe25b42 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -565,17 +565,21 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, > return 0; > } > > + /* > + * We cannot skip this even if state->period == pwm->state.period && > + * state->duty_cycle == pwm->state.duty_cycle because we might have > + * exited early in the last call to pwm_apply_state because of > + * !state->enabled and so the two values in pwm->state might not be > + * configured in hardware. > + */ > + err = chip->ops->config(pwm->chip, pwm, > + state->duty_cycle, > + state->period); > + if (err) > + goto rollback; > + > + pwm->state.period = state->period; > + pwm->state.duty_cycle = state->duty_cycle; > - 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 rollback; > - > - pwm->state.period = state->period; > - pwm->state.duty_cycle = state->duty_cycle; > - } > > if (!pwm->state.enabled) { > err = chip->ops->enable(chip, pwm); > > I slightly prefer the latter patch, but if this is indeed your problem > both should fix it for you. > > Can you give that a try please? Works, too. Thanks! Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-06-30 16:21 ` Uwe Kleine-König 2021-06-30 17:08 ` Geert Uytterhoeven @ 2021-06-30 17:18 ` Thierry Reding 1 sibling, 0 replies; 11+ messages in thread From: Thierry Reding @ 2021-06-30 17:18 UTC (permalink / raw) To: Uwe Kleine-König Cc: Geert Uytterhoeven, Linux-Renesas, Linux PWM List, Lee Jones, Sascha Hauer [-- Attachment #1: Type: text/plain, Size: 6419 bytes --] On Wed, Jun 30, 2021 at 06:21:28PM +0200, Uwe Kleine-König wrote: > Hello, > > On Wed, Jun 30, 2021 at 12:22:22PM +0200, Geert Uytterhoeven wrote: > > Hi Uwe, > > > > On Wed, Jun 30, 2021 at 8:48 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Tue, Jun 29, 2021 at 09:44:38PM +0200, Geert Uytterhoeven wrote: > > > > 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@pengutronix.de> > > > > > > > > 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? > > > > > > I starred at the patch for some time now and couldn't find a problem. > > > Looking at drivers/pwm/pwm-renesas-tpu.c I don't see something obvious. > > > (The .set_polarity callback is faulty as I doesn't commit the request to > > > hardware, but that shouldn't matter here.) > > > > > > I guess the first request the backlight driver emits is > > > > > > .period = 33333, > > > .duty_cycle = 33333, > > > .enabled = true, > > > .polarity = PWM_POLARITY_INVERSED, > > > > > > which should result into the following driver calls (with and without > > > the breaking commit): > > > > > > tpu_pwm_set_polarity(chip, pwm, PWM_POLARITY_INVERSED); > > > tpu_pwm_config(chip, pwm, 33333, 33333); > > > tpu_pwm_enable(chip, pwm); > > > > > > Can you confirm that? > > > > tpu_pwm_config() is no longer called: > > > > renesas-tpu-pwm e6600000.pwm: tpu_pwm_set_polarity:334: channel > > 2, polarity = 1 > > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, > > duty_ns = 0, period_ns = 33333 > > -renesas-tpu-pwm e6600000.pwm: tpu_pwm_config:257: channel = 2, > > duty_ns = 33333, period_ns = 33333 > > renesas-tpu-pwm e6600000.pwm: tpu_pwm_enable:346: channel 2 > > OK, I see a problem (though this doesn't explain the display staying > off directly after boot): > > After doing: > > pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = false, .polarity = ..}); > > .period and .duty_cycle are assumed to be set even though calling > ->config was skipped because .enabled is false. > > So when > > pwm_apply_state(pwm, { .period = 33333, .duty_cycle = 0, .enabled = true, .polarity = ..}); > > is called next, ->config isn't called because the core assumes > .duty_cycle and .period are already setup fine. > > So we either must not skip calling ->config when .enabled is false: > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index ab38627bcacd..f8a5a095a410 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -558,12 +558,8 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, > pwm->state.polarity = state->polarity; > } > > - if (!state->enabled) { > - if (pwm->state.enabled) > - chip->ops->disable(chip, pwm); > - > - return 0; > - } > + if (!state->enabled && pwm->state.enabled) > + chip->ops->disable(chip, pwm); > > if (state->period != pwm->state.period || > state->duty_cycle != pwm->state.duty_cycle) { > @@ -577,7 +573,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, > pwm->state.duty_cycle = state->duty_cycle; > } > > - if (!pwm->state.enabled) { > + if (state->enabled && !pwm->state.enabled) { > err = chip->ops->enable(chip, pwm); > if (err) > goto rollback; > > or we have to call ->config unconditionally: > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index ab38627bcacd..05d7afe25b42 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -565,17 +565,21 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, > return 0; > } > > + /* > + * We cannot skip this even if state->period == pwm->state.period && > + * state->duty_cycle == pwm->state.duty_cycle because we might have > + * exited early in the last call to pwm_apply_state because of > + * !state->enabled and so the two values in pwm->state might not be > + * configured in hardware. > + */ > + err = chip->ops->config(pwm->chip, pwm, > + state->duty_cycle, > + state->period); > + if (err) > + goto rollback; > + > + pwm->state.period = state->period; > + pwm->state.duty_cycle = state->duty_cycle; > - 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 rollback; > - > - pwm->state.period = state->period; > - pwm->state.duty_cycle = state->duty_cycle; > - } > > if (!pwm->state.enabled) { > err = chip->ops->enable(chip, pwm); > > I slightly prefer the latter patch, but if this is indeed your problem > both should fix it for you. The first variant has the benefit of writing all of the state through to hardware, which is more likely to be what a proper atomic implementation would do. Technically it shouldn't be necessary to reconfigure *and* disable a PWM, but it's traditionally the safer way to make sure the hardware is really "off". In any case, I've dropped the original patch for now. Feel free to send an updated patch with either of the above, I don't have a strong enough preference for either, so take your pick. But given that this is riskier than I thought, I'll defer this to v5.15. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 2021-05-01 16:09 ` [PATCH v2] " Uwe Kleine-König 2021-06-28 11:46 ` Thierry Reding 2021-06-29 19:44 ` Geert Uytterhoeven @ 2021-06-29 19:52 ` Geert Uytterhoeven 2 siblings, 0 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2021-06-29 19:52 UTC (permalink / raw) To: Uwe Kleine-König Cc: Thierry Reding, Lee Jones, linux-pwm, kernel, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 6009 bytes --] 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@pengutronix.de> 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@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-30 17:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-11 16:04 [PATCH] pwm: Ensure for legacy drivers that pwm->state stays consistent Uwe Kleine-König 2021-05-01 16:09 ` [PATCH v2] " Uwe Kleine-König 2021-06-28 11:46 ` Thierry Reding 2021-06-28 17:24 ` Uwe Kleine-König 2021-06-29 19:44 ` Geert Uytterhoeven 2021-06-30 6:48 ` Uwe Kleine-König 2021-06-30 10:22 ` Geert Uytterhoeven 2021-06-30 16:21 ` Uwe Kleine-König 2021-06-30 17:08 ` Geert Uytterhoeven 2021-06-30 17:18 ` Thierry Reding 2021-06-29 19:52 ` Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.