Hi Dmitry,
> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Sent: Monday, August 26, 2024 8:18 PM
> To: Agarwal, Utsav <Utsav.Agarwal@xxxxxxxxxx>
> Cc: Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; linux-
> input@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Artamonovs, Arturs
> <Arturs.Artamonovs@xxxxxxxxxx>; Bimpikas, Vasileios
> <Vasileios.Bimpikas@xxxxxxxxxx>; Gaskell, Oliver
> <Oliver.Gaskell@xxxxxxxxxx>
> Subject: Re: [PATCH RESEND v11 2/3] Input: adp5588-keys - add support for
> pure gpio
>
> [External]
>
> Hi Utsav,
>
> On Mon, Aug 26, 2024 at 06:22:02PM +0100, Utsav Agarwal via B4 Relay
> wrote:
> > From: Utsav Agarwal <utsav.agarwal@xxxxxxxxxx>
> >
> > Keypad specific setup is relaxed if no keypad rows/columns are specified,
> > enabling a purely gpio operation.
> >
> > Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > Signed-off-by: Utsav Agarwal <utsav.agarwal@xxxxxxxxxx>
> > ---
> > drivers/input/keyboard/adp5588-keys.c | 37
> +++++++++++++++++++++++++++++++----
> > 1 file changed, 33 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> > index 09bcfc6b9408..7c32f8b69a3e 100644
> > --- a/drivers/input/keyboard/adp5588-keys.c
> > +++ b/drivers/input/keyboard/adp5588-keys.c
> > @@ -188,6 +188,7 @@ struct adp5588_kpad {
> > u32 cols;
> > u32 unlock_keys[2];
> > int nkeys_unlock;
> > + bool gpio_only;
> > unsigned short keycode[ADP5588_KEYMAPSIZE];
> > unsigned char gpiomap[ADP5588_MAXGPIO];
> > struct gpio_chip gc;
> > @@ -431,10 +432,12 @@ static int adp5588_gpio_add(struct adp5588_kpad
> *kpad)
> > kpad->gc.label = kpad->client->name;
> > kpad->gc.owner = THIS_MODULE;
> >
> > - girq = &kpad->gc.irq;
> > - gpio_irq_chip_set_chip(girq, &adp5588_irq_chip);
> > - girq->handler = handle_bad_irq;
> > - girq->threaded = true;
> > + if (kpad->client->irq) {
> > + girq = &kpad->gc.irq;
> > + gpio_irq_chip_set_chip(girq, &adp5588_irq_chip);
> > + girq->handler = handle_bad_irq;
> > + girq->threaded = true;
> > + }
>
> I think we should only set up irqchip if we have "interrupt-controller"
> property in the device tree.
>
> >
> > mutex_init(&kpad->gpio_lock);
> >
> > @@ -632,6 +635,21 @@ static int adp5588_fw_parse(struct adp5588_kpad
> *kpad)
> > struct i2c_client *client = kpad->client;
> > int ret, i;
> >
> > + /*
> > + * Check if the device is to be operated purely in GPIO mode. To do
> > + * so, check that no keypad rows or columns have been specified,
> > + * since all GPINS should be configured as GPIO.
> > + */
> > + ret = device_property_present(&client->dev,
> > + "keypad,num-rows");
> > + ret |= device_property_present(&client->dev,
> > + "keypad,num-columns");
> > + /* If purely GPIO, skip keypad setup */
> > + if (!ret) {
> > + kpad->gpio_only = true;
> > + return 0;
> > + }
> > +
> > ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
> > &kpad->cols);
> > if (ret)
> > @@ -775,6 +793,11 @@ static int adp5588_probe(struct i2c_client *client)
> > if (error)
> > return error;
> >
> > + if (kpad->gpio_only && !client->irq) {
> > + dev_info(&client->dev, "Rev.%d, started as GPIO only\n",
> revid);
> > + return 0;
> > + }
>
> This is way too noisy. I think one message logging the revision ID
> should be enough. The rest of the data can be found from elsewhere if
> needed.
>
> Can you try the below on top of yours? If this works I'' squash it
> together with your change.
I have applied and tested the same on relevant hardware, it works as
intended.
> Thanks.
>
> --
> Dmitry
>
- Utsav
>
> diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> index 7c32f8b69a3e..b5f4becf5cb6 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -432,7 +432,12 @@ static int adp5588_gpio_add(struct adp5588_kpad
> *kpad)
> kpad->gc.label = kpad->client->name;
> kpad->gc.owner = THIS_MODULE;
>
> - if (kpad->client->irq) {
> + if (device_property_present(dev, "interrupt-controller")) {
> + if (!kpad->client->irq) {
> + dev_err(dev, "Unable to serve as interrupt controller
> without interrupt");
> + return -EINVAL;
> + }
> +
> girq = &kpad->gc.irq;
> gpio_irq_chip_set_chip(girq, &adp5588_irq_chip);
> girq->handler = handle_bad_irq;
> @@ -640,12 +645,9 @@ static int adp5588_fw_parse(struct adp5588_kpad
> *kpad)
> * so, check that no keypad rows or columns have been specified,
> * since all GPINS should be configured as GPIO.
> */
> - ret = device_property_present(&client->dev,
> - "keypad,num-rows");
> - ret |= device_property_present(&client->dev,
> - "keypad,num-columns");
> - /* If purely GPIO, skip keypad setup */
> - if (!ret) {
> + if (!device_property_present(&client->dev, "keypad,num-rows") &&
> + !device_property_present(&client->dev, "keypad,num-columns"))
> {
> + /* If purely GPIO, skip keypad setup */
> kpad->gpio_only = true;
> return 0;
> }
> @@ -793,28 +795,19 @@ static int adp5588_probe(struct i2c_client *client)
> if (error)
> return error;
>
> - if (kpad->gpio_only && !client->irq) {
> - dev_info(&client->dev, "Rev.%d, started as GPIO only\n",
> revid);
> - return 0;
> - }
> -
> - error = devm_request_threaded_irq(&client->dev, client->irq,
> - adp5588_hard_irq,
> adp5588_thread_irq,
> - IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> - client->dev.driver->name, kpad);
> - if (error) {
> - dev_err(&client->dev, "failed to request irq %d: %d\n",
> - client->irq, error);
> - return error;
> - }
> -
> - if (kpad->gpio_only) {
> - dev_info(&client->dev, "Rev.%d GPIO only, irq %d\n",
> - revid, client->irq);
> - return 0;
> + if (client->irq) {
> + error = devm_request_threaded_irq(&client->dev, client-
> >irq,
> + adp5588_hard_irq,
> adp5588_thread_irq,
> + IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> + client->dev.driver->name,
> kpad);
> + if (error) {
> + dev_err(&client->dev, "failed to request irq %d:
> %d\n",
> + client->irq, error);
> + return error;
> + }
> }
>
> - dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client-
> >irq);
> + dev_info(&client->dev, "Rev.%d controller\n", revid);
> return 0;
> }
>
