On Thu, Nov 19, 2020 at 5:00 AM Clemens Gruber <clemens.gru...@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. We can do this safely because this chip never actively writes to its registers (as far as I know). 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... 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?
