Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6444 bytes --]

On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote:
> Hello Thierry, hello Sven,
> 
> On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > If this is already in the old code, this probably warrants a separate
> > > > fix, and yes, I consider this a severe bug. (Consider one channel
> > > > driving a motor and reconfiguring an LED modifies the motor's speed.)
> > > >
> > > 
> > > I think you are 100% correct, this would be a severe bug. I have only used
> > > this chip to drive LEDs, where the actual period is not that important. But
> > > for motor control, it's a different story.
> > > 
> > > Basically you are suggesting: the period (prescaler) can only be changed iff
> > > its use-count is 1.
> > > 
> > > This however brings up a whole load of additional questions: consider the case
> > > where the chip outputs are also used in gpio mode. the gpio functionality
> > > only sets "full on" and "full off" bits. On a scope, a gpio output will look
> > > identical, no matter the value of the period. So when a gpio output is in use,
> > > does it increment the prescaler use-count ?
> > > 
> > > Example:
> > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > > 2. output 2: set led mode (full-on bit set)
> > > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz)
> > > 
> > > Do we have to make (3) fail? I would say no: although output 2 is in use,
> > > it's not actually using the prescaler. Changing prescale won't modify
> > > output 2 in any way.
> > > 
> > > Which brings us to an even trickier question: what happens if a pwm output
> > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > > So when it's enabled, it does not use the prescaler.
> > > But! what happens if we now set that output to a different duty cycle?
> > > 
> > > Example:
> > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> > >   fail? no, because it's not actually using the period (it's full on)
> > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> > >   fail? no, because it's not actually using the period (it's full on)
> > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> > >   fail? no, because only output 1 is using the prescaler
> > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> > >   fail? no, because output 2 is not changing the prescaler
> > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > >   fail? yes, because output 2 is changing prescaler and it's already in use
> > > 
> > > IMHO all this can get very complicated and tricky.
> > 
> > Is this really that complicated?
> 
> I think it is.

Care to specify what exactly is complicated about it? You're just saying
that you don't like the restrictions that this implements, but there's
really nothing we can do about that because the hardware just doesn't
give you that flexibility.

> > I sounds to me like the only thing that you need is to have some sort
> > of usage count for the prescaler. Whenever you want to use the
> > prescaler you check that usage count. If it is zero, then you can just
> > set it to whatever you need. If it isn't zero, that means somebody
> > else is already using it and you can't change it, which means you have
> > to check if you're trying to request the value that's already set. If
> > so, you can succeed, but otherwise you'll have to fail.
> 
> With this abstraction Sven's questions are changed to:
> 
> Does a PWM that runs at 0% or 100% use the prescaler?
> 
> If yes, you limit the possibilities of the brother channels. And if not,
> it will not be possible to go to a 50% relative duty cycle while
> retaining the period. Both sounds not optimal.

Again, this is a restriction imposed by the hardware design and there's
nothing in software that we can do about that. The only thing I proposed
was a simple way to detect the circumstances and make sure we can deal
with it.

And that's obviously subject to the kind of policy we want to implement.
I don't think it's necessarily a bad thing to give people the most
flexibility. If they know that one PWM channel is only ever going to be
full-on/full-off, then they can still use that other channel in whatever
way they want. If, on the other hand, we assume that the prescaler is
always going to be used we limit the flexibility even if we don't
necessarily have to.

Obviously if you want to use both channels at partial duty-cycles there
isn't much you can do and you really have to make sure they both run at
the same frequency/period. But that's usually something that you can
deal with by just choosing a period that works for both.

And if that's not possible, well, then you better just use a different
PWM controller to begin with, because you just can't make it work.

> > > We can of course make this much simpler by assumung that gpio or on/off pwms
> > > are actually using the prescaler. But then we'd be limiting this chip's
> > > functionality.
> > 
> > Yeah, this is obviously much simpler, but the cost is a bit high, in my
> > opinion. I'm fine with this alternative if there aren't any use-cases
> > where multiple outputs are actually used.
> 
> This metric is wishy-washy; of course you can construct a use-case. I'd
> still go for this simpler option and assume the prescaler used if the
> PWM runs at 0% or 100%, but not when it is a GPIO.

I don't understand what you're saying here. On one hand you seem to
object to what I was saying, but then you agree with it?

And I'm not asking anyone to make up any artificial use-case. What I'm
saying is that if there aren't any existing use-cases that would break
if we assume a pre-scaler is used for full-on/full-off, then I'm okay
with making that assumption and simplifying the driver. If there were
use-cases, then that assumption would break existing users and that's
not something I'm willing to accept.

Anything wrong with that metric in your opinion?

Thierry

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