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

PATCH In review Daniel Mack 2 participants 6 messages Jul 22, 2012 – Jul 24, 2012 GPIO

Share & tools

Daniel Mack

author

2012-07-22 17:19 UTC #0 lore raw

PATCH 1/1 gpiolib: fix chip->base handling in of_gpio_simple_xlate()

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 <[email protected]>

Cc Grant Likely <[email protected]>

Cc Linus Walleij <[email protected]>

---

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

drivers/gpio/gpiolib-of.c +2 -2

index d18068a..51bc232 100644
@@ -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);

Signature · 1 line

1.7.10.4

Daniel Mack

author

2012-07-23 14:44 UTC #1 lore raw

(Cc: Arnd)

Quoted 45 lines · 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 <[email protected]>
Cc Grant Likely <[email protected]>
Cc Linus Walleij <[email protected]>
---
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(-)
drivers/gpio/gpiolib-of.c +2 -2
index d18068a..51bc232 100644
@@ -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);
 

Arnd Bergmann

2012-07-24 12:57 UTC #2 lore raw

Quoted 20 lines · Daniel Mack

(Cc: Arnd)
Quoted 17 lines · 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 <[email protected]>
Cc Grant Likely <[email protected]>
Cc Linus Walleij <[email protected]>
---
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 ;-)

Quoted 12 lines

drivers/gpio/gpiolib-of.c +1 -1

index d18068a..51bc232 100644
@@ -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;

Quoted 7 lines

drivers/gpio/gpiolib-of.c int of_gpio_simple_xlate(struct gpio_chip *gc, · line 158
Quoted 7 lines
drivers/gpio/gpiolib-of.c +1 -1
 	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

Daniel Mack

author

2012-07-24 13:04 UTC #3 lore raw

Hi Arnd,

Quoted 17 lines · Arnd Bergmann

Quoted 16 lines · Daniel Mack
(Cc: Arnd)
Quoted 13 lines · 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 <[email protected]>
Cc Grant Likely <[email protected]>
Cc Linus Walleij <[email protected]>
---

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
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.

Quoted 16 lines

drivers/gpio/gpiolib-of.c +1 -1

index d18068a..51bc232 100644
@@ -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];

Quoted 6 lines

Quoted 4 lines
drivers/gpio/gpiolib-of.c int of_gpio_simple_xlate(struct gpio_chip *gc, · line 158
Quoted 4 lines
drivers/gpio/gpiolib-of.c +1 -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

Arnd Bergmann

2012-07-24 14:08 UTC #4 lore raw

Quoted 11 lines · Daniel Mack

Quoted 3 lines
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.

Quoted 22 lines

Quoted 15 lines

drivers/gpio/gpiolib-of.c +1 -1

index d18068a..51bc232 100644
@@ -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];

Quoted 6 lines

Quoted 4 lines
drivers/gpio/gpiolib-of.c int of_gpio_simple_xlate(struct gpio_chip *gc, · line 158
Quoted 4 lines
drivers/gpio/gpiolib-of.c +1 -1
-   return gpiospec->args[0];
+   return gpiospec->args[0] - gc->base;
 }
 EXPORT_SYMBOL(of_gpio_simple_xlate);
Where would that gc->base come from?

Quoted 3 lines

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

Daniel Mack

author

2012-07-24 15:22 UTC #5 lore raw

Quoted 28 lines · Arnd Bergmann

Quoted 22 lines · Daniel Mack
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
drivers/gpio/gpiolib-of.c +1 -1
index d18068a..51bc232 100644
@@ -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];
 
Quoted 6 lines
Quoted 4 lines
drivers/gpio/gpiolib-of.c int of_gpio_simple_xlate(struct gpio_chip *gc, · line 158
Quoted 4 lines
drivers/gpio/gpiolib-of.c +1 -1
-   return gpiospec->args[0];
+   return gpiospec->args[0] - gc->base;
 }
 EXPORT_SYMBOL(of_gpio_simple_xlate);
Where would that gc->base come from?
Quoted 3 lines
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?

Quoted 8 lines

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