Hi Thierry,
Le mar. 29 nov. 2022 à 13:16:05 +0100, Thierry Reding thierry.reding@gmail.com a écrit :
On Mon, Nov 28, 2022 at 03:39:11PM +0100, Uwe Kleine-König wrote:
Hello,
On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König u.kleine-koenig@pengutronix.de a écrit :
On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with
the
active part"), the trick to set duty > period to properly shut down TCU2
channels
did not work anymore, because of the polarity inversion.
Address this issue by restoring the proper polarity before disabling the channels.
Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the
active
part") Signed-off-by: Paul Cercueil paul@crapouillou.net Cc: stable@vger.kernel.org
drivers/pwm/pwm-jz4740.c | 62 ++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/drivers/pwm/pwm-jz4740.c
b/drivers/pwm/pwm-jz4740.c
index 228eb104bf1e..65462a0052af 100644 --- a/drivers/pwm/pwm-jz4740.c +++ b/drivers/pwm/pwm-jz4740.c @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct
pwm_chip
*chip, struct pwm_device *pwm) return 0; }
+static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip
*jz,
unsigned int hwpwm,enum pwm_polarity polarity)+{
- unsigned int value = 0;
- if (polarity == PWM_POLARITY_INVERSED)
value = TCU_TCSR_PWM_INITL_HIGH;- regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
TCU_TCSR_PWM_INITL_HIGH, value);+}
static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) { struct jz4740_pwm_chip *jz = to_jz4740(chip); @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct
pwm_chip
*chip, struct pwm_device *pwm, unsigned long long tmp = 0xffffull * NSEC_PER_SEC; struct clk *clk = pwm_get_chip_data(pwm); unsigned long period, duty;
- enum pwm_polarity polarity; long rate; int err;
@@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct
pwm_chip
*chip, struct pwm_device *pwm, if (duty >= period) duty = period - 1;
- /* Restore regular polarity before disabling the channel.
*/
- jz4740_pwm_set_polarity(jz4740, pwm->hwpwm,
state->polarity);
Does this introduce a glitch?
Maybe. But the PWM is shut down before finishing its period
anyway, so there
was already a glitch.
jz4740_pwm_disable(chip, pwm);
err = clk_set_rate(clk, rate); @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct
pwm_chip
*chip, struct pwm_device *pwm, regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
- /*
* Set polarity.** The PWM starts in inactive state until the internaltimer
reaches the
* duty value, then becomes active until the timer reachesthe
period
* value. In theory, we should then use (period - duty) asthe
real duty
* value, as a high duty value would otherwise result inthe PWM
pin
* being inactive most of the time.** Here, we don't do that, and instead invert the polarityof the
PWM
* when it is active. This trick makes the PWM start withits
active
* state instead of its inactive state.*/- if ((state->polarity == PWM_POLARITY_NORMAL) ^
state->enabled)
regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),TCU_TCSR_PWM_INITL_HIGH, 0);- else
regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),TCU_TCSR_PWM_INITL_HIGH,TCU_TCSR_PWM_INITL_HIGH);- if (state->enabled)
- if (state->enabled) {
/** Set polarity.** The PWM starts in inactive state until the internaltimer
* reaches the duty value, then becomes active until thetimer
* reaches the period value. In theory, we should then use* (period - duty) as the real duty value, as a high dutyvalue
* would otherwise result in the PWM pin being inactivemost of
* the time.** Here, we don't do that, and instead invert thepolarity of
* the PWM when it is active. This trick makes the PWMstart
* with its active state instead of its inactive state.*/if (state->polarity == PWM_POLARITY_NORMAL)polarity = PWM_POLARITY_INVERSED;elsepolarity = PWM_POLARITY_NORMAL;jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);- jz4740_pwm_enable(chip, pwm);
- }
Note that for disabled PWMs there is no official guaranty about
the pin
state. So it would be ok (but admittedly not great) to simplify
the
driver and accept that the pinstate is active while the PWM is
off.
IMHO this is also better than a glitch.
If a consumer wants the PWM to be in its inactive state, they
should
not disable it.
Completely disagree. I absolutely do not want the backlight to go
full
bright mode when the PWM pin is disabled. And disabling the
backlight is a
thing (for screen blanking and during mode changes).
For some hardwares there is no pretty choice. So the gist is: If the backlight driver wants to ensure that the PWM pin is driven to its inactive level, it should use:
pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled = true });
and better not
pwm_apply(pwm, { ..., .enabled = false });
Depending on your hardware capabilities you may also be able to use pinctrl to configure the pin to behave properly when the PWM is disabled. Not all hardware can do that, though.
Been there, done that. It got refused. https://lkml.org/lkml/2019/5/22/607
Cheers, -Paul