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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx>
> > > Sent: Donnerstag, 5. Mai 2022 09:50
> > > To: Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>
> > > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>; Bogdan, Dragos
> > > <Dragos.Bogdan@xxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Arnd
> > > Bergmann <arnd@xxxxxxxx>; kernel@xxxxxxxxxxxxxx; linux-
> > > input@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxx>
> > > > > Sent: Mittwoch, 4. Mai 2022 10:46
> > > > > To: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>; Hennerich,
> > > > > Michael
> > > > > <Michael.Hennerich@xxxxxxxxxx>
> > > > > Cc: linux-input@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; Arnd
> > > > > Bergmann <arnd@xxxxxxxx>
> > > > > 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@xxxxxxxxxxxxxxxxxxxxx>

[1]:
https://lore.kernel.org/all/3230ace6f820fccadb51097ae9ba9f5ee247d79b.1549379326.git.nikolaus.voss@xxxxxxxxxxxxxxxxxxxxx/

- Nuno Sá




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux