[PATCH 0/3] pwm: Some improvements for legacy drivers

[PATCH 0/3] pwm: Some improvements for legacy drivers

[Uwe Kleine-König]

Hello,

this is the successor of my earlier patch "pwm: Ensure for legacy
drivers that pwm->state stays consistent" that was applied shortly to
next until Geert found a problem with it.

I split the patch in three parts now: First the legacy handling is just
moved to a separate function without any semantic change. Then a glitch
is fixed, but without the regression I introduced initially. In the
third and last patch the longstanding FIXME about breaking pwm->state if
a callback fails is addressed.

Uwe Kleine-König (3):
  pwm: Move legacy driver handling into a dedicated function
  pwm: Prevent a glitch for legacy drivers
  pwm: Restore initial state if a legacy callback fails

 drivers/pwm/core.c | 139 ++++++++++++++++++++++++++-------------------
 1 file changed, 79 insertions(+), 60 deletions(-)


base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
-- 
2.30.2

[PATCH 1/3] pwm: Move legacy driver handling into a dedicated function

[Uwe Kleine-König]

There is no change in behaviour, only some code is moved from
pwm_apply_state to a separate function.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 130 ++++++++++++++++++++++++---------------------
 1 file changed, 70 insertions(+), 60 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c4d5c0667137..3c72f8963073 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -535,6 +535,64 @@ 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;
+
+	/*
+	 * 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);
+
+			/*
+			 * 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)
+			return err;
+
+		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.period = state->period;
+		pwm->state.duty_cycle = state->duty_cycle;
+	}
+
+	if (state->enabled != pwm->state.enabled) {
+		if (!pwm->state.enabled) {
+			err = chip->ops->enable(chip, pwm);
+			if (err)
+				return err;
+		} else {
+			chip->ops->disable(chip, pwm);
+		}
+	}
+
+	return 0;
+}
+
 /**
  * pwm_apply_state() - atomically apply a new state to a PWM device
  * @pwm: PWM device
@@ -557,70 +615,22 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	    state->enabled == pwm->state.enabled)
 		return 0;
 
-	if (chip->ops->apply) {
+	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;
-			}
-
-			err = chip->ops->set_polarity(chip, pwm,
-						      state->polarity);
-			if (err)
-				return err;
-
-			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;
+	else
+		err = pwm_apply_legacy(chip, pwm, state);
+	if (err)
+		return err;
 
-			pwm->state.duty_cycle = state->duty_cycle;
-			pwm->state.period = state->period;
-		}
+	trace_pwm_apply(pwm, state);
 
-		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 = *state;
 
-			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

[PATCH 2/3] pwm: Prevent a glitch for legacy drivers

[Uwe Kleine-König]

If a running PWM is reconfigured to disabled calling the ->config()
callback before disabling the hardware might result in a glitch where
the (maybe) new period and duty_cycle are visible on the output before
disabling the hardware.

So handle disabling before calling ->config(). Also exit early in this case
which is possible because period and duty_cycle don't matter for disabled PWMs.
In return however ->config has to be called even if state->period ==
pwm->state.period && state->duty_cycle != pwm->state.duty_cycle because setting
these might have been skipped in the previous call.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3c72f8963073..20afe6d0bc5e 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -568,26 +568,33 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
 		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;
+	if (!state->enabled) {
+		if (pwm->state.enabled)
+			chip->ops->disable(chip, pwm);
 
-		pwm->state.period = state->period;
-		pwm->state.duty_cycle = state->duty_cycle;
+		return 0;
 	}
 
-	if (state->enabled != pwm->state.enabled) {
-		if (!pwm->state.enabled) {
-			err = chip->ops->enable(chip, pwm);
-			if (err)
-				return err;
-		} else {
-			chip->ops->disable(chip, pwm);
-		}
+	/*
+	 * We cannot skip calling ->config 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)
+		return 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)
+			return err;
 	}
 
 	return 0;
-- 
2.30.2

[PATCH 3/3] pwm: Restore initial state if a legacy callback fails

[Uwe Kleine-König]

It is not entirely accurate to go back to the initial state after e.g.
.enable() failed, as .config() still modified the hardware, but this same
inconsistency exists for drivers that implement .apply().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 20afe6d0bc5e..6e30ef9b9b79 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -539,10 +539,8 @@ 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;
 
-	/*
-	 * FIXME: restore the initial state in case of error.
-	 */
 	if (state->polarity != pwm->state.polarity) {
 		if (!chip->ops->set_polarity)
 			return -EINVAL;
@@ -563,7 +561,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		err = chip->ops->set_polarity(chip, pwm, state->polarity);
 		if (err)
-			return err;
+			goto rollback;
 
 		pwm->state.polarity = state->polarity;
 	}
