Re: [PATCH] gpiolib: fix chip->base handling in of_gpio_simple_xlate()

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 24 July 2012, Daniel Mack wrote:
> > But why would you do that? Both the "gpiochip" and its DT representation
> > attempt to represent the hardware structure. If they don't match, then
> > I'd assume one of them is wrong ;-)
> 
> Well, have a look at what's currently there in drivers/gpio/gpio-pxa.c.
> There are several gpio_chips that are registered. On the DT side,
> however, I would much like to present all GPIO line in one array, so the
> numbers match the hardware documentation.
> 
> I prepared patches for all that and they work find, the only thing I
> need to touch in the core for that is this minor detail.

We recently reworked the gpiolib code to allow multiple gpiochips
to be registered for the same device, but in that use case (lpc32xx)
we had separate banks listed in the data sheet, and it still made more
sense to have the bank number listed in the gpio specifier.

> >>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>> index d18068a..51bc232 100644
> >>> --- a/drivers/gpio/gpiolib-of.c
> >>> +++ b/drivers/gpio/gpiolib-of.c
> >>> @@ -147,13 +147,13 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
> >>>     if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> >>>             return -EINVAL;
> >>>  
> >>> -   if (gpiospec->args[0] >= gc->ngpio)
> >>> +   if (gpiospec->args[0] >= gc->ngpio + gc->base)
> >>>             return -EINVAL;
> >>>  
> >>>     if (flags)
> >>>             *flags = gpiospec->args[1];
> >>>  
> >>> -   return gpiospec->args[0];
> >>> +   return gpiospec->args[0] - gc->base;
> >>>  }
> >>>  EXPORT_SYMBOL(of_gpio_simple_xlate);
> > 
> > Where would that gc->base come from?
> 
> It is set up when the chips are initialized. Let's put it that way: why
> would we have this ->base if it is practically unusable in devicetree
> environments?

The base gets used to put the gpiochip into the Linux gpio number space,
which is not necessarily the same as the number space used in the device
tree. You can dynamically add other gpio controllers that would get
some arbitrary base assigned at runtime, so you cannot subtract that
base from the hardware number to get a local one in the common code.

I fear you will have to provide your own xlate function for pxa if
you want to use this numbering. Something like this:?

static int pxa_of_xlate(struct gpio_chip *gc,
                        const struct of_phandle_args *gpiospec, u32 *flags)
{
	if (gpiospec->args[0] > pxa_last_gpio)
		return -EINVAL;

	if (gc != &pxa_gpio_chips[gpiospec->args[0] / 32]->chip)
		return -EINVAL;

        if (flags)
                *flags = gpiospec->args[1];

        return gpiospec->args[0] % 32;
}

	Arnd