diff mbox series

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

Message ID 20240704-adp5588_gpio_support-v8-2-208cf5d4c2d6@analog.com
State Superseded
Headers show
Series adp5588-keys: Support for dedicated gpio operation | expand

Commit Message

Utsav Agarwal via B4 Relay July 4, 2024, 2:26 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.

Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 37 +++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

Comments

Agarwal, Utsav July 22, 2024, 3:27 p.m. UTC | #1 
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Tuesday, July 9, 2024 7:37 AM
> To: Agarwal, Utsav <Utsav.Agarwal@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Sa,
> Nuno <Nuno.Sa@analog.com>
> Cc: 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 v8 2/3] Input: adp5588-keys - add support for pure gpio
> 
> [External]
> 
> On Thu, 2024-07-04 at 15:26 +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.
> >
> > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com>
> > ---
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
Hi,

Since there have not been feedback comments on the same for some time, I wanted to check if any further changes/actions are required for it to be accepted.

Thanks,
Utsav
Agarwal, Utsav July 23, 2024, 8:26 a.m. UTC | #2 
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Monday, July 22, 2024 4:47 PM
> To: Agarwal, Utsav <Utsav.Agarwal@analog.com>; Nuno Sá
> <noname.nuno@gmail.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Sa,
> Nuno <Nuno.Sa@analog.com>
> Cc: 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 v8 2/3] Input: adp5588-keys - add support for pure gpio
> 
> [External]
> 
> On 22/07/2024 17:27, Agarwal, Utsav wrote:
> >> From: Nuno Sá <noname.nuno@gmail.com>
> >> Sent: Tuesday, July 9, 2024 7:37 AM
> >> To: Agarwal, Utsav <Utsav.Agarwal@analog.com>; Hennerich, Michael
> >> <Michael.Hennerich@analog.com>; Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com>; Rob Herring <robh@kernel.org>;
> Krzysztof
> >> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> Sa,
> >> Nuno <Nuno.Sa@analog.com>
> >> Cc: 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 v8 2/3] Input: adp5588-keys - add support for pure
> gpio
> >>
> >> [External]
> >>
> >> On Thu, 2024-07-04 at 15:26 +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.
> >>>
> >>> Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com>
> >>> ---
> >>
> >> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> >>
> > Hi,
> >
> > Since there have not been feedback comments on the same for some time,
> I wanted to check if any further changes/actions are required for it to be
> accepted.
> 
> Please observe merge window timeline.
> 
> Best regards,
> Krzysztof

I understand, thank you for pointing me to the same.

Thanks,
Utsav
Dmitry Torokhov Aug. 3, 2024, 12:37 a.m. UTC | #3 
Hi Utsav,

On Thu, Jul 04, 2024 at 03:26:31PM +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.
> 
> 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;
> +	}
>  
>  	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;

I think we need more elaborate handling here (and probably more
elaborate binding yaml file): now that you are making interrupt optional
you should check if interrupt-controller functionality of the GPIO
block/gpiochip is requested. If it was, then we should not allow missing
interrupt. If only GPIO controller is needed, without interrupt
capabilities, tnen running without interrupt is fine.

Thanks.
Nuno Sá Aug. 5, 2024, 6:43 a.m. UTC | #4 
On Fri, 2024-08-02 at 17:37 -0700, Dmitry Torokhov wrote:
> Hi Utsav,
> 
> On Thu, Jul 04, 2024 at 03:26:31PM +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.
> > 
> > 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;
> > +	}
> >  
> >  	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;
> 
> I think we need more elaborate handling here (and probably more
> elaborate binding yaml file): now that you are making interrupt optional
> you should check if interrupt-controller functionality of the GPIO
> block/gpiochip is requested. If it was, then we should not allow missing
> interrupt. If only GPIO controller is needed, without interrupt
> capabilities, tnen running without interrupt is fine.
> 
Hi Dmitry,

I need to double check but I don't think we can act as an interrupt-controller
without the interrupt line connected. So, I think the only thing we could likely
add/improve is to express that dependency in the bindings.

- Nuno Sá
Agarwal, Utsav Aug. 5, 2024, 6:36 p.m. UTC | #5 
> -----Original Message-----
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Monday, August 5, 2024 7:44 AM
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; 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 v8 2/3] Input: adp5588-keys - add support for pure gpio
> 
> [External]
> 
> On Fri, 2024-08-02 at 17:37 -0700, Dmitry Torokhov wrote:
> > Hi Utsav,
> >
> > On Thu, Jul 04, 2024 at 03:26:31PM +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.
> > >
> > > 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;
> > > +	}
> > >
> > >  	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;
> >
> > I think we need more elaborate handling here (and probably more
> > elaborate binding yaml file): now that you are making interrupt optional
> > you should check if interrupt-controller functionality of the GPIO
> > block/gpiochip is requested. If it was, then we should not allow missing
> > interrupt. If only GPIO controller is needed, without interrupt
> > capabilities, tnen running without interrupt is fine.
> >
> Hi Dmitry,
> 
> I need to double check but I don't think we can act as an interrupt-controller
> without the interrupt line connected. So, I think the only thing we could likely
> add/improve is to express that dependency in the bindings.
> 
> - Nuno Sá

Hi Dmitry,

Thank you for pointing out the dependency, I will add the same in the bindings.

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