| Message ID | SJ0PR03MB6343CB033C1005A36B102BF89BC92@SJ0PR03MB6343.namprd03.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | adp5588-keys: Support for dedicated gpio operation | expand |
Hi Utsav, On Fri, Jun 21, 2024 at 10:44:12AM +0000, Agarwal, Utsav wrote: > From: UtsavAgarwalADI <utsav.agarwal@analog.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@analog.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. > + 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. > + > + } > > 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? > 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.
Hi Dmitry, Thank you for your reply. > -----Original Message----- > From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > Sent: Saturday, June 22, 2024 9:21 AM > To: Agarwal, Utsav <Utsav.Agarwal@analog.com> > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; linux- > input@vger.kernel.org; linux-kernel@vger.kernel.org; Artamonovs, Arturs > <Arturs.Artamonovs@analog.com>; Bimpikas, Vasileios > <Vasileios.Bimpikas@analog.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@analog.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@analog.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. > > > + > > + } > > > > 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
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@gmail.com <dmitry.torokhov@gmail.com> > > Sent: Saturday, June 22, 2024 9:21 AM > > To: Agarwal, Utsav <Utsav.Agarwal@analog.com> > > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; linux- > > input@vger.kernel.org; linux-kernel@vger.kernel.org; Artamonovs, Arturs > > <Arturs.Artamonovs@analog.com>; Bimpikas, Vasileios > > <Vasileios.Bimpikas@analog.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@analog.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@analog.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. > > > > > + > > > + } > > > > > > 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á
Hi Nuno, > -----Original Message----- > From: Nuno Sá <noname.nuno@gmail.com> > Sent: Monday, June 24, 2024 12:12 PM > To: Agarwal, Utsav <Utsav.Agarwal@analog.com>; > dmitry.torokhov@gmail.com > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; linux- > input@vger.kernel.org; linux-kernel@vger.kernel.org; Artamonovs, Arturs > <Arturs.Artamonovs@analog.com>; Bimpikas, Vasileios > <Vasileios.Bimpikas@analog.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@gmail.com <dmitry.torokhov@gmail.com> > > > Sent: Saturday, June 22, 2024 9:21 AM > > > To: Agarwal, Utsav <Utsav.Agarwal@analog.com> > > > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; linux- > > > input@vger.kernel.org; linux-kernel@vger.kernel.org; Artamonovs, Arturs > > > <Arturs.Artamonovs@analog.com>; Bimpikas, Vasileios > > > <Vasileios.Bimpikas@analog.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@analog.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@analog.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
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"); + 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; + + } 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; + } + 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; }