Date:   Wed, 25 Nov 2020 14:46:55 -0500
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     linux-pwm@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, 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 v3 1/4] pwm: pca9685: Switch to atomic API

On Wed, Nov 25, 2020 at 12:28 PM Clemens Gruber
<clemens.gruber@...ruber.com> wrote:
>
> What's the lesser evil in your opinion, always returning 0 duty and
> disabled for the ALL channel or caching it?
>

I would definitely suggest returning a pwm_state consisting of all zeroes
for the "all led" channel, instead of caching stuff.

And I don't think it's in any way evil: from a user point of view, reading
back the "all led" channel state makes no sense. Why? Because setting
that channel is just a write to all 16 channels at once. And those channels
may change over time.

For example:
1. set all leds channel = enabled, 50% duty cycle
   => all 16 leds are enabled at 50% duty cycle
2. set led 0 = disabled
3. set led 1 = enabled, 70% duty cycle
4. imagine we now read back the "all leds" state, what should it return?
   it makes no sense, because not all leds are in the same state !

>
> Please let me know if I misunderstood you.
>

All good, thanks !

>
> I meant that with sysfs, the period does not change if the new value is
> the same as the last time that channel's period was set.
> See my example above.
>
> But we probably can't do anything about that.
>

Oh I see what you mean. This is because the pwm core makes certain basic
assumptions about pwm devices. It assumes that channels are completely
independent, ie. setting channel X state won't influence channel Y.
And that's not the case for this chip.

I think the current pwm core behaviour is not ideal for us, but acceptable.
Let's get our own house in order before worrying about the core's behaviour.

>
> OK, regmap accesses are protected with locks but what about the SLEEP
> bit that needs to be set/cleared + sleep. Shouldn't we only allow one
> thread at one time to change the prescaler of a chip? Otherwise, there
> could be synchronization issues there too. (Possible writing to the
> prescale register with the SLEEP bit unset by another thread)

Good point. Luckily, the runtime_pm code is completely thread-safe.
It's quite common for drivers to use that code from multiple threads.
That's the cool thing about re-using existing kernel infrastructure,
you get all of that for free, and tested too.

You only reach the danger zone if you're doing things that the core
is completely not expecting. In this case, the core doesn't expect
that a pwm_apply() on one channel modifies any unprotected shared state.

>
> If we drop the cache we would have to read the prescale register
> whenever we need it (for every channel) but with the upcoming regmap
> cache feature, this would probably be OK.

We only need to read the prescale register during pwm_get_state() and
pwm_apply(), correct? I count many regmap_read()s in those functions.

One extra regmap_read() to retrieve the prescale, won't make much difference,
even when regmap cache is off.

>
> Do you think this should be solved in the same patch as the atomic API
> conversion or can we do this in a follow-up patch?
>

Suggestion: in the atomic API patch, do not cache prescale, just read it
out as needed. This is slightly slower, but I suspect that the code
will become shorter and simpler.

Then if you have time && motivation left, enable regmap cache.
But watch out... you'll probably have to invalidate the cache
each time the driver writes to an ALL_LED register. Because that
implies a write to 16 other registers.

But the efficiency saving is probably very limited, and may not be
worth the added complexity.

Powered by blists - more mailing lists