[6/7] pwm: jz4740: Make PWM start with the active part
diff mbox series

Message ID 20190809123031.24219-7-paul@crapouillou.net
State New
Headers show
Series
  • pwm: jz4740: Driver update
Related show

Commit Message

Paul Cercueil Aug. 9, 2019, 12:30 p.m. UTC
The PWM will always start with the inactive part. To counter that,
when PWM is enabled we switch the configured polarity, and use
'period - duty + 1' as the real duty.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Uwe Kleine-König Aug. 9, 2019, 5:10 p.m. UTC | #1
On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> The PWM will always start with the inactive part. To counter that,
> when PWM is enabled we switch the configured polarity, and use
> 'period - duty + 1' as the real duty.

Where does the + 1 come from? This looks wrong. (So if duty=0 is
requested you use duty = period + 1?)

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 85e2110aae4f..8df898429d47 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		   *parent_clk = clk_get_parent(clk);
>  	unsigned long rate, parent_rate, period, duty;
>  	unsigned long long tmp;
> +	bool polarity_inversed;
>  	int ret;
>  
>  	parent_rate = clk_get_rate(parent_clk);
> @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* Reset counter to 0 */
>  	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>  
> -	/* Set duty */
> -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
> -
>  	/* Set period */
>  	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>  
> +	/*
> +	 * The PWM will always start with the inactive part. To counter that,
> +	 * when PWM is enabled we switch the configured polarity, and use
> +	 * 'period - duty + 1' as the real duty.
> +	 */
> +
> +	/* Set duty */
> +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
> +

Before you set duty first, then period, now you do it the other way
round. Is there a good reason?

>  	/* Set polarity */
> -	switch (state->polarity) {
> -	case PWM_POLARITY_NORMAL:
> +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
> +	if (!polarity_inversed ^ state->enabled)

Why does state->enabled suddenly matter here?

>  		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>  				   TCU_TCSR_PWM_INITL_HIGH, 0);
> -		break;
> -	case PWM_POLARITY_INVERSED:
> +	else
>  		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>  				   TCU_TCSR_PWM_INITL_HIGH,
>  				   TCU_TCSR_PWM_INITL_HIGH);
> -		break;
> -	}
>  
>  	if (state->enabled)
>  		jz4740_pwm_enable(chip, pwm);

Best regards
Uwe
Paul Cercueil Aug. 9, 2019, 5:33 p.m. UTC | #2
Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
>>  The PWM will always start with the inactive part. To counter that,
>>  when PWM is enabled we switch the configured polarity, and use
>>  'period - duty + 1' as the real duty.
> 
> Where does the + 1 come from? This looks wrong. (So if duty=0 is
> requested you use duty = period + 1?)

You'd never request duty == 0, would you?

Your duty must always be in the inclusive range [1, period]
(hardware values, not ns). A duty of 0 is a hardware fault
(on the jz4740 it is).

If you request duty == 1 (the minimum), then the new duty is equal
to (period - 1 + 1) == period, which is the maximum of your range.

If you request duty == period (the maximum), then the new duty
calculated is equal to (period - period + 1) == 1, which is the
minimum of your range.


>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>>   1 file changed, 13 insertions(+), 9 deletions(-)
>> 
>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>  index 85e2110aae4f..8df898429d47 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   		   *parent_clk = clk_get_parent(clk);
>>   	unsigned long rate, parent_rate, period, duty;
>>   	unsigned long long tmp;
>>  +	bool polarity_inversed;
>>   	int ret;
>> 
>>   	parent_rate = clk_get_rate(parent_clk);
>>  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	/* Reset counter to 0 */
>>   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>> 
>>  -	/* Set duty */
>>  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
>>  -
>>   	/* Set period */
>>   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>> 
>>  +	/*
>>  +	 * The PWM will always start with the inactive part. To counter 
>> that,
>>  +	 * when PWM is enabled we switch the configured polarity, and use
>>  +	 * 'period - duty + 1' as the real duty.
>>  +	 */
>>  +
>>  +	/* Set duty */
>>  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - 
>> duty + 1);
>>  +
> 
> Before you set duty first, then period, now you do it the other way
> round. Is there a good reason?

