diff mbox series

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

Message ID 20240826-adp5588_gpio_support-v11-2-3e5ac2bd31b7@analog.com (mailing list archive)
State Mainlined
Commit dc748812fca013c00d9b657e0cc1416fd71a1b06
Headers show
Series adp5588-keys: Support for dedicated gpio operation | expand

Commit Message

Utsav Agarwal via B4 Relay Aug. 26, 2024, 5:22 p.m. UTC 
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(-)

Comments

Dmitry Torokhov Aug. 26, 2024, 7:17 p.m. UTC | #1 
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.
Agarwal, Utsav Aug. 27, 2024, 10:03 a.m. UTC | #2 
Hi Dmitry,

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

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;
>  }
>
diff mbox series

Patch

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;
+	}
 
 	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;
+	}
+
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  adp5588_hard_irq, adp5588_thread_irq,
 					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
@@ -785,6 +808,12 @@  static int adp5588_probe(struct i2c_client *client)
 		return error;
 	}
 
+	if (kpad->gpio_only) {
+		dev_info(&client->dev, "Rev.%d GPIO only, irq %d\n",
+				revid, client->irq);
+		return 0;
+	}
+
 	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
 	return 0;
 }