@@ -586,7 +584,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
 				state->duty_cycle,
 				state->period);
 	if (err)
-		return err;
+		goto rollback;
 
 	pwm->state.period = state->period;
 	pwm->state.duty_cycle = state->duty_cycle;
@@ -594,10 +592,14 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (!pwm->state.enabled) {
 		err = chip->ops->enable(chip, pwm);
 		if (err)
-			return err;
+			goto rollback;
 	}
 
 	return 0;
+
+rollback:
+	pwm->state = initial_state;
+	return err;
 }
 
 /**
-- 
2.30.2

Re: [PATCH 0/3] pwm: Some improvements for legacy drivers

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Jul 1, 2021 at 9:29 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> this is the successor of my earlier patch "pwm: Ensure for legacy
> drivers that pwm->state stays consistent" that was applied shortly to
> next until Geert found a problem with it.
>
> I split the patch in three parts now: First the legacy handling is just
> moved to a separate function without any semantic change. Then a glitch
> is fixed, but without the regression I introduced initially. In the
> third and last patch the longstanding FIXME about breaking pwm->state if
> a callback fails is addressed.
>
> Uwe Kleine-König (3):
>   pwm: Move legacy driver handling into a dedicated function
>   pwm: Prevent a glitch for legacy drivers
>   pwm: Restore initial state if a legacy callback fails
>
>  drivers/pwm/core.c | 139 ++++++++++++++++++++++++++-------------------
>  1 file changed, 79 insertions(+), 60 deletions(-)

Thanks, works fine on Armadillo 800 EVA!
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

> base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5

That's plain v5.13-rc1, which is probably not what Thierry is targeting?

The first patch applies with some fuzz on my local tree including
yesterday's pwm/for-next and a revert of the bad version.

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 0/3] pwm: Some improvements for legacy drivers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

On Thu, Jul 01, 2021 at 10:58:32AM +0200, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> On Thu, Jul 1, 2021 at 9:29 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > this is the successor of my earlier patch "pwm: Ensure for legacy
> > drivers that pwm->state stays consistent" that was applied shortly to
> > next until Geert found a problem with it.
> >
> > I split the patch in three parts now: First the legacy handling is just
> > moved to a separate function without any semantic change. Then a glitch
> > is fixed, but without the regression I introduced initially. In the
> > third and last patch the longstanding FIXME about breaking pwm->state if
> > a callback fails is addressed.
> >
> > Uwe Kleine-König (3):
> >   pwm: Move legacy driver handling into a dedicated function
> >   pwm: Prevent a glitch for legacy drivers
> >   pwm: Restore initial state if a legacy callback fails
> >
> >  drivers/pwm/core.c | 139 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 79 insertions(+), 60 deletions(-)
> 
> Thanks, works fine on Armadillo 800 EVA!
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks for testing.

> > base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
> 
> That's plain v5.13-rc1, which is probably not what Thierry is targeting?

his for-next branch is based on v5.13-rc1 and there are no changes in it
touching drivers/pwm/core.c, so I expect this to be fine.
 
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 0/3] pwm: Some improvements for legacy drivers

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Jul 1, 2021 at 12:45 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Jul 01, 2021 at 10:58:32AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Jul 1, 2021 at 9:29 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > this is the successor of my earlier patch "pwm: Ensure for legacy
> > > drivers that pwm->state stays consistent" that was applied shortly to
> > > next until Geert found a problem with it.
> > >
> > > I split the patch in three parts now: First the legacy handling is just
> > > moved to a separate function without any semantic change. Then a glitch
> > > is fixed, but without the regression I introduced initially. In the
> > > third and last patch the longstanding FIXME about breaking pwm->state if
> > > a callback fails is addressed.
> > >
> > > Uwe Kleine-König (3):
> > >   pwm: Move legacy driver handling into a dedicated function
> > >   pwm: Prevent a glitch for legacy drivers
> > >   pwm: Restore initial state if a legacy callback fails
> > >
> > >  drivers/pwm/core.c | 139 ++++++++++++++++++++++++++-------------------
> > >  1 file changed, 79 insertions(+), 60 deletions(-)
> >
> > Thanks, works fine on Armadillo 800 EVA!
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Thanks for testing.
>
> > > base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
> >
> > That's plain v5.13-rc1, which is probably not what Thierry is targeting?
>
> his for-next branch is based on v5.13-rc1 and there are no changes in it
> touching drivers/pwm/core.c, so I expect this to be fine.

Git tends to disagree:

$ git log --oneline v5.13-rc1..pwm/for-next -- drivers/pwm/core.c
9ae241d06ef7aca8 pwm: core: Simplify some devm_*pwm*() functions
c333b936c1530e76 pwm: core: Remove unused devm_pwm_put()
e625fb70a6d21e4d pwm: core: Unify fwnode checks in the module
e5c38ba9f2813beb pwm: core: Reuse fwnode_to_pwmchip() in ACPI case
ca06616b1eed3112 pwm: core: Convert to use fwnode for matching
ad5e085c63f59391 pwm: Drop irrelevant error path from pwmchip_remove()
bcda91bf86c1ff76 pwm: Add a device-managed function to add PWM chips
9e40ee18a1dc1623 pwm: core: Support new usage_power setting in PWM state
69230cfac3d02c1b pwm: Autodetect default value for of_pwm_n_cells from
device tree
5447e7833629ee42 pwm: Drop of_pwm_simple_xlate() in favour of
of_pwm_xlate_with_flags()
cf38c978cf1d2a28 pwm: Make of_pwm_xlate_with_flags() work with #pwm-cells = <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 0/3] pwm: Some improvements for legacy drivers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

Hello Geert,

On Thu, Jul 01, 2021 at 01:41:19PM +0200, Geert Uytterhoeven wrote:
> On Thu, Jul 1, 2021 at 12:45 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > his for-next branch is based on v5.13-rc1 and there are no changes in it
> > touching drivers/pwm/core.c, so I expect this to be fine.
> 
> Git tends to disagree:
> 
> $ git log --oneline v5.13-rc1..pwm/for-next -- drivers/pwm/core.c
> 9ae241d06ef7aca8 pwm: core: Simplify some devm_*pwm*() functions
> c333b936c1530e76 pwm: core: Remove unused devm_pwm_put()
> e625fb70a6d21e4d pwm: core: Unify fwnode checks in the module
> e5c38ba9f2813beb pwm: core: Reuse fwnode_to_pwmchip() in ACPI case
> ca06616b1eed3112 pwm: core: Convert to use fwnode for matching
> ad5e085c63f59391 pwm: Drop irrelevant error path from pwmchip_remove()
> bcda91bf86c1ff76 pwm: Add a device-managed function to add PWM chips
> 9e40ee18a1dc1623 pwm: core: Support new usage_power setting in PWM state
> 69230cfac3d02c1b pwm: Autodetect default value for of_pwm_n_cells from
> device tree
> 5447e7833629ee42 pwm: Drop of_pwm_simple_xlate() in favour of
> of_pwm_xlate_with_flags()
> cf38c978cf1d2a28 pwm: Make of_pwm_xlate_with_flags() work with #pwm-cells = <2>

I thought this was my command line, too, but *now* it shows some output.
I'm unable to find out what I did wrong ...  *shrug* I assume and hope
Thierry is able to cope with that.

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 3/3] pwm: Restore initial state if a legacy callback fails

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

On Thu, Jul 01, 2021 at 09:29:27AM +0200, Uwe Kleine-König wrote:
> It is not entirely accurate to go back to the initial state after e.g.
> .enable() failed, as .config() still modified the hardware, but this same
> inconsistency exists for drivers that implement .apply().
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 20afe6d0bc5e..6e30ef9b9b79 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -539,10 +539,8 @@ 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;
>  
> -	/*
> -	 * FIXME: restore the initial state in case of error.
> -	 */
>  	if (state->polarity != pwm->state.polarity) {
>  		if (!chip->ops->set_polarity)
>  			return -EINVAL;
> @@ -563,7 +561,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  		err = chip->ops->set_polarity(chip, pwm, state->polarity);
>  		if (err)
> -			return err;
> +			goto rollback;
>  
>  		pwm->state.polarity = state->polarity;
>  	}
> @@ -586,7 +584,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  				state->duty_cycle,
>  				state->period);
>  	if (err)
> -		return err;
> +		goto rollback;
>  
>  	pwm->state.period = state->period;
>  	pwm->state.duty_cycle = state->duty_cycle;
> @@ -594,10 +592,14 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (!pwm->state.enabled) {
>  		err = chip->ops->enable(chip, pwm);
>  		if (err)
> -			return err;
> +			goto rollback;
>  	}
>  
>  	return 0;
> +
> +rollback:
> +	pwm->state = initial_state;
> +	return err;
>  }