To move it below the comment that explains why we use 'period - duty + 
1'.

We modify that line anyway, so it's not like it makes the patch much 
more
verbose.


> 
>>   	/* Set polarity */
>>  -	switch (state->polarity) {
>>  -	case PWM_POLARITY_NORMAL:
>>  +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
>>  +	if (!polarity_inversed ^ state->enabled)
> 
> Why does state->enabled suddenly matter here?

The pin stay inactive when the PWM is disabled, but the level of the
inactive state depends on the polarity of the pin. So we need to switch
the polarity only when the PWM is enabled.


>>   		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>   				   TCU_TCSR_PWM_INITL_HIGH, 0);
>>  -		break;
>>  -	case PWM_POLARITY_INVERSED:
>>  +	else
>>   		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>>   				   TCU_TCSR_PWM_INITL_HIGH,
>>   				   TCU_TCSR_PWM_INITL_HIGH);
>>  -		break;
>>  -	}
>> 
>>   	if (state->enabled)
>>   		jz4740_pwm_enable(chip, pwm);
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Uwe Kleine-König Aug. 12, 2019, 5:55 a.m. UTC | #3
On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
> 
> 
> Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> > >  The PWM will always start with the inactive part. To counter that,
> > >  when PWM is enabled we switch the configured polarity, and use
> > >  'period - duty + 1' as the real duty.
> > 
> > Where does the + 1 come from? This looks wrong. (So if duty=0 is
> > requested you use duty = period + 1?)
> 
> You'd never request duty == 0, would you?
> 
> Your duty must always be in the inclusive range [1, period]
> (hardware values, not ns). A duty of 0 is a hardware fault
> (on the jz4740 it is).

From the PWM framework's POV duty cycle = 0 is perfectly valid. Similar
to duty == period. Not supporting dutz cycle 0 is another limitation of
your PWM that should be documented.

For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
drivers/leds/leds-pwm.c.

> If you request duty == 1 (the minimum), then the new duty is equal
> to (period - 1 + 1) == period, which is the maximum of your range.
> 
> If you request duty == period (the maximum), then the new duty
> calculated is equal to (period - period + 1) == 1, which is the
> minimum of your range.
> 
> 
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
> > >   1 file changed, 13 insertions(+), 9 deletions(-)
> > > 
> > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > >  index 85e2110aae4f..8df898429d47 100644
> > >  --- a/drivers/pwm/pwm-jz4740.c
> > >  +++ b/drivers/pwm/pwm-jz4740.c
> > >  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   		   *parent_clk = clk_get_parent(clk);
> > >   	unsigned long rate, parent_rate, period, duty;
> > >   	unsigned long long tmp;
> > >  +	bool polarity_inversed;
> > >   	int ret;
> > > 
> > >   	parent_rate = clk_get_rate(parent_clk);
> > >  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > *chip, struct pwm_device *pwm,
> > >   	/* Reset counter to 0 */
> > >   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
> > > 
> > >  -	/* Set duty */
> > >  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
> > >  -
> > >   	/* Set period */
> > >   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
> > > 
> > >  +	/*
> > >  +	 * The PWM will always start with the inactive part. To counter that,
> > >  +	 * when PWM is enabled we switch the configured polarity, and use
> > >  +	 * 'period - duty + 1' as the real duty.
> > >  +	 */
> > >  +
> > >  +	/* Set duty */
> > >  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
> > >  +
> > 
> > Before you set duty first, then period, now you do it the other way
> > round. Is there a good reason?
> 
> To move it below the comment that explains why we use 'period - duty + 1'.
> 
> We modify that line anyway, so it's not like it makes the patch much more
> verbose.

