Messages in this thread | | | Date | Wed, 26 Feb 2020 20:21:03 +0100 | From | Uwe Kleine-König <> | Subject | Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element |
| |
On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote: > On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote: > > Hello Matthias, > > > > as you seem to have this hardware on your desk, it would be great if > > you > > could answer the following questions: > > > > - Does the hardware complete the currently running period before > > applying a new setting? > > The datasheet claims: > > > Because the loading of the LEDn_ON and LEDn_OFF registers is via the > > I 2 C-bus, and > > asynchronous to the internal oscillator, we want to ensure that we do > > not see any visual > > artifacts of changing the ON and OFF values. This is achieved by > > updating the changes at > > the end of the LOW cycle. > > My interpretation is that the hardware will complete its period before > applying the new settings. I might check with a scope tomorrow-ish.
I agree given that you can update duty_cycle and period in a single write as you considered below. Maybe it is worth playing with small periods and a slow i2c bus speed (or hijack the bus by simulating a clock stretch).
> > - Is this racy somehow (i.e. can it happen that when going from > > duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 the > > output is 1000/10000 (or 4000/5000) for one cycle)? > > It currently is racy. It should be possible to fix that either by > updating all 4 registers in a single I2C write, or by using the "update > on ACK" mode which requires all 4 registers to be updated before the > new setting is applied (I'm not sure if this mode would require using a > single I2C write as well though).
I can offer a second pair of eyeballs to interpret the datasheet. Will take a look tomorrow.
> > - Does the hardware complete the currently running period before > > .enabled = false is configured? > > As my interpretation is that new settings are applied asynchronously, I > assume that the final running period is completed after .enabled is set > to false. > > > - How does the output pin behave on a disabled PWM. (Usual candidates > > are: freeze where is just happens to be, constant inactive and > > High-Z). > > Constant inactive. This is also the case when the chip is put into > sleep mode. Note that the interpretation of "inactive" depends in the > invert flag in the MODE2 register.
This is optimal.
> As it turns out, this driver is broken in yet another way I didn't find > before: For changing the global prescaler the chip needs to be put into > sleep mode, but the driver doesn't follow the restart sequence > described in the datasheet when waking it back up. In consequence, > changing the period of one PWM does not only modify the period of all > PWMs (which is bad enough, but can't be avoided with this hardware), > but it also leaves all PWMs disabled... > > As this hardware only has a single prescaler for all PWMs, should > changing the period for individual PWMs even be allowed at all? Maybe > only when all other PWMs are inactive?
yes, that is the general approach. Please document this in a Limitiations: paragraph. See drivers/pwm/pwm-imx-tpm.c which has a similar problem.
> I could imagine setting it in DTS instead (but I'm not sure what to do > about non-OF users of this driver, for example when configured via > ACPI).
I don't like fixing the period in the device tree. This isn't a hardware property and it is less flexible than possible.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
| |