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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200330130724.GA2431644@ulmo>
Date:   Mon, 30 Mar 2020 15:07:24 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: (EXT) Re: [PATCH 3/4] pwm: pca9685: initialize all LED registers
 during probe

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@...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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)