| Message ID | 20230303205821.2285418-1-lorenz@brun.one (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: mediatek: support inverted polarity | expand |
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
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
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
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;
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(-)