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

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

[Daniel Mack]

of_gpio_simple_xlate() is called for each chip when a GPIO is looked up.
When registering several chips off the same DT node (with different pin
offsets) however, the lookup fails as the GPIO number passed in to
of_gpio_simple_xlate() is likely higher than the chip's ->ngpio value.

Fix that by taking into account the chip's ->base value, and return the
relative offset of the pin inside the chip.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@stericsson.com>
---

I'm currently porting the PXA pieces over to DT, and stumbled over what
looks like an obvious bug to me. Correct me if I'm mistaken, but I see
no reason why one shouldn't be able to instanciate several GPIO chips
from a single DT node.

Thanks,
Daniel

 drivers/gpio/gpiolib-of.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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);
 
-- 
1.7.10.4

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

[Daniel Mack]

(Cc: Arnd)

On 22.07.2012 19:10, Daniel Mack wrote:
> of_gpio_simple_xlate() is called for each chip when a GPIO is looked up.
> When registering several chips off the same DT node (with different pin
> offsets) however, the lookup fails as the GPIO number passed in to
> of_gpio_simple_xlate() is likely higher than the chip's ->ngpio value.
> 
> Fix that by taking into account the chip's ->base value, and return the
> relative offset of the pin inside the chip.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> ---
> 
> I'm currently porting the PXA pieces over to DT, and stumbled over what
> looks like an obvious bug to me. Correct me if I'm mistaken, but I see
> no reason why one shouldn't be able to instanciate several GPIO chips
> from a single DT node.
> 
> Thanks,
> Daniel
> 
>  drivers/gpio/gpiolib-of.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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);
>  
> 

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

[Arnd Bergmann]

On Monday 23 July 2012, Daniel Mack wrote:
> (Cc: Arnd)
> 
> On 22.07.2012 19:10, Daniel Mack wrote:
> > of_gpio_simple_xlate() is called for each chip when a GPIO is looked up.
> > When registering several chips off the same DT node (with different pin
> > offsets) however, the lookup fails as the GPIO number passed in to
> > of_gpio_simple_xlate() is likely higher than the chip's ->ngpio value.
> > 
> > Fix that by taking into account the chip's ->base value, and return the
> > relative offset of the pin inside the chip.
> > 
> > Signed-off-by: Daniel Mack <zonque@gmail.com>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Linus Walleij <linus.walleij@stericsson.com>
> > ---
> > 
> > I'm currently porting the PXA pieces over to DT, and stumbled over what
> > looks like an obvious bug to me. Correct me if I'm mistaken, but I see
> > no reason why one shouldn't be able to instanciate several GPIO chips
> > from a single DT node.

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 ;-)

> > 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?

	Arnd

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

[Daniel Mack]

Hi Arnd,

On 24.07.2012 14:56, Arnd Bergmann wrote:
> On Monday 23 July 2012, Daniel Mack wrote:
>> (Cc: Arnd)
>>
>> On 22.07.2012 19:10, Daniel Mack wrote:
>>> of_gpio_simple_xlate() is called for each chip when a GPIO is looked up.
>>> When registering several chips off the same DT node (with different pin
>>> offsets) however, the lookup fails as the GPIO number passed in to
>>> of_gpio_simple_xlate() is likely higher than the chip's ->ngpio value.
>>>
>>> Fix that by taking into account the chip's ->base value, and return the
>>> relative offset of the pin inside the chip.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> Cc: Grant Likely <grant.likely@secretlab.ca>
>>> Cc: Linus Walleij <linus.walleij@stericsson.com>
>>> ---
>>>
>>> I'm currently porting the PXA pieces over to DT, and stumbled over what
>>> looks like an obvious bug to me. Correct me if I'm mistaken, but I see
>>> no reason why one shouldn't be able to instanciate several GPIO chips
>>> from a single DT node.
> 
> 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.

> 
>>> 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?

And In case ->base equals 0, this patch is a no-op anyway.


Thanks,
Daniel

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

[Arnd Bergmann]

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

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

[Daniel Mack]

On 24.07.2012 16:08, Arnd Bergmann wrote:
> On Tuesday 24 July 2012, Daniel Mack wrote:

>>>>> 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:?

I did my own xlate function in the first place, but then I thought it
could be made working in a more general way. But your explanation makes
sense, so I will add something that is based on your sniplet.

Thanks,
Daniel