[PATCH v2] pwm: mediatek: support inverted polarity

[PATCH v2] 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.

To still support inverted PWM for common use cases, this relaxes the
check for inverted polarity within the driver to allow it to work in
case usage_power is set to true, i.e. the exact waveform does not
matter. If usage_power is true and the polarity is inverted the duty
cycle is mathematically inverted before being applied to the hardware.

Signed-off-by: Lorenz Brun <lorenz@brun.one>
---
V2: Only allow mathematically inverted PWM if usage_power is true
---
 drivers/pwm/pwm-mediatek.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5b5eeaff35da..18791304d1ca 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -202,8 +202,16 @@ 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)
+	u64 duty_cycle;
+
+	/* According to the MT7986 Reference Manual the peripheral does not
+	 * appear to have the capability to invert the output.
+	 * This means that inverted mode can not be fully supported as the
+	 * waveform will always start with the low period and end with the high
+	 * period. Thus reject non-normal polarity if the shape of the waveform
+	 * matters, i.e. usage_power is not set.
+	 */
+	if (state->polarity != PWM_POLARITY_NORMAL && !state->usage_power)
 		return -EINVAL;
 
 	if (!state->enabled) {
@@ -213,7 +221,11 @@ 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);
+	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.2

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

[Thierry Reding]

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

On Thu, Mar 09, 2023 at 02:04:10AM +0100, Lorenz Brun wrote:
> According to the MT7986 Reference Manual the Mediatek PWM controller
> doesn't appear to have support for inverted polarity.
> 
> To still support inverted PWM for common use cases, this relaxes the
> check for inverted polarity within the driver to allow it to work in
> case usage_power is set to true, i.e. the exact waveform does not
> matter. If usage_power is true and the polarity is inverted the duty
> cycle is mathematically inverted before being applied to the hardware.
> 
> Signed-off-by: Lorenz Brun <lorenz@brun.one>
> ---
> V2: Only allow mathematically inverted PWM if usage_power is true
> ---
>  drivers/pwm/pwm-mediatek.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5b5eeaff35da..18791304d1ca 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -202,8 +202,16 @@ 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)
> +	u64 duty_cycle;
> +
> +	/* According to the MT7986 Reference Manual the peripheral does not

Block comments should have no text on the first line:

	/*
	 * According
	 * ...
	 */

> +	 * appear to have the capability to invert the output.
> +	 * This means that inverted mode can not be fully supported as the
> +	 * waveform will always start with the low period and end with the high
> +	 * period. Thus reject non-normal polarity if the shape of the waveform
> +	 * matters, i.e. usage_power is not set.
> +	 */
> +	if (state->polarity != PWM_POLARITY_NORMAL && !state->usage_power)
>  		return -EINVAL;
>  
>  	if (!state->enabled) {
> @@ -213,7 +221,11 @@ 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);
> +	duty_cycle = state->duty_cycle;
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		duty_cycle = state->period - state->duty_cycle;

That's not really what state->usage_power was meant to address. What's
wrong with just reversing the duty cycle in the pwm-fan? If you use DT
it's quite trivial to do that by just reversing the entries in your
cooling-levels property. Does that not work for you?

Thierry

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

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

[Uwe Kleine-König]

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

Hello Thierry,

On Thu, Apr 06, 2023 at 03:38:48PM +0200, Thierry Reding wrote:
> On Thu, Mar 09, 2023 at 02:04:10AM +0100, Lorenz Brun wrote:
> > +	 * appear to have the capability to invert the output.
> > +	 * This means that inverted mode can not be fully supported as the
> > +	 * waveform will always start with the low period and end with the high
> > +	 * period. Thus reject non-normal polarity if the shape of the waveform
> > +	 * matters, i.e. usage_power is not set.
> > +	 */
> > +	if (state->polarity != PWM_POLARITY_NORMAL && !state->usage_power)
> >  		return -EINVAL;
> >  
> >  	if (!state->enabled) {
> > @@ -213,7 +221,11 @@ 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);
> > +	duty_cycle = state->duty_cycle;
> > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > +		duty_cycle = state->period - state->duty_cycle;
> 
> That's not really what state->usage_power was meant to address.

I don't understand your concern here. I don't like .usage_power, but
AFAICT this is a legitimite use. With .usage_power = true, the lowlevel
driver is free to shift the phase_offset and even modify the period size
and the goal is just that the average power-output matches.

Lorenz's patch does exactly this: It even keeps the period and only
shifts the phase (by period - duty_cycle). If you consider this not
legitmate, I think we have to improve the docs about .usage_power.

> What's wrong with just reversing the duty cycle in the pwm-fan? If you
> use DT it's quite trivial to do that by just reversing the entries in
> your cooling-levels property. Does that not work for you?

