Re: [PATCH] adp5588-keys: Support for dedicated gpio operation

[Nuno Sá]

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á