Can't we achieve the same thing by just removing all the updates to
pwm->state in pwm_apply_legacy()? Patch 1 in the series now does
pwm->state = *state for both the atomic and the legacy cases, so if
we don't update pwm->state explicitly in pwm_apply_legacy(), then
there should be no need to rollback, right?

What we currently do is a bit redundant anyway. pwm->state = *state
should be a no-op after pwm_apply_legacy().

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] pwm: Prevent a glitch for legacy drivers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3189 bytes --]

On Thu, Jul 01, 2021 at 09:29:26AM +0200, Uwe Kleine-König wrote:
> If a running PWM is reconfigured to disabled calling the ->config()
> callback before disabling the hardware might result in a glitch where
> the (maybe) new period and duty_cycle are visible on the output before
> disabling the hardware.
> 
> So handle disabling before calling ->config(). Also exit early in this case
> which is possible because period and duty_cycle don't matter for disabled PWMs.
> In return however ->config has to be called even if state->period ==
> pwm->state.period && state->duty_cycle != pwm->state.duty_cycle because setting
> these might have been skipped in the previous call.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3c72f8963073..20afe6d0bc5e 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -568,26 +568,33 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
>  		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;
> +	if (!state->enabled) {
> +		if (pwm->state.enabled)
> +			chip->ops->disable(chip, pwm);
>  
> -		pwm->state.period = state->period;
> -		pwm->state.duty_cycle = state->duty_cycle;
> +		return 0;
>  	}
>  
> -	if (state->enabled != pwm->state.enabled) {
> -		if (!pwm->state.enabled) {
> -			err = chip->ops->enable(chip, pwm);
> -			if (err)
> -				return err;
> -		} else {
> -			chip->ops->disable(chip, pwm);
> -		}
> +	/*
> +	 * We cannot skip calling ->config 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)
> +		return 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)
> +			return err;
>  	}

I thought we might have discussed this, but I can't find any record of
it. How is this better than always configuring, whether the PWM is
disabled or not?

From an atomic point of view, the hardware state is expected to match
the PWM state that was passed to ->apply() after it returns. That means
that calling ->get_state() after ->apply() should return the same state
that was passed to ->apply(). With the above that's no longer true. It
doesn't actually matter, because legacy drivers don't support
->get_state(), but conceptually it's not mimicking the atomic API as
well as it could.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] pwm: Prevent a glitch for legacy drivers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4797 bytes --]

Hello Thierry,

On Thu, Aug 19, 2021 at 03:36:26PM +0200, Thierry Reding wrote:
> On Thu, Jul 01, 2021 at 09:29:26AM +0200, Uwe Kleine-König wrote:
> > If a running PWM is reconfigured to disabled calling the ->config()
> > callback before disabling the hardware might result in a glitch where
> > the (maybe) new period and duty_cycle are visible on the output before
> > disabling the hardware.
> > 
> > So handle disabling before calling ->config(). Also exit early in this case
> > which is possible because period and duty_cycle don't matter for disabled PWMs.
> > In return however ->config has to be called even if state->period ==
> > pwm->state.period && state->duty_cycle != pwm->state.duty_cycle because setting
> > these might have been skipped in the previous call.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/core.c | 41 ++++++++++++++++++++++++-----------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 3c72f8963073..20afe6d0bc5e 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -568,26 +568,33 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> >  		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;
> > +	if (!state->enabled) {
> > +		if (pwm->state.enabled)
> > +			chip->ops->disable(chip, pwm);
> >  
> > -		pwm->state.period = state->period;
> > -		pwm->state.duty_cycle = state->duty_cycle;
> > +		return 0;
> >  	}
> >  
> > -	if (state->enabled != pwm->state.enabled) {
> > -		if (!pwm->state.enabled) {
> > -			err = chip->ops->enable(chip, pwm);
> > -			if (err)
> > -				return err;
> > -		} else {
> > -			chip->ops->disable(chip, pwm);
> > -		}
> > +	/*
> > +	 * We cannot skip calling ->config 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)
> > +		return 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)
> > +			return err;
> >  	}
> 
> I thought we might have discussed this, but I can't find any record of
> it. How is this better than always configuring, whether the PWM is
> disabled or not?

The question is: Should we call .config() if state->enabled is false,
right?

> From an atomic point of view, the hardware state is expected to match
> the PWM state that was passed to ->apply() after it returns. That means
> that calling ->get_state() after ->apply() should return the same state
> that was passed to ->apply(). With the above that's no longer true. It
> doesn't actually matter, because legacy drivers don't support
> ->get_state(), but conceptually it's not mimicking the atomic API as
> well as it could.

In my book the value of .period and .duty_cycle is irrelevant when
state->enabled is false as it has no effect on the actual behaviour of
the hardware. And then calling .config() with all the calculations that
are done there just to have the values stored in hardware doesn't make
much sense for me. Still more because this isn't possible for all
hardwares as not all support the concept of "disabled" and have to
simulate it using .duty_cycle = 0.

For me it is ok to deviate here, callers of .get_state() have to be
aware of other hardware induced deviations anyhow. If having a match
here is really wanted, then I suggest to fixup in the framework instead
of adapting all drivers (if and when we will have a consumer reachable
function that makes use of .get_state()). So for example do something
like:

	ret = chip->ops->get_state(..., &state);
	if (ret)
		return ret;

	/* Normalize */
	if (!state->enabled) {
		state->period = pwm->state->period;
		state->duty_cycle = pwm->state->duty_cycle;
	}

but also assigning 0 instead of pwm->state->{period,duty_cycle} would
work for me.

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 3/3] pwm: Restore initial state if a legacy callback fails

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]

