Re: [PATCH RESEND v11 2/3] Input: adp5588-keys - add support for pure gpio

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Utsav,

On Mon, Aug 26, 2024 at 06:22:02PM +0100, Utsav Agarwal via B4 Relay wrote:
> From: Utsav Agarwal <utsav.agarwal@analog.com>
> 
> 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 <utsav.agarwal@analog.com>
> ---
>  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.

Thanks.

-- 
Dmitry


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