| 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
| ||
|
Message-Id: <Y90BKR.1BA4VWKIBIKU@crapouillou.net>
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