On Thu, Aug 19, 2021 at 03:28:31PM +0200, Thierry Reding wrote:
> On Thu, Jul 01, 2021 at 09:29:27AM +0200, Uwe Kleine-König wrote:
> > It is not entirely accurate to go back to the initial state after e.g.
> > .enable() failed, as .config() still modified the hardware, but this same
> > inconsistency exists for drivers that implement .apply().
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/core.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 20afe6d0bc5e..6e30ef9b9b79 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -539,10 +539,8 @@ 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;
> >  
> > -	/*
> > -	 * FIXME: restore the initial state in case of error.
> > -	 */
> >  	if (state->polarity != pwm->state.polarity) {
> >  		if (!chip->ops->set_polarity)
> >  			return -EINVAL;
> > @@ -563,7 +561,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> >  
> >  		err = chip->ops->set_polarity(chip, pwm, state->polarity);
> >  		if (err)
> > -			return err;
> > +			goto rollback;
> >  
> >  		pwm->state.polarity = state->polarity;
> >  	}
> > @@ -586,7 +584,7 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> >  				state->duty_cycle,
> >  				state->period);
> >  	if (err)
> > -		return err;
> > +		goto rollback;
> >  
> >  	pwm->state.period = state->period;
> >  	pwm->state.duty_cycle = state->duty_cycle;
> > @@ -594,10 +592,14 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (!pwm->state.enabled) {
> >  		err = chip->ops->enable(chip, pwm);
> >  		if (err)
> > -			return err;
> > +			goto rollback;
> >  	}
> >  
> >  	return 0;
> > +
> > +rollback:
> > +	pwm->state = initial_state;
> > +	return err;
> >  }
> 
> Can't we achieve the same thing by just removing all the updates to
> pwm->state in pwm_apply_legacy()? Patch 1 in the series now does
> pwm->state = *state for both the atomic and the legacy cases, so if
> we don't update pwm->state explicitly in pwm_apply_legacy(), then
> there should be no need to rollback, right?

