Hi Dmitry,

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Sent: Monday, August 26, 2024 8:18 PM
To: Agarwal, Utsav <redacted>
Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; Rob Herring
[off-list ref]; Krzysztof Kozlowski [off-list ref]; Conor
Dooley [off-list ref]; Sa, Nuno [off-list ref]; linux-
input@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; Artamonovs, Arturs
[off-list ref]; Bimpikas, Vasileios
[off-list ref]; Gaskell, Oliver
[off-list ref]
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:

quoted

From: Utsav Agarwal <redacted>

Keypad specific setup is relaxed if no keypad rows/columns are specified,
enabling a purely gpio operation.

Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Utsav Agarwal <redacted>
---
 drivers/input/keyboard/adp5588-keys.c | 37

+++++++++++++++++++++++++++++++----

quoted

 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c

b/drivers/input/keyboard/adp5588-keys.c

quoted

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)

quoted

 	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.

quoted

 	mutex_init(&kpad->gpio_lock);

@@ -632,6 +635,21 @@ static int adp5588_fw_parse(struct adp5588_kpad

*kpad)

quoted

 	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);

quoted

+		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