On Fri, Jun 03, 2022 at 03:39:20PM +0200, Nuno Sá wrote:
> 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@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>
> >
>
> Looks like it does not exist... Trying the other one in the patch:
>
> +cc Nikolaus Voss <nv@xxxxxxx
I'd like remove special case of reporting switches from adp5588-keys.c
and instead implement enough of irqchip to allow gpio-keys to attach to
those pins and report switches from there.
Thanks.
--
Dmitry
