[PATCH] pwm: Ensure for legacy drivers that pwm->state stays consistent

[PATCH] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Uwe Kleine-König]

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

[PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Uwe Kleine-König]

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

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Thierry Reding]

[-- 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 --]

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Uwe Kleine-König]

[-- 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 --]

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Geert Uytterhoeven]

[-- 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

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Geert Uytterhoeven]

[-- 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

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Uwe Kleine-König]

[-- 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 --]

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Geert Uytterhoeven]

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

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Uwe Kleine-König]

[-- 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 --]

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Geert Uytterhoeven]

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

Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent

[Thierry Reding]

[-- 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 --]