Re: [PATCH] Input: adp5588-keys: Remove unused driver

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	"Hennerich, Michael" <Michael.Hennerich@analog.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Bogdan, Dragos" <Dragos.Bogdan@analog.com>,
	"Sa, Nuno" <Nuno.Sa@analog.com>, "Arnd Bergmann" <arnd@arndb.de>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"Nikolaus Voss" <nv@vosn.de>
Subject: Re: [PATCH] Input: adp5588-keys: Remove unused driver
Date: Fri, 03 Jun 2022 15:39:20 +0200	[thread overview]
Message-ID: <b66fda1f80c77e7c38cd1d557d39eb3263e40000.camel@gmail.com> (raw)
In-Reply-To: <294e3abdd3e8eb0423ea024cef4a437b1e09a65e.camel@gmail.com>

On Fri, 2022-06-03 at 15:34 +0200, Nuno Sá wrote:
> On Sat, 2022-05-28 at 22:20 -0700, Dmitry Torokhov wrote:
> > On Fri, May 06, 2022 at 11:57:57AM +0000, Hennerich, Michael wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Sent: Donnerstag, 5. Mai 2022 09:50
> > > > To: Hennerich, Michael <Michael.Hennerich@analog.com>
> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Bogdan, Dragos
> > > > <Dragos.Bogdan@analog.com>; Sa, Nuno <Nuno.Sa@analog.com>; Arnd
> > > > Bergmann <arnd@arndb.de>; kernel@pengutronix.de; linux-
> > > > input@vger.kernel.org
> > > > Subject: Re: [PATCH] Input: adp5588-keys: Remove unused driver
> > > > 
> > > > 
> > > > Hello Michael,
> > > > 
> > > > On Thu, May 05, 2022 at 06:20:22AM +0000, Hennerich, Michael
> > > > wrote:
> > > > > > -----Original Message-----
> > > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > Sent: Mittwoch, 4. Mai 2022 10:46
> > > > > > To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Hennerich,
> > > > > > Michael
> > > > > > <Michael.Hennerich@analog.com>
> > > > > > Cc: linux-input@vger.kernel.org; kernel@pengutronix.de;
> > > > > > Arnd
> > > > > > Bergmann <arnd@arndb.de>
> > > > > > Subject: [PATCH] Input: adp5588-keys: Remove unused driver
> > > > > > 
> > > > > > The last user is gone since 2018 (commit 4ba66a976072
> > > > > > ("arch:
> > > > > > remove
> > > > > > blackfin port")). This is an i2c driver, so it could be
> > > > > > used
> > > > > > on a
> > > > > > non-blackfin machine, but this driver requires platform
> > > > > > data,
> > > > > > so it
> > > > > > cannot be bound using device tree.
> > > > > 
> > > > > Hi Uwe,
> > > > > 
> > > > > If we start removing drivers which obviously don't have a
> > > > > mainline
> > > > > in-tree user, we would upset up many users of these drivers.
> > > > > I agree on updating this driver to make platform data
> > > > > optional.
> > > > > We could provide a patch in a few days.
> > > > 
> > > > Just to add some background why I stumbled over this driver: On
> > > > of my current
> > > > quests is to make i2c remove callbacks return void. As a
> > > > preparation for that I
> > > > work on updating all i2c drivers to return 0 in
> > > > .remove() to make the change to void have no side effects.
> > > > 
> > > > One of the offenders is drivers/gpio/gpio-adp5588.c, which in
> > > > the
> > > > presence of a
> > > > pdata->teardown callback might return a non-zero value from
> > > > .remove(). While
> > > > looking at the pdata of possible devices I only found
> > > > drivers/input/keyboard/adp5588-keys.c.
> > > > 
> > > > So the options for my quest are in increasing impact order:
> > > > 
> > > >  a) just warn if struct adp5588_gpio_platform_data::teardown
> > > > fails and
> > > >     still return 0 from .remove()
> > > >  b) make struct adp5588_gpio_platform_data::teardown return
> > > > void
> > > >  c) drop teardown support from adp5588_gpio_platform_data
> > > >  d) drop platform support from gpio-adp5588
> > > >  e) drop gpio-adp5588
> > > > 
> > > > Currently I'd go for at least d).
> > > > 
> > > > Having said that I think e) has a net benefit. If there is no
> > > > user left it reduces
> > > > maintainance burden. If there is a user left, they hopefully
> > > > will
> > > > tell us, we can
> > > > restore the driver from git history and then at least know a
> > > > tester for future
> > > > cleanups and changes.
> > > 
> > > Hi Uwe,
> > > 
> > > Thanks for the explanation.
> > > 
> > > I know that there are users of this driver. But I admit, we
> > > should
> > > have earlier
> > > made platform_data support optional and also add proper dt
> > > bindings.
> > > We're in progress doing so. And in the meanwhile, I would prefer
> > > a
> > > less
> > > disruptive intermediate change. For example c) with the promise
> > > we're working on d).
> > 
> > I am looking at the 2 drivers (adp5588-keys and gpio-adp5588) and I
> > think we need to add the missing functionality to adp5588-keys (and
> > make
> > keyboard part optional) and drop the gpio one.
> > 
> > Thanks.
> > 
> 
> Hi,
> 
> Just to note that I intend to start working on this next week so yes,
> this is not forgotten :).
> 
> I think the only functionality we are missing is the interrupt
> generator capability (irqchip) which needs to be handled more
> carefully
> in the keys driver because, there, we also have the capability of
> adding GPIs to the keys event. I can see that this patch [1] also
> linked the irq generation to the keys event and I don't really think
> this is the way it's supposed to work looking at the datasheet. For
> interrupts generation, we should be using GPIN irqs.
> 
> In the adp5588-keys driver we are already doing
> 'input_report_switch()'
> when we get an event for that key so I'm not sure if also "attaching"
> an interrupt to it is the way to go. The way I see it, the pin is
> either used as interrupt generator or key events...
> 
> +cc  Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
> 

Looks like it does not exist... Trying the other one in the patch:

+cc Nikolaus Voss <nv@vosn.de
> 