linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Linux PWM List <linux-pwm@vger.kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent
Date: Wed, 30 Jun 2021 18:21:28 +0200	[thread overview]
Message-ID: <20210630162128.ufmul6euxkwnou25@pengutronix.de> (raw)
In-Reply-To: <CAMuHMdV0w8mxxLVT4abrD-FgqS9Kg4BLomDQbXS_csdMyqhypQ@mail.gmail.com>

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

  reply	other threads:[~2021-06-30 16:21 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [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 [this message]
2021-06-30 17:08           ` Geert Uytterhoeven
2021-06-30 17:18           ` Thierry Reding
2021-06-29 19:52   ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210630162128.ufmul6euxkwnou25@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=geert@linux-m68k.org \
    --cc=kernel@pengutronix.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).