| Message ID | 20240704-adp5588_gpio_support-v8-2-208cf5d4c2d6@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adp5588-keys: Support for dedicated gpio operation | expand |
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>
> 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
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
> 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
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.
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á
> -----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 --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; }