dm: gpio: handle GPIO_ACTIVE_LOW flag in DT

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

Signed-off-by Eric Nelson <[email protected]>

---
drivers/gpio/gpio-uclass.c | 9 +++++++--

1 file changed, 7 insertions(+), 2 deletions(-)

drivers/gpio/gpio-uclass.c +7 -2

index b58d4e6..6d30612 100644
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <dm.h>
+#include <dt-bindings/gpio/gpio.h>
 #include <errno.h>
 #include <fdtdec.h>
 #include <malloc.h>

@@ -118,12 +119,16 @@ static int gpio_find_and_xlate(struct gpio_desc *desc,
 {
 	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
 
+	desc->flags = 0;
 	/* Use the first argument as the offset by default */
-	if (args->args_count > 0)
+	if (args->args_count > 0) {
 		desc->offset = args->args[0];
+		if ((args->args_count > 1) &&
+		    (args->args[1] & GPIO_ACTIVE_LOW))
+			desc->flags = GPIOD_ACTIVE_LOW;
+	}
 	else
 		desc->offset = -1;
-	desc->flags = 0;
 
 	return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0;
 }

Signature · 6 lines

2.6.2

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Eric,

Quoted 5 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the intel_broadwell driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/gpio/intel_broadwell_gpio.c | 10 ----------
 1 file changed, 10 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the omap gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/gpio/omap_gpio.c | 10 ----------
 1 file changed, 10 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the pic32 gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/gpio/pic32_gpio.c | 9 ---------
 1 file changed, 9 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the Rockchip gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/gpio/rk_gpio.c | 10 ----------
 1 file changed, 10 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the Exynos/S5P gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

---
 drivers/gpio/s5p_gpio.c | 10 ----------
 1 file changed, 10 deletions(-)

Quoted 9 lines · Eric Nelson

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, it is no longer necessary for the tegra-specific xlate function to do this.

drivers/gpio/tegra_gpio.c static int tegra_gpio_xlate(struct udevice *dev, · line 250

Quoted 6 lines

drivers/gpio/tegra_gpio.c +0 -1

@@ -246,7 +246,6 @@ static int tegra_gpio_xlate(struct udevice *dev,
 struct gpio_desc *desc,
        if (ret)
                return ret;
        desc->offset = gpio % TEGRA_GPIOS_PER_PORT;
-       desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW :
 0;

I expect that after that, you can also remove the following at the top of the file:

#include <dt-bindings/gpio/gpio.h>

I expect that's true of other GPIO drivers too.

Quoted 21 lines · Stephen Warren

Quoted 9 lines · Eric Nelson

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, it is no longer necessary for the tegra-specific xlate function to do this.

drivers/gpio/tegra_gpio.c static int tegra_gpio_xlate(struct udevice *dev, · line 250

Quoted 6 lines

drivers/gpio/tegra_gpio.c +0 -1

@@ -246,7 +246,6 @@ static int tegra_gpio_xlate(struct udevice *dev,
 struct gpio_desc *desc,
        if (ret)
                return ret;
        desc->offset = gpio % TEGRA_GPIOS_PER_PORT;
-       desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW :
 0;

I expect that after that, you can also remove the following at the top of the file:

Quoted 3 lines

#include <dt-bindings/gpio/gpio.h>

I expect that's true of other GPIO drivers too.

But I don't think we want this patch for Tegra since we need to keep the xlate() method, and with your suggested approach earlier, we won't call the default xlate() method in the uclass for Tegra.

Quoted 20 lines · Simon Glass

Quoted 19 lines · Stephen Warren

drivers/gpio/tegra_gpio.c static int tegra_gpio_xlate(struct udevice *dev, · line 250

Quoted 13 lines · Eric Nelson

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, it is no longer necessary for the tegra-specific xlate function to do this.

drivers/gpio/tegra_gpio.c +0 -1

@@ -246,7 +246,6 @@ static int tegra_gpio_xlate(struct udevice *dev,
 struct gpio_desc *desc,
        if (ret)
                return ret;
        desc->offset = gpio % TEGRA_GPIOS_PER_PORT;
-       desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW :
 0;

I expect that after that, you can also remove the following at the top of the file:

Quoted 8 lines

Quoted 3 lines

#include <dt-bindings/gpio/gpio.h>

I expect that's true of other GPIO drivers too.

But I don't think we want this patch for Tegra since we need to keep the xlate() method, and with your suggested approach earlier, we won't call the default xlate() method in the uclass for Tegra.

Based on your comments and Stephens, I can re-work this to have a __maybe_unused xlate function for the common case of handling offset+active low, but I think the handling of offset inside of gpio_find_and_xlate() should be removed at the same time.

It seems weird to have processing for one argument in the common code when it may not be used (as is the case with tegra).

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

	<&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

drivers/gpio/gpio-uclass.c +3 -0

 
+int gpio_xlate_offs_flags(struct udevice *dev,
+					 struct gpio_desc *desc,
+					 struct fdtdec_phandle_args *args)

Quoted 10 lines · Eric Nelson

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

    <&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

The series,

Acked-by Stephen Warren <[email protected]>

Although one nit below.

Quoted 5 lines

drivers/gpio/gpio-uclass.c +3 -0

 
+int gpio_xlate_offs_flags(struct udevice *dev,
+                     struct gpio_desc *desc,
+                     struct fdtdec_phandle_args *args)

drivers/gpio/gpio-uclass.c int gpio_lookup_name(const char *name, struct udevice **devp, · line 129

Quoted 10 lines

drivers/gpio/gpio-uclass.c +10 -0

+{
+    int ret = -EINVAL;
+    if (args->args_count > 1) {
+        desc->offset = args->args[0];
+        if (args->args[1] & GPIO_ACTIVE_LOW)
+            desc->flags = GPIOD_ACTIVE_LOW;
+        ret = 0;
+    }
+    return ret;
+}

Why not return the error first, and avoid the need for an extra indentation level for the rest of the function, and make it quite a bit more readable in my opinion. The default could also be made to cover more situations (e.g. bindings that define a single cell for the GPIO but not cell at all for any flags):

if (args->args_count < 1)

    return -EINVAL;

desc->offset = args->args[0];

if (args->args_count < 2)

    return 0;

if (args->args[1] & GPIO_ACTIVE_LOW)

    desc->flags = GPIOD_ACTIVE_LOW;

return 0;

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

	<&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Stephen Warren <[email protected]>

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the pic32 gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the Exynos/S5P gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

---
V2 removes the include of <dt-bindings/gpio/gpio.h>

 drivers/gpio/s5p_gpio.c | 11 -----------
 1 file changed, 11 deletions(-)

drivers/gpio/s5p_gpio.c +0 -10

index 0f22b23..377fed4 100644
@@ -13,7 +13,6 @@
 #include <asm/io.h>
 #include <asm/gpio.h>
 #include <dm/device-internal.h>
-#include <dt-bindings/gpio/gpio.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 

@@ -276,22 +275,12 @@ static int exynos_gpio_get_function(struct udevice *dev, unsigned offset)
 		return GPIOF_FUNC;
 }
 
-static int exynos_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
-			     struct fdtdec_phandle_args *args)
-{
-	desc->offset = args->args[0];
-	desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
-
-	return 0;
-}
-

Hi Peng,

Quoted 11 lines · Peng Fan

Hi Eric,

Quoted 5 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

Thanks for pointing this out.

This patch also works, but it has me confused.

How/why is parsing the ACTIVE_LOW flag specific to MXC?

This is a general-purpose flag in the kernel, not something machine- specific.

It also appears that there are a bunch of other copies of this same bit of code in the various mach_xlate() routines:

desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;

If it's done in gpio-uclass, this isn't needed and xlate can be removed from mxc-gpio and quite a few other architectures.

Please advise,

Eric

Quoted 27 lines · Eric Nelson

Quoted 9 lines · Peng Fan

Quoted 5 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

Thanks for pointing this out.

This patch also works, but it has me confused.

How/why is parsing the ACTIVE_LOW flag specific to MXC?

This is a general-purpose flag in the kernel, not something machine- specific.

It also appears that there are a bunch of other copies of this same bit of code in the various mach_xlate() routines:

desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;

If it's done in gpio-uclass, this isn't needed and xlate can be removed from mxc-gpio and quite a few other architectures.

Quoted 1 line

Please advise,

I saw you have posted a patch set to convert other gpio drivers. But actually the translation of gpio property should be done by each gpio driver. Alought we have gpio-cells=<2> for most gpio drivers, but if there is one case that gpio-cells=<3>, and it have different meaning for each cell from other most drivers?

So I suggest we follow the linux way,

434 if (!chip->of_xlate) {
435 chip->of_gpio_n_cells = 2;
436 chip->of_xlate = of_gpio_simple_xlate;
437 }

If gpio drivers does not provide xlate function, then use a simple xlate function. If gpio drivers have their own xlate function, then use their own way.

Also I do no think delete the xlate implementation is not a good idea.

Hi Peng,

Quoted 28 lines · Peng Fan

Quoted 27 lines · Eric Nelson

Quoted 9 lines · Peng Fan

Quoted 5 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

Thanks for pointing this out.

This patch also works, but it has me confused.

How/why is parsing the ACTIVE_LOW flag specific to MXC?

This is a general-purpose flag in the kernel, not something machine- specific.

It also appears that there are a bunch of other copies of this same bit of code in the various mach_xlate() routines:

desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;

If it's done in gpio-uclass, this isn't needed and xlate can be removed from mxc-gpio and quite a few other architectures.

Quoted 7 lines

Quoted 1 line

Please advise,

I saw you have posted a patch set to convert other gpio drivers. But actually the translation of gpio property should be done by each gpio driver. Alought we have gpio-cells=<2> for most gpio drivers, but if there is one case that gpio-cells=<3>, and it have different meaning for each cell from other most drivers?

Which case has gpio-cells=<3>?

As far as I can tell, only tegra and sandbox have something other than parsing of offset and the GPIO_ACTIVE_LOW flag.

Tegra seems to have gpio-cells=<2> and sandbox has either 0 or 1.

Quoted 7 lines

So I suggest we follow the linux way,

434 if (!chip->of_xlate) {
435 chip->of_gpio_n_cells = 2;
436 chip->of_xlate = of_gpio_simple_xlate;
437 }

Quoted 3 lines

If gpio drivers does not provide xlate function, then use a simple xlate function. If gpio drivers have their own xlate function, then use their own way.

The recommendation in device-tree-bindings/gpio/gpio.txt is to have the GPIO_ACTIVE_LOW/HIGH flag in the second cell, so parsing that directly in gpio_find_and_xlate() seems right.

Quoted 51 lines · Eric Nelson

Quoted 28 lines · Peng Fan

Quoted 27 lines · Eric Nelson

Quoted 9 lines · Peng Fan

Quoted 5 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

Thanks for pointing this out.

This patch also works, but it has me confused.

How/why is parsing the ACTIVE_LOW flag specific to MXC?

This is a general-purpose flag in the kernel, not something machine- specific.

It also appears that there are a bunch of other copies of this same bit of code in the various mach_xlate() routines:

desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;

If it's done in gpio-uclass, this isn't needed and xlate can be removed from mxc-gpio and quite a few other architectures.

Quoted 7 lines

Quoted 1 line

Please advise,

I saw you have posted a patch set to convert other gpio drivers. But actually the translation of gpio property should be done by each gpio driver. Alought we have gpio-cells=<2> for most gpio drivers, but if there is one case that gpio-cells=<3>, and it have different meaning for each cell from other most drivers?

Which case has gpio-cells=<3>?

As far as I can tell, only tegra and sandbox have something other than parsing of offset and the GPIO_ACTIVE_LOW flag.

Tegra seems to have gpio-cells=<2> and sandbox has either 0 or 1.

Quoted 7 lines

So I suggest we follow the linux way,

434 if (!chip->of_xlate) {
435 chip->of_gpio_n_cells = 2;
436 chip->of_xlate = of_gpio_simple_xlate;
437 }

Quoted 7 lines

Quoted 3 lines

If gpio drivers does not provide xlate function, then use a simple xlate function. If gpio drivers have their own xlate function, then use their own way.

The recommendation in device-tree-bindings/gpio/gpio.txt is to have the GPIO_ACTIVE_LOW/HIGH flag in the second cell, so parsing that directly in gpio_find_and_xlate() seems right.

That's a recommendation when designing GPIO controller bindings, not a definition of something that is categorically true for all bindings. Each binding is free to represent its flags (if any) in whatever way it wants, and so as Peng says, each driver has be in full control over its own of_xlate functionality. Now, for drivers that happen to use a common flag representation, we can plug in a common implementation of the xlate function.

Hi Stephen and Peng,

Quoted 60 lines · Stephen Warren

Quoted 51 lines · Eric Nelson

Quoted 28 lines · Peng Fan

Quoted 27 lines · Eric Nelson

Quoted 9 lines · Peng Fan

Quoted 5 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

Thanks for pointing this out.

This patch also works, but it has me confused.

How/why is parsing the ACTIVE_LOW flag specific to MXC?

This is a general-purpose flag in the kernel, not something machine- specific.

It also appears that there are a bunch of other copies of this same bit of code in the various mach_xlate() routines:

desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;

If it's done in gpio-uclass, this isn't needed and xlate can be removed from mxc-gpio and quite a few other architectures.

Quoted 7 lines

Quoted 1 line

Please advise,

I saw you have posted a patch set to convert other gpio drivers. But actually the translation of gpio property should be done by each gpio driver. Alought we have gpio-cells=<2> for most gpio drivers, but if there is one case that gpio-cells=<3>, and it have different meaning for each cell from other most drivers?

Which case has gpio-cells=<3>?

As far as I can tell, only tegra and sandbox have something other than parsing of offset and the GPIO_ACTIVE_LOW flag.

Tegra seems to have gpio-cells=<2> and sandbox has either 0 or 1.

Quoted 7 lines

So I suggest we follow the linux way,

434 if (!chip->of_xlate) {
435 chip->of_gpio_n_cells = 2;
436 chip->of_xlate = of_gpio_simple_xlate;
437 }

Quoted 7 lines

Quoted 3 lines

If gpio drivers does not provide xlate function, then use a simple xlate function. If gpio drivers have their own xlate function, then use their own way.

The recommendation in device-tree-bindings/gpio/gpio.txt is to have the GPIO_ACTIVE_LOW/HIGH flag in the second cell, so parsing that directly in gpio_find_and_xlate() seems right.

Quoted 7 lines

That's a recommendation when designing GPIO controller bindings, not a definition of something that is categorically true for all bindings. Each binding is free to represent its flags (if any) in whatever way it wants, and so as Peng says, each driver has be in full control over its own of_xlate functionality. Now, for drivers that happen to use a common flag representation, we can plug in a common implementation of the xlate function.

Isn't that what this patch-set does?

	http://lists.denx.de/pipermail/u-boot/2016-April/250228.html

For the cost of a couple of lines of code in gpio-uclass, we remove ~50 lines from existing implementations, essentially allowing them to use the common (or default) implementation. Nothing in the patch prevents completely custom handling by a driver.

If we want to be pedantic about requiring each driver to have function xlate, then we should get rid of gpio_find_and_xlate entirely from gpio-uclass.c.

Quoted 85 lines · Eric Nelson

Hi Stephen and Peng,

Quoted 72 lines · Stephen Warren

Quoted 65 lines · Eric Nelson

Quoted 33 lines · Peng Fan

Quoted 31 lines · Eric Nelson

Quoted 12 lines · Peng Fan

Quoted 6 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

You may need to try this: https://patchwork.ozlabs.org/patch/597363/

Thanks for pointing this out.

This patch also works, but it has me confused.

How/why is parsing the ACTIVE_LOW flag specific to MXC?

This is a general-purpose flag in the kernel, not something machine- specific.

It also appears that there are a bunch of other copies of this same bit of code in the various mach_xlate() routines:

desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;

If it's done in gpio-uclass, this isn't needed and xlate can be removed from mxc-gpio and quite a few other architectures.

Quoted 8 lines

Quoted 1 line

Please advise,

I saw you have posted a patch set to convert other gpio drivers. But actually the translation of gpio property should be done by each gpio driver. Alought we have gpio-cells=<2> for most gpio drivers, but if there is one case that gpio-cells=<3>, and it have different meaning for each cell from other most drivers?

Which case has gpio-cells=<3>?

As far as I can tell, only tegra and sandbox have something other than parsing of offset and the GPIO_ACTIVE_LOW flag.

Tegra seems to have gpio-cells=<2> and sandbox has either 0 or 1.

Quoted 11 lines

So I suggest we follow the linux way,

434 if (!chip->of_xlate) {
435 chip->of_gpio_n_cells = 2;
436 chip->of_xlate = of_gpio_simple_xlate;
437 }

If gpio drivers does not provide xlate function, then use a simple xlate
function. If gpio drivers have their own xlate function, then use their own way.

Quoted 3 lines

The recommendation in device-tree-bindings/gpio/gpio.txt is to have the GPIO_ACTIVE_LOW/HIGH flag in the second cell, so parsing that directly in gpio_find_and_xlate() seems right.

Quoted 7 lines

That's a recommendation when designing GPIO controller bindings, not a definition of something that is categorically true for all bindings. Each binding is free to represent its flags (if any) in whatever way it wants, and so as Peng says, each driver has be in full control over its own of_xlate functionality. Now, for drivers that happen to use a common flag representation, we can plug in a common implementation of the xlate function.

Quoted 2 lines

Isn't that what this patch-set does?

        http://lists.denx.de/pipermail/u-boot/2016-April/250228.html

No, I don't believe so. Rather, it forcibly decodes the common layout in the common code, in such a way that it's always used. Then, the driver-specific of_xlate is called, which could fix up (i.e. undo) the incorrect results if they weren't appropriate, and then apply the correct translation.

Better would be to never decode the results incorrectly in the first place, yet allow each driver to use the common decoder function if it's appropriate.

gpio_find_and_xlate() should do either exactly:

a)

return ops->xlate(desc->dev, desc, args);

In this case, .xlate could never be NULL, and all drivers would need to provide some implementation. We could provide a common implementation gpio_xlate_common() that all drivers (that use the common DT specifier layout) can plug into their .xlate pointer.

b)

if (ops->xlate)

    return ops->xlate(desc->dev, desc, args);
else
    return gpio_xlate_common(desc->dev, desc, args);

Making that else clause call a separate function allows any custom ops->xlate implementation to call it too, assuming the code there is valid but simply needs extension rather than completely custom replacement.

Quoted 8 lines

For the cost of a couple of lines of code in gpio-uclass, we remove ~50 lines from existing implementations, essentially allowing them to use the common (or default) implementation. Nothing in the patch prevents completely custom handling by a driver.

If we want to be pedantic about requiring each driver to have function xlate, then we should get rid of gpio_find_and_xlate entirely from gpio-uclass.c.

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

Quoted 8 lines · Peng Fan

Quoted 6 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Can you please take a look at what Stephen suggests?

Hi Simon,

Quoted 15 lines · Simon Glass

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

Quoted 8 lines · Peng Fan

Quoted 6 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 16 lines

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Quoted 2 lines

Can you please take a look at what Stephen suggests?

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Hi Eric,

Quoted 40 lines · Eric Nelson

Hi Simon,

Quoted 15 lines · Simon Glass

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

Quoted 8 lines · Peng Fan

Quoted 6 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 18 lines

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Can you please take a look at what Stephen suggests?

Quoted 4 lines

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Which patch did I send? My understanding is:

- Add my review/ack tag to the patches as necessary
- Drop the tegra patch
- Update gpio_find_and_xlate() to call a default function if there is no xlate() method
- Resend the series

I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method.

Hi Simon,

Quoted 43 lines · Simon Glass

Hi Eric,

Quoted 40 lines · Eric Nelson

Hi Simon,

Quoted 15 lines · Simon Glass

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

Quoted 8 lines · Peng Fan

Quoted 6 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence of the flag and setting the desc->flags field to the driver model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 18 lines

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Can you please take a look at what Stephen suggests?

Quoted 7 lines

Quoted 4 lines

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Which patch did I send? My understanding is:

At the time I sent this, you had just submitted the patch set adding more driver-model support for block devices.

        http://lists.denx.de/pipermail/u-boot/2016-April/251095.html

Quoted 11 lines

- Add my review/ack tag to the patches as necessary
- Drop the tegra patch
- Update gpio_find_and_xlate() to call a default function if there is no xlate() method
- Resend the series

I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method.

Reviewing the use of the offset field did yield some information about the broken sunxi support and also that Vybrid was also missing the xlate routine.

Since reviewing your patch sets (driver model updates for blk and also driver model updates for mmc) will take some time, so I'll base an updated patch set on master. My guess is that any merge issues will be trivial.

I'll remove your acks in the updated patch set, since the updates to the drivers won't drop the xlate field, but will connect them to the common (__maybe_unused) routine. This will prevent the code from leaking into machines like Tegra that don't need the common code.

Quoted 65 lines · Eric Nelson

Quoted 39 lines · Simon Glass

Quoted 38 lines · Eric Nelson

Quoted 15 lines · Simon Glass

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

Quoted 8 lines · Peng Fan

Quoted 6 lines · Eric Nelson

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence
of the flag and setting the desc->flags field to the driver
model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 18 lines

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Can you please take a look at what Stephen suggests?

Quoted 7 lines

Quoted 4 lines

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Which patch did I send? My understanding is:

At the time I sent this, you had just submitted the patch set adding more driver-model support for block devices.

        http://lists.denx.de/pipermail/u-boot/2016-April/251095.html

Quoted 11 lines

- Add my review/ack tag to the patches as necessary
- Drop the tegra patch
- Update gpio_find_and_xlate() to call a default function if there is no xlate() method
- Resend the series

I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method.

Quoted 8 lines

Reviewing the use of the offset field did yield some information about the broken sunxi support and also that Vybrid was also missing the xlate routine.

Since reviewing your patch sets (driver model updates for blk and also driver model updates for mmc) will take some time, so I'll base an updated patch set on master. My guess is that any merge issues will be trivial.

Yes, that's right.

Quoted 5 lines

I'll remove your acks in the updated patch set, since the updates to the drivers won't drop the xlate field, but will connect them to the common (__maybe_unused) routine. This will prevent the code from leaking into machines like Tegra that don't need the common code.

I'm pretty sure you can drop the xlate() implementations from the functions, though, and those at the patches I acked.

I don't think you need __maybe_unused

static int gpio_find_and_xlate(...)

{
   get ops...

   if (ops->xlate)
      return ops->xlate(....)
   else
      return gpio_default_xlate()...
}

gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it.

Hi Simon,

Quoted 97 lines · Simon Glass

Quoted 65 lines · Eric Nelson

Quoted 39 lines · Simon Glass

Quoted 38 lines · Eric Nelson

Quoted 15 lines · Simon Glass

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

Quoted 8 lines · Peng Fan

On Fri, Mar 25, 2016 at 01:12:11PM -0700, Eric Nelson wrote:

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence
of the flag and setting the desc->flags field to the driver
model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 18 lines

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Can you please take a look at what Stephen suggests?

Quoted 7 lines

Quoted 4 lines

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Which patch did I send? My understanding is:

At the time I sent this, you had just submitted the patch set adding more driver-model support for block devices.

        http://lists.denx.de/pipermail/u-boot/2016-April/251095.html

Quoted 11 lines

- Add my review/ack tag to the patches as necessary
- Drop the tegra patch
- Update gpio_find_and_xlate() to call a default function if there is no xlate() method
- Resend the series

I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method.

Quoted 8 lines

Reviewing the use of the offset field did yield some information about the broken sunxi support and also that Vybrid was also missing the xlate routine.

Since reviewing your patch sets (driver model updates for blk and also driver model updates for mmc) will take some time, so I'll base an updated patch set on master. My guess is that any merge issues will be trivial.

Yes, that's right.

Quoted 5 lines

I'll remove your acks in the updated patch set, since the updates to the drivers won't drop the xlate field, but will connect them to the common (__maybe_unused) routine. This will prevent the code from leaking into machines like Tegra that don't need the common code.

I'm pretty sure you can drop the xlate() implementations from the functions, though, and those at the patches I acked.

I don't think you need __maybe_unused

static int gpio_find_and_xlate(...)

{
   get ops...

   if (ops->xlate)
      return ops->xlate(....)
   else
      return gpio_default_xlate()...
}

Quoted 3 lines

gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it.

This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) into machines that don't need it.

I can go the route you suggest above, but it will cost the tegra and sandbox builds ~64 bytes ;)

Hi Eric,

Quoted 104 lines · Eric Nelson

Hi Simon,

Quoted 100 lines · Simon Glass

Quoted 65 lines · Eric Nelson

Quoted 39 lines · Simon Glass

Quoted 38 lines · Eric Nelson

Quoted 15 lines · Simon Glass

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Eric Nelson

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Eric Nelson

Quoted 10 lines · Peng Fan

Quoted 9 lines · Eric Nelson

On 03/28/2016 09:57 PM, Peng Fan wrote:
On Fri, Mar 25, 2016 at 01:12:11PM -0700, Eric Nelson wrote:

Device tree parsing of GPIO nodes is currently ignoring flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence
of the flag and setting the desc->flags field to the driver
model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 18 lines

Quoted 8 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API for clients so they don't need to know how to access driver functionality through the ops pointer, which I think is an internal/private implementation detail. Is that detail exposed to clients in other places? If so, removing the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we should follow along.

Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available.

Can you please take a look at what Stephen suggests?

Quoted 7 lines

Quoted 4 lines

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Which patch did I send? My understanding is:

At the time I sent this, you had just submitted the patch set adding more driver-model support for block devices.

         http://lists.denx.de/pipermail/u-boot/2016-April/251095.html

Quoted 11 lines

- Add my review/ack tag to the patches as necessary
- Drop the tegra patch
- Update gpio_find_and_xlate() to call a default function if there is no xlate() method
- Resend the series

I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method.

Quoted 8 lines

Reviewing the use of the offset field did yield some information about the broken sunxi support and also that Vybrid was also missing the xlate routine.

Since reviewing your patch sets (driver model updates for blk and also driver model updates for mmc) will take some time, so I'll base an updated patch set on master. My guess is that any merge issues will be trivial.

Yes, that's right.

Quoted 5 lines

I'll remove your acks in the updated patch set, since the updates to the drivers won't drop the xlate field, but will connect them to the common (__maybe_unused) routine. This will prevent the code from leaking into machines like Tegra that don't need the common code.

I'm pretty sure you can drop the xlate() implementations from the functions, though, and those at the patches I acked.

I don't think you need __maybe_unused

static int gpio_find_and_xlate(...)

{
    get ops...

    if (ops->xlate)
       return ops->xlate(....)
    else
       return gpio_default_xlate()...
}

gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it.

Quoted 6 lines

This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) into machines that don't need it.

I can go the route you suggest above, but it will cost the tegra and sandbox builds ~64 bytes ;)

