diff mbox

[U-Boot] gpio: generalize for all generic gpio providers

Message ID 1301820218-26319-1-git-send-email-vapier@gentoo.org
State Accepted
Commit a972b8d701814317be2b8bcca4103f37bcbb467c
Delegated to: Wolfgang Denk
Headers show

Commit Message

Mike Frysinger April 3, 2011, 8:43 a.m. UTC 
The Blackfin gpio command isn't terribly Blackfin-specific.  So generalize
the few pieces into two new optional helpers:
	name_to_gpio() - turn a string name into a GPIO #
	gpio_status() - display current pin bindings (think /proc/gpio)

Once these pieces are pulled out, we can relocate the cmd_gpio.c into the
common directory.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 arch/blackfin/cpu/Makefile       |    1 -
 arch/blackfin/cpu/cmd_gpio.c     |  118 --------------------------------------
 arch/blackfin/include/asm/gpio.h |   53 +++++++++++++++++
 common/Makefile                  |    1 +
 common/cmd_gpio.c                |   86 +++++++++++++++++++++++++++
 5 files changed, 140 insertions(+), 119 deletions(-)
 delete mode 100644 arch/blackfin/cpu/cmd_gpio.c
 create mode 100644 common/cmd_gpio.c

Comments

Andreas Pretzsch April 11, 2011, 7:34 p.m. UTC | #1 
Am Sonntag, den 03.04.2011, 04:43 -0400 schrieb Mike Frysinger:
> The Blackfin gpio command isn't terribly Blackfin-specific.  So generalize
> the few pieces into two new optional helpers:
> 	name_to_gpio() - turn a string name into a GPIO #
> 	gpio_status() - display current pin bindings (think /proc/gpio)
> 
> Once these pieces are pulled out, we can relocate the cmd_gpio.c into the
> common directory.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Tested-by: Andreas Pretzsch <apr@cn-eng.de>

Verified on Blackfin BF561 with full port range.


