Hi Utsav,
On Mon, 2024-06-24 at 10:43 +0000, Agarwal, Utsav wrote:
> 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.
>
What you need to ask yourself is... can the part work without a power supply :)? Note
that if you don't specify a regulator in DT and call devm_regulator_get_enable(), you
don't get an error. Just a dummy regulator.
> >
> > > +
> > > + }
> > >
> > > 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?
>
Wouldn't make sense to still allow it in a more relaxed way? I mean, even in "pure"
gpio mode, we could still want to use gpio-keys for example, right? IMO, I think we
should make the IRQ optional got gpio mode and configure the gpiochip accordingly.
Maybe this was exactly what you meant but I wasn't sure about it :)
- Nuno Sá
