Re: [PATCH v9 2/9] Input: goodix - reset device at init

["mika.westerberg@linux.intel.com" <mika.westerberg@linux.intel.com>]

On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote:
> cpi_dev_add_driver_gpios() does not really help with generic drivers
> (unless we keep adding more and more board specific data to them). How
> about we keep track of names used and only allow conversion for the
> first name used, like in the patch below?

That works but it misses one case: When you have optional GPIOs and not
all of them are provided. Currently it ends up the same situation
returning the same GPIO.

I've commented below how we could handle that case as well.

> -- 
> Dmitry
> 
> gpiolib: tighten up ACPI legacy gpio lookups
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/gpio/gpiolib.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5db3445..4ae5447 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct acpi_gpio_lookup {
> +	struct list_head node;
> +	struct device *dev;
> +	const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
> +{
> +	struct acpi_gpio_lookup *l, *lookup = NULL;

I'm thinking if we here do

	struct acpi_device *adev = ACPI_COMPANION(dev);

	/* Never fallback if the device has properties */
	if (adev->data.properties || adev->driver_gpios)
		return false;

> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> +		if (l->dev == dev) {
> +			lookup = l;
> +			break;
> +		}
> +	}
> +
> +	if (!lookup) {
> +		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> +		if (lookup) {
> +			lookup->dev = dev;
> +			lookup->con_id = con_id;
> +			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> +		}
> +	}
> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	return lookup &&
> +		((!lookup->con_id && !con_id) ||
> +		 (lookup->con_id && con_id &&
> +		  strcmp(lookup->con_id, con_id) == 0));
> +}
> +
>  static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  					unsigned int idx,
>  					enum gpio_lookup_flags *flags)
> @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  
>  	/* Then from plain _CRS GPIOs */
>  	if (IS_ERR(desc)) {
> -		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> +		if (acpi_can_fallback_crs(dev, con_id))
> +			desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

and here we could do

		if (!acpi_can_fallback_crs(dev, con_id))
			return ERR_PTR(-ENOENT);
		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

So instead return -ENOENT so that gpiod_get_index_optional() handles the
missing optional GPIO properly.

>  		if (IS_ERR(desc))
>  			return desc;
>  	}