It doesn't make it more verbose, but that's not the background of my
question. For most(?) PWM implementation the order of hardware accesses
matters and introducing such a difference as an unneeded side effect
isn't optimal.

Why not add the comment above the line that already used to set the duty
in hardware?

> > >   	/* Set polarity */
> > >  -	switch (state->polarity) {
> > >  -	case PWM_POLARITY_NORMAL:
> > >  +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
> > >  +	if (!polarity_inversed ^ state->enabled)
> > 
> > Why does state->enabled suddenly matter here?
> 
> The pin stay inactive when the PWM is disabled, but the level of the
> inactive state depends on the polarity of the pin. So we need to switch
> the polarity only when the PWM is enabled.

After some thought I got that. When knowing this, this is already
mentioned in the comment you introduced as you write about enabled PWMs
only. Maybe it's just me, but mentioning that case more explicit would
have helped me. Something like:

	/*
	 * The hardware always starts a period with the inactive part.
	 * So invert polarity and duty cycle to yield the output that is
	 * expected by the PWM framework and its users. This inversion
	 * must not be done for a disabled PWM however because otherwise
	 * it outputs a constant active level.
	 */

Best regards
Uwe
Paul Cercueil Aug. 12, 2019, 8:50 p.m. UTC | #4
Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
>> 
>> 
>>  Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
>>  > >  The PWM will always start with the inactive part. To counter 
>> that,
>>  > >  when PWM is enabled we switch the configured polarity, and use
>>  > >  'period - duty + 1' as the real duty.
>>  >
>>  > Where does the + 1 come from? This looks wrong. (So if duty=0 is
>>  > requested you use duty = period + 1?)
>> 
>>  You'd never request duty == 0, would you?
>> 
>>  Your duty must always be in the inclusive range [1, period]
>>  (hardware values, not ns). A duty of 0 is a hardware fault
>>  (on the jz4740 it is).
> 
> From the PWM framework's POV duty cycle = 0 is perfectly valid. 
> Similar
> to duty == period. Not supporting dutz cycle 0 is another limitation 
> of
> your PWM that should be documented.
> 
> For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
> drivers/leds/leds-pwm.c.

Perfectly valid for the PWM framework, maybe; but what is the expected
output then? A constant inactive state? Then I guess I can just disable
the PWM output in the driver when configured with duty == 0.


>>  If you request duty == 1 (the minimum), then the new duty is equal
>>  to (period - 1 + 1) == period, which is the maximum of your range.
>> 
>>  If you request duty == period (the maximum), then the new duty
>>  calculated is equal to (period - period + 1) == 1, which is the
>>  minimum of your range.
>> 
>> 
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  ---
>>  > >   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>>  > >   1 file changed, 13 insertions(+), 9 deletions(-)
>>  > >
>>  > >  diff --git a/drivers/pwm/pwm-jz4740.c 
>> b/drivers/pwm/pwm-jz4740.c
>>  > >  index 85e2110aae4f..8df898429d47 100644
>>  > >  --- a/drivers/pwm/pwm-jz4740.c
>>  > >  +++ b/drivers/pwm/pwm-jz4740.c
>>  > >  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   		   *parent_clk = clk_get_parent(clk);
>>  > >   	unsigned long rate, parent_rate, period, duty;
>>  > >   	unsigned long long tmp;
>>  > >  +	bool polarity_inversed;
>>  > >   	int ret;
>>  > >
>>  > >   	parent_rate = clk_get_rate(parent_clk);
>>  > >  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct 
>> pwm_chip
>>  > > *chip, struct pwm_device *pwm,
>>  > >   	/* Reset counter to 0 */
>>  > >   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>>  > >
>>  > >  -	/* Set duty */
>>  > >  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
>>  > >  -
>>  > >   	/* Set period */
>>  > >   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>>  > >
>>  > >  +	/*
>>  > >  +	 * The PWM will always start with the inactive part. To 
>> counter that,
>>  > >  +	 * when PWM is enabled we switch the configured polarity, 
>> and use
>>  > >  +	 * 'period - duty + 1' as the real duty.
>>  > >  +	 */
>>  > >  +
>>  > >  +	/* Set duty */
>>  > >  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period 
>> - duty + 1);
>>  > >  +
>>  >
>>  > Before you set duty first, then period, now you do it the other 
>> way
>>  > round. Is there a good reason?
>> 
>>  To move it below the comment that explains why we use 'period - 
>> duty + 1'.
>> 
>>  We modify that line anyway, so it's not like it makes the patch 
>> much more
>>  verbose.
> 
> It doesn't make it more verbose, but that's not the background of my
> question. For most(?) PWM implementation the order of hardware 
> accesses
> matters and introducing such a difference as an unneeded side effect
> isn't optimal.

