Re: [PATCH 8/9] gpiolib: use gpio_chips list in gpio_to_desc

[Grant Likely <grant.likely@secretlab.ca>]

On Sun,  3 Feb 2013 01:29:31 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Parse the list of chips to find the descriptor corresponding to a GPIO
> number instead of directly picking the entry of the global gpio_desc[]
> array, which is due to be removed.
> 
> This turns the complexity of converting a GPIO number into a descriptor
> from O(1) to O(n) where n is the number of GPIO chips in the system.
> Since n is ought to be small anyway, there should be no noticeable
> performance impact. Moreover, GPIO users who care for speed already have
> implemented their own gpio_get_value() and gpio_set_value() with a
> fast path for the GPIO numbers that matter and this change does not
> affect such use cases.
> 
> The descriptor-based GPIO API, due to be introduced soon, will make this
> lookup unnecessary.

I'm actually going to NAK this one. This is a hot path. Having a O(1)
lookup from gpio number to gpio desc is important. I know you want to be
rid of the gpio_desc table entirely, but in this case I think it is
warranted. However, you can change the gpio_desc table to be a table of
pointers to gpio_descs instead of a table of gpio_descs. That would save
a lot of memory since unused GPIOs wouldn't have gpio_descs associated
with them. It is also the mechanism used by the IRQ subsystem.

So, it makes sense for the primary gpio_chip lookup to be the list, but
the table needs to stick around as a secondary lookup.

g.

> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9599b9a..0247c48 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -122,10 +122,21 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
>   */
>  static struct gpio_desc *gpio_to_desc(unsigned gpio)
>  {
> -	if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
> -		return NULL;
> -	else
> -		return &gpio_desc[gpio];
> +	struct gpio_chip *chip;
> +
> +	list_for_each_entry(chip, &gpio_chips, list) {
> +		int gpio_min = chip->base;
> +		int gpio_max = gpio_min + chip->ngpio;
> +		if (gpio >= gpio_min && gpio < gpio_max)
> +			return &chip->desc[gpio - gpio_min];
> +		else if (gpio < gpio_min)
> +			/* gpio_chips are ordered by base, so we won't get any
> +			 * hit if we arrive here... */
> +			break;
> +	}
> +
> +	pr_warn("%s: no registered chip to handle GPIO %d\n", __func__, gpio);
> +	return NULL;
>  }
>  
>  /**
> -- 
> 1.8.1.1
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.