No, this doesn't work because the implemention of the callbacks might
rely on ->state holding the result from a previous callback.
See for example drivers/pwm/pwm-lpc18xx-sct.c where .enable() calls
pwm_get_polarity().

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 0/3] pwm: Some improvements for legacy drivers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

Hello Thierry,

On Thu, Jul 01, 2021 at 09:29:24AM +0200, Uwe Kleine-König wrote:
> this is the successor of my earlier patch "pwm: Ensure for legacy
> drivers that pwm->state stays consistent" that was applied shortly to
> next until Geert found a problem with it.
> 
> I split the patch in three parts now: First the legacy handling is just
> moved to a separate function without any semantic change. Then a glitch
> is fixed, but without the regression I introduced initially. In the
> third and last patch the longstanding FIXME about breaking pwm->state if
> a callback fails is addressed.

I noticed you collected patches for the current merge window, but among
others skipped this series.

You mentioned two doubts, but never reacted on my replies that (IMHO)
should remove your doubts.

Can you please voice your concerns?

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 0/3] pwm: Some improvements for legacy drivers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Thu, Jul 01, 2021 at 09:29:24AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this is the successor of my earlier patch "pwm: Ensure for legacy
> drivers that pwm->state stays consistent" that was applied shortly to
> next until Geert found a problem with it.
> 
> I split the patch in three parts now: First the legacy handling is just
> moved to a separate function without any semantic change. Then a glitch
> is fixed, but without the regression I introduced initially. In the
> third and last patch the longstanding FIXME about breaking pwm->state if
> a callback fails is addressed.
> 
> Uwe Kleine-König (3):
>   pwm: Move legacy driver handling into a dedicated function
>   pwm: Prevent a glitch for legacy drivers
>   pwm: Restore initial state if a legacy callback fails
> 
>  drivers/pwm/core.c | 139 ++++++++++++++++++++++++++-------------------
>  1 file changed, 79 insertions(+), 60 deletions(-)

