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

[Uwe Kleine-König]

Hello Thierry,

On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote:
> 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:
> > > > 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.

The complicated thing is to chose how to map the hardware imposed
limitations to the consumers of the two (or more?) channels. And the
problem is there is no golden way that is objectively better than all
others.

> > > 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.

The point I want to make is, that with the usage counter you suggested
you just shifted the problem and didn't solve it. I agree we need a
usage counter, but you still have to think about how you want to answer
the original questions by Sven. Depending on that you have to
consider a channel running at 0% or 100% a user, or not.  (Or the other
way round: You select a policy if you consider 0% and 100% a use and
implicitly answer the questions 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.

I think we agree here, just with different words. The only thing I doubt
is: You wrote: "If they know $X, then they can still use that other
channel in whatever way they want." Who is "they"? How can they know
that $X is valid for someone else, or anyone else? Or is it enough that
"they" know this about their own use? Now Clemens wants to improve the
driver, does he need to consider "them" in the mainline driver
implementation? If Clemens chooses one way or the other, will there be
someone who then will produce a use case contradicting the implemented
policy? How will you (or who will then) decide which use-case is more
important?

> 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.

Yes.

> > > > 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?

The part I called wishy-washy is: "[....] if there aren't any use-cases
where [...]". Who should decide what are (relevant) use-cases? We
already agreed above that we talk about a hardware that doesn't allow us
to consider it consisting of 2 independent channels and so we somehow
have to limit their capabilities exposed by the PWM API. And whatever
compromise we make, it's not hard to find a more or less realistic use
case where the compromise hurts. So my interpretation of your words are:
"I'm fine with this alternative if $somethingimpossible" which is not
helpful.

So yes, there is something wrong with your metric because it's IMHO
impossible for a driver author to actually use it.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature