linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent
       [not found] ` <20210501160943.108821-1-u.kleine-koenig@pengutronix.de>
@ 2021-06-29 19:44   ` Geert Uytterhoeven
  2021-06-30  6:48     ` Uwe Kleine-König
  2021-06-29 19:52   ` Geert Uytterhoeven
  1 sibling, 1 reply; 7+ 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] 7+ messages in thread

* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent
       [not found] ` <20210501160943.108821-1-u.kleine-koenig@pengutronix.de>
  2021-06-29 19:44   ` [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent Geert Uytterhoeven
@ 2021-06-29 19:52   ` Geert Uytterhoeven
  1 sibling, 0 replies; 7+ 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] 7+ messages in thread

* Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent
  2021-06-29 19:44   ` [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent Geert Uytterhoeven
@ 2021-06-30  6:48     ` Uwe Kleine-König
  2021-06-30 10:22       ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread

end of thread, other threads:[~2021-06-30 17:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210411160451.1207799-1-u.kleine-koenig@pengutronix.de>
     [not found] ` <20210501160943.108821-1-u.kleine-koenig@pengutronix.de>
2021-06-29 19:44   ` [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).