Re: [RFC] gpiolib: introduce descriptor-based GPIO interface

[Alex Courbot <acourbot@nvidia.com>]

Hi Guenter,

On Friday 07 December 2012 10:49:47 Guenter Roeck wrote:
> My own idea for a solution was to keep integer based handles, but replace
> gpio_desc[] with a hash table. But ultimately I don't really care how
> it is done.
> 
> Do you have a solution for gpiochip_find_base() in mind, and how to handle
> reservations ? I had thought about using bit maps, but maybe there is
> a better solution.

My plan so far is to use a sorted linked list of gpio_chips. Each chip 
contains its base address and size, so this will make it possible to find 
usable areas through a single parse. Current gpiochip_find_base() start from 
ARCH_NR_GPIOS and look backwards in the integer space to find a free range, a 
similar behavior can also be done if this is deemed better (GPIO numbers might 
become high, but since we want to hide them this should not matter).

The counterpart of the list is that fetching the descriptor corresponding to a 
GPIO number is going to be linear instead of constant, but (1) the number of 
gpio_chips on the system should never grow very high and (2) this is a good 
incentive to use the descriptor-based API instead. :) Existing code could 
easily be converted - once a GPIO is acquired, its number should be converted 
immediatly to a descriptor and the gpiod_* functions used from them on. We can 
probably write a sed or Coccinelle rule to do that through the whole kernel.

gpiochip_reserve() will require some more thinking using this model, but 
something like a dummy chip can probably be introduced in the list. It will 
need to be statically allocated however since memory allocation cannot be used 
there.

Actually the only user of gpiochip_reserve() seems to be Tosa, so I wonder if 
there would be no way to simply get rid of it?

> If/when you have some code to test, please let me know.

Sure!

> > +     mutex_lock(&sysfs_lock);
> > 
> >       /* can't export until sysfs is available ... */
> >       if (!gpio_class.p) {
> > 
> > @@ -713,13 +743,7 @@ int gpio_export(unsigned gpio, bool
> > direction_may_change)> 
> >               return -ENOENT;
> 
> mutex is not released here.

Oops.

> > -     chip = gpio_to_chip(gpio);
> > +     chip = desc->chip;
> > +     gpio = gpio_chip_offset(desc);
> 
> Might be better to use a separate variable named 'offset' when dealing with
> the offset, to avoid confusion and accidential bugs. You are doing it
> below, so you might as well do it everywhere. This would also simplify the
> code in a couple of places where gpio is first converted into an offset
> only to use "chip->base + gpio" later on.

Makes sense. Fixed, thanks.

> > -                                     __func__, gpio, err);
> > +                                     __func__, offset + chip->base, err);
> 
> I know I am nitpicking, but everywhere in the existing code it is
> "chip->base + offset/gpio".

Fixed.

> >       return chip->to_irq ? chip->to_irq(chip, gpio - chip->base) :
> >       -ENXIO;
> 
> s/gpio - chip->base/gpio_chip_offset(desc)/
> 
> then you don't need gpio at all.

Done, thanks.

> > -     chip = gpio_to_chip(gpio);
> > +     chip = desc->chip;
> > +     gpio = desc_to_gpio(desc);
> > 
> >       trace_gpio_value(gpio, 0, value);
> >       if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
> 
> Use desc directly.
> 
> > -             _gpio_set_open_drain_value(gpio, chip, value);
> > +             _gpio_set_open_drain_value(desc, value);
> > 
> >       else if (test_bit(FLAG_OPEN_SOURCE,  &gpio_desc[gpio].flags))
> 
> Use desc directly.

Right.

Thanks,
Alex.