[PATCH 0/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[PATCH 0/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

From: Uwe Kleine-König <uwe@kleine-koenig.org>

Hello,

Geert Uytterhoeven found a regression in one of my patches. The same
problem exists in several further commits. The respective drivers are
fixed in this series.

The affected commits for the first patch is already in v5.4, so this
patch should maybe backported to stable.
The others are in Thierry's for-next branch only.

Uwe Kleine-König (5):
  pwm: sprd: Ensure configuring period and duty_cycle isn't wrongly
    skipped
  pwm: spear: Ensure configuring period and duty_cycle isn't wrongly
    skipped
  pwm: tiecap: Ensure configuring period and duty_cycle isn't wrongly
    skipped
  pwm: berlin: Ensure configuring period and duty_cycle isn't wrongly
    skipped
  pwm: ep93xx: Ensure configuring period and duty_cycle isn't wrongly
    skipped

 drivers/pwm/pwm-berlin.c |  9 ++---
 drivers/pwm/pwm-ep93xx.c | 85 +++++++++++++++++++---------------------
 drivers/pwm/pwm-spear.c  |  9 ++---
 drivers/pwm/pwm-sprd.c   | 11 ++----
 drivers/pwm/pwm-tiecap.c | 15 +++----
 5 files changed, 56 insertions(+), 73 deletions(-)


base-commit: 96e45e5202377da39d086ec19f8934ebcc5b0fd6
-- 
2.30.2

[PATCH 1/5] pwm: sprd: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

As the last call to sprd_pwm_apply() might have exited early if
state->enabled was false, the values for period and duty_cycle stored in
pwm->state might not have been written to hardware and it must be
ensured that they are configured before enabling the PWM.

Fixes: 8aae4b02e8a6 ("pwm: sprd: Add Spreadtrum PWM support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-sprd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
index f2a85e8dd941..7004f55bbf11 100644
--- a/drivers/pwm/pwm-sprd.c
+++ b/drivers/pwm/pwm-sprd.c
@@ -183,13 +183,10 @@ static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			}
 		}
 
-		if (state->period != cstate->period ||
-		    state->duty_cycle != cstate->duty_cycle) {
-			ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
-					      state->period);
-			if (ret)
-				return ret;
-		}
+		ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
+				      state->period);
+		if (ret)
+			return ret;
 
 		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
 	} else if (cstate->enabled) {
-- 
2.30.2

[PATCH 2/5] pwm: spear: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

As the last call to spear_pwm_apply() might have exited early if
state->enabled was false, the values for period and duty_cycle stored in
pwm->state might not have been written to hardware and it must be
ensured that they are configured before enabling the PWM.

Fixes: 98761ce4b91b ("pwm: spear: Implement .apply() callback")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-spear.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
index 48c31dac2f32..54c7990967dd 100644
--- a/drivers/pwm/pwm-spear.c
+++ b/drivers/pwm/pwm-spear.c
@@ -177,12 +177,9 @@ static int spear_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	if (state->period != pwm->state.period ||
-	    state->duty_cycle != pwm->state.duty_cycle) {
-		err = spear_pwm_config(chip, pwm, state->duty_cycle, state->period);
-		if (err)
-			return err;
-	}
+	err = spear_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
 
 	if (!pwm->state.enabled)
 		return spear_pwm_enable(chip, pwm);
-- 
2.30.2

[PATCH 3/5] pwm: tiecap: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

As the last call to ecap_pwm_apply() might have exited early if
state->enabled was false, the values for period and duty_cycle stored in
pwm->state might not have been written to hardware and it must be
ensured that they are configured before enabling the PWM.

Fixes: 0ca7acd84766 ("pwm: tiecap: Implement .apply() callback")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-tiecap.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index dec3f1fb150c..35eb19a5a0d1 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -189,16 +189,13 @@ static int ecap_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	if (state->period != pwm->state.period ||
-	    state->duty_cycle != pwm->state.duty_cycle) {
-		if (state->period > NSEC_PER_SEC)
-			return -ERANGE;
+	if (state->period > NSEC_PER_SEC)
+		return -ERANGE;
 
-		err = ecap_pwm_config(chip, pwm, state->duty_cycle,
-				      state->period, enabled);
-		if (err)
-			return err;
-	}
+	err = ecap_pwm_config(chip, pwm, state->duty_cycle,
+			      state->period, enabled);
+	if (err)
+		return err;
 
 	if (!enabled)
 		return ecap_pwm_enable(chip, pwm);
-- 
2.30.2

[PATCH 4/5] pwm: berlin: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

As the last call to berlin_pwm_apply() might have exited early if
state->enabled was false, the values for period and duty_cycle stored in
pwm->state might not have been written to hardware and it must be
ensured that they are configured before enabling the PWM.

Fixes: 30dffb42fcd4 ("pwm: berlin: Implement .apply() callback")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-berlin.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 5537b5f6dd5d..e157273fd2f7 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -190,12 +190,9 @@ static int berlin_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	if (state->period != pwm->state.period ||
-	    state->duty_cycle != pwm->state.duty_cycle) {
-		err = berlin_pwm_config(chip, pwm, state->duty_cycle, state->period);
-		if (err)
-			return err;
-	}
+	err = berlin_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
 
 	if (!enabled)
 		return berlin_pwm_enable(chip, pwm);
-- 
2.30.2

[PATCH 5/5] pwm: ep93xx: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

As the last call to ep93xx_pwm_apply() might have exited early if
state->enabled was false, the values for period and duty_cycle stored in
pwm->state might not have been written to hardware and it must be
ensured that they are configured before enabling the PWM.

Fixes: 6d45374af539 ("pwm: ep93xx: Implement .apply callback")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-ep93xx.c | 85 +++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
index 8a3d781e6514..fc3cb7d669c6 100644
--- a/drivers/pwm/pwm-ep93xx.c
+++ b/drivers/pwm/pwm-ep93xx.c
@@ -64,6 +64,11 @@ static int ep93xx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret;
 	struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
 	bool enabled = state->enabled;
+	void __iomem *base = ep93xx_pwm->base;
+	unsigned long long c;
+	unsigned long period_cycles;
+	unsigned long duty_cycles;
+	unsigned long term;
 
 	if (state->polarity != pwm->state.polarity) {
 		if (enabled) {
@@ -97,57 +102,47 @@ static int ep93xx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	if (state->period != pwm->state.period ||
-	    state->duty_cycle != pwm->state.duty_cycle) {
-		struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
-		void __iomem *base = ep93xx_pwm->base;
-		unsigned long long c;
-		unsigned long period_cycles;
-		unsigned long duty_cycles;
-		unsigned long term;
+	/*
+	 * The clock needs to be enabled to access the PWM registers.
+	 * Configuration can be changed at any time.
+	 */
+	if (!pwm_is_enabled(pwm)) {
+		ret = clk_prepare_enable(ep93xx_pwm->clk);
+		if (ret)
+			return ret;
+	}
 
-		/*
-		 * The clock needs to be enabled to access the PWM registers.
-		 * Configuration can be changed at any time.
-		 */
-		if (!pwm_is_enabled(pwm)) {
-			ret = clk_prepare_enable(ep93xx_pwm->clk);
-			if (ret)
-				return ret;
-		}
+	c = clk_get_rate(ep93xx_pwm->clk);
+	c *= state->period;
+	do_div(c, 1000000000);
+	period_cycles = c;
+
+	c = period_cycles;
+	c *= state->duty_cycle;
+	do_div(c, state->period);
+	duty_cycles = c;
 
-		c = clk_get_rate(ep93xx_pwm->clk);
-		c *= state->period;
-		do_div(c, 1000000000);
-		period_cycles = c;
-
-		c = period_cycles;
-		c *= state->duty_cycle;
-		do_div(c, state->period);
-		duty_cycles = c;
-
-		if (period_cycles < 0x10000 && duty_cycles < 0x10000) {
-			term = readw(base + EP93XX_PWMx_TERM_COUNT);
-
-			/* Order is important if PWM is running */
-			if (period_cycles > term) {
-				writew(period_cycles, base + EP93XX_PWMx_TERM_COUNT);
-				writew(duty_cycles, base + EP93XX_PWMx_DUTY_CYCLE);
-			} else {
-				writew(duty_cycles, base + EP93XX_PWMx_DUTY_CYCLE);
-				writew(period_cycles, base + EP93XX_PWMx_TERM_COUNT);
-			}
-			ret = 0;
+	if (period_cycles < 0x10000 && duty_cycles < 0x10000) {
+		term = readw(base + EP93XX_PWMx_TERM_COUNT);
+
+		/* Order is important if PWM is running */
+		if (period_cycles > term) {
+			writew(period_cycles, base + EP93XX_PWMx_TERM_COUNT);
+			writew(duty_cycles, base + EP93XX_PWMx_DUTY_CYCLE);
 		} else {
-			ret = -EINVAL;
+			writew(duty_cycles, base + EP93XX_PWMx_DUTY_CYCLE);
+			writew(period_cycles, base + EP93XX_PWMx_TERM_COUNT);
 		}
+		ret = 0;
+	} else {
+		ret = -EINVAL;
+	}
 
-		if (!pwm_is_enabled(pwm))
-			clk_disable_unprepare(ep93xx_pwm->clk);
+	if (!pwm_is_enabled(pwm))
+		clk_disable_unprepare(ep93xx_pwm->clk);
 
-		if (ret)
-			return ret;
-	}
+	if (ret)
+		return ret;
 
 	if (!enabled) {
 		ret = clk_prepare_enable(ep93xx_pwm->clk);
-- 
2.30.2

Re: [PATCH 0/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

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

Hello Thierry,

On Thu, Jul 01, 2021 at 10:27:50AM +0200, Uwe Kleine-König wrote:
> Geert Uytterhoeven found a regression in one of my patches. The same
> problem exists in several further commits. The respective drivers are
> fixed in this series.
> 
> The affected commits for the first patch is already in v5.4, so this
> patch should maybe backported to stable.
> The others are in Thierry's for-next branch only.

These four broken patches were now included in your pull request to
Linus for v5.14-rc1 but these fixes were not. I wonder that the
regression Geert reported made you back out the offending commit but you
didn't care for the four identical problems in pwm-spear, pwm-tiecap,
pwm-berlin and pwm-ep93xx. Did you miss this series?

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/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[Thierry Reding]

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

On Thu, Jul 08, 2021 at 02:36:39PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Jul 01, 2021 at 10:27:50AM +0200, Uwe Kleine-König wrote:
> > Geert Uytterhoeven found a regression in one of my patches. The same
> > problem exists in several further commits. The respective drivers are
> > fixed in this series.
> > 
> > The affected commits for the first patch is already in v5.4, so this
> > patch should maybe backported to stable.
> > The others are in Thierry's for-next branch only.
> 
> These four broken patches were now included in your pull request to
> Linus for v5.14-rc1 but these fixes were not. I wonder that the
> regression Geert reported made you back out the offending commit but you
> didn't care for the four identical problems in pwm-spear, pwm-tiecap,
> pwm-berlin and pwm-ep93xx. Did you miss this series?

Ugh... this is a nice big mess now. In retrospect I should've just
backed out all those patches. Or rather not have applied them in the
first place until they got a Tested-by.

I'll pull in this series and will send this as a follow-up pull request.

Thierry

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

Re: [PATCH 0/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

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

Hello Thierry,

On Thu, Jul 08, 2021 at 04:07:14PM +0200, Thierry Reding wrote:
> On Thu, Jul 08, 2021 at 02:36:39PM +0200, Uwe Kleine-König wrote:
> > On Thu, Jul 01, 2021 at 10:27:50AM +0200, Uwe Kleine-König wrote:
> > > Geert Uytterhoeven found a regression in one of my patches. The same
> > > problem exists in several further commits. The respective drivers are
> > > fixed in this series.
> > > 
> > > The affected commits for the first patch is already in v5.4, so this
> > > patch should maybe backported to stable.
> > > The others are in Thierry's for-next branch only.
> > 
> > These four broken patches were now included in your pull request to
> > Linus for v5.14-rc1 but these fixes were not. I wonder that the
> > regression Geert reported made you back out the offending commit but you
> > didn't care for the four identical problems in pwm-spear, pwm-tiecap,
> > pwm-berlin and pwm-ep93xx. Did you miss this series?
> 
> Ugh... this is a nice big mess now. In retrospect I should've just
> backed out all those patches. Or rather not have applied them in the
> first place until they got a Tested-by.

Agreed, this isn't as optimal as it could have been. My conclusions are
a bit different though. I took the time to look at the details for these
changes:

 - 2021-04-11
   I sent "pwm: Ensure for legacy drivers that pwm->state stays
   consistent" to the linux-pwm list.

 - 2021-06-27
   The merge window for 5.14 opened

 - 2021-06-28
   You applied the patch, it then appeared in next-20210629 for the
   first time in next.

 - 2021-06-29
   Geert reported the regression

 - 2021-06-30
   You dropped the commit.

 - 2021-07-01
   I sent a fixed patch and incremental fixes for the same problems in
   the other drivers.

 - 2021-07-08
   Thierry sent a pull request containing the four broken (and unfixed)
   commits.

For me the conclusions here are:

 - Patches on the mailing list are not widely tested
   (So I think waiting for Tested-bys isn't a pragmatic option unless
   maybe we start adding more people to MAINTAINERS.)

 - Changes in next get (some) testing.

And so I think changes should be put into next earlier than it was the
case in this release cycle and it might be beneficial to check for
unapplied fixes before sending out a PR. Feel free to communicate with
me before sending the next PR if there is something on my radar that is
missing in your for-next branch.

> I'll pull in this series and will send this as a follow-up pull request.

Great.

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/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[Uwe Kleine-König]

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

Hello Thierry,

On Thu, Jul 08, 2021 at 04:07:14PM +0200, Thierry Reding wrote:
> On Thu, Jul 08, 2021 at 02:36:39PM +0200, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Thu, Jul 01, 2021 at 10:27:50AM +0200, Uwe Kleine-König wrote:
> > > Geert Uytterhoeven found a regression in one of my patches. The same
> > > problem exists in several further commits. The respective drivers are
> > > fixed in this series.
> > > 
> > > The affected commits for the first patch is already in v5.4, so this
> > > patch should maybe backported to stable.
> > > The others are in Thierry's for-next branch only.
> > 
> > These four broken patches were now included in your pull request to
> > Linus for v5.14-rc1 but these fixes were not. I wonder that the
> > regression Geert reported made you back out the offending commit but you
> > didn't care for the four identical problems in pwm-spear, pwm-tiecap,
> > pwm-berlin and pwm-ep93xx. Did you miss this series?
> 
> Ugh... this is a nice big mess now. In retrospect I should've just
> backed out all those patches. Or rather not have applied them in the
> first place until they got a Tested-by.
> 
> I'll pull in this series and will send this as a follow-up pull request.

I saw you applied the series to your for-next branch, so next is fixed
since next-20210709. I wonder what makes you wait sending these to
Linus.

The facts are:

 - Fixes were sent to the list on 2021-07-01
 - next was broken since at least next-20210701 up to next-20210708
 - linus/master is broken since v5.14-rc1~42 (2021-07-08) for ep93xx,
   berlin, tiecap, spear and since v5.4-rc1~27 (2019-09-27) for sprd

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/5] pwm: Ensure configuring period and duty_cycle isn't wrongly skipped

[Thierry Reding]

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

On Wed, Jul 14, 2021 at 08:39:01AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Jul 08, 2021 at 04:07:14PM +0200, Thierry Reding wrote:
> > On Thu, Jul 08, 2021 at 02:36:39PM +0200, Uwe Kleine-König wrote:
> > > Hello Thierry,
> > > 
> > > On Thu, Jul 01, 2021 at 10:27:50AM +0200, Uwe Kleine-König wrote:
> > > > Geert Uytterhoeven found a regression in one of my patches. The same
> > > > problem exists in several further commits. The respective drivers are
> > > > fixed in this series.
> > > > 
> > > > The affected commits for the first patch is already in v5.4, so this
> > > > patch should maybe backported to stable.
> > > > The others are in Thierry's for-next branch only.
> > > 
> > > These four broken patches were now included in your pull request to
> > > Linus for v5.14-rc1 but these fixes were not. I wonder that the
> > > regression Geert reported made you back out the offending commit but you
> > > didn't care for the four identical problems in pwm-spear, pwm-tiecap,
> > > pwm-berlin and pwm-ep93xx. Did you miss this series?
> > 
> > Ugh... this is a nice big mess now. In retrospect I should've just
> > backed out all those patches. Or rather not have applied them in the
> > first place until they got a Tested-by.
> > 
> > I'll pull in this series and will send this as a follow-up pull request.
> 
> I saw you applied the series to your for-next branch, so next is fixed
> since next-20210709. I wonder what makes you wait sending these to
> Linus.

Given the mess that this caused I didn't feel comfortable pushing this
to Linus immediately and instead give people a bit of time to find any
potential issues in linux-next first.

I'm going to send the pull request later today.

Thierry

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