Message-ID: <20200401174540.GA2978178@ulmo>
Date:   Wed, 1 Apr 2020 19:45:40 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sven Van Asbroeck <TheSven73@...il.com>
Subject: Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

On Wed, Apr 01, 2020 at 06:36:40PM +0200, Clemens Gruber wrote:
> On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote:
> > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote:
> > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@...il.com> wrote:
> > > >
> > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote:
> > > > > duty_cycle was only set, never read.
> > > > >
> > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@...tq-group.com>
> > > > > ---
> > > > >  drivers/pwm/pwm-pca9685.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > >
> > > > Applied, thanks.
> > > 
> > > I'm not sure this patch is correct.
> > 
> > What makes you say that? If you look at the code, the driver sets this
> > field to either 0 or some duty cycle value but ends up never using it.
> > Why would it be wrong to remove that code?
> > 
> > > We already have broken GPIO in this driver. Do we need more breakage?
> > 
> > My understanding is that nobody was able to pinpoint exactly when this
> > regressed, or if this only worked by accident to begin with. It sounds
> > like Clemens has a way of testing this driver, so perhaps we can solve
> > that GPIO issue while we're at it.
> > 
> > The last discussion on this seems to have been around the time when you
> > posted a fix for it:
> > 
> >     https://patchwork.ozlabs.org/patch/1156012/
> > 
> > But then Sven had concerns that that also wasn't guaranteed to work:
> > 
> >     https://lkml.org/lkml/2019/6/2/73
> > 
> > So I think we could either apply your patch to restore the old behaviour
> > which I assume you tested, so at least it seems to work in practice,
> > even if there's still a potential race that Sven pointed out in the
> > above link.
> > 
> > I'd prefer something alternative because it's obviously confusing and
> > completely undocumented. Mika had already proposed something that's a
> > little bit better, though still somewhat confusing.
> > 
> > Oh... actually reading further through those threads there seems to be a
> > patch from Sven that was reviewed by Mika but then nothing happened:
> > 
> > 	https://lkml.org/lkml/2019/6/4/1039
> > 
> > with the corresponding patchwork URL:
> > 
> > 	https://patchwork.ozlabs.org/patch/1110083/
> > 
> > Andy, Clemens, do you have a way of testing the GPIO functionality of
> > this driver? If so, it'd be great if you could check the above patch
> > from Sven to fix PWM/GPIO interop.
> 
> Looks good. Tested it today and I can no longer reproduce the issues
> when switching between PWM and GPIO modes.
> It did not apply cleanly on the current mainline or for-next branch, so
> I'll send a fixed up version of the patch with my Tested-by tag shortly.

Awesome, thank you!

> I noticed an unrelated issue when disabling and enabling the channel
> though, for which I will either send a patch or maybe try to convert the
> driver to the atomic API first and then look if it is still a problem.
> (Issue is that if you disable the channel, the LED_OFF counter is
> cleared, which means you have to reconfigure the duty cycle after
> reenabling. It's probably better if only the FULL_OFF bit is toggled in
> enable/disable as it has precedence over the others anyway and then the
> previous state would not be changed..?)

Converting to the atomic API would certainly be beneficial because it
gives you much more control over what you want to program to hardware.

Other than that, yes, if ->enable() called after ->disable() doesn't put
the PWM into the same state that it was before ->disable(), then that'd
be a bug. From what you're saying this will make the driver only work if
there's a ->config() call after ->disable(). That's wrong.

So yes, setting only that LED_FULL bit in ->disable() and clearing it in
->enable() sounds like a better option than the current implementation.

Thierry

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

Powered by blists - more mailing lists