There's no side effect. The PWM is disabled when reconfigured.


> Why not add the comment above the line that already used to set the 
> duty
> in hardware?

I thought it made sense to have the two parts of the trick closer 
together
in the code, below the comment, so that it's clearer what it does.


>>  > >   	/* Set polarity */
>>  > >  -	switch (state->polarity) {
>>  > >  -	case PWM_POLARITY_NORMAL:
>>  > >  +	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
>>  > >  +	if (!polarity_inversed ^ state->enabled)
>>  >
>>  > Why does state->enabled suddenly matter here?
>> 
>>  The pin stay inactive when the PWM is disabled, but the level of the
>>  inactive state depends on the polarity of the pin. So we need to 
>> switch
>>  the polarity only when the PWM is enabled.
> 
> After some thought I got that. When knowing this, this is already
> mentioned in the comment you introduced as you write about enabled 
> PWMs
> only. Maybe it's just me, but mentioning that case more explicit would
> have helped me. Something like:
> 
> 	/*
> 	 * The hardware always starts a period with the inactive part.
> 	 * So invert polarity and duty cycle to yield the output that is
> 	 * expected by the PWM framework and its users. This inversion
> 	 * must not be done for a disabled PWM however because otherwise
> 	 * it outputs a constant active level.
> 	 */

Ok.


> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Uwe Kleine-König Aug. 12, 2019, 9:58 p.m. UTC | #5
On Mon, Aug 12, 2019 at 10:50:01PM +0200, Paul Cercueil wrote:
> 
> 
> Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
> > > 
> > > 
> > >  Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
> > >  > >  The PWM will always start with the inactive part. To counter
> > > that,
> > >  > >  when PWM is enabled we switch the configured polarity, and use
> > >  > >  'period - duty + 1' as the real duty.
> > >  >
> > >  > Where does the + 1 come from? This looks wrong. (So if duty=0 is
> > >  > requested you use duty = period + 1?)
> > > 
> > >  You'd never request duty == 0, would you?
> > > 
> > >  Your duty must always be in the inclusive range [1, period]
> > >  (hardware values, not ns). A duty of 0 is a hardware fault
> > >  (on the jz4740 it is).
> > 
> > From the PWM framework's POV duty cycle = 0 is perfectly valid. Similar
> > to duty == period. Not supporting dutz cycle 0 is another limitation of
> > your PWM that should be documented.
> > 
> > For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
> > drivers/leds/leds-pwm.c.
> 
> Perfectly valid for the PWM framework, maybe; but what is the expected
> output then? A constant inactive state?

Yes, a constant inactive state is expected. This is consistent and in a
similar way when using duty == period an constant active output is
expected.

> Then I guess I can just disable the PWM output in the driver when
> configured with duty == 0.

Some time ago I argued with Thierry that we could drop the concept of
enabled/disabled for a PWM because a disabled PWM is supposed to behave
identically to duty=0. This is however only nearly true because with
duty=0 the time the PWM is inactive still is a multiple of the period.

I tend to agree that disabling the PWM when duty=0 is requested is
better than to fail the request (or configure for duty=1 $whateverunit).
I'm looking forward to what Thierry's opinion is here.

> > >  If you request duty == 1 (the minimum), then the new duty is equal
> > >  to (period - 1 + 1) == period, which is the maximum of your range.
> > > 
> > >  If you request duty == period (the maximum), then the new duty
> > >  calculated is equal to (period - period + 1) == 1, which is the
> > >  minimum of your range.

Note that the wrong border (because duty=0 is impossible for your
hardware) shifts the whole space. The right inverse of duty = period - 1
is duty = 1, isn't it?

> > > > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > > >  ---
> > > > >   drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
> > > > >   1 file changed, 13 insertions(+), 9 deletions(-)
> > > > >
> > > > >  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > > > >  index 85e2110aae4f..8df898429d47 100644
> > > > >  --- a/drivers/pwm/pwm-jz4740.c
> > > > >  +++ b/drivers/pwm/pwm-jz4740.c
> > > > >  @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >   		   *parent_clk = clk_get_parent(clk);
> > > > >   	unsigned long rate, parent_rate, period, duty;
> > > > >   	unsigned long long tmp;
> > > > >  +	bool polarity_inversed;
> > > > >   	int ret;
> > > > >
> > > > >   	parent_rate = clk_get_rate(parent_clk);
> > > > >  @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct pwm_chip
> > > > > *chip, struct pwm_device *pwm,
> > > > >   	/* Reset counter to 0 */
> > > > >   	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
> > > > >
> > > > >  -	/* Set duty */
> > > > >  -	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
> > > > >  -
> > > > >   	/* Set period */
> > > > >   	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
> > > > >
> > > > >  +	/*
> > > > >  +	 * The PWM will always start with the inactive part. To counter that,
> > > > >  +	 * when PWM is enabled we switch the configured polarity, and use
> > > > >  +	 * 'period - duty + 1' as the real duty.
> > > > >  +	 */
> > > > >  +
> > > > >  +	/* Set duty */
> > > > >  +	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
> > > > >  +
> > > >
> > > > Before you set duty first, then period, now you do it the other way
> > > > round. Is there a good reason?
> > > 
> > >  To move it below the comment that explains why we use 'period - duty + 1'.
> > > 
> > >  We modify that line anyway, so it's not like it makes the patch much more
> > >  verbose.
> > 
> > It doesn't make it more verbose, but that's not the background of my
> > question. For most(?) PWM implementation the order of hardware accesses
> > matters and introducing such a difference as an unneeded side effect
> > isn't optimal.
> 
> There's no side effect. The PWM is disabled when reconfigured.

Then please mention it in the commit log.

Best regards
Uwe

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 85e2110aae4f..8df898429d47 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -121,6 +121,7 @@  static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		   *parent_clk = clk_get_parent(clk);
 	unsigned long rate, parent_rate, period, duty;
 	unsigned long long tmp;
+	bool polarity_inversed;
 	int ret;
 
 	parent_rate = clk_get_rate(parent_clk);
@@ -183,24 +184,27 @@  static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* Reset counter to 0 */
 	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
 
-	/* Set duty */
-	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
-
 	/* Set period */
 	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
+	/*
+	 * The PWM will always start with the inactive part. To counter that,
+	 * when PWM is enabled we switch the configured polarity, and use
+	 * 'period - duty + 1' as the real duty.
+	 */
+
+	/* Set duty */
+	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period - duty + 1);
+
 	/* Set polarity */
-	switch (state->polarity) {
-	case PWM_POLARITY_NORMAL:
+	polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
+	if (!polarity_inversed ^ state->enabled)
 		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 				   TCU_TCSR_PWM_INITL_HIGH, 0);
-		break;
-	case PWM_POLARITY_INVERSED:
+	else
 		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
 				   TCU_TCSR_PWM_INITL_HIGH,
 				   TCU_TCSR_PWM_INITL_HIGH);
-		break;
-	}
 
 	if (state->enabled)
 		jz4740_pwm_enable(chip, pwm);