Sure, but we can live with that.

Quoted 144 lines · Simon Glass

Hi Eric,

Quoted 139 lines · Eric Nelson

Hi Simon,

Quoted 125 lines · Simon Glass

Quoted 89 lines · Eric Nelson

Quoted 62 lines · Simon Glass

Quoted 60 lines · Eric Nelson

Quoted 27 lines · Simon Glass

Quoted 24 lines · Stephen Warren

Quoted 22 lines · Eric Nelson

Quoted 20 lines · Stephen Warren

Quoted 18 lines · Eric Nelson

Quoted 16 lines · Peng Fan

On Thu, Mar 31, 2016 at 01:41:04PM -0700, Eric Nelson wrote:

On 03/28/2016 09:57 PM, Peng Fan wrote:

On Fri, Mar 25, 2016 at 01:12:11PM -0700, Eric Nelson wrote:

Device tree parsing of GPIO nodes is currently ignoring
flags.

Add support for GPIO_ACTIVE_LOW by checking for the presence
of the flag and setting the desc->flags field to the driver
model constant GPIOD_ACTIVE_LOW.

<snip>

Quoted 27 lines

Quoted 13 lines

The intent of the change is good.

I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API
for clients so they don't need to know how to access driver functionality
through the ops pointer, which I think is an internal/private implementation
detail. Is that detail exposed to clients in other places? If so, removing
the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we
should follow along.

