On Mon, Jul 20, 2015 at 11:50:55AM +0200, Clemens Gruber wrote: > 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?
None of the users would actually notice the kernel log entry, so I don't think it makes much sense. One problem I see with this is that we might start to see some sort of ping-pong happening between various users of the PWMs of the same chip. I guess it depends a bit on what the typical use-cases are for this chip. Is it usually used only for LEDs (the register names suggest it)? If so, how likely is it that people will want to drive them at different periods? If it's reasonable to require the same period for all of them, maybe it would be worth adding some sort of WARN_ON() or dev_warn() or something to notify integrators of this fact. Thierry
signature.asc
Description: PGP signature
