linux-kernel - RE: [PATCH] adp5588-keys: Support for dedicated gpio operation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 24 Jun 2024 13:19:49 +0000
From: "Agarwal, Utsav" <Utsav.Agarwal@...log.com>
To: Nuno Sá <noname.nuno@...il.com>,
        "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>
CC: "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Artamonovs,
 Arturs" <Arturs.Artamonovs@...log.com>,
        "Bimpikas, Vasileios"
	<Vasileios.Bimpikas@...log.com>
Subject: RE: [PATCH] adp5588-keys: Support for dedicated gpio operation

Hi Nuno,

> -----Original Message-----
> From: Nuno Sá <noname.nuno@...il.com>
> Sent: Monday, June 24, 2024 12:12 PM
> To: Agarwal, Utsav <Utsav.Agarwal@...log.com>;
> dmitry.torokhov@...il.com
> Cc: Hennerich, Michael <Michael.Hennerich@...log.com>; linux-
> input@...r.kernel.org; linux-kernel@...r.kernel.org; Artamonovs, Arturs
> <Arturs.Artamonovs@...log.com>; Bimpikas, Vasileios
> <Vasileios.Bimpikas@...log.com>
> 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@...il.com <dmitry.torokhov@...il.com>
> > > Sent: Saturday, June 22, 2024 9:21 AM
> > > To: Agarwal, Utsav <Utsav.Agarwal@...log.com>
> > > Cc: Hennerich, Michael <Michael.Hennerich@...log.com>; linux-
> > > input@...r.kernel.org; linux-kernel@...r.kernel.org; Artamonovs, Arturs
> > > <Arturs.Artamonovs@...log.com>; Bimpikas, Vasileios
> > > <Vasileios.Bimpikas@...log.com>
> > > 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@...log.com>
> > > >
> > > > 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@...log.com>
> > > > ---
> > > >  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