Re: (EXT) Re: [PATCH 3/4] pwm: pca9685: initialize all LED registers during probe

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3315 bytes --]

On Wed, Feb 26, 2020 at 05:13:51PM +0100, Matthias Schiffer wrote:
> On Wed, 2020-02-26 at 16:00 +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 26, 2020 at 02:52:28PM +0100, Matthias Schiffer wrote:
> > > Initialize all ON delays to 0 during probe, rather than doing it in
> > > pca9685_pwm_enable.
> > > 
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > > >
> > > ---
> > >  drivers/pwm/pwm-pca9685.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 393ab92aa945..370691b21107 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -289,13 +289,6 @@ static int pca9685_pwm_enable(struct pwm_chip
> > > *chip, struct pwm_device *pwm)
> > >  {
> > >  	struct pca9685 *pca = to_pca(chip);
> > >  
> > > -	/*
> > > -	 * The PWM subsystem does not support a pre-delay.
> > > -	 * So, set the ON-timeout to 0
> > > -	 */
> > > -	regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm), 0);
> > > -	regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), 0);
> > > -
> > >  	/*
> > >  	 * Clear the full-off bit.
> > >  	 * It has precedence over the others and must be off.
> > > @@ -388,6 +381,13 @@ static int pca9685_pwm_probe(struct i2c_client
> > > *client,
> > >  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
> > >  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
> > >  
> > > +	/*
> > > +	 * The PWM subsystem does not support a pre-delay.
> > > +	 * So, set the ON-timeout to 0
> > > +	 */
> > > +	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> > > +	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > > +
> > 
> > What is a pre-delay: Something like:
> >           _________                   ______
> >     _____/         \_________________/
> >     ^                           ^
> > 
> > Where ^ marks the period start and then the time between period start
> > and the rising signal is the pre-delay?
> > 
> > If so, the IMHO more right approach is to keep the pre-delay until a
> > new
> > setting is applied and in .get_state ignore the pre-delay. This way
> > you
> > don't modify the output in .probe() which sounds right.
> > 
> 
> My approach was to get the hardware into a known state by resetting
> most registers (which is what the driver attempted to do so far). If
> getting the hardware state via get_state is preferable, I can implement
> that instead.

I think what Uwe was suggesting was to delay setting these registers
until the PWM is actually used. We typically do that in order not to
mess with whatever the bootloader may have set up.

Consider for example the case where the PWM is used to drive a
backlight, which might be showing a splash screen. If you reinitialize
the PWM at probe time, the backlight might go dark some time before the
kernel gets around to initializing the backlight.

For now I think we should just leave this where it is because likely a
change in PWM will be followed by a pwm_enable() call with the legacy
API. Ideally this driver would be converted to the new atomic PWM API,
in which case it'd be best to write the entirety of the hardware
registers during ->apply().

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]