[PATCH] pwm: mediatek: support inverted polarity

[PATCH] pwm: mediatek: support inverted polarity

[Lorenz Brun]

According to the MT7986 Reference Manual the Mediatek  PWM controller
doesn't appear to have support for inverted polarity.

This implements the same solution as in pwm-meson and just inverts the
duty cycle instead, which results in the same outcome.

Signed-off-by: Lorenz Brun <lorenz@brun.one>
---
 drivers/pwm/pwm-mediatek.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5b5eeaff35da..6f4a54c8299f 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -202,9 +202,7 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			      const struct pwm_state *state)
 {
 	int err;
-
-	if (state->polarity != PWM_POLARITY_NORMAL)
-		return -EINVAL;
+	u64 duty_cycle;
 
 	if (!state->enabled) {
 		if (pwm->state.enabled)
@@ -213,7 +211,14 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	err = pwm_mediatek_config(pwm->chip, pwm, state->duty_cycle, state->period);
+	// According to the MT7986 Reference Manual the peripheral does not
+	// appear to have the capability to invert the output. Instead just
+	// invert the duty cycle.
+	duty_cycle = state->duty_cycle;
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		duty_cycle = state->period - state->duty_cycle;
+
+	err = pwm_mediatek_config(pwm->chip, pwm, duty_cycle, state->period);
 	if (err)
 		return err;
 
-- 
2.39.1

Re: [PATCH] pwm: mediatek: support inverted polarity

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

On Fri, Mar 03, 2023 at 09:58:21PM +0100, Lorenz Brun wrote:
> According to the MT7986 Reference Manual the Mediatek  PWM controller
> doesn't appear to have support for inverted polarity.
> 
> This implements the same solution as in pwm-meson and just inverts the
> duty cycle instead, which results in the same outcome.

This idea is broken. This was recently discussed on the linux-pwm list
and I hope will be fixed soon. See
https://lore.kernel.org/linux-pwm/20230228093911.bh2sbp4tyfir2z5g@pengutronix.de/T/#meda75ffbd4ef2048991ea2cd091c0c14b1bb09c2

So this patch won't be accepted, still pointing out a style problem
below.

> Signed-off-by: Lorenz Brun <lorenz@brun.one>
> ---
>  drivers/pwm/pwm-mediatek.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5b5eeaff35da..6f4a54c8299f 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -202,9 +202,7 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			      const struct pwm_state *state)
>  {
>  	int err;
> -
> -	if (state->polarity != PWM_POLARITY_NORMAL)
> -		return -EINVAL;
> +	u64 duty_cycle;
>  
>  	if (!state->enabled) {
>  		if (pwm->state.enabled)
> @@ -213,7 +211,14 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return 0;
>  	}
>  
> -	err = pwm_mediatek_config(pwm->chip, pwm, state->duty_cycle, state->period);
> +	// According to the MT7986 Reference Manual the peripheral does not
> +	// appear to have the capability to invert the output. Instead just
> +	// invert the duty cycle.

Wrong commenting style, please stick to C-style comments (/* ... */)

> +	duty_cycle = state->duty_cycle;
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		duty_cycle = state->period - state->duty_cycle;
> +
> +	err = pwm_mediatek_config(pwm->chip, pwm, duty_cycle, state->period);
>  	if (err)
>  		return err;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: mediatek: support inverted polarity

[Lorenz Brun]

On Fri, Mar 3 2023 at 22:17:25 +01:00:00, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Mar 03, 2023 at 09:58:21PM +0100, Lorenz Brun wrote:
>>  According to the MT7986 Reference Manual the Mediatek  PWM 
>> controller
>>  doesn't appear to have support for inverted polarity.
>> 
>>  This implements the same solution as in pwm-meson and just inverts 
>> the
>>  duty cycle instead, which results in the same outcome.
> 
> This idea is broken. This was recently discussed on the linux-pwm list
> and I hope will be fixed soon. See
> https://lore.kernel.org/linux-pwm/20230228093911.bh2sbp4tyfir2z5g@pengutronix.de/T/#meda75ffbd4ef2048991ea2cd091c0c14b1bb09c2
> 
Is the issue here emulating PWM_POLARITY_INVERSED by inverting the 
period or the overflow issues?
This driver currently rejects PWM_POLARITY_INVERSED, but the problem is 
that I have a board which inverts the output of the PWM peripheral 
(low-side MOSFET for higher-voltage open-drain output), thus I need to 
set the PWM node to output an inverted signal so that the final 
open-drain output behaves correctly as the signal has been inverted 
twice now.

In my specific case this logic could also be added to pwm-fan, but this 
would lead to more complexity there as this type of circuit is 
generally handled by the PWM driver.

> So this patch won't be accepted, still pointing out a style problem
> below.
> 
>>  Signed-off-by: Lorenz Brun <lorenz@brun.one>
>>  ---
>>   drivers/pwm/pwm-mediatek.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>>  index 5b5eeaff35da..6f4a54c8299f 100644
>>  --- a/drivers/pwm/pwm-mediatek.c
>>  +++ b/drivers/pwm/pwm-mediatek.c
>>  @@ -202,9 +202,7 @@ static int pwm_mediatek_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   			      const struct pwm_state *state)
>>   {
>>   	int err;
>>  -
>>  -	if (state->polarity != PWM_POLARITY_NORMAL)
>>  -		return -EINVAL;
>>  +	u64 duty_cycle;
>> 
>>   	if (!state->enabled) {
>>   		if (pwm->state.enabled)
>>  @@ -213,7 +211,14 @@ static int pwm_mediatek_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   		return 0;
>>   	}
>> 
>>  -	err = pwm_mediatek_config(pwm->chip, pwm, state->duty_cycle, 
>> state->period);
>>  +	// According to the MT7986 Reference Manual the peripheral does 
>> not
>>  +	// appear to have the capability to invert the output. Instead 
>> just
>>  +	// invert the duty cycle.
> 
> Wrong commenting style, please stick to C-style comments (/* ... */)
I can fix that if I end up submitting a V2 of this patch, but this 
didn't get picked up by checkpatch.

> 
>>  +	duty_cycle = state->duty_cycle;
>>  +	if (state->polarity == PWM_POLARITY_INVERSED)
>>  +		duty_cycle = state->period - state->duty_cycle;
>>  +
>>  +	err = pwm_mediatek_config(pwm->chip, pwm, duty_cycle, 
>> state->period);
>>   	if (err)
>>   		return err;
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> https://www.pengutronix.de/ |

Regards,
Lorenz

Re: [PATCH] pwm: mediatek: support inverted polarity

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

Hello Lorenz,

On Fri, Mar 03, 2023 at 11:23:07PM +0100, Lorenz Brun wrote:
> On Fri, Mar 3 2023 at 22:17:25 +01:00:00, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Mar 03, 2023 at 09:58:21PM +0100, Lorenz Brun wrote:
> > >  According to the MT7986 Reference Manual the Mediatek  PWM
> > > controller
> > >  doesn't appear to have support for inverted polarity.
> > > 
> > >  This implements the same solution as in pwm-meson and just inverts
> > > the
> > >  duty cycle instead, which results in the same outcome.
> > 
> > This idea is broken. This was recently discussed on the linux-pwm list
> > and I hope will be fixed soon. See
> > https://lore.kernel.org/linux-pwm/20230228093911.bh2sbp4tyfir2z5g@pengutronix.de/T/#meda75ffbd4ef2048991ea2cd091c0c14b1bb09c2
> > 
> Is the issue here emulating PWM_POLARITY_INVERSED by inverting the period or
> the overflow issues?
> This driver currently rejects PWM_POLARITY_INVERSED, but the problem is that
> I have a board which inverts the output of the PWM peripheral (low-side
> MOSFET for higher-voltage open-drain output), thus I need to set the PWM
> node to output an inverted signal so that the final open-drain output
> behaves correctly as the signal has been inverted twice now.
> 
> In my specific case this logic could also be added to pwm-fan, but this
> would lead to more complexity there as this type of circuit is generally
> handled by the PWM driver.

The issue is clear, and I'm sure the motivation was similar for meson.

However just inverting duty_cycle might hurt consumers who rely on
actually inversed polarity.

There is an approach available: You could implement support for
.usage_power. However I don't like that concept because its semantic is
unclear (but in the past there is no agreement about that betweeen
Thierry and me).

My favourite would be to add a u64 duty_offset to struct pwm_state that
would allow to request something like:

       ________          ________          ________
   ___/        \________/        \________/        \______
   ^                 ^                 ^                 ^
   <->                duty_offset
      <------->       duty_cycle
   <----------------> period

Then todays requests would be equivalent to .duty_offset = 0, and
drivers would be advised to implement the biggest duty_offset not bigger
than requested (i.e. similar to how period and duty_cycle work).

This could even replace .polarity by setting .duty_offset = .period -
.duty_cycle. And a consumer who doesn't care about polarity but only
about percentage of the active time during a period could signal that by
.duty_offset = .period (or .period - 1?).

Of course that would be a bigger effort.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]