That's an option, too. With a different PWM (i.e. one that can do proper
inverted polarity) Lorenz's solution would be ok, though, right? And the
pwm-fan only cares about the relative duty_cycle and not the phase
shift, so setting .usage_power = true is fine, too?!

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 v2] pwm: mediatek: support inverted polarity

[Thierry Reding]

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

On Thu, Apr 06, 2023 at 03:53:58PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Apr 06, 2023 at 03:38:48PM +0200, Thierry Reding wrote:
> > On Thu, Mar 09, 2023 at 02:04:10AM +0100, Lorenz Brun wrote:
> > > +	 * appear to have the capability to invert the output.
> > > +	 * This means that inverted mode can not be fully supported as the
> > > +	 * waveform will always start with the low period and end with the high
> > > +	 * period. Thus reject non-normal polarity if the shape of the waveform
> > > +	 * matters, i.e. usage_power is not set.
> > > +	 */
> > > +	if (state->polarity != PWM_POLARITY_NORMAL && !state->usage_power)
> > >  		return -EINVAL;
> > >  
> > >  	if (!state->enabled) {
> > > @@ -213,7 +221,11 @@ 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);
> > > +	duty_cycle = state->duty_cycle;
> > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > +		duty_cycle = state->period - state->duty_cycle;
> > 
> > That's not really what state->usage_power was meant to address.
> 
> I don't understand your concern here. I don't like .usage_power, but
> AFAICT this is a legitimite use. With .usage_power = true, the lowlevel
> driver is free to shift the phase_offset and even modify the period size
> and the goal is just that the average power-output matches.
> 
> Lorenz's patch does exactly this: It even keeps the period and only
> shifts the phase (by period - duty_cycle). If you consider this not
> legitmate, I think we have to improve the docs about .usage_power.

I realize that I'm being nitpicky here. Setting usage_power = true and
duty = period - duty is a lazy way of achieving what you can easily do
by adjusting the input duty cycle.

If you all really want this, then it should go into the core, because
it's something that can be implemented on basically every single PWM
controller.

Thierry

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

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

[Uwe Kleine-König]

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

On Thu, Apr 06, 2023 at 04:30:23PM +0200, Thierry Reding wrote:
> On Thu, Apr 06, 2023 at 03:53:58PM +0200, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Thu, Apr 06, 2023 at 03:38:48PM +0200, Thierry Reding wrote:
> > > On Thu, Mar 09, 2023 at 02:04:10AM +0100, Lorenz Brun wrote:
> > > > +	 * appear to have the capability to invert the output.
> > > > +	 * This means that inverted mode can not be fully supported as the
> > > > +	 * waveform will always start with the low period and end with the high
> > > > +	 * period. Thus reject non-normal polarity if the shape of the waveform
> > > > +	 * matters, i.e. usage_power is not set.
> > > > +	 */
> > > > +	if (state->polarity != PWM_POLARITY_NORMAL && !state->usage_power)
> > > >  		return -EINVAL;
> > > >  
> > > >  	if (!state->enabled) {
> > > > @@ -213,7 +221,11 @@ 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);
> > > > +	duty_cycle = state->duty_cycle;
> > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > +		duty_cycle = state->period - state->duty_cycle;
> > > 
> > > That's not really what state->usage_power was meant to address.
> > 
> > I don't understand your concern here. I don't like .usage_power, but
> > AFAICT this is a legitimite use. With .usage_power = true, the lowlevel
> > driver is free to shift the phase_offset and even modify the period size
> > and the goal is just that the average power-output matches.
> > 
> > Lorenz's patch does exactly this: It even keeps the period and only
> > shifts the phase (by period - duty_cycle). If you consider this not
> > legitmate, I think we have to improve the docs about .usage_power.
> 
> I realize that I'm being nitpicky here. Setting usage_power = true and
> duty = period - duty is a lazy way of achieving what you can easily do
> by adjusting the input duty cycle.
> 
> If you all really want this, then it should go into the core, because
> it's something that can be implemented on basically every single PWM
> controller.

You'd need something like:

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index e01147f66e15..6bb851c2e55e 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -556,6 +556,7 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 {
 	struct pwm_chip *chip;
+	bool retry_inverted = true;
 	int err;
 
 	/*
@@ -580,10 +581,19 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	    state->usage_power == pwm->state.usage_power)
 		return 0;
 
+retry:
 	err = chip->ops->apply(chip, pwm, state);
 	trace_pwm_apply(pwm, state, err);
-	if (err)
+	if (err) {
+		if (err == -EINVAL && state->usage_power && retry_inverted) {
+			state->duty_cycle = state->period - state->duty_cycle;
+			state->polarity = 1 - state->polarity;
+			retry_inverted = false;
+			goto retry;
+		}
+
 		return err;
+	}
 
 	pwm->state = *state;
 
(Just to show the idea. It doesn't work like that, because *state is const.)

I don't like that .apply() is called twice and without having thought
much about it, I'd prefer explicit support in the lowlevel drivers over
this approach.

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