Date:   Tue, 8 Dec 2020 17:57:12 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Clemens Gruber <clemens.gruber@...ruber.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org,
        Lee Jones <lee.jones@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        David Jander <david@...tonic.nl>
Subject: Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> Uwe, Thierry,
> 
> On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
> <u.kleine-koenig@...gutronix.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 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.

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

Thierry

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

Powered by blists - more mailing lists