[U-Boot] [PATCH] 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 eric@nelint.com --- drivers/gpio/gpio-uclass.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..6d30612 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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; }
Hi Eric,
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.
You may need to try this: https://patchwork.ozlabs.org/patch/597363/
Regards, Peng.
Signed-off-by: Eric Nelson eric@nelint.com
drivers/gpio/gpio-uclass.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..6d30612 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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;
}
2.6.2
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Peng,
On 03/28/2016 09:57 PM, Peng Fan wrote:
Hi Eric,
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.
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
As Peng pointed out in [1], GPIO_ACTIVE_LOW is currently being parsed by driver-specific xlate routines, and an NXP/mxc-specific patch ([2]) to do the same on those processors is pending.
Upon further inspection, I found that many arch-specific xlate routines were present only to parse either or both offset and the GPIO_ACTIVE_LOW flag, both of which can be handled at a global level.
This series adds global support for GPIO_ACTIVE_LOW and removes the architecture-specific gpio xlate routine from five drivers.
It also removes the handling of flags in the tegra gpio driver, though a custom xlate is still needed.
[1] - http://lists.denx.de/pipermail/u-boot/2016-March/thread.html#249946 [2] - https://patchwork.ozlabs.org/patch/597363/
Eric Nelson (7): dm: gpio: handle GPIO_ACTIVE_LOW flag in DT gpio: intel_broadwell: remove gpio_xlate routine gpio: omap: remove gpio_xlate routine gpio: pic32: remove gpio_xlate routine gpio: rk: remove gpio_xlate routine gpio: exynos(s5p): remove gpio_xlate routine gpio: tegra: remove flags parsing in xlate routine
drivers/gpio/gpio-uclass.c | 12 ++++++++---- drivers/gpio/intel_broadwell_gpio.c | 10 ---------- drivers/gpio/omap_gpio.c | 10 ---------- drivers/gpio/pic32_gpio.c | 9 --------- drivers/gpio/rk_gpio.c | 10 ---------- drivers/gpio/s5p_gpio.c | 10 ---------- drivers/gpio/tegra_gpio.c | 1 - 7 files changed, 8 insertions(+), 54 deletions(-)
Device tree parsing of GPIO nodes is currently ignoring flags and architecture-specific xlate routines are handling the parsing of GPIO_ACTIVE_LOW.
Since GPIO_ACTIVE_LOW isn't specific to a particular device type, this patch adds support at a global level and removes the need for many of the driver-specific xlate routines.
Signed-off-by: Eric Nelson eric@nelint.com --- drivers/gpio/gpio-uclass.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..a3cbb83 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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,15 @@ static int gpio_find_and_xlate(struct gpio_desc *desc, { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+ desc->offset = -1; + 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]; - else - desc->offset = -1; - desc->flags = 0; + if ((args->args_count > 1) && + (args->args[1] & GPIO_ACTIVE_LOW)) + desc->flags = GPIOD_ACTIVE_LOW; + }
return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; }
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 eric@nelint.com --- drivers/gpio/intel_broadwell_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/intel_broadwell_gpio.c b/drivers/gpio/intel_broadwell_gpio.c index 8cf76f9..81ce446 100644 --- a/drivers/gpio/intel_broadwell_gpio.c +++ b/drivers/gpio/intel_broadwell_gpio.c @@ -162,15 +162,6 @@ static int broadwell_gpio_ofdata_to_platdata(struct udevice *dev) return 0; }
-static int broadwell_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; -} - static const struct dm_gpio_ops gpio_broadwell_ops = { .request = broadwell_gpio_request, .direction_input = broadwell_gpio_direction_input, @@ -178,7 +169,6 @@ static const struct dm_gpio_ops gpio_broadwell_ops = { .get_value = broadwell_gpio_get_value, .set_value = broadwell_gpio_set_value, .get_function = broadwell_gpio_get_function, - .xlate = broadwell_gpio_xlate, };
static const struct udevice_id intel_broadwell_gpio_ids[] = {
On 1 April 2016 at 09:47, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com
drivers/gpio/intel_broadwell_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
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 eric@nelint.com --- drivers/gpio/omap_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c index 93d18e4..5ab53bc 100644 --- a/drivers/gpio/omap_gpio.c +++ b/drivers/gpio/omap_gpio.c @@ -277,22 +277,12 @@ static int omap_gpio_get_function(struct udevice *dev, unsigned offset) return GPIOF_INPUT; }
-static int omap_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; -} - static const struct dm_gpio_ops gpio_omap_ops = { .direction_input = omap_gpio_direction_input, .direction_output = omap_gpio_direction_output, .get_value = omap_gpio_get_value, .set_value = omap_gpio_set_value, .get_function = omap_gpio_get_function, - .xlate = omap_gpio_xlate, };
static int omap_gpio_probe(struct udevice *dev)
On 1 April 2016 at 09:47, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com
drivers/gpio/omap_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
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 eric@nelint.com --- drivers/gpio/pic32_gpio.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpio/pic32_gpio.c b/drivers/gpio/pic32_gpio.c index 499b4fa..6999bc0 100644 --- a/drivers/gpio/pic32_gpio.c +++ b/drivers/gpio/pic32_gpio.c @@ -99,14 +99,6 @@ static int pic32_gpio_direction_output(struct udevice *dev, return 0; }
-static int pic32_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, - struct fdtdec_phandle_args *args) -{ - desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; - - return 0; -} - static int pic32_gpio_get_function(struct udevice *dev, unsigned offset) { int ret = GPIOF_UNUSED; @@ -131,7 +123,6 @@ static const struct dm_gpio_ops gpio_pic32_ops = { .get_value = pic32_gpio_get_value, .set_value = pic32_gpio_set_value, .get_function = pic32_gpio_get_function, - .xlate = pic32_gpio_xlate, };
static int pic32_gpio_probe(struct udevice *dev)
On 1 April 2016 at 09:47, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com
drivers/gpio/pic32_gpio.c | 9 --------- 1 file changed, 9 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
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 eric@nelint.com --- drivers/gpio/rk_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c index 40e87bd..253ccec 100644 --- a/drivers/gpio/rk_gpio.c +++ b/drivers/gpio/rk_gpio.c @@ -98,15 +98,6 @@ static int rockchip_gpio_get_function(struct udevice *dev, unsigned offset) #endif }
-static int rockchip_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; -} - static int rockchip_gpio_probe(struct udevice *dev) { struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); @@ -135,7 +126,6 @@ static const struct dm_gpio_ops gpio_rockchip_ops = { .get_value = rockchip_gpio_get_value, .set_value = rockchip_gpio_set_value, .get_function = rockchip_gpio_get_function, - .xlate = rockchip_gpio_xlate, };
static const struct udevice_id rockchip_gpio_ids[] = {
On 1 April 2016 at 09:47, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com
drivers/gpio/rk_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
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 eric@nelint.com --- drivers/gpio/s5p_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c index 0f22b23..591c036 100644 --- a/drivers/gpio/s5p_gpio.c +++ b/drivers/gpio/s5p_gpio.c @@ -276,22 +276,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; -} - static const struct dm_gpio_ops gpio_exynos_ops = { .direction_input = exynos_gpio_direction_input, .direction_output = exynos_gpio_direction_output, .get_value = exynos_gpio_get_value, .set_value = exynos_gpio_set_value, .get_function = exynos_gpio_get_function, - .xlate = exynos_gpio_xlate, };
static int gpio_exynos_probe(struct udevice *dev)
On 1 April 2016 at 09:47, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com
drivers/gpio/s5p_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
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.
Signed-off-by: Eric Nelson eric@nelint.com --- drivers/gpio/tegra_gpio.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c index 5a03115..7ae1509 100644 --- a/drivers/gpio/tegra_gpio.c +++ b/drivers/gpio/tegra_gpio.c @@ -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;
return 0; }
On 04/01/2016 09:47 AM, Eric Nelson wrote:
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.
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
@@ -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.
Hi Stephen,
On 5 April 2016 at 16:09, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/01/2016 09:47 AM, Eric Nelson wrote:
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.
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
@@ -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.
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.
Regards, Simon
Thanks for the feedback Simon.
On 04/09/2016 11:34 AM, Simon Glass wrote:
On 5 April 2016 at 16:09, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/01/2016 09:47 AM, Eric Nelson wrote:
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.
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
@@ -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.
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).
I see that you've sent some updates, so I'm not sure whether I should send an update on top of your patch set or without it.
Regards,
Eric
On 04/10/2016 07:45 AM, Eric Nelson wrote:
On 04/09/2016 11:34 AM, Simon Glass wrote:
On 5 April 2016 at 16:09, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/01/2016 09:47 AM, Eric Nelson wrote:
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.
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
@@ -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.
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).
While reviewing this, I think I found that the sunxi gpio driver is broken.
The device trees have #gpio-cells=<3>, and gpio declaration like this: <&pio 1 8 GPIO_ACTIVE_HIGH>
but there's no custom xlate routine, the offset is currently going to have what I presume is the bank number.
Hans, can you confirm this?
Please advise,
Eric
As Peng pointed out in [1], GPIO_ACTIVE_LOW is currently being parsed by driver-specific xlate routines, and an NXP/mxc-specific patch ([2]) to do the same on those processors is pending.
This patch series takes a different approach and provides a default routine for xlate that handles the most common case of GPIO device tree node parsing:
<&gpio1 2 GPIO_ACTIVE_LOW>
The first routine adds the default gpio_xlate_offs_flags() routine, and the remainder of the patches remove the driver-specific versions from the intel_broadwell, omap, pic32, rk, and s5p drivers.
V2 of this patch set 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.
V2 also drops the update to the tegra_gpio driver.
Eric Nelson (6): dm: gpio: add a default gpio xlate routine gpio: intel_broadwell: remove gpio_xlate routine gpio: omap: remove gpio_xlate routine gpio: pic32: remove gpio_xlate routine gpio: rk: remove gpio_xlate routine gpio: exynos(s5p): remove gpio_xlate routine
drivers/gpio/gpio-uclass.c | 26 +++++++++++++++++++------- drivers/gpio/intel_broadwell_gpio.c | 10 ---------- drivers/gpio/omap_gpio.c | 11 ----------- drivers/gpio/pic32_gpio.c | 10 ---------- drivers/gpio/rk_gpio.c | 11 ----------- drivers/gpio/s5p_gpio.c | 11 ----------- include/asm-generic/gpio.h | 19 ++++++++++++++----- 7 files changed, 33 insertions(+), 65 deletions(-)
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 eric@nelint.com --- drivers/gpio/gpio-uclass.c | 26 +++++++++++++++++++------- include/asm-generic/gpio.h | 19 ++++++++++++++----- 2 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..6c24e65 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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> @@ -113,19 +114,29 @@ int gpio_lookup_name(const char *name, struct udevice **devp, return 0; }
+int gpio_xlate_offs_flags(struct udevice *dev, + struct gpio_desc *desc, + struct fdtdec_phandle_args *args) +{ + 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; +} + static int gpio_find_and_xlate(struct gpio_desc *desc, struct fdtdec_phandle_args *args) { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
- /* Use the first argument as the offset by default */ - if (args->args_count > 0) - desc->offset = args->args[0]; + if (ops->xlate) + return ops->xlate(desc->dev, desc, args); else - desc->offset = -1; - desc->flags = 0; - - return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; + return gpio_xlate_offs_flags(desc->dev, desc, args); }
int dm_gpio_request(struct gpio_desc *desc, const char *label) @@ -605,6 +616,7 @@ static int _gpio_request_by_name_nodev(const void *blob, int node,
desc->dev = NULL; desc->offset = 0; + desc->flags = 0; ret = fdtdec_parse_phandle_with_args(blob, node, list_name, "#gpio-cells", 0, index, &args); if (ret) { diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 68b5f0b..2500c10 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -207,6 +207,16 @@ int gpio_requestf(unsigned gpio, const char *fmt, ...) struct fdtdec_phandle_args;
/** + * gpio_xlate_offs_flags() - implementation for common use of dm_gpio_ops.xlate + * + * This routine sets the offset field to args[0] and the flags field to + * GPIOD_ACTIVE_LOW if the GPIO_ACTIVE_LOW flag is present in args[1]. + * + */ +int gpio_xlate_offs_flags(struct udevice *dev, struct gpio_desc *desc, + struct fdtdec_phandle_args *args); + +/** * struct struct dm_gpio_ops - Driver model GPIO operations * * Refer to functions above for description. These function largely copy @@ -258,12 +268,11 @@ struct dm_gpio_ops { * * @desc->dev to @dev * @desc->flags to 0 - * @desc->offset to the value of the first argument in args, if any, - * otherwise -1 (which is invalid) + * @desc->offset to 0 * - * This method is optional so if the above defaults suit it can be - * omitted. Typical behaviour is to set up the GPIOD_ACTIVE_LOW flag - * in desc->flags. + * This method is optional and defaults to gpio_xlate_offs_flags, + * which will parse offset and the GPIO_ACTIVE_LOW flag in the first + * two arguments. * * Note that @dev is passed in as a parameter to follow driver model * uclass conventions, even though it is already available as
On 04/11/2016 11:00 AM, Eric Nelson wrote:
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 swarren@nvidia.com
Although one nit below.
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
+int gpio_xlate_offs_flags(struct udevice *dev,
struct gpio_desc *desc,struct fdtdec_phandle_args *args)+{
- 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;
Hi Stephen,
On 04/11/2016 10:16 AM, Stephen Warren wrote:
On 04/11/2016 11:00 AM, Eric Nelson wrote:
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 swarren@nvidia.com
Although one nit below.
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
+int gpio_xlate_offs_flags(struct udevice *dev,
struct gpio_desc *desc,struct fdtdec_phandle_args *args)+{
- 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;
I like it. V3 of that one patch forthcoming.
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 eric@nelint.com Acked-by: Stephen Warren swarren@wwwdotorg.org --- 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>
drivers/gpio/gpio-uclass.c | 30 +++++++++++++++++++++++------- include/asm-generic/gpio.h | 19 ++++++++++++++----- 2 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..3e83cec 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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> @@ -113,19 +114,33 @@ int gpio_lookup_name(const char *name, struct udevice **devp, return 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; + + if (args->args[1] & GPIO_ACTIVE_LOW) + desc->flags = GPIOD_ACTIVE_LOW; + + return 0; +} + static int gpio_find_and_xlate(struct gpio_desc *desc, struct fdtdec_phandle_args *args) { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
- /* Use the first argument as the offset by default */ - if (args->args_count > 0) - desc->offset = args->args[0]; + if (ops->xlate) + return ops->xlate(desc->dev, desc, args); else - desc->offset = -1; - desc->flags = 0; - - return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; + return gpio_xlate_offs_flags(desc->dev, desc, args); }
int dm_gpio_request(struct gpio_desc *desc, const char *label) @@ -605,6 +620,7 @@ static int _gpio_request_by_name_nodev(const void *blob, int node,
desc->dev = NULL; desc->offset = 0; + desc->flags = 0; ret = fdtdec_parse_phandle_with_args(blob, node, list_name, "#gpio-cells", 0, index, &args); if (ret) { diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 68b5f0b..2500c10 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -207,6 +207,16 @@ int gpio_requestf(unsigned gpio, const char *fmt, ...) struct fdtdec_phandle_args;
/** + * gpio_xlate_offs_flags() - implementation for common use of dm_gpio_ops.xlate + * + * This routine sets the offset field to args[0] and the flags field to + * GPIOD_ACTIVE_LOW if the GPIO_ACTIVE_LOW flag is present in args[1]. + * + */ +int gpio_xlate_offs_flags(struct udevice *dev, struct gpio_desc *desc, + struct fdtdec_phandle_args *args); + +/** * struct struct dm_gpio_ops - Driver model GPIO operations * * Refer to functions above for description. These function largely copy @@ -258,12 +268,11 @@ struct dm_gpio_ops { * * @desc->dev to @dev * @desc->flags to 0 - * @desc->offset to the value of the first argument in args, if any, - * otherwise -1 (which is invalid) + * @desc->offset to 0 * - * This method is optional so if the above defaults suit it can be - * omitted. Typical behaviour is to set up the GPIOD_ACTIVE_LOW flag - * in desc->flags. + * This method is optional and defaults to gpio_xlate_offs_flags, + * which will parse offset and the GPIO_ACTIVE_LOW flag in the first + * two arguments. * * Note that @dev is passed in as a parameter to follow driver model * uclass conventions, even though it is already available as
On 04/11/2016 11:01 PM, Eric Nelson wrote:
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 eric@nelint.com Acked-by: Stephen Warren swarren@wwwdotorg.org
Reviewed-by: Purna Chandra Mandal purna.mandal@microchip.com
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- Nothing changed in V2. drivers/gpio/intel_broadwell_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/intel_broadwell_gpio.c b/drivers/gpio/intel_broadwell_gpio.c index 8cf76f9..81ce446 100644 --- a/drivers/gpio/intel_broadwell_gpio.c +++ b/drivers/gpio/intel_broadwell_gpio.c @@ -162,15 +162,6 @@ static int broadwell_gpio_ofdata_to_platdata(struct udevice *dev) return 0; }
-static int broadwell_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; -} - static const struct dm_gpio_ops gpio_broadwell_ops = { .request = broadwell_gpio_request, .direction_input = broadwell_gpio_direction_input, @@ -178,7 +169,6 @@ static const struct dm_gpio_ops gpio_broadwell_ops = { .get_value = broadwell_gpio_get_value, .set_value = broadwell_gpio_set_value, .get_function = broadwell_gpio_get_function, - .xlate = broadwell_gpio_xlate, };
static const struct udevice_id intel_broadwell_gpio_ids[] = {
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/omap_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c index 93d18e4..cd960dc 100644 --- a/drivers/gpio/omap_gpio.c +++ b/drivers/gpio/omap_gpio.c @@ -25,7 +25,6 @@ #include <asm/io.h> #include <asm/errno.h> #include <malloc.h> -#include <dt-bindings/gpio/gpio.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -277,22 +276,12 @@ static int omap_gpio_get_function(struct udevice *dev, unsigned offset) return GPIOF_INPUT; }
-static int omap_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; -} - static const struct dm_gpio_ops gpio_omap_ops = { .direction_input = omap_gpio_direction_input, .direction_output = omap_gpio_direction_output, .get_value = omap_gpio_get_value, .set_value = omap_gpio_set_value, .get_function = omap_gpio_get_function, - .xlate = omap_gpio_xlate, };
static int omap_gpio_probe(struct udevice *dev)
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/pic32_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/pic32_gpio.c b/drivers/gpio/pic32_gpio.c index 499b4fa..7a037f3 100644 --- a/drivers/gpio/pic32_gpio.c +++ b/drivers/gpio/pic32_gpio.c @@ -12,7 +12,6 @@ #include <asm/io.h> #include <asm/gpio.h> #include <linux/compat.h> -#include <dt-bindings/gpio/gpio.h> #include <mach/pic32.h>
DECLARE_GLOBAL_DATA_PTR; @@ -99,14 +98,6 @@ static int pic32_gpio_direction_output(struct udevice *dev, return 0; }
-static int pic32_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, - struct fdtdec_phandle_args *args) -{ - desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; - - return 0; -} - static int pic32_gpio_get_function(struct udevice *dev, unsigned offset) { int ret = GPIOF_UNUSED; @@ -131,7 +122,6 @@ static const struct dm_gpio_ops gpio_pic32_ops = { .get_value = pic32_gpio_get_value, .set_value = pic32_gpio_set_value, .get_function = pic32_gpio_get_function, - .xlate = pic32_gpio_xlate, };
static int pic32_gpio_probe(struct udevice *dev)
On 04/11/2016 10:30 PM, Eric Nelson wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org
Reviewed-by: Purna Chandra Mandal purna.mandal@microchip.com
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/rk_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c index 40e87bd..fefe3ca 100644 --- a/drivers/gpio/rk_gpio.c +++ b/drivers/gpio/rk_gpio.c @@ -16,7 +16,6 @@ #include <asm/io.h> #include <asm/arch/clock.h> #include <dm/pinctrl.h> -#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/clock/rk3288-cru.h>
enum { @@ -98,15 +97,6 @@ static int rockchip_gpio_get_function(struct udevice *dev, unsigned offset) #endif }
-static int rockchip_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; -} - static int rockchip_gpio_probe(struct udevice *dev) { struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); @@ -135,7 +125,6 @@ static const struct dm_gpio_ops gpio_rockchip_ops = { .get_value = rockchip_gpio_get_value, .set_value = rockchip_gpio_set_value, .get_function = rockchip_gpio_get_function, - .xlate = rockchip_gpio_xlate, };
static const struct udevice_id rockchip_gpio_ids[] = {
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/s5p_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c index 0f22b23..377fed4 100644 --- a/drivers/gpio/s5p_gpio.c +++ b/drivers/gpio/s5p_gpio.c @@ -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; -} - static const struct dm_gpio_ops gpio_exynos_ops = { .direction_input = exynos_gpio_direction_input, .direction_output = exynos_gpio_direction_output, .get_value = exynos_gpio_get_value, .set_value = exynos_gpio_set_value, .get_function = exynos_gpio_get_function, - .xlate = exynos_gpio_xlate, };
static int gpio_exynos_probe(struct udevice *dev)
On 12/04/16 02:00, Eric Nelson wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org
V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/s5p_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c index 0f22b23..377fed4 100644 --- a/drivers/gpio/s5p_gpio.c +++ b/drivers/gpio/s5p_gpio.c @@ -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;
-}
static const struct dm_gpio_ops gpio_exynos_ops = { .direction_input = exynos_gpio_direction_input, .direction_output = exynos_gpio_direction_output, .get_value = exynos_gpio_get_value, .set_value = exynos_gpio_set_value, .get_function = exynos_gpio_get_function,
- .xlate = exynos_gpio_xlate,
};
static int gpio_exynos_probe(struct udevice *dev)
Acked-by: Minkyu Kang mk7.kang@samsung.com
Thanks, Minkyu Kang.
Hi Eric,
On Thu, Mar 31, 2016 at 01:41:04PM -0700, Eric Nelson wrote:
Hi Peng,
On 03/28/2016 09:57 PM, Peng Fan wrote:
Hi Eric,
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.
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,
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.
Simon may give more comments on this.
Regards, Peng.
Eric
Hi Peng,
On 04/01/2016 10:46 PM, Peng Fan wrote:
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.
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,
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.
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.
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 way, driver-specific parsing only needs to handle additional flags or needs as is the case with tegra.
Also I do no think delete the xlate implementation is not a good idea.
Which xlate routine? All of those that my patch set removes?
Since none of them do anything besides parsing the offset and GPIO_ACTIVE_LOW flag, this seems like code bloat.
Please advise,
Eric
On 04/02/2016 09:13 AM, Eric Nelson wrote:
Hi Peng,
On 04/01/2016 10:46 PM, Peng Fan wrote:
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.
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,
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.
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.
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,
On 04/02/2016 08:37 PM, Stephen Warren wrote:
On 04/02/2016 09:13 AM, Eric Nelson wrote:
On 04/01/2016 10:46 PM, Peng Fan wrote:
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.
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,
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.
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.
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.
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.
Regards,
Eric
On 04/03/2016 08:07 AM, Eric Nelson wrote:
Hi Stephen and Peng,
On 04/02/2016 08:37 PM, Stephen Warren wrote:
On 04/02/2016 09:13 AM, Eric Nelson wrote:
On 04/01/2016 10:46 PM, Peng Fan wrote:
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.
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,
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.
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.
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.
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.
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.
Hi,
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/03/2016 08:07 AM, Eric Nelson wrote:
Hi Stephen and Peng,
On 04/02/2016 08:37 PM, Stephen Warren wrote:
On 04/02/2016 09:13 AM, Eric Nelson wrote:
On 04/01/2016 10:46 PM, Peng Fan wrote:
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. > > > 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,
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.
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.
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.
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.
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.
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?
Regards, Simon
Hi Simon,
On 04/09/2016 11:33 AM, Simon Glass wrote:
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/03/2016 08:07 AM, Eric Nelson wrote:
On 04/02/2016 08:37 PM, Stephen Warren wrote:
On 04/02/2016 09:13 AM, Eric Nelson wrote:
On 04/01/2016 10:46 PM, Peng Fan wrote:
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>
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?
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,
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/09/2016 11:33 AM, Simon Glass wrote:
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/03/2016 08:07 AM, Eric Nelson wrote:
On 04/02/2016 08:37 PM, Stephen Warren wrote:
On 04/02/2016 09:13 AM, Eric Nelson wrote:
On 04/01/2016 10:46 PM, Peng Fan wrote: > 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>
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?
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.
Regards, Simon
Hi Simon,
On 04/11/2016 07:47 AM, Simon Glass wrote:
Hi Eric,
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/09/2016 11:33 AM, Simon Glass wrote:
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/03/2016 08:07 AM, Eric Nelson wrote:
On 04/02/2016 08:37 PM, Stephen Warren wrote:
On 04/02/2016 09:13 AM, Eric Nelson wrote: > On 04/01/2016 10:46 PM, Peng Fan wrote: >> 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>
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?
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
- 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.
Regards,
Eric
Hi Eric,
On 11 April 2016 at 08:55, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/11/2016 07:47 AM, Simon Glass wrote:
Hi Eric,
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/09/2016 11:33 AM, Simon Glass wrote:
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/03/2016 08:07 AM, Eric Nelson wrote:
On 04/02/2016 08:37 PM, Stephen Warren wrote: > On 04/02/2016 09:13 AM, Eric Nelson wrote: >> On 04/01/2016 10:46 PM, Peng Fan wrote: >>> 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>
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?
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
- 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.
Yes, that's right.
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.
Regards, Simon
Hi Simon,
On 04/11/2016 07:59 AM, Simon Glass wrote:
On 11 April 2016 at 08:55, Eric Nelson eric@nelint.com wrote:
On 04/11/2016 07:47 AM, Simon Glass wrote:
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote:
On 04/09/2016 11:33 AM, Simon Glass wrote:
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/03/2016 08:07 AM, Eric Nelson wrote: > On 04/02/2016 08:37 PM, Stephen Warren wrote: >> On 04/02/2016 09:13 AM, Eric Nelson wrote: >>> On 04/01/2016 10:46 PM, Peng Fan wrote: >>>> 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>
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?
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
- 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.
Yes, that's right.
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.
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,
On 11 April 2016 at 09:10, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/11/2016 07:59 AM, Simon Glass wrote:
On 11 April 2016 at 08:55, Eric Nelson eric@nelint.com wrote:
On 04/11/2016 07:47 AM, Simon Glass wrote:
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote:
On 04/09/2016 11:33 AM, Simon Glass wrote:
On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote: > On 04/03/2016 08:07 AM, Eric Nelson wrote: >> On 04/02/2016 08:37 PM, Stephen Warren wrote: >>> On 04/02/2016 09:13 AM, Eric Nelson wrote: >>>> On 04/01/2016 10:46 PM, Peng Fan wrote: >>>>> 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>
> > 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?
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
- 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.
Yes, that's right.
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.
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.
Regards, Simon
On 04/11/2016 09:12 AM, Simon Glass wrote:
Hi Eric,
On 11 April 2016 at 09:10, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/11/2016 07:59 AM, Simon Glass wrote:
On 11 April 2016 at 08:55, Eric Nelson eric@nelint.com wrote:
On 04/11/2016 07:47 AM, Simon Glass wrote:
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote:
On 04/09/2016 11:33 AM, Simon Glass wrote: > On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org wrote: >> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>> On 04/02/2016 08:37 PM, Stephen Warren wrote: >>>> On 04/02/2016 09:13 AM, Eric Nelson wrote: >>>>> On 04/01/2016 10:46 PM, Peng Fan wrote: >>>>>> 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>
>> >> 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? >
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
- 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.
Yes, that's right.
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.
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.
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,
On 11 April 2016 at 10:10, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/11/2016 09:12 AM, Simon Glass wrote:
Hi Eric,
On 11 April 2016 at 09:10, Eric Nelson eric@nelint.com wrote:
Hi Simon,
On 04/11/2016 07:59 AM, Simon Glass wrote:
On 11 April 2016 at 08:55, Eric Nelson eric@nelint.com wrote:
On 04/11/2016 07:47 AM, Simon Glass wrote:
On 10 April 2016 at 08:48, Eric Nelson eric@nelint.com wrote: > > On 04/09/2016 11:33 AM, Simon Glass wrote: >> >> On 4 April 2016 at 11:50, Stephen Warren swarren@wwwdotorg.org >> wrote: >>> >>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>> >>>> On 04/02/2016 08:37 PM, Stephen Warren wrote: >>>>> >>>>> On 04/02/2016 09:13 AM, Eric Nelson wrote: >>>>>> >>>>>> On 04/01/2016 10:46 PM, Peng Fan wrote: >>>>>>> >>>>>>> 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> > >>> >>> 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? >> > > 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
- 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.
Yes, that's right.
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.
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.
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.
Regards, Simon
On 04/11/2016 09:53 AM, Simon Glass wrote:
Hi,
On 11 April 2016 at 10:10, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/11/2016 09:12 AM, Simon Glass wrote:
Hi Eric,
On 11 April 2016 at 09:10, Eric Nelson eric@nelint.com wrote:
<snip>
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.
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.
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.
Regards,
Eric
Hi Eric,
On 11 April 2016 at 11:17, Eric Nelson eric@nelint.com wrote:
On 04/11/2016 09:53 AM, Simon Glass wrote:
Hi,
On 11 April 2016 at 10:10, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/11/2016 09:12 AM, Simon Glass wrote:
Hi Eric,
On 11 April 2016 at 09:10, Eric Nelson eric@nelint.com wrote:
<snip>
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.
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.
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.
Yes that seems OK to me. Can you please send a new version of the series?
Regards, Simon
Hi Simon,
On 04/20/2016 07:40 AM, Simon Glass wrote:
Hi Eric,
On 11 April 2016 at 11:17, Eric Nelson eric@nelint.com wrote:
On 04/11/2016 09:53 AM, Simon Glass wrote:
On 11 April 2016 at 10:10, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/11/2016 09:12 AM, Simon Glass wrote:
On 11 April 2016 at 09:10, Eric Nelson eric@nelint.com wrote:
<snip>
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?
Sure. I'll re-send shortly.
As Peng pointed out in [1], GPIO_ACTIVE_LOW is currently being parsed by driver-specific xlate routines, and an NXP/mxc-specific patch ([2]) to do the same on those processors is pending.
Upon further inspection, I found that many arch-specific xlate routines were present only to parse either or both offset and the GPIO_ACTIVE_LOW flag, both of which can be handled at a global level.
This series adds global support for GPIO_ACTIVE_LOW and removes the architecture-specific gpio xlate routine from five drivers.
It also removes the handling of flags in the tegra gpio driver, though a custom xlate is still needed.
[1] - http://lists.denx.de/pipermail/u-boot/2016-March/thread.html#249946 [2] - https://patchwork.ozlabs.org/patch/597363/
Eric Nelson (7): dm: gpio: handle GPIO_ACTIVE_LOW flag in DT gpio: intel_broadwell: remove gpio_xlate routine gpio: omap: remove gpio_xlate routine gpio: pic32: remove gpio_xlate routine gpio: rk: remove gpio_xlate routine gpio: exynos(s5p): remove gpio_xlate routine gpio: tegra: remove flags parsing in xlate routine
drivers/gpio/gpio-uclass.c | 12 ++++++++---- drivers/gpio/intel_broadwell_gpio.c | 10 ---------- drivers/gpio/omap_gpio.c | 10 ---------- drivers/gpio/pic32_gpio.c | 9 --------- drivers/gpio/rk_gpio.c | 10 ---------- drivers/gpio/s5p_gpio.c | 10 ---------- drivers/gpio/tegra_gpio.c | 1 - 7 files changed, 8 insertions(+), 54 deletions(-)
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 eric@nelint.com --- 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>
drivers/gpio/gpio-uclass.c | 30 +++++++++++++++++++++++------- include/asm-generic/gpio.h | 19 ++++++++++++++----- 2 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..3e83cec 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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> @@ -113,19 +114,33 @@ int gpio_lookup_name(const char *name, struct udevice **devp, return 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; + + if (args->args[1] & GPIO_ACTIVE_LOW) + desc->flags = GPIOD_ACTIVE_LOW; + + return 0; +} + static int gpio_find_and_xlate(struct gpio_desc *desc, struct fdtdec_phandle_args *args) { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
- /* Use the first argument as the offset by default */ - if (args->args_count > 0) - desc->offset = args->args[0]; + if (ops->xlate) + return ops->xlate(desc->dev, desc, args); else - desc->offset = -1; - desc->flags = 0; - - return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; + return gpio_xlate_offs_flags(desc->dev, desc, args); }
int dm_gpio_request(struct gpio_desc *desc, const char *label) @@ -605,6 +620,7 @@ static int _gpio_request_by_name_nodev(const void *blob, int node,
desc->dev = NULL; desc->offset = 0; + desc->flags = 0; ret = fdtdec_parse_phandle_with_args(blob, node, list_name, "#gpio-cells", 0, index, &args); if (ret) { diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 68b5f0b..2500c10 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -207,6 +207,16 @@ int gpio_requestf(unsigned gpio, const char *fmt, ...) struct fdtdec_phandle_args;
/** + * gpio_xlate_offs_flags() - implementation for common use of dm_gpio_ops.xlate + * + * This routine sets the offset field to args[0] and the flags field to + * GPIOD_ACTIVE_LOW if the GPIO_ACTIVE_LOW flag is present in args[1]. + * + */ +int gpio_xlate_offs_flags(struct udevice *dev, struct gpio_desc *desc, + struct fdtdec_phandle_args *args); + +/** * struct struct dm_gpio_ops - Driver model GPIO operations * * Refer to functions above for description. These function largely copy @@ -258,12 +268,11 @@ struct dm_gpio_ops { * * @desc->dev to @dev * @desc->flags to 0 - * @desc->offset to the value of the first argument in args, if any, - * otherwise -1 (which is invalid) + * @desc->offset to 0 * - * This method is optional so if the above defaults suit it can be - * omitted. Typical behaviour is to set up the GPIOD_ACTIVE_LOW flag - * in desc->flags. + * This method is optional and defaults to gpio_xlate_offs_flags, + * which will parse offset and the GPIO_ACTIVE_LOW flag in the first + * two arguments. * * Note that @dev is passed in as a parameter to follow driver model * uclass conventions, even though it is already available as
On 04/20/2016 09:37 AM, Eric Nelson wrote:
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.
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
+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.
Aside from that, the series, Reviewed-by: Stephen Warren swarren@nvidia.com
Thanks Stephen,
On 04/21/2016 10:03 AM, Stephen Warren wrote:
On 04/20/2016 09:37 AM, Eric Nelson wrote:
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.
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
+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.
I seem to have fat fingered this.
V4 on its way.
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 eric@nelint.com Reviewed-by: Stephen Warren swarren@nvidia.com --- 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(-)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..732b6c2 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -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> @@ -113,19 +114,33 @@ int gpio_lookup_name(const char *name, struct udevice **devp, return 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; + + if (args->args[1] & GPIO_ACTIVE_LOW) + desc->flags = GPIOD_ACTIVE_LOW; + + return 0; +} + static int gpio_find_and_xlate(struct gpio_desc *desc, struct fdtdec_phandle_args *args) { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
- /* Use the first argument as the offset by default */ - if (args->args_count > 0) - desc->offset = args->args[0]; + if (ops->xlate) + return ops->xlate(desc->dev, desc, args); else - desc->offset = -1; - desc->flags = 0; - - return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; + return gpio_xlate_offs_flags(desc->dev, desc, args); }
int dm_gpio_request(struct gpio_desc *desc, const char *label) @@ -605,6 +620,7 @@ static int _gpio_request_by_name_nodev(const void *blob, int node,
desc->dev = NULL; desc->offset = 0; + desc->flags = 0; ret = fdtdec_parse_phandle_with_args(blob, node, list_name, "#gpio-cells", 0, index, &args); if (ret) { diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 68b5f0b..2500c10 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -207,6 +207,16 @@ int gpio_requestf(unsigned gpio, const char *fmt, ...) struct fdtdec_phandle_args;
/** + * gpio_xlate_offs_flags() - implementation for common use of dm_gpio_ops.xlate + * + * This routine sets the offset field to args[0] and the flags field to + * GPIOD_ACTIVE_LOW if the GPIO_ACTIVE_LOW flag is present in args[1]. + * + */ +int gpio_xlate_offs_flags(struct udevice *dev, struct gpio_desc *desc, + struct fdtdec_phandle_args *args); + +/** * struct struct dm_gpio_ops - Driver model GPIO operations * * Refer to functions above for description. These function largely copy @@ -258,12 +268,11 @@ struct dm_gpio_ops { * * @desc->dev to @dev * @desc->flags to 0 - * @desc->offset to the value of the first argument in args, if any, - * otherwise -1 (which is invalid) + * @desc->offset to 0 * - * This method is optional so if the above defaults suit it can be - * omitted. Typical behaviour is to set up the GPIOD_ACTIVE_LOW flag - * in desc->flags. + * This method is optional and defaults to gpio_xlate_offs_flags, + * which will parse offset and the GPIO_ACTIVE_LOW flag in the first + * two arguments. * * Note that @dev is passed in as a parameter to follow driver model * uclass conventions, even though it is already available as
On 24 April 2016 at 17:32, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Reviewed-by: Stephen Warren swarren@nvidia.com
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(-)
Acked-by: Simon Glass sjg@chromium.org
On 27 April 2016 at 09:12, Simon Glass sjg@chromium.org wrote:
On 24 April 2016 at 17:32, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Reviewed-by: Stephen Warren swarren@nvidia.com
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(-)
Acked-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- Nothing changed in V2. drivers/gpio/intel_broadwell_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/intel_broadwell_gpio.c b/drivers/gpio/intel_broadwell_gpio.c index 8cf76f9..81ce446 100644 --- a/drivers/gpio/intel_broadwell_gpio.c +++ b/drivers/gpio/intel_broadwell_gpio.c @@ -162,15 +162,6 @@ static int broadwell_gpio_ofdata_to_platdata(struct udevice *dev) return 0; }
-static int broadwell_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; -} - static const struct dm_gpio_ops gpio_broadwell_ops = { .request = broadwell_gpio_request, .direction_input = broadwell_gpio_direction_input, @@ -178,7 +169,6 @@ static const struct dm_gpio_ops gpio_broadwell_ops = { .get_value = broadwell_gpio_get_value, .set_value = broadwell_gpio_set_value, .get_function = broadwell_gpio_get_function, - .xlate = broadwell_gpio_xlate, };
static const struct udevice_id intel_broadwell_gpio_ids[] = {
On 20 April 2016 at 09:37, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org
Nothing changed in V2. drivers/gpio/intel_broadwell_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
Applied to u-boot-dm, thanks!
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/omap_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c index 93d18e4..cd960dc 100644 --- a/drivers/gpio/omap_gpio.c +++ b/drivers/gpio/omap_gpio.c @@ -25,7 +25,6 @@ #include <asm/io.h> #include <asm/errno.h> #include <malloc.h> -#include <dt-bindings/gpio/gpio.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -277,22 +276,12 @@ static int omap_gpio_get_function(struct udevice *dev, unsigned offset) return GPIOF_INPUT; }
-static int omap_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; -} - static const struct dm_gpio_ops gpio_omap_ops = { .direction_input = omap_gpio_direction_input, .direction_output = omap_gpio_direction_output, .get_value = omap_gpio_get_value, .set_value = omap_gpio_set_value, .get_function = omap_gpio_get_function, - .xlate = omap_gpio_xlate, };
static int omap_gpio_probe(struct udevice *dev)
On 20 April 2016 at 09:37, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org
V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/omap_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
Applied to u-boot-dm, thanks!
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org Reviewed-by: Purna Chandra Mandal purna.mandal@microchip.com --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/pic32_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/drivers/gpio/pic32_gpio.c b/drivers/gpio/pic32_gpio.c index 499b4fa..7a037f3 100644 --- a/drivers/gpio/pic32_gpio.c +++ b/drivers/gpio/pic32_gpio.c @@ -12,7 +12,6 @@ #include <asm/io.h> #include <asm/gpio.h> #include <linux/compat.h> -#include <dt-bindings/gpio/gpio.h> #include <mach/pic32.h>
DECLARE_GLOBAL_DATA_PTR; @@ -99,14 +98,6 @@ static int pic32_gpio_direction_output(struct udevice *dev, return 0; }
-static int pic32_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, - struct fdtdec_phandle_args *args) -{ - desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; - - return 0; -} - static int pic32_gpio_get_function(struct udevice *dev, unsigned offset) { int ret = GPIOF_UNUSED; @@ -131,7 +122,6 @@ static const struct dm_gpio_ops gpio_pic32_ops = { .get_value = pic32_gpio_get_value, .set_value = pic32_gpio_set_value, .get_function = pic32_gpio_get_function, - .xlate = pic32_gpio_xlate, };
static int pic32_gpio_probe(struct udevice *dev)
On 20 April 2016 at 09:37, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org Reviewed-by: Purna Chandra Mandal purna.mandal@microchip.com
V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/pic32_gpio.c | 10 ---------- 1 file changed, 10 deletions(-)
Applied to u-boot-dm, thanks!
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/rk_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c index 40e87bd..fefe3ca 100644 --- a/drivers/gpio/rk_gpio.c +++ b/drivers/gpio/rk_gpio.c @@ -16,7 +16,6 @@ #include <asm/io.h> #include <asm/arch/clock.h> #include <dm/pinctrl.h> -#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/clock/rk3288-cru.h>
enum { @@ -98,15 +97,6 @@ static int rockchip_gpio_get_function(struct udevice *dev, unsigned offset) #endif }
-static int rockchip_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; -} - static int rockchip_gpio_probe(struct udevice *dev) { struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); @@ -135,7 +125,6 @@ static const struct dm_gpio_ops gpio_rockchip_ops = { .get_value = rockchip_gpio_get_value, .set_value = rockchip_gpio_set_value, .get_function = rockchip_gpio_get_function, - .xlate = rockchip_gpio_xlate, };
static const struct udevice_id rockchip_gpio_ids[] = {
On 20 April 2016 at 09:37, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org
V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/rk_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
Applied to u-boot-dm, thanks!
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org Acked-by: Minkyu Kang mk7.kang@samsung.com --- V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/s5p_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c index 0f22b23..377fed4 100644 --- a/drivers/gpio/s5p_gpio.c +++ b/drivers/gpio/s5p_gpio.c @@ -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; -} - static const struct dm_gpio_ops gpio_exynos_ops = { .direction_input = exynos_gpio_direction_input, .direction_output = exynos_gpio_direction_output, .get_value = exynos_gpio_get_value, .set_value = exynos_gpio_set_value, .get_function = exynos_gpio_get_function, - .xlate = exynos_gpio_xlate, };
static int gpio_exynos_probe(struct udevice *dev)
On 20 April 2016 at 09:37, Eric Nelson eric@nelint.com wrote:
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 eric@nelint.com Acked-by: Simon Glass sjg@chromium.org Acked-by: Minkyu Kang mk7.kang@samsung.com
V2 removes the include of <dt-bindings/gpio/gpio.h> drivers/gpio/s5p_gpio.c | 11 ----------- 1 file changed, 11 deletions(-)
Applied to u-boot-dm, thanks!
participants (6)
-
Eric Nelson -
Minkyu Kang -
Peng Fan -
Purna Chandra Mandal -
Simon Glass -
Stephen Warren