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

[Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>]

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@xxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> 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