Eric you still get to remove the code from all the GPIO drivers - the
difference is just creating a common function to call when no xlate()
method is available.

Can you please take a look at what Stephen suggests?

Quoted 7 lines

Quoted 4 lines

Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().

Which patch did I send? My understanding is:

At the time I sent this, you had just submitted the patch set adding more driver-model support for block devices.

         http://lists.denx.de/pipermail/u-boot/2016-April/251095.html

Quoted 11 lines

- Add my review/ack tag to the patches as necessary
- Drop the tegra patch
- Update gpio_find_and_xlate() to call a default function if there is no xlate() method
- Resend the series

I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method.

Quoted 8 lines

Reviewing the use of the offset field did yield some information about the broken sunxi support and also that Vybrid was also missing the xlate routine.

Since reviewing your patch sets (driver model updates for blk and also driver model updates for mmc) will take some time, so I'll base an updated patch set on master. My guess is that any merge issues will be trivial.

Yes, that's right.

Quoted 6 lines

I'll remove your acks in the updated patch set, since the updates to the drivers won't drop the xlate field, but will connect them to the common (__maybe_unused) routine. This will prevent the code from leaking into machines like Tegra that don't need the common code.

I'm pretty sure you can drop the xlate() implementations from the functions, though, and those at the patches I acked.

