diff mbox series

[2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

Message ID 20221024205213.327001-3-paul@crapouillou.net
State New
Headers
Series pwm: jz4740: Fixes and some light changes |

Commit Message

Paul Cercueil Oct. 24, 2022, 8:52 p.m. UTC 
  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(-)

Comments

Uwe Kleine-König Oct. 25, 2022, 6:44 a.m. UTC | #1 
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?

>  	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.

Best regards
Uwe
Paul Cercueil Oct. 25, 2022, 10:10 a.m. UTC | #2 
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 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
Uwe Kleine-König Nov. 28, 2022, 2:39 p.m. UTC | #3 
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 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).

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 });

Best regards
Uwe
Thierry Reding Nov. 29, 2022, 12:16 p.m. UTC | #4 
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 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).
> 
> 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.

Thierry
Paul Cercueil Nov. 29, 2022, 12:25 p.m. UTC | #5 
Hi Uwe,

Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> 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 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).
> 
> 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 });

Well that sounds pretty stupid to me; why doesn't the PWM subsystem 
enforce that the pins must be driven to their inactive level when the 
PWM function is disabled? Then for such hardware you describe, the 
corresponding PWM driver could itself apply a duty_cycle = 0 if that's 
what it takes to get an inactive state.

Cheers,
-Paul
Paul Cercueil Nov. 29, 2022, 12:34 p.m. UTC | #6 
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 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).
>> 
>>  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
Uwe Kleine-König Nov. 29, 2022, 4:24 p.m. UTC | #7 
Hello Paul,

On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> Hi Uwe,
> 
> Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > Hello,
> > 
> > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > 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 });
> 
> Well that sounds pretty stupid to me; why doesn't the PWM subsystem enforce
> that the pins must be driven to their inactive level when the PWM function
> is disabled?
> 
> Then for such hardware you describe, the corresponding PWM
> driver could itself apply a duty_cycle = 0 if that's what it takes to get an
> inactive state.

Let's assume we claim that on disable the pin is driven to the inactive level.

The (bad) effect is that for a use case where the pin state doesn't
matter (e.g. a backlight where the power regulator is off), the PWM
keeps running even though it could be disabled and so save some power.

So to make this use case properly supported, we need another flag in
struct pwm_state that allows the consumer to tell the lowlevel driver
that it's ok to disable the hardware even with the output being UB.
Let's call this new flag "spam" and the pin is allowed to do whatever it
wants with .spam = false.

After that you can realize that applying any state with:

	.duty_cycle = A,
	.period = B,
	.polarity = C,
	.enabled = false,
	.spam = true,

semantically (i.e. just looking at the output) has the same effect as

	.duty_cycle = 0,
	.period = $something,
	.polarity = C,
	.enabled = true,
	.spam = true,

So having .enabled doesn't add to the expressiveness of pwm_apply(),
because you can specify any configuration without having to resort to
.enabled = false. So the enabled member of struct pwm_state can be
dropped.

Then we end up with the exact scenario we have now, just that the flag
that specifies if the output should be held in the inactive state has a
bad name.

