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
>