I don't think you need __maybe_unused

static int gpio_find_and_xlate(...)

{
    get ops...

    if (ops->xlate)
       return ops->xlate(....)
    else
       return gpio_default_xlate()...
}

Quoted 3 lines

gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it.

This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) into machines that don't need it.

I can go the route you suggest above, but it will cost the tegra and sandbox builds ~64 bytes ;)

Quoted 1 line

Sure, but we can live with that.

You can avoid that by requiring that ops->xlate always be non-NULL, and simply referencing the default function from drivers that want to use it. Not a big deal either way though.

Hi,

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Hi Eric,

Quoted 1 line · Eric Nelson

Quoted 26 lines

Quoted 24 lines

Quoted 16 lines

Quoted 12 lines

I don't think you need __maybe_unused

static int gpio_find_and_xlate(...)

{
    get ops...

    if (ops->xlate)
       return ops->xlate(....)
    else
       return gpio_default_xlate()...
}

Quoted 3 lines

gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it.

Quoted 6 lines

This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) into machines that don't need it.

I can go the route you suggest above, but it will cost the tegra and sandbox builds ~64 bytes ;)

Sure, but we can live with that.

Quoted 3 lines

You can avoid that by requiring that ops->xlate always be non-NULL, and simply referencing the default function from drivers that want to use it. Not a big deal either way though.

