[PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Uwe Kleine-König]

The driver only supports normal polarity. Complete the implementation of
.get_state() by setting .polarity accordingly.

This fixes a regression that was possible since commit c73a3107624d
("pwm: Handle .get_state() failures") which stopped to zero-initialize
the state passed to the .get_state() callback. This was reported at
https://forum.odroid.com/viewtopic.php?f=177&t=46360 . While this was an
unintended side effect, the real issue is the driver's callback not
setting the polarity.

There is a complicating fact, that the .apply() callback fakes support
for inversed polarity. This is not (and cannot) be matched by
.get_state(). As fixing this isn't easy, only point it out in a comment
to prevent authors of other drivers from copying that approach.

Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()")
Reported-by: Munehisa Kamata <kamatam@amazon.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

similar to the patches for four other drivers[1], I think we should
apply this patch as a fix.

Best regards
Uwe

[1] https://lore.kernel.org/linux-pwm/20230228135508.1798428-1-u.kleine-koenig@pengutronix.de

 drivers/pwm/pwm-meson.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 16d79ca5d8f5..5cd7b90872c6 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -162,6 +162,12 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 	duty = state->duty_cycle;
 	period = state->period;
 
+	/*
+	 * Note this is wrong. The result is an output wave that isn't really
+	 * inverted and so is wrongly identified by .get_state as normal.
+	 * Fixing this needs some care however as some machines might rely on
+	 * this.
+	 */
 	if (state->polarity == PWM_POLARITY_INVERSED)
 		duty = period - duty;
 
@@ -358,6 +364,8 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		state->duty_cycle = 0;
 	}
 
+	state->polarity = PWM_POLARITY_NORMAL;
+
 	return 0;
 }
 
-- 
2.39.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Martin Blumenstingl]

Hi Uwe,

On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
[...]
> There is a complicating fact, that the .apply() callback fakes support
> for inversed polarity. This is not (and cannot) be matched by
> .get_state(). As fixing this isn't easy, only point it out in a comment
> to prevent authors of other drivers from copying that approach.
If you have any suggestions on how to fix this then please let us know.
I don't recall any board needing support for inverted PWM - but they
may be out there somewhere...

> Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()")
> Reported-by: Munehisa Kamata <kamatam@amazon.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2750 bytes --]

On Sat, Mar 11, 2023 at 10:00:50PM +0100, Martin Blumenstingl wrote:
> Hi Uwe,
> 
> On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> [...]
> > There is a complicating fact, that the .apply() callback fakes support
> > for inversed polarity. This is not (and cannot) be matched by
> > .get_state(). As fixing this isn't easy, only point it out in a comment
> > to prevent authors of other drivers from copying that approach.
> If you have any suggestions on how to fix this then please let us know.
> I don't recall any board needing support for inverted PWM - but they
> may be out there somewhere...

And that's the problem. As the hardware doesn't support inverted
polarity there is no way to implement it correctly. The only right way
would be to return -EINVAL in this case, but this might break some
consumers.

I have an idea how to evolve the PWM API. That's by introducing an
.offset parameter to struct pwm_state. This would describe the following
PWM signal:


   ___________/¯¯¯¯¯¯¯¯¯\_______________/¯¯¯¯¯¯¯¯¯\____
   ^                         ^                         ^                         ^                         ^
   <------ period ---------->
   <- offset->
              <--------> duty_cycle

This is more general than polarity: It can describe normal polarity
(.offset = 0) and inversed polarity (.offset = .period - .duty_cycle).

Then the policy to implement a pwm_state like that would probably be:

 - Pick the biggest period not bigger than requested
 - for that period pick the biggest duty cycle not bigger than requested
 - for that period and duty_cycle pick the biggest offset not bigger
   than requested.

With these rules in place it would be allowed to configure normal
polarity for a request with inverted polarity, but not the other way
around. Then the algorithm currently implemented in the meson driver
would be allowed.

A consumer that doesn't care about the offset (i.e. most drivers) could
just pass .offset = .period - 1.

To be practical for consumers who care about polarity, we first would
need a way to test the capabilities of a PWM though. I have an idea for
that, too, but today this is still vapourware.

> > Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()")
> > Reported-by: Munehisa Kamata <kamatam@amazon.com>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Martin Blumenstingl]