I've applied this to for-next, let's see if this blows up or not.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] pwm: Some improvements for legacy drivers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Wed, Nov 17, 2021 at 05:12:17PM +0100, Thierry Reding wrote:
> On Thu, Jul 01, 2021 at 09:29:24AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this is the successor of my earlier patch "pwm: Ensure for legacy
> > drivers that pwm->state stays consistent" that was applied shortly to
> > next until Geert found a problem with it.
> > 
> > I split the patch in three parts now: First the legacy handling is just
> > moved to a separate function without any semantic change. Then a glitch
> > is fixed, but without the regression I introduced initially. In the
> > third and last patch the longstanding FIXME about breaking pwm->state if
> > a callback fails is addressed.
> > 
> > Uwe Kleine-König (3):
> >   pwm: Move legacy driver handling into a dedicated function
> >   pwm: Prevent a glitch for legacy drivers
> >   pwm: Restore initial state if a legacy callback fails
> > 
> >  drivers/pwm/core.c | 139 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 79 insertions(+), 60 deletions(-)
> 
> I've applied this to for-next, let's see if this blows up or not.

Up to now for-next didn't blow up, but the simplest explanation isn't
that my patches are fine but that for-next still points at
pwm/for-5.16-rc1 which is already in Linus' tree. I suspect you forget
to push?

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