I'd prefer not to do that. It just adds cruft, the removal of which is the purpose of Eric's series.

Quoted 9 lines · Simon Glass

Hi,

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Hi Eric,

Quoted 1 line · Eric Nelson

<snip>

Quoted 26 lines

Quoted 26 lines

Quoted 24 lines

Quoted 16 lines

Quoted 12 lines

I don't think you need __maybe_unused

static int gpio_find_and_xlate(...)

{
    get ops...

    if (ops->xlate)
       return ops->xlate(....)
    else
       return gpio_default_xlate()...
}

Quoted 3 lines

gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it.

Quoted 6 lines

This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) into machines that don't need it.

I can go the route you suggest above, but it will cost the tegra and sandbox builds ~64 bytes ;)

Sure, but we can live with that.

Quoted 7 lines

Quoted 3 lines

You can avoid that by requiring that ops->xlate always be non-NULL, and simply referencing the default function from drivers that want to use it. Not a big deal either way though.

I'd prefer not to do that. It just adds cruft, the removal of which is the purpose of Eric's series.

We must be close to the goal now, since we're picking apart stuff like this!

Since I've done it both ways locally, I'll try to recap.

Requiring an xlate puts a greater demand on the drivers, and requires an extra patch to get Vybrid working (adding xlate to vybrid_gpio.c) but does make it clearer which drivers need updates to handle DT parsing.