> --- a/arch/blackfin/include/asm/gpio.h
> +++ b/arch/blackfin/include/asm/gpio.h
> @@ -196,6 +196,59 @@ static inline int gpio_is_valid(int number)
>  	return number >= 0 && number < MAX_BLACKFIN_GPIOS;
>  }
>  
> +#include <linux/ctype.h>
> +
> +static inline int name_to_gpio(const char *name)
> +{
> +	int port_base;
> +
> +	if (tolower(*name) == 'p') {
> +		++name;
> +
> +		switch (tolower(*name)) {
> +#ifdef GPIO_PA0
> +		case 'a': port_base = GPIO_PA0; break;
> +#endif
> +#ifdef GPIO_PB0
> +		case 'b': port_base = GPIO_PB0; break;
> +#endif
> +#ifdef GPIO_PC0
> +		case 'c': port_base = GPIO_PC0; break;
> +#endif
> +#ifdef GPIO_PD0
> +		case 'd': port_base = GPIO_PD0; break;
> +#endif
> +#ifdef GPIO_PE0
> +		case 'e': port_base = GPIO_PE0; break;
> +#endif
> +#ifdef GPIO_PF0
> +		case 'f': port_base = GPIO_PF0; break;
> +#endif
> +#ifdef GPIO_PG0
> +		case 'g': port_base = GPIO_PG0; break;
> +#endif
> +#ifdef GPIO_PH0
> +		case 'h': port_base = GPIO_PH0; break;
> +#endif
> +#ifdef GPIO_PI0
> +		case 'i': port_base = GPIO_PI0; break;
> +#endif
> +#ifdef GPIO_PJ
> +		case 'j': port_base = GPIO_PJ0; break;
> +#endif
> +		default:  return -1;
> +		}
> +
> +		++name;
> +	} else
> +		port_base = 0;
> +
> +	return port_base + simple_strtoul(name, NULL, 10);

Remark: Leads to an oom access when exceeding the processor number of
GPIOs, e.g. PF48 on a BF561. IMHO, no problem but only a cosmetic issue,
not worth adding an additional per-cpu check.
In the end, no difference to other user errors like an invalid memory
address.
Mike Frysinger April 12, 2011, 3:14 a.m. UTC | #2 
On Monday, April 11, 2011 15:34:17 Andreas Pretzsch wrote:
> Am Sonntag, den 03.04.2011, 04:43 -0400 schrieb Mike Frysinger:
> > +	return port_base + simple_strtoul(name, NULL, 10);
> 
> Remark: Leads to an oom access when exceeding the processor number of
> GPIOs, e.g. PF48 on a BF561. IMHO, no problem but only a cosmetic issue,
> not worth adding an additional per-cpu check.
> In the end, no difference to other user errors like an invalid memory
> address.

err, oom ?  i guess you mean oob ?

simple to fix by having the return of gpio_request() get checked ...
-mike
Andreas Pretzsch April 12, 2011, 1:13 p.m. UTC | #3 
Am Montag, den 11.04.2011, 23:14 -0400 schrieb Mike Frysinger:
> On Monday, April 11, 2011 15:34:17 Andreas Pretzsch wrote:
> > Am Sonntag, den 03.04.2011, 04:43 -0400 schrieb Mike Frysinger:
> > > +	return port_base + simple_strtoul(name, NULL, 10);
> > 
> > Remark: Leads to an oom access when exceeding the processor number of
> > GPIOs, e.g. PF48 on a BF561. IMHO, no problem but only a cosmetic issue,
> > not worth adding an additional per-cpu check.
> > In the end, no difference to other user errors like an invalid memory
> > address.
> 
> err, oom ?  i guess you mean oob ?

bfin> gpio input PF0
gpio: pin PF0 (gpio 0) value is 1
bfin> gpio input pf47
gpio: pin pf47 (gpio 47) value is 1
bfin> gpio input pf48
DCPLB exception outside of memory map at 0x33000000
[...]
PANIC: Blackfin internal error


> simple to fix by having the return of gpio_request() get checked ...

Works, thanks. With "[U-Boot] [PATCH] gpio: check request result":

bfin> gpio input PF0
gpio: pin PF0 (gpio 0) value is 1
bfin> gpio input pf47
gpio: pin pf47 (gpio 47) value is 1
bfin> gpio input pf48
gpio: requesting pin 48 failed
bfin>
Mike Frysinger April 12, 2011, 4:14 p.m. UTC | #4 
On Tuesday, April 12, 2011 09:13:35 Andreas Pretzsch wrote:
> Am Montag, den 11.04.2011, 23:14 -0400 schrieb Mike Frysinger:
> > On Monday, April 11, 2011 15:34:17 Andreas Pretzsch wrote:
> > > Am Sonntag, den 03.04.2011, 04:43 -0400 schrieb Mike Frysinger:
> > > > +	return port_base + simple_strtoul(name, NULL, 10);
> > > 
> > > Remark: Leads to an oom access when exceeding the processor number of
> > > GPIOs, e.g. PF48 on a BF561. IMHO, no problem but only a cosmetic
> > > issue, not worth adding an additional per-cpu check.
> > > In the end, no difference to other user errors like an invalid memory
> > > address.
> > 
> > err, oom ?  i guess you mean oob ?
> 
> bfin> gpio input PF0
> gpio: pin PF0 (gpio 0) value is 1
> bfin> gpio input pf47
> gpio: pin pf47 (gpio 47) value is 1
> bfin> gpio input pf48
> DCPLB exception outside of memory map at 0x33000000
> [...]
> PANIC: Blackfin internal error

"oom" is usually reserved for "out of memory" as in "memory exhausted"
-mike
diff mbox

Patch

diff --git a/arch/blackfin/cpu/Makefile b/arch/blackfin/cpu/Makefile
index 4a9e577..df10f1b 100644
--- a/arch/blackfin/cpu/Makefile
+++ b/arch/blackfin/cpu/Makefile
@@ -18,7 +18,6 @@  CEXTRA   := initcode.o
 SEXTRA   := start.o
 SOBJS    := interrupt.o cache.o
 COBJS-$(CONFIG_BOOTCOUNT_LIMIT) += bootcount.o
-COBJS-$(CONFIG_CMD_GPIO) += cmd_gpio.o
 COBJS-y  += cpu.o
 COBJS-y  += gpio.o
 COBJS-y  += interrupts.o
diff --git a/arch/blackfin/cpu/cmd_gpio.c b/arch/blackfin/cpu/cmd_gpio.c
deleted file mode 100644
index e96413b..0000000
--- a/arch/blackfin/cpu/cmd_gpio.c
+++ /dev/null
@@ -1,118 +0,0 @@ 
-/*
- * Control GPIO pins on the fly
- *
- * Copyright (c) 2008-2010 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
- */
-
-#include <common.h>
-#include <command.h>
-#include <linux/ctype.h>
-
-#include <asm/blackfin.h>
-#include <asm/gpio.h>
-
-enum {
-	GPIO_INPUT,
-	GPIO_SET,
-	GPIO_CLEAR,
-	GPIO_TOGGLE,
-};
-
-int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	if (argc == 2 && !strcmp(argv[1], "status")) {
-		bfin_gpio_labels();
-		return 0;
-	}
-
-	if (argc != 3)
- show_usage:
-		return cmd_usage(cmdtp);
-
-	/* parse the behavior */
-	ulong sub_cmd;
-	switch (argv[1][0]) {
-		case 'i': sub_cmd = GPIO_INPUT;  break;
-		case 's': sub_cmd = GPIO_SET;    break;
-		case 'c': sub_cmd = GPIO_CLEAR;  break;
-		case 't': sub_cmd = GPIO_TOGGLE; break;
-		default:  goto show_usage;
-	}
-
-	/* parse the pin with format: [p][port]<#> */
-	const char *str_pin = argv[2];
-
-	/* grab the [p]<port> portion */
-	ulong port_base;
-	if (tolower(*str_pin) == 'p') ++str_pin;
-	switch (tolower(*str_pin)) {
-#ifdef GPIO_PA0
-		case 'a': port_base = GPIO_PA0; break;
-#endif
-#ifdef GPIO_PB0
-		case 'b': port_base = GPIO_PB0; break;
-#endif
-#ifdef GPIO_PC0
-		case 'c': port_base = GPIO_PC0; break;
-#endif
-#ifdef GPIO_PD0
-		case 'd': port_base = GPIO_PD0; break;
-#endif
-#ifdef GPIO_PE0
-		case 'e': port_base = GPIO_PE0; break;
-#endif
-#ifdef GPIO_PF0
-		case 'f': port_base = GPIO_PF0; break;
-#endif
-#ifdef GPIO_PG0
-		case 'g': port_base = GPIO_PG0; break;
-#endif
-#ifdef GPIO_PH0
-		case 'h': port_base = GPIO_PH0; break;
-#endif
-#ifdef GPIO_PI0
-		case 'i': port_base = GPIO_PI0; break;
-#endif
-#ifdef GPIO_PJ
-		case 'j': port_base = GPIO_PJ0; break;
-#endif
-		default:  goto show_usage;
-	}
-
-	/* grab the <#> portion */
-	ulong pin = simple_strtoul(str_pin + 1, NULL, 10);
-	if (pin > 15)
-		goto show_usage;
-
-	/* grab the pin before we tweak it */
-	ulong gpio = port_base + pin;
-	gpio_request(gpio, "cmd_gpio");
-
-	/* finally, let's do it: set direction and exec command */
-	ulong value;
-	if (sub_cmd == GPIO_INPUT) {
-		gpio_direction_input(gpio);
-		value = gpio_get_value(gpio);
-	} else {
-		switch (sub_cmd) {
-			case GPIO_SET:    value = 1; break;
-			case GPIO_CLEAR:  value = 0; break;
-			case GPIO_TOGGLE: value = !gpio_get_value(gpio); break;
-			default:          goto show_usage;
-		}
-		gpio_direction_output(gpio, value);
-	}
-	printf("gpio: pin %lu on port %c (gpio %lu) value is %lu\n",
-		pin, *str_pin, gpio, value);
-
-	gpio_free(gpio);
-
-	return value;
-}
-
-U_BOOT_CMD(gpio, 3, 0, do_gpio,
-	"input/set/clear/toggle gpio output pins",
-	"<input|set|clear|toggle> <port><pin>\n"
-	"    - input/set/clear/toggle the specified pin (e.g. PF10)");
diff --git a/arch/blackfin/include/asm/gpio.h b/arch/blackfin/include/asm/gpio.h
index b650ef0..9c0e5d1 100644
--- a/arch/blackfin/include/asm/gpio.h
+++ b/arch/blackfin/include/asm/gpio.h
@@ -196,6 +196,59 @@  static inline int gpio_is_valid(int number)
 	return number >= 0 && number < MAX_BLACKFIN_GPIOS;
 }
 
