On Thu, Jan 14, 2021 at 10:28:02PM +0000, Russell King - ARM Linux admin wrote: > On Thu, Jan 14, 2021 at 09:25:45PM +0100, Uwe Kleine-König wrote: > > Hello Baruch, > > > > On Thu, Jan 14, 2021 at 08:57:37PM +0200, Baruch Siach wrote: > > > Add a comment on why the code never sets on/off registers to zero. > > > > > > Reported-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > > > Analyzed-by: Russell King <linux@xxxxxxxxxxxxxxx> > > > Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx> > > > --- > > > drivers/gpio/gpio-mvebu.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > > index 6b017854ce61..09780944bef9 100644 > > > --- a/drivers/gpio/gpio-mvebu.c > > > +++ b/drivers/gpio/gpio-mvebu.c > > > @@ -706,6 +706,10 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > do_div(val, NSEC_PER_SEC); > > > if (val > UINT_MAX) > > > return -EINVAL; > > > + /* > > > + * Zero on/off values don't work as expected. Experimentation shows > > > + * that zero value is treated as 2^32. This behavior is not documented. > > > + */ > > > > This is too easy. The right thing to do is to adapt .apply and > > .get_state to use this new information. > > What exactly do you expect the changes to be? What I expect is: - let .apply() write 0 if the intention is to configure 2^32 clock steps for the on or off register; and symmetrically - let .get_state report 2^32 * NSEC_PER_SEC / clk_rate if the register value is 0. > Bear in mind that the hardware is not capable of atomically updating > e.g. the duty cycle without affecting the period, because any change > in duty cycle needs the "on" and "off" durations to be separately > programmed, and there's a chance that the hardware could start using > either value mid-update. > > Also, disabling "blink" mode to achieve a steady output (for 0% or 100% > duty cycle) would require further investigation to find out how the > hardware behaves at the moment where blink mode is disabled: does the > output retain its current state (does the bit in the output register > toggle with the blink) or does it revert to the value in the output > register that was programmed before blink mode was enabled. I have some plans here about what is the right behaviour, but this needs some preparatory work that I didn't do yet. I'll come back to this eventually. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
