Hi Nuno,
> -----Original Message-----
> From: Nuno Sá <noname.nuno@xxxxxxxxx>
> Sent: Monday, June 24, 2024 12:12 PM
> To: Agarwal, Utsav <Utsav.Agarwal@xxxxxxxxxx>;
> dmitry.torokhov@xxxxxxxxx
> 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 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.
>
Thank you I understand now 😊, I will make the change accordingly.
> > >
> > > > +
> > > > + }
> > > >
> > > > 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 :)
>
Yes, something similar, essentially, I did not want to limit functionality anywhere I could. I will make the adjustments accordingly 😊.
> - Nuno Sá
>
Regards,
Utsav Agarwal
