Re: [PATCH v3 2/2] pwm-pca9685: Support changing the output frequency

[Clemens Gruber <clemens.gruber@pqgruber.com>]

On Mon, Jul 20, 2015 at 11:46:12AM +0200, Clemens Gruber wrote:
> On Mon, Jul 20, 2015 at 11:30:22AM +0200, Thierry Reding wrote:
> > On Mon, Jul 20, 2015 at 10:36:09AM +0200, Clemens Gruber wrote:
> > > Previously, period_ns and duty_ns were only used to determine the
> > > ratio of ON and OFF time, the default frequency of 200 Hz was never
> > > changed.
> > > The PCA9685 however is capable of changing the PWM output frequency,
> > > which is expected when changing the period.
> > > 
> > > This patch configures the prescaler accordingly, using the formula
> > > and notes provided in the PCA9685 datasheet.
> > > Bounds checking for the minimum and maximum frequencies, last updated
> > > in revision v.4 of said datasheet, is also added.
> > > 
> > > The prescaler is only touched if the period changed, because we have to
> > > put the chip into sleep mode to unlock the prescale register.
> > > If it is changed, the PWM output frequency changes for all outputs,
> > > because there is one prescaler per chip. This is documented in the
> > > PCA9685 datasheet and in the comments.
> > 
> > I think the reason why the driver doesn't support changing the frequency
> > is precisely because it has a per-chip prescaler. So you'd be changing
> > the frequency from other all other users if one of the channels requests
> > to do so. If I remember correctly there had been some discussion back at
> > the time that this wasn't safe to do.
> > 
> > If you have to do this, I'd think you'd need to at least update the duty
> > cycle of all other users according to the new period.
> 
> Hi, thanks for your feedback.
> 
> Yes, that's true. You'd have to set the prescaler first and then manually set
> all duty cycle values. Although this is a hardware feature, I can understand
> that this might be confusing.
> 
> I will send a v4 to update the duty cycle of all other users after the chip
> woke up again!
> (Using the ratio from duty cycle to old period to set the new duty cycle)
> 
> Clemens

Do you think it would be a good idea to print something to the kernel log when
changing the prescaler frequency? At the moment I only print an error when it is
out of bounds.
To notify the users that the frequency of all channels changed and the duty
cycle values have been adjusted?

Clemens