Messages in this thread | | | Date | Thu, 19 Nov 2020 17:00:13 +0100 | From | Clemens Gruber <> | Subject | Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API |
| |
On Thu, Nov 19, 2020 at 09:58:26AM -0500, Sven Van Asbroeck wrote: > On Thu, Nov 19, 2020 at 5:00 AM Clemens Gruber > <clemens.gruber@pqgruber.com> wrote: > > > > > You appear to mix cached and uncached uses of prescale, > > > is there a need for this? If not, perhaps pick one and use > > > it consistently? > > > > Yes, sticking to the cached value is probably the way to go. > > > > I would suggest going one step further, and turn on the cache in > regmap, i.e. .cache_type = REGCACHE_RBTREE, then: > - no need to cache pca->prescale explicitly, you can just read it with > regmap_read() every time, and it won't result in bus activity. > then you can eliminate pca->prescale, which simplifies the driver. > - pca9685_pwm_get_state() no longer results in bus reads, every regmap_read() > is cached, this is extremely efficient. > - pca9685_pwm_apply() and pca9685_pwm_gpio_set() now only does bus writes if > registers actually change, i.e. calling pwm_apply() multiple times in a row > with the same parameters, writes the registers only once.
Interesting, I will look into that.
> > We can do this safely because this chip never actively writes to its > registers (as far as I know).
I think so too.
> > But maybe that's a suggestion for a follow-up patch... > > > > Also, if the prescale register contains an invalid value > > > during probe(), e.g. 0x00 or 0x01, would it make sense > > > to explicitly overwrite it with a valid setting? > > > > As long as it is overwritten with a correct setting when the PWM is used > > for the first time, it should be OK? > > I'm not sure. Consider the following scenario: > - prescale register is invalid at probe, say it contains 0x02 > - user calls pwm_apply() but with an invalid period, which results > in a calculated prescale value of 0x02 > - pca9685_pwm_apply() skips prescale setup because prescale does not > change, returns OK(0) > - user believes setup was ok, actually it's broken...
Makes sense. I will write the default prescale setting in case we read an invalid one from the register.
> > Also, some people use this chip exclusively as a gpiochip, in that > case the prescale register is never touched. So an invalid prescale > at probe is never corrected. > > Speaking of the gpiochip side, would it make sense to call > pca9685_pwm_full_on()/_off() in pca9685_pwm_gpio_set() too?
Yes, I think so. Would be cleaner and we avoid setting all registers to 0 when the GPIO is disabled.
--
One thing I noticed: The driver currently assumes that it comes out of POR in "active" state (comment at end of probe and PM calls). However, the SLEEP bit is set by default / after POR.
Do you agree with the following solution? 1) In .probe: call pm_runtime_set_suspended() instead of _set_active() (If CONFIG_PM is enabled, the SLEEP bit will be cleared in .resume) 2) If !CONFIG_PM: Clear the SLEEP bit in .probe
Thanks, Clemens
| |