+#include <linux/ctype.h>
+
+static inline int name_to_gpio(const char *name)
+{
+	int port_base;
+
+	if (tolower(*name) == 'p') {
+		++name;
+
+		switch (tolower(*name)) {
+#ifdef GPIO_PA0
+		case 'a': port_base = GPIO_PA0; break;
+#endif
+#ifdef GPIO_PB0
+		case 'b': port_base = GPIO_PB0; break;
+#endif
+#ifdef GPIO_PC0
+		case 'c': port_base = GPIO_PC0; break;
+#endif
+#ifdef GPIO_PD0
+		case 'd': port_base = GPIO_PD0; break;
+#endif
+#ifdef GPIO_PE0
+		case 'e': port_base = GPIO_PE0; break;
+#endif
+#ifdef GPIO_PF0
+		case 'f': port_base = GPIO_PF0; break;
+#endif
+#ifdef GPIO_PG0
+		case 'g': port_base = GPIO_PG0; break;
+#endif
+#ifdef GPIO_PH0
+		case 'h': port_base = GPIO_PH0; break;
+#endif
+#ifdef GPIO_PI0
+		case 'i': port_base = GPIO_PI0; break;
+#endif
+#ifdef GPIO_PJ
+		case 'j': port_base = GPIO_PJ0; break;
+#endif
+		default:  return -1;
+		}
+
+		++name;
+	} else
+		port_base = 0;
+
+	return port_base + simple_strtoul(name, NULL, 10);
+}
+#define name_to_gpio(n) name_to_gpio(n)
+
+#define gpio_status() bfin_gpio_labels()
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ARCH_BLACKFIN_GPIO_H__ */
diff --git a/common/Makefile b/common/Makefile
index 167612b..2ea0075 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -95,6 +95,7 @@  COBJS-$(CONFIG_CMD_FLASH) += cmd_flash.o
 ifdef CONFIG_FPGA
 COBJS-$(CONFIG_CMD_FPGA) += cmd_fpga.o
 endif
