[2/3] Input: gpio_keys - always try fwnode first

Message ID 20180906075902.31240-3-linus.walleij@linaro.org
State New
Headers show
Series
  • [1/3] gpio: fwnode: Obtains descs from machine tables
Related show

Commit Message

Linus Walleij Sept. 6, 2018, 7:59 a.m.
This makes the gpio_keys input driver try fwnode first when
looking up GPIO descriptors, even if no fwnode was passed.

With the changes to the gpiolib, if NULL us passed as
fwnode, the gpiolib will attempt to look up the GPIO
descriptor from the machine descriptor tables.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
Dmitry: I'm looking for your ACK if you agree with this
approach overall so I can apply this with the changes to
gpiolib to the GPIO tree.
---
 drivers/input/keyboard/gpio_keys.c | 47 ++++++++++++++++++------------
 1 file changed, 28 insertions(+), 19 deletions(-)

-- 
2.17.1

Comments

Dmitry Torokhov Sept. 17, 2018, 6:15 p.m. | #1
On Thu, Sep 06, 2018 at 09:59:01AM +0200, Linus Walleij wrote:
> This makes the gpio_keys input driver try fwnode first when

> looking up GPIO descriptors, even if no fwnode was passed.

> 

> With the changes to the gpiolib, if NULL us passed as

> fwnode, the gpiolib will attempt to look up the GPIO

> descriptor from the machine descriptor tables.

> 

> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> Cc: Andy Shevchenko <andy@infradead.org>

> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> Dmitry: I'm looking for your ACK if you agree with this

> approach overall so I can apply this with the changes to

> gpiolib to the GPIO tree.


Linus, sorry for the delay. The issue I see here is that gpio-keys still
needs platform data to deal with per-button config. I will be sending a
patch set in a minute that introduces notion of "children" to legacy
device properties (expressed via property_entry/property_set) and wiring
up gpiolib lookup tables to work with them. Hopefully you and Rafael
would be OK with it, and then we could do proper cleanup of gpio-keys
and similar drivers.

Thanks!

> ---

>  drivers/input/keyboard/gpio_keys.c | 47 ++++++++++++++++++------------

>  1 file changed, 28 insertions(+), 19 deletions(-)

> 

> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c

> index 492a971b95b5..eef2dcbc9185 100644

> --- a/drivers/input/keyboard/gpio_keys.c

> +++ b/drivers/input/keyboard/gpio_keys.c

> @@ -499,25 +499,34 @@ static int gpio_keys_setup_key(struct platform_device *pdev,

>  	bdata->button = button;

>  	spin_lock_init(&bdata->lock);

>  

> -	if (child) {

> -		bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL,

> -								child,

> -								GPIOD_IN,

> -								desc);

> -		if (IS_ERR(bdata->gpiod)) {

> -			error = PTR_ERR(bdata->gpiod);

> -			if (error == -ENOENT) {

> -				/*

> -				 * GPIO is optional, we may be dealing with

> -				 * purely interrupt-driven setup.

> -				 */

> -				bdata->gpiod = NULL;

> -			} else {

> -				if (error != -EPROBE_DEFER)

> -					dev_err(dev, "failed to get gpio: %d\n",

> -						error);

> -				return error;

> -			}

> +	/*

> +	 * We try this first as it will find GPIOs even from board

> +	 * files if properly done.

> +	 */

> +	bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL,

> +							child,

> +							GPIOD_IN,

> +							desc);

> +	/*

> +	 * If we have a valid fwnode and this lookup fails, we need

> +	 * to give up. Otherwise we try to use the GPIO from the

> +	 * platform data.

> +	 */

> +	if (!IS_ERR(bdata->gpiod)) {

> +		/* All is good */

> +	} else if (child) {

> +		error = PTR_ERR(bdata->gpiod);

> +		if (error == -ENOENT) {

> +			/*

> +			 * GPIO is optional, we may be dealing with

> +			 * purely interrupt-driven setup.

> +			 */

> +			bdata->gpiod = NULL;

> +		} else {

> +			if (error != -EPROBE_DEFER)

> +				dev_err(dev, "failed to get gpio: %d\n",

> +					error);

> +			return error;

>  		}

>  	} else if (gpio_is_valid(button->gpio)) {

>  		/*

> -- 

> 2.17.1

> 


-- 
Dmitry
Linus Walleij Sept. 18, 2018, 5:36 p.m. | #2
On Mon, Sep 17, 2018 at 11:15 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> The issue I see here is that gpio-keys still

> needs platform data to deal with per-button config.


Yes I was just thinking inside the GPIO box and solving my own
little problem...

>I will be sending a

> patch set in a minute that introduces notion of "children" to legacy

> device properties (expressed via property_entry/property_set) and wiring

> up gpiolib lookup tables to work with them. Hopefully you and Rafael

> would be OK with it, and then we could do proper cleanup of gpio-keys

> and similar drivers.


I love it. I will look closely at it.

I think also Andy has some of these boardfile type deployments on
Intel when no ACPI description is available.

Yours,
Linus Walleij

Patch

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 492a971b95b5..eef2dcbc9185 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -499,25 +499,34 @@  static int gpio_keys_setup_key(struct platform_device *pdev,
 	bdata->button = button;
 	spin_lock_init(&bdata->lock);
 
-	if (child) {
-		bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL,
-								child,
-								GPIOD_IN,
-								desc);
-		if (IS_ERR(bdata->gpiod)) {
-			error = PTR_ERR(bdata->gpiod);
-			if (error == -ENOENT) {
-				/*
-				 * GPIO is optional, we may be dealing with
-				 * purely interrupt-driven setup.
-				 */
-				bdata->gpiod = NULL;
-			} else {
-				if (error != -EPROBE_DEFER)
-					dev_err(dev, "failed to get gpio: %d\n",
-						error);
-				return error;
-			}
+	/*
+	 * We try this first as it will find GPIOs even from board
+	 * files if properly done.
+	 */
+	bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL,
+							child,
+							GPIOD_IN,
+							desc);
+	/*
+	 * If we have a valid fwnode and this lookup fails, we need
+	 * to give up. Otherwise we try to use the GPIO from the
+	 * platform data.
+	 */
+	if (!IS_ERR(bdata->gpiod)) {
+		/* All is good */
+	} else if (child) {
+		error = PTR_ERR(bdata->gpiod);
+		if (error == -ENOENT) {
+			/*
+			 * GPIO is optional, we may be dealing with
+			 * purely interrupt-driven setup.
+			 */
+			bdata->gpiod = NULL;
+		} else {
+			if (error != -EPROBE_DEFER)
+				dev_err(dev, "failed to get gpio: %d\n",
+					error);
+			return error;
 		}
 	} else if (gpio_is_valid(button->gpio)) {
 		/*