On Sat, Mar 11, 2023 at 10:44 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Sat, Mar 11, 2023 at 10:00:50PM +0100, Martin Blumenstingl wrote:
> > Hi Uwe,
> >
> > On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > [...]
> > > There is a complicating fact, that the .apply() callback fakes support
> > > for inversed polarity. This is not (and cannot) be matched by
> > > .get_state(). As fixing this isn't easy, only point it out in a comment
> > > to prevent authors of other drivers from copying that approach.
> > If you have any suggestions on how to fix this then please let us know.
> > I don't recall any board needing support for inverted PWM - but they
> > may be out there somewhere...
>
> And that's the problem. As the hardware doesn't support inverted
> polarity there is no way to implement it correctly. The only right way
> would be to return -EINVAL in this case, but this might break some
> consumers.
>
> I have an idea how to evolve the PWM API. That's by introducing an
> .offset parameter to struct pwm_state. This would describe the following
> PWM signal:
>
>
>    ___________/¯¯¯¯¯¯¯¯¯\_______________/¯¯¯¯¯¯¯¯¯\____
>    ^                         ^                         ^                         ^                         ^
>    <------ period ---------->
>    <- offset->
>               <--------> duty_cycle
>
> This is more general than polarity: It can describe normal polarity
> (.offset = 0) and inversed polarity (.offset = .period - .duty_cycle).
>
> Then the policy to implement a pwm_state like that would probably be:
>
>  - Pick the biggest period not bigger than requested
>  - for that period pick the biggest duty cycle not bigger than requested
>  - for that period and duty_cycle pick the biggest offset not bigger
>    than requested.
>
> With these rules in place it would be allowed to configure normal
> polarity for a request with inverted polarity, but not the other way
> around. Then the algorithm currently implemented in the meson driver
> would be allowed.
>
> A consumer that doesn't care about the offset (i.e. most drivers) could
> just pass .offset = .period - 1.
>
> To be practical for consumers who care about polarity, we first would
> need a way to test the capabilities of a PWM though. I have an idea for
> that, too, but today this is still vapourware.
In my opinion your proposal makes sense. I don't have the time to
implement it myself at the moment though. I can help test on some of
the Amlogic SBCs that I have (and use a cheap signal analyzer I have
to look at the generated PWM signal).


Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Jerome Brunet]

On Sat 11 Mar 2023 at 22:00, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Uwe,
>
> On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> [...]
>> There is a complicating fact, that the .apply() callback fakes support
>> for inversed polarity. This is not (and cannot) be matched by
>> .get_state(). As fixing this isn't easy, only point it out in a comment
>> to prevent authors of other drivers from copying that approach.
> If you have any suggestions on how to fix this then please let us know.
> I don't recall any board needing support for inverted PWM - but they
> may be out there somewhere...

AFAIK, PWM are essentially used for the SDIO 32k clock and voltage
regulators. I don't recall seeing anything else.

It should be safe to change polarity if necessary, except for the DVFS
PWM regulators maybe ? I fear that if we change the PWM setting it might
trigger a glitch on the supply and possibly stall the CPU.

That being said, I don't think there is any particular care regarding
that right now, so maybe it is fine.

>
>> Fixes: c375bcbaabdb ("pwm: meson: Read the full hardware state in meson_pwm_get_state()")
>> Reported-by: Munehisa Kamata <kamatam@amazon.com>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
>
> Best regards,
> Martin


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2299 bytes --]

On Mon, Mar 13, 2023 at 10:07:48AM +0100, Jerome Brunet wrote:
> 
> On Sat 11 Mar 2023 at 22:00, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> 
> > Hi Uwe,
> >
> > On Fri, Mar 10, 2023 at 8:14 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > [...]
> >> There is a complicating fact, that the .apply() callback fakes support
> >> for inversed polarity. This is not (and cannot) be matched by
> >> .get_state(). As fixing this isn't easy, only point it out in a comment
> >> to prevent authors of other drivers from copying that approach.
> > If you have any suggestions on how to fix this then please let us know.
> > I don't recall any board needing support for inverted PWM - but they
> > may be out there somewhere...
> 
> AFAIK, PWM are essentially used for the SDIO 32k clock and voltage
> regulators. I don't recall seeing anything else.
> 
> It should be safe to change polarity if necessary, except for the DVFS
> PWM regulators maybe ? I fear that if we change the PWM setting it might
> trigger a glitch on the supply and possibly stall the CPU.
> 
> That being said, I don't think there is any particular care regarding
> that right now, so maybe it is fine.

Another option is to do something like that:

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 16d79ca5d8f5..25a177a3fa00 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -162,8 +162,10 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 	duty = state->duty_cycle;
 	period = state->period;
 
-	if (state->polarity == PWM_POLARITY_INVERSED)
+	if (state->polarity == PWM_POLARITY_INVERSED) {
+		WARN_ONCE(1, "Wrongly trying to support inversed polarity. Please report to linux-pwm@vger.kernel.org if you rely on this\n");
 		duty = period - duty;
+	}
 
 	fin_freq = clk_get_rate(channel->clk);
 	if (fin_freq == 0) {

and then drop that faked support in a year or so if nobody spoke up.

Disclaimer: I assume Thierry is not a fan of this approach, he opposed
similar warnings in the past.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] pwm: meson: Explicitly set .polarity in .get_state()

[Martin Blumenstingl]

On Mon, Mar 13, 2023 at 10:51 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
[...]
> Another option is to do something like that:
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 16d79ca5d8f5..25a177a3fa00 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -162,8 +162,10 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>         duty = state->duty_cycle;
>         period = state->period;
>
> -       if (state->polarity == PWM_POLARITY_INVERSED)
> +       if (state->polarity == PWM_POLARITY_INVERSED) {
> +               WARN_ONCE(1, "Wrongly trying to support inversed polarity. Please report to linux-pwm@vger.kernel.org if you rely on this\n");
>                 duty = period - duty;
> +       }
>
>         fin_freq = clk_get_rate(channel->clk);
>         if (fin_freq == 0) {
>
> and then drop that faked support in a year or so if nobody spoke up.
>
> Disclaimer: I assume Thierry is not a fan of this approach, he opposed
> similar warnings in the past.
I personally think it's fine to have this warning.
If Thierry has no objections in this case then it'll help us find
whether we really need proper support in PWM core or we can just
remove this fake support from pwm-meson


Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic