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 <[email protected]>
---
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
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/[email protected]/T/#meda75ffbd4ef2048991ea2cd091c0c14b1bb09c2
So this patch won't be accepted, still pointing out a style problem
below.
> Signed-off-by: Lorenz Brun <[email protected]>
> ---
> 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/ |
On Fri, Mar 3 2023 at 22:17:25 +01:00:00, Uwe Kleine-K?nig
<[email protected]> 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/[email protected]/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 <[email protected]>
>> ---
>> 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
> <[email protected]> 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/[email protected]/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/ |