There are a lot of them:

        altera_pio
        at91_gpio
        atmel_pio4
        axp_gpio
        bcm2835_gpio
        dwapb_gpio
        gpio-uniphier
        hi6220_gpio
        intel_ich6_gpio
        lpc32xx_gpio
        msm_gpio
        mvebu_gpio
        pm8916_gpio

None of these have dts files in either U-Boot or the kernel, so this doesn't appear to be a problem.

Calling gpio_xlate_offs_flags as done in the V2 I just sent adds 64 bytes of code to the output for all machines, but transparently adds support for machines like vybrid and mxc.

Hi Eric,

Quoted 6 lines · Eric Nelson

Quoted 4 lines · Simon Glass

Quoted 3 lines · Stephen Warren

Quoted 2 lines · Simon Glass

Quoted 1 line · Eric Nelson

Quoted 6 lines

None of these have dts files in either U-Boot or the kernel, so this doesn't appear to be a problem.

Calling gpio_xlate_offs_flags as done in the V2 I just sent adds 64 bytes of code to the output for all machines, but transparently adds support for machines like vybrid and mxc.

Yes that seems OK to me. Can you please send a new version of the series?

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

	<&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

drivers/gpio/gpio-uclass.c +1 -0

 
+int gpio_xlate_offs_flags(struct udevice *dev,

Quoted 12 lines · Eric Nelson

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

    <&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

drivers/gpio/gpio-uclass.c +0 -0

drivers/gpio/gpio-uclass.c int gpio_lookup_name(const char *name, struct udevice **devp, · line 127

Quoted 11 lines

drivers/gpio/gpio-uclass.c +11 -0

+int gpio_xlate_offs_flags(struct udevice *dev,
+                     struct gpio_desc *desc,
+                     struct fdtdec_phandle_args *args)
+{
+    if (args->args_count < 1)
+        return -EINVAL;
+
+    desc->offset = args->args[0];
+
+        if (args->args_count < 2)
+        return 0;

Nit: There's an indentation inconsistency there.

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

        <&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

Signed-off-by Eric Nelson <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

---
V2 removes parsing of offset from the gpio_find_and_xlate routine, and only parses the offset and GPIO_ACTIVE_LOW flag when a driver-specific xlate is unavailable.

V3 re-works tests of the argument count as suggested by Stephen Warren and will allow parsing of nodes with no flags field:

        <&gpio1 2>

V4 fixes an indent with spaces instead of tabs

 drivers/gpio/gpio-uclass.c | 30 +++++++++++++++++++++++-------
 include/asm-generic/gpio.h | 19 ++++++++++++++-----
 2 files changed, 37 insertions(+), 12 deletions(-)

Quoted 22 lines · Eric Nelson

Many drivers use a common form of offset + flags for device tree nodes. e.g.:

        <&gpio1 2 GPIO_ACTIVE_LOW>

This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine.

This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers.

Signed-off-by Eric Nelson <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

---
V2 removes parsing of offset from the gpio_find_and_xlate routine, and only parses the offset and GPIO_ACTIVE_LOW flag when a driver-specific xlate is unavailable.

V3 re-works tests of the argument count as suggested by Stephen Warren and will allow parsing of nodes with no flags field:

        <&gpio1 2>

Quoted 5 lines

V4 fixes an indent with spaces instead of tabs

 drivers/gpio/gpio-uclass.c | 30 +++++++++++++++++++++++-------
 include/asm-generic/gpio.h | 19 ++++++++++++++-----
 2 files changed, 37 insertions(+), 12 deletions(-)

Acked-by Simon Glass <[email protected]>

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the intel_broadwell driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

---
Nothing changed in V2.

 drivers/gpio/intel_broadwell_gpio.c | 10 ----------
 1 file changed, 10 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the omap gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

---
V2 removes the include of <dt-bindings/gpio/gpio.h>

 drivers/gpio/omap_gpio.c | 11 -----------
 1 file changed, 11 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the pic32 gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

Reviewed-by Purna Chandra Mandal <[email protected]>

---
V2 removes the include of <dt-bindings/gpio/gpio.h>

 drivers/gpio/pic32_gpio.c | 10 ----------
 1 file changed, 10 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the Rockchip gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

---
V2 removes the include of <dt-bindings/gpio/gpio.h>

 drivers/gpio/rk_gpio.c | 11 -----------
 1 file changed, 11 deletions(-)

With the addition of GPIO_ACTIVE_LOW parsing in gpio-uclass, the Exynos/S5P gpio driver doesn't need a custom xlate routine.

Signed-off-by Eric Nelson <[email protected]>

Acked-by Simon Glass <[email protected]>

Acked-by Minkyu Kang <[email protected]>

---
V2 removes the include of <dt-bindings/gpio/gpio.h>

 drivers/gpio/s5p_gpio.c | 11 -----------
 1 file changed, 11 deletions(-)