* [PATCH v2] pwm: mediatek: support inverted polarity @ 2023-03-09 1:04 Lorenz Brun 2023-04-06 13:38 ` Thierry Reding 0 siblings, 1 reply; 5+ messages in thread From: Lorenz Brun @ 2023-03-09 1:04 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-König, Matthias Brugger, AngeloGioacchino Del Regno Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-mediatek 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: mediatek: support inverted polarity 2023-03-09 1:04 [PATCH v2] pwm: mediatek: support inverted polarity Lorenz Brun @ 2023-04-06 13:38 ` Thierry Reding 2023-04-06 13:53 ` Uwe Kleine-König 0 siblings, 1 reply; 5+ messages in thread From: Thierry Reding @ 2023-04-06 13:38 UTC (permalink / raw) To: Lorenz Brun Cc: Uwe Kleine-König, Matthias Brugger, AngeloGioacchino Del Regno, linux-pwm, linux-kernel, linux-arm-kernel, linux-mediatek [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: mediatek: support inverted polarity 2023-04-06 13:38 ` Thierry Reding @ 2023-04-06 13:53 ` Uwe Kleine-König 2023-04-06 14:30 ` Thierry Reding 0 siblings, 1 reply; 5+ messages in thread From: Uwe Kleine-König @ 2023-04-06 13:53 UTC (permalink / raw) To: Thierry Reding Cc: Lorenz Brun, Matthias Brugger, AngeloGioacchino Del Regno, linux-pwm, linux-kernel, linux-arm-kernel, linux-mediatek [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: mediatek: support inverted polarity 2023-04-06 13:53 ` Uwe Kleine-König @ 2023-04-06 14:30 ` Thierry Reding 2023-04-14 5:39 ` Uwe Kleine-König 0 siblings, 1 reply; 5+ messages in thread From: Thierry Reding @ 2023-04-06 14:30 UTC (permalink / raw) To: Uwe Kleine-König Cc: Lorenz Brun, Matthias Brugger, AngeloGioacchino Del Regno, linux-pwm, linux-kernel, linux-arm-kernel, linux-mediatek [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: mediatek: support inverted polarity 2023-04-06 14:30 ` Thierry Reding @ 2023-04-14 5:39 ` Uwe Kleine-König 0 siblings, 0 replies; 5+ messages in thread From: Uwe Kleine-König @ 2023-04-14 5:39 UTC (permalink / raw) To: Thierry Reding Cc: Lorenz Brun, Matthias Brugger, AngeloGioacchino Del Regno, linux-pwm, linux-kernel, linux-arm-kernel, linux-mediatek [-- 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 --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-14 5:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-09 1:04 [PATCH v2] pwm: mediatek: support inverted polarity Lorenz Brun 2023-04-06 13:38 ` Thierry Reding 2023-04-06 13:53 ` Uwe Kleine-König 2023-04-06 14:30 ` Thierry Reding 2023-04-14 5:39 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).