Hi Dmitry,
Thank you for your reply.
> -----Original Message-----
> From: dmitry.torokhov@xxxxxxxxx <dmitry.torokhov@xxxxxxxxx>
> Sent: Saturday, June 22, 2024 9:21 AM
> To: Agarwal, Utsav <Utsav.Agarwal@xxxxxxxxxx>
> Cc: Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; linux-
> input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Artamonovs, Arturs
> <Arturs.Artamonovs@xxxxxxxxxx>; Bimpikas, Vasileios
> <Vasileios.Bimpikas@xxxxxxxxxx>
> Subject: Re: [PATCH] adp5588-keys: Support for dedicated gpio operation
>
> [External]
>
> Hi Utsav,
>
> On Fri, Jun 21, 2024 at 10:44:12AM +0000, Agarwal, Utsav wrote:
> > From: UtsavAgarwalADI <utsav.agarwal@xxxxxxxxxx>
> >
> > We have a SoC which uses ADP5587 exclusively as an I2C GPIO expander.
> > The current state of the driver for the ADP5588/87 only allows partial
> > I/O to be used as GPIO. This support was present before as a separate
> > gpio driver, which was dropped with the commit
> > 5ddc896088b0 ("gpio: gpio-adp5588: drop the driver") since the
> > functionality was deemed to have been merged with adp5588-keys.
> >
> > To restore this functionality, the "gpio-only" property allows
> > indicating if the device is to be used for GPIO only.
> > When specified, the driver skips relevant input device checks/parsing
> > and allows all GPINS to be registered as GPIO.
> >
> > Signed-off-by: Utsav Agarwal <utsav.agarwal@xxxxxxxxxx>
> > ---
> > drivers/input/keyboard/adp5588-keys.c | 30
> > ++++++++++++++++++++-------
> > 1 file changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/adp5588-keys.c
> > b/drivers/input/keyboard/adp5588-keys.c
> > index 1b0279393df4..78770b2dfe1b 100644
> > --- a/drivers/input/keyboard/adp5588-keys.c
> > +++ b/drivers/input/keyboard/adp5588-keys.c
> > @@ -719,7 +719,7 @@ static int adp5588_probe(struct i2c_client *client)
> > struct input_dev *input;
> > struct gpio_desc *gpio;
> > unsigned int revid;
> > - int ret;
> > + int ret, gpio_mode_only;
> > int error;
> >
> > if (!i2c_check_functionality(client->adapter,
> > @@ -739,13 +739,17 @@ static int adp5588_probe(struct i2c_client
> *client)
> > kpad->client = client;
> > kpad->input = input;
> >
> > - error = adp5588_fw_parse(kpad);
> > - if (error)
> > - return error;
> > + gpio_mode_only = device_property_present(&client->dev, "gpio-
> only");
>
> Do we really need a new property? Can we simply allow omitting
> keypad,num-rows/cols properties in case where we only want to have GPIO
> functionality?
>
> In any case this requires DT binding update.
I agree that we may not require another property however there are the following two options to accomplish the same:
- The probe function utilizes a function called matrix_keypad_parse_properties(), which parses the rows and columns specified - which I would have to read as well in order to identify if all GPINS should be configured as GPIO. This would however mean that in case this is not a GPIO only mode, we would have redundant code execution.
- If we avoid that and just use the return value from the function, it will print out a dev_err message :"number of keypad rows/columns not specified " message.
How would you suggest I should proceed? I will add the DT bindings in the v2 patch.
>
> > + if (!gpio_mode_only) {
> > + error = adp5588_fw_parse(kpad);
> > + if (error)
> > + return error;
> >
> > - error = devm_regulator_get_enable(&client->dev, "vcc");
> > - if (error)
> > - return error;
> > + error = devm_regulator_get_enable(&client->dev, "vcc");
> > + if (error)
> > + return error;
>
> Why regulator is not needed for the pure GPIO mode? Please add a
> comment.
We don't specify a regulator for our kernel and the driver seems to work without it. I agree however that it may not be mutually exclusive to the same, I will be fixing the same in the v2 patch.
>
> > +
> > + }
> >
> > gpio = devm_gpiod_get_optional(&client->dev, "reset",
> GPIOD_OUT_HIGH);
> > if (IS_ERR(gpio))
> > @@ -790,6 +794,11 @@ static int adp5588_probe(struct i2c_client *client)
> > if (error)
> > return error;
> >
> > + if (!client->irq && gpio_mode_only) {
> > + dev_info(&client->dev, "Rev.%d, started as GPIO only\n",
> revid);
> > + return 0;
> > + }
> > +
>
> What is the reason for requesting interrupt in pure GPIO mode? Can we
> program the controller to not raise attention in this case?
I do not think irq is needed in case of GPIO mode since we do not use the same either. This was an attempt to preserve the driver functionality as it could be easily excluded if not specified.
>
> > error = devm_request_threaded_irq(&client->dev, client->irq,
> > adp5588_hard_irq,
> adp5588_thread_irq,
> > IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT, @@ -800,6 +809,13 @@
> > static int adp5588_probe(struct i2c_client *client)
> > return error;
> > }
> >
> > +
> > + if (gpio_mode_only) {
> > + dev_info(&client->dev, "Rev.%d irq %d, started as GPIO
> only\n",
> > + revid, client->irq);
> > + return 0;
> > + }
> > +
> > dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
> > return 0;
> > }
> > --
> > 2.34.1
>
> Thanks.
>
> --
> Dmitry
Regards,
Utsav Agarwal