+COBJS-$(CONFIG_CMD_GPIO) += cmd_gpio.o
 COBJS-$(CONFIG_CMD_I2C) += cmd_i2c.o
 COBJS-$(CONFIG_CMD_IDE) += cmd_ide.o
 COBJS-$(CONFIG_CMD_IMMAP) += cmd_immap.o
diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
new file mode 100644
index 0000000..9c9de28
--- /dev/null
+++ b/common/cmd_gpio.c
@@ -0,0 +1,86 @@ 
+/*
+ * Control GPIO pins on the fly
+ *
+ * Copyright (c) 2008-2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <common.h>
+#include <command.h>
+
+#include <asm/gpio.h>
+
+#ifndef name_to_gpio
+#define name_to_gpio(name) simple_strtoul(name, NULL, 10)
+#endif
+
+enum gpio_cmd {
+	GPIO_INPUT,
+	GPIO_SET,
+	GPIO_CLEAR,
+	GPIO_TOGGLE,
+};
+
+static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int gpio;
+	enum gpio_cmd sub_cmd;
+	ulong value;
+	const char *str_cmd, *str_gpio;
+
+#ifdef gpio_status
+	if (argc == 2 && !strcmp(argv[1], "status")) {
+		gpio_status();
+		return 0;
+	}
+#endif
+
+	if (argc != 3)
+ show_usage:
+		return cmd_usage(cmdtp);
+	str_cmd = argv[1];
+	str_gpio = argv[2];
+
+	/* parse the behavior */
+	switch (*str_cmd) {
+		case 'i': sub_cmd = GPIO_INPUT;  break;
+		case 's': sub_cmd = GPIO_SET;    break;
+		case 'c': sub_cmd = GPIO_CLEAR;  break;
+		case 't': sub_cmd = GPIO_TOGGLE; break;
+		default:  goto show_usage;
+	}
+
+	/* turn the gpio name into a gpio number */
+	gpio = name_to_gpio(str_gpio);
+	if (gpio < 0)
+		goto show_usage;
+
+	/* grab the pin before we tweak it */
+	gpio_request(gpio, "cmd_gpio");
+
+	/* finally, let's do it: set direction and exec command */
+	if (sub_cmd == GPIO_INPUT) {
+		gpio_direction_input(gpio);
+		value = gpio_get_value(gpio);
+	} else {
+		switch (sub_cmd) {
+			case GPIO_SET:    value = 1; break;
+			case GPIO_CLEAR:  value = 0; break;
+			case GPIO_TOGGLE: value = !gpio_get_value(gpio); break;
+			default:          goto show_usage;
+		}
+		gpio_direction_output(gpio, value);
+	}
+	printf("gpio: pin %s (gpio %i) value is %lu\n",
+		str_gpio, gpio, value);
+
+	gpio_free(gpio);
+
+	return value;
+}
+
+U_BOOT_CMD(gpio, 3, 0, do_gpio,
+	"input/set/clear/toggle gpio pins",
+	"<input|set|clear|toggle> <pin>\n"
+	"    - input/set/clear/toggle the specified pin");