Best regards
Uwe
Paul Cercueil Nov. 29, 2022, 4:58 p.m. UTC | #8 
Le mardi 29 novembre 2022 à 17:24 +0100, Uwe Kleine-König a écrit :
> Hello Paul,
> 
> On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> > Hi Uwe,
> > 
> > Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > Hello,
> > > 
> > > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > > 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 });
> > 
> > Well that sounds pretty stupid to me; why doesn't the PWM subsystem
> > enforce
> > that the pins must be driven to their inactive level when the PWM
> > function
> > is disabled?
> > 
> > Then for such hardware you describe, the corresponding PWM
> > driver could itself apply a duty_cycle = 0 if that's what it takes
> > to get an
> > inactive state.
> 
> Let's assume we claim that on disable the pin is driven to the
> inactive level.
> 
> The (bad) effect is that for a use case where the pin state doesn't
> matter (e.g. a backlight where the power regulator is off), the PWM
> keeps running even though it could be disabled and so save some
> power.
> 
> So to make this use case properly supported, we need another flag in
> struct pwm_state that allows the consumer to tell the lowlevel driver
> that it's ok to disable the hardware even with the output being UB.
> Let's call this new flag "spam" and the pin is allowed to do whatever
> it
> wants with .spam = false.
> 
> After that you can realize that applying any state with:
> 
>         .duty_cycle = A,
>         .period = B,
>         .polarity = C,
>         .enabled = false,
>         .spam = true,
> 
> semantically (i.e. just looking at the output) has the same effect as
> 
>         .duty_cycle = 0,
>         .period = $something,
>         .polarity = C,
>         .enabled = true,
>         .spam = true,
> 
> So having .enabled doesn't add to the expressiveness of pwm_apply(),
> because you can specify any configuration without having to resort to
> .enabled = false. So the enabled member of struct pwm_state can be
> dropped.
> 
> Then we end up with the exact scenario we have now, just that the
> flag
> that specifies if the output should be held in the inactive state has
> a
> bad name.

If I follow you, then it means that the PWM backlight driver pwm_bl.c
should set state.enabled=true in pwm_backlight_power_off() to make sure
that the pin is inactive?

-Paul
Uwe Kleine-König Nov. 29, 2022, 5:46 p.m. UTC | #9 
On Tue, Nov 29, 2022 at 04:58:28PM +0000, Paul Cercueil wrote:
> Le mardi 29 novembre 2022 à 17:24 +0100, Uwe Kleine-König a écrit :
> > Hello Paul,
> > 
> > On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> > > Hi Uwe,
> > > 
> > > Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > Hello,
> > > > 
> > > > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > > > 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 });
> > > 
> > > Well that sounds pretty stupid to me; why doesn't the PWM subsystem
> > > enforce
> > > that the pins must be driven to their inactive level when the PWM
> > > function
> > > is disabled?
> > > 
> > > Then for such hardware you describe, the corresponding PWM
> > > driver could itself apply a duty_cycle = 0 if that's what it takes
> > > to get an
> > > inactive state.
> > 
> > Let's assume we claim that on disable the pin is driven to the
> > inactive level.
> > 
> > The (bad) effect is that for a use case where the pin state doesn't
> > matter (e.g. a backlight where the power regulator is off), the PWM
> > keeps running even though it could be disabled and so save some
> > power.
> > 
> > So to make this use case properly supported, we need another flag in
> > struct pwm_state that allows the consumer to tell the lowlevel driver
> > that it's ok to disable the hardware even with the output being UB.
> > Let's call this new flag "spam" and the pin is allowed to do whatever
> > it
> > wants with .spam = false.
> > 
> > After that you can realize that applying any state with:
> > 
> >         .duty_cycle = A,
> >         .period = B,
> >         .polarity = C,
> >         .enabled = false,
> >         .spam = true,
> > 
> > semantically (i.e. just looking at the output) has the same effect as
> > 
> >         .duty_cycle = 0,
> >         .period = $something,
> >         .polarity = C,
> >         .enabled = true,
> >         .spam = true,
> > 
> > So having .enabled doesn't add to the expressiveness of pwm_apply(),
> > because you can specify any configuration without having to resort to
> > .enabled = false. So the enabled member of struct pwm_state can be
> > dropped.
> > 
> > Then we end up with the exact scenario we have now, just that the
> > flag
> > that specifies if the output should be held in the inactive state has
> > a
> > bad name.
> 
> If I follow you, then it means that the PWM backlight driver pwm_bl.c
> should set state.enabled=true in pwm_backlight_power_off() to make sure
> that the pin is inactive?

Correct, that's the only way to ensure that the pinlevel stays at the
intended level.

And lowlevel PWM drivers can be improved to disable the hardware when
they are asked for .duty_cycle = 0 (maybe under some additional
conditions).

Best regards
Uwe
diff mbox series

Patch

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);
+
 	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);
+	}
 
 	return 0;
 }