lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Oct 2022 11:10:46 +0100 From: Paul Cercueil <paul@...pouillou.net> To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de> Cc: Thierry Reding <thierry.reding@...il.com>, od@...ndingux.net, linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org, stable@...r.kernel.org Subject: Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2 Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König <u.kleine-koenig@...gutronix.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@...pouillou.net> >> Cc: stable@...r.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 internal timer >> reaches the >> - * duty value, then becomes active until the timer reaches the >> period >> - * value. In theory, we should then use (period - duty) as the >> real duty >> - * value, as a high duty value would otherwise result in the PWM >> pin >> - * being inactive most of the time. >> - * >> - * Here, we don't do that, and instead invert the polarity of the >> PWM >> - * when it is active. This trick makes the PWM start with its >> 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 internal timer >> + * reaches the duty value, then becomes active until the timer >> + * reaches the period value. In theory, we should then use >> + * (period - duty) as the real duty value, as a high duty value >> + * would otherwise result in the PWM pin being inactive most of >> + * the time. >> + * >> + * Here, we don't do that, and instead invert the polarity of >> + * the PWM when it is active. This trick makes the PWM start >> + * with its active state instead of its inactive state. >> + */ >> + if (state->polarity == PWM_POLARITY_NORMAL) >> + polarity = PWM_POLARITY_INVERSED; >> + else >> + polarity = 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). -Paul
Powered by blists - more mailing lists