Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

[Andy Shevchenko]

On Wed, 2017-01-18 at 14:53 +0100, Clemens Gruber wrote:
> On Wed, Jan 18, 2017 at 01:09:24PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-01-18 at 11:57 +0100, Thierry Reding wrote:
> > > On Tue, Dec 13, 2016 at 04:52:51PM +0100, Clemens Gruber wrote:
> > > > Until now, we assumed that the period is the hardware default of
> > > > 1/200Hz
> > > > at probe time, but if the period was changed and the user
> > > > reboots,
> > > > this
> > > > assumption is wrong.
> > > > 
> > > > Solution: Check if the prescaler is set to the hardware default.
> > > > If
> > > > not,
> > > > reprogram the prescaler at first configuration.
> > 
> > AFAIU the PWM is off until user space or kernel user enables it
> > explicitly after reboot. Is it correct?
> 
> Yes, but the period could be different, maybe modified in the
> bootloader
> or at a previous boot without hardware reset in between. (We do not
> send
> a SWRST to the chip, so the period register could be different)

It's fragile to rely on some external settings, right? Wouldn't be
better to leave device in a known state after ->probe()?

> Until now, we assumed it is always 1/200 Hz and skipped the lengthy
> prescale configuration (put chip into sleep mode, set prescaler, go
> out
> of sleep mode, udelay for 0.5ms until the oscillator is back up) if
> the
> user wants a period of 1/200 Hz.
> 
> With this patch, we check if it is in fact set to the hardware
> default.
> If not, we set pca->period_ns to 0 which leads to changing the
> prescaler
> in the next call to pca9685_pwm_config.

And this contradicts, for my opinion, to the logic you referred in the
first paragraph. If you would like to use preset values, you need to
read and recalculate period correctly.

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy