Message-ID: <20200226192103.bodplhjson7drvgm@pengutronix.de>
Date:   Wed, 26 Feb 2020 20:21:03 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc:     thierry.reding@...il.com, linux-pwm@...r.kernel.org,
        linux-kernel@...r.kernel.org
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/ |

Powered by blists - more mailing lists