Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element

From: Thierry Reding <thierry.reding@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>,
	Clemens Gruber <clemens.gruber@pqgruber.com>
Cc: "Matthias Schiffer" <matthias.schiffer@ew.tq-group.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	linux-pwm@vger.kernel.org,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/4] pwm: pca9685: remove unused duty_cycle struct element
Date: Mon, 30 Mar 2020 18:02:38 +0200	[thread overview]
Message-ID: <20200330160238.GD2817345@ulmo> (raw)
In-Reply-To: <CAHp75Vc_=czuRtyqgnmnYfie50gDnzrNqq3Bt+Gp_42MikX6VA@mail.gmail.com>

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

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

Thierry

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