Message-ID: <20200714160856.rjqi7lv63geil3hm@pengutronix.de>
Date:   Tue, 14 Jul 2020 18:08:56 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Michael Walle <michael@...le.cc>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-pwm@...r.kernel.org, linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Lee Jones <lee.jones@...aro.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Shawn Guo <shawnguo@...nel.org>, Li Yang <leoyang.li@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <maz@...nel.org>, Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller

Hello Michael,

On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote:
> Am 2020-07-13 10:47, schrieb Uwe Kleine-König:
> > I already thought about proposing pwm_get_rate_hw(), but for now there
> > is (AFAICT) no user who would need it. And it's hard to know which
> > variant is actually preferred by consumers. My expectation is that most
> > don't even care.
> > 
> > I also have a pwm_round_rate() function in mind that will give you the
> > actual rate without applying it. This can then be used by consumers who
> > care. But also there is no user who would need it today.
> 
> Ok. I take it that all such improvements are still in the making ;)

If you have a real use case, present it, then I give it a boost on my
todo list.

> > > But the PWM subsystem returns the cached state,
> > > right? get_state() is called only on device request (and during
> > > debug it seems). Actually, enabling PWM_DEBUG might choke on this
> > > workaround (".apply didn't pick the best available period"). Is
> > > this ok?
> > 
> > hmm, I didn't consider this when writing the checks for PWM_DEBUG.
> > According to the currently checked rules the expected configuration is
> > to pick the 250Hz mode and use cycle = 0x7f.
> 
> Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 500Hz
> mode.

No, when I thought about a sane set of rules (and so checks for
PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for
all otherwise available period lengths. I'm still amazed sometimes how
different the capabilities of different implementations for something so
seemingly easy like a PWM are.

> > Hmm, I have to think about
> > this. Maybe we should weaken the check to the cases with
> > 0 < duty_cycle < period. Thierry, what do you think?
> > 
> > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems
> > to be far from reality. (Is it?)
> 
> If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because
> the user wants an all-high line, but in the end it would be a toggling
> line. It wouldn't be that bad for anything in between 0% and 100% but
> IMHO its bad for exactly 0% and 100%.
> 
> You could also ask the driver about known quirks, like special 0% and
> 100% handling and exclude it from the tests accordingly.

Do you care enough to propose a patch? You're in the situation to test
it.

> > > > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
> > > > > (u8)cycle);
> > > >
> > > > I assume this can result in broken output? Consider the hardware runs
> > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
> > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
> > > 
> > > Isn't that always the case if a write may fail and there are more than
> > > one register to configure?
> > 
> > Depending on hardware capabilities you might not be able to prevent
> > this yes. Unfortunately this is quite common.
> > 
> > But there are hardware implementations that are not prone to such
> > failures. (E.g. the registers written can be only shadow values that are
> > latched into hardware only when the last value is written.)
> 
> Maybe this could be improved in the future.

We should somewhere describe, what an ideal PWM can do. 
My wishlist (just as it comes to my mind, so no guarantee of
completeness):

 - can do 0% duty cycle for all supported period lengths
 - can do 100% duty cycle for all supported period lengths
 - supports both polarities
 - supports immediate change of configuration and after completion of
   the currently running period
 - atomic update (i.e. if you go from configuration A to configuration B
   the hardware guarantees to only emit periods of type A and then type
   B. (Depending on the item above, the last A period might be cut off.)
 - emits an irq when configuration changes

> > If you change only cycle but not mode, does the hardware complete the
> > currently running period?
> 
> No it does not.

Please document this as a Limitation.
 
> > What about disable()?
> 
> Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
> fix that (in hardware), not much we can do in the driver here. We are
> _very_ constraint in size, therefore all that little edge cases fall off
> the table.

You're saying that on disable the hardware emits a constant high level
for one cycle? I hope not ...

I never programmed a CPLD to emulate a hardware PWM, but I wonder if
these are really edge cases that increase the size of the binary?!

Best regards
Uwe


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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists