[PATCH v3 5/5] gpio: mvebu: document zero pwm duty cycle limitation

[Uwe Kleine-König]

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 at pengutronix.de>
> > > Analyzed-by: Russell King <linux at armlinux.org.uk>
> > > Signed-off-by: Baruch Siach <baruch at tkos.co.il>
> > > ---
> > >  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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20210115/8cae1196/attachment.sig>