[U-Boot] [PATCH v2] M28: Added pin name support in GPIO driver

[U-Boot] [PATCH v2] M28: Added pin name support in GPIO driver

[Robert Deliën]

This patch adds adds pin name support in the GPIO driver. With this patch applied, the gpio command supports pins to be addressed with friendly names.

Please note:
Function gpio_name_to_pin checks for set bits getting lost in translation and returns -EINVAL if this occurs, but a correctly translated name doesn't automatically produce a valid pin. It only means the bank/pin numbers fit the designated bitfields. After successful translation, validity needs to be checked separately.

Signed-off-by: Robert Deli?n <robert@delien.nl>

diff --git a/arch/arm/include/asm/arch-mx28/gpio.h b/arch/arm/include/asm/arch-mx28/gpio.h
index 36f3be8..1eb54ee 100644
--- a/arch/arm/include/asm/arch-mx28/gpio.h
+++ b/arch/arm/include/asm/arch-mx28/gpio.h
@@ -26,6 +26,9 @@
#ifdef	CONFIG_MXS_GPIO
void mxs_gpio_init(void);

+extern int gpio_name_to_pin(const char *name);
+#define gpio_name_to_pin(name)	gpio_name_to_pin(name)
+
extern int gpio_invalid(int gp);
#define gpio_invalid(gpio)	gpio_invalid(gpio)

diff --git a/arch/blackfin/include/asm/gpio.h b/arch/blackfin/include/asm/gpio.h
index 224688f..8128df1 100644
--- a/arch/blackfin/include/asm/gpio.h
+++ b/arch/blackfin/include/asm/gpio.h
@@ -1,7 +1,24 @@
/*
- * Copyright 2006-2009 Analog Devices Inc.
+ * (C) Copyright 2006-2009
+ * Analog Devices Inc.
 *
- * Licensed under the GPL-2 or later.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
 */

#ifndef __ARCH_BLACKFIN_GPIO_H__
@@ -199,7 +216,7 @@ static inline int gpio_is_valid(int number)

#include <linux/ctype.h>

-static inline int name_to_gpio(const char *name)
+static inline int gpio_name_to_pin(const char *name)
{
	int port_base;

@@ -246,7 +263,7 @@ static inline int name_to_gpio(const char *name)

	return port_base + simple_strtoul(name, NULL, 10);
}
-#define name_to_gpio(n) name_to_gpio(n)
+#define gpio_name_to_pin(n) gpio_name_to_pin(n)

#define gpio_status() bfin_gpio_labels()

diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
index f81c7d8..d214f7a 100644
--- a/common/cmd_gpio.c
+++ b/common/cmd_gpio.c
@@ -1,9 +1,24 @@
/*
- * Control GPIO pins on the fly
+ * (C) Copyright 2008-2011
+ * Analog Devices Inc.
 *
- * Copyright (c) 2008-2011 Analog Devices Inc.
+ * See file CREDITS for list of people who contributed to this
+ * project.
 *
- * Licensed under the GPL-2 or later.
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
 */

#include <common.h>
@@ -11,8 +26,8 @@

#include <asm/gpio.h>

-#ifndef name_to_gpio
-#define name_to_gpio(name) simple_strtoul(name, NULL, 10)
+#ifndef gpio_name_to_pin
+#define gpio_name_to_pin(name)	simple_strtoul(name, NULL, 10)
#endif

#ifndef gpio_invalid
@@ -36,13 +51,13 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
#ifdef gpio_status
	if (argc == 2 && !strcmp(argv[1], "status")) {
		gpio_status();
-		return 0;
+		return (0);
	}
#endif

	if (argc != 3)
- show_usage:
-		return cmd_usage(cmdtp);
+		goto show_usage;
+
	str_cmd = argv[1];
	str_gpio = argv[2];

@@ -56,9 +71,11 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
	}

	/* turn the gpio name into a gpio number */
-	gpio = name_to_gpio(str_gpio);
-	if (gpio < 0)
-		goto show_usage;
+	gpio = gpio_name_to_pin(str_gpio);
+	if (gpio < 0) {
+		printf("gpio: unknown pin %s\n", str_gpio);
+		return (-1);
+	}

	/* check bank and pin number for validity */
	if (gpio_invalid(gpio)) {
@@ -69,7 +86,7 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
	/* grab the pin before we tweak it */
	if (gpio_request(gpio, "cmd_gpio")) {
		printf("gpio: requesting pin %u failed\n", gpio);
-		return -1;
+		return (-1);
	}

	/* finally, let's do it: set direction and exec command */
@@ -90,7 +107,9 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

	gpio_free(gpio);

-	return value;
+	return (value);
+show_usage:
}

U_BOOT_CMD(gpio, 3, 0, do_gpio,
diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
index d2e9bac..50a0e6a 100644
--- a/drivers/gpio/mxs_gpio.c
+++ b/drivers/gpio/mxs_gpio.c
@@ -23,6 +23,7 @@
 * MA 02111-1307 USA
 */

+#include <linux/ctype.h>
#include <common.h>
#include <netdev.h>
#include <asm/errno.h>
@@ -81,6 +82,45 @@ void mxs_gpio_init(void)
	}
}

+int gpio_name_to_pin(const char *name)
+{
+	int bank;
+	int pin;
+
+	if (!name)
+		return (-EINVAL);
+
+	if (name[0] >= '0' && name[0] <= '9') {
+		pin = simple_strtoul(name, (char **)&name, 10);
+		if (name[0])
+			return (-EINVAL);
+		if (pin & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK))
+			return (-EINVAL);
+
+		return (pin);
+	}
+
+	if (tolower(name[0]) != 'b')
+		return (-EINVAL);
+	name++;
+	bank = simple_strtoul(name, (char **)&name, 10);
+	if (bank & ~(MXS_PAD_BANK_MASK >> MXS_PAD_BANK_SHIFT))
+		return (-EINVAL);
+
+	if (tolower(name[0]) != 'p')
+		return (-EINVAL);
+	name++;
+	pin = simple_strtoul(name, (char **)&name, 10);
+	if (pin & ~(MXS_PAD_PIN_MASK >> MXS_PAD_PIN_SHIFT))
+		return (-EINVAL);
+
+	if (name[0])
+		return (-EINVAL);
+
+	return ( ((bank << MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK) |
+		 ((pin  << MXS_PAD_PIN_SHIFT ) & MXS_PAD_PIN_MASK ) );
+}
+
int gpio_invalid(int gp)
{
	if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||

[U-Boot] [PATCH v2] M28: Added pin name support in GPIO driver

[Marek Vasut]

> This patch adds adds 

adds adds ?

> pin name support in the GPIO driver. With this patch
> applied, the gpio command supports pins to be addressed with friendly
> names.
> 
> Please note:
> Function gpio_name_to_pin checks for set bits getting lost in translation
> and returns -EINVAL if this occurs, but a correctly translated name
> doesn't automatically produce a valid pin. It only means the bank/pin
> numbers fit the designated bitfields. After successful translation,
> validity needs to be checked separately.
> 
> Signed-off-by: Robert Deli?n <robert@delien.nl>
> 
> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h
> b/arch/arm/include/asm/arch-mx28/gpio.h index 36f3be8..1eb54ee 100644
> --- a/arch/arm/include/asm/arch-mx28/gpio.h
> +++ b/arch/arm/include/asm/arch-mx28/gpio.h
> @@ -26,6 +26,9 @@
> #ifdef	CONFIG_MXS_GPIO
> void mxs_gpio_init(void);
> 
> +extern int gpio_name_to_pin(const char *name);
> +#define gpio_name_to_pin(name)	gpio_name_to_pin(name)
> +

Wasn't this called differently in the previous patch?

> extern int gpio_invalid(int gp);
> #define gpio_invalid(gpio)	gpio_invalid(gpio)
> 
> diff --git a/arch/blackfin/include/asm/gpio.h
> b/arch/blackfin/include/asm/gpio.h index 224688f..8128df1 100644
> --- a/arch/blackfin/include/asm/gpio.h
> +++ b/arch/blackfin/include/asm/gpio.h
> @@ -1,7 +1,24 @@
> /*
> - * Copyright 2006-2009 Analog Devices Inc.
> + * (C) Copyright 2006-2009
> + * Analog Devices Inc.
>  *
> - * Licensed under the GPL-2 or later.
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
>  */
> 
> #ifndef __ARCH_BLACKFIN_GPIO_H__
> @@ -199,7 +216,7 @@ static inline int gpio_is_valid(int number)
> 
> #include <linux/ctype.h>
> 
> -static inline int name_to_gpio(const char *name)
> +static inline int gpio_name_to_pin(const char *name)
> {
> 	int port_base;
> 
> @@ -246,7 +263,7 @@ static inline int name_to_gpio(const char *name)
> 
> 	return port_base + simple_strtoul(name, NULL, 10);
> }
> -#define name_to_gpio(n) name_to_gpio(n)
> +#define gpio_name_to_pin(n) gpio_name_to_pin(n)

I see, make it a series, don't intermix such changes in one patch.

> 
> #define gpio_status() bfin_gpio_labels()
> 
> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
> index f81c7d8..d214f7a 100644
> --- a/common/cmd_gpio.c
> +++ b/common/cmd_gpio.c
> @@ -1,9 +1,24 @@
> /*
> - * Control GPIO pins on the fly
> + * (C) Copyright 2008-2011
> + * Analog Devices Inc.
>  *
> - * Copyright (c) 2008-2011 Analog Devices Inc.
> + * See file CREDITS for list of people who contributed to this
> + * project.
>  *
> - * Licensed under the GPL-2 or later.
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
>  */
> 
> #include <common.h>
> @@ -11,8 +26,8 @@
> 
> #include <asm/gpio.h>
> 
> -#ifndef name_to_gpio
> -#define name_to_gpio(name) simple_strtoul(name, NULL, 10)
> +#ifndef gpio_name_to_pin
> +#define gpio_name_to_pin(name)	simple_strtoul(name, NULL, 10)
> #endif
> 
> #ifndef gpio_invalid
> @@ -36,13 +51,13 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) #ifdef gpio_status
> 	if (argc == 2 && !strcmp(argv[1], "status")) {
> 		gpio_status();
> -		return 0;
> +		return (0);
> 	}
> #endif
> 
> 	if (argc != 3)
> - show_usage:
> -		return cmd_usage(cmdtp);
> +		goto show_usage;
> +
> 	str_cmd = argv[1];
> 	str_gpio = argv[2];
> 
> @@ -56,9 +71,11 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[]) }
> 
> 	/* turn the gpio name into a gpio number */
> -	gpio = name_to_gpio(str_gpio);
> -	if (gpio < 0)
> -		goto show_usage;
> +	gpio = gpio_name_to_pin(str_gpio);
> +	if (gpio < 0) {
> +		printf("gpio: unknown pin %s\n", str_gpio);
> +		return (-1);
> +	}
> 
> 	/* check bank and pin number for validity */
> 	if (gpio_invalid(gpio)) {
> @@ -69,7 +86,7 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[]) /* grab the pin before we tweak it */
> 	if (gpio_request(gpio, "cmd_gpio")) {
> 		printf("gpio: requesting pin %u failed\n", gpio);
> -		return -1;
> +		return (-1);
> 	}
> 
> 	/* finally, let's do it: set direction and exec command */
> @@ -90,7 +107,9 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[])
> 
> 	gpio_free(gpio);
> 
> -	return value;
> +	return (value);
> +show_usage:
> }
> 
> U_BOOT_CMD(gpio, 3, 0, do_gpio,
> diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
> index d2e9bac..50a0e6a 100644
> --- a/drivers/gpio/mxs_gpio.c
> +++ b/drivers/gpio/mxs_gpio.c
> @@ -23,6 +23,7 @@
>  * MA 02111-1307 USA
>  */
> 
> +#include <linux/ctype.h>
> #include <common.h>
> #include <netdev.h>
> #include <asm/errno.h>
> @@ -81,6 +82,45 @@ void mxs_gpio_init(void)
> 	}
> }
> 
> +int gpio_name_to_pin(const char *name)
> +{
> +	int bank;
> +	int pin;
> +
> +	if (!name)
> +		return (-EINVAL);
> +
> +	if (name[0] >= '0' && name[0] <= '9') {
> +		pin = simple_strtoul(name, (char **)&name, 10);
> +		if (name[0])
> +			return (-EINVAL);
> +		if (pin & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK))
> +			return (-EINVAL);
> +
> +		return (pin);
> +	}
> +
> +	if (tolower(name[0]) != 'b')
> +		return (-EINVAL);
> +	name++;
> +	bank = simple_strtoul(name, (char **)&name, 10);
> +	if (bank & ~(MXS_PAD_BANK_MASK >> MXS_PAD_BANK_SHIFT))
> +		return (-EINVAL);
> +
> +	if (tolower(name[0]) != 'p')
> +		return (-EINVAL);
> +	name++;
> +	pin = simple_strtoul(name, (char **)&name, 10);
> +	if (pin & ~(MXS_PAD_PIN_MASK >> MXS_PAD_PIN_SHIFT))
> +		return (-EINVAL);
> +
> +	if (name[0])
> +		return (-EINVAL);

Why the parenthesis ?
> +
> +	return ( ((bank << MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK) |
> +		 ((pin  << MXS_PAD_PIN_SHIFT ) & MXS_PAD_PIN_MASK ) );
> +}
> +
> int gpio_invalid(int gp)
> {
> 	if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||

This was there before or are we missing some previous commit here ?

M

[U-Boot] [PATCH v2] M28: Added pin name support in GPIO driver

[Robert Deliën]

>> +
>> +	if (name[0])
>> +		return (-EINVAL);
> 
> Why the parenthesis ?

Because you asked for them? But OK, I'll remove them again...

>> +
>> +	return ( ((bank << MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK) |
>> +		 ((pin  << MXS_PAD_PIN_SHIFT ) & MXS_PAD_PIN_MASK ) );
>> +}
>> +
>> int gpio_invalid(int gp)
>> {
>> 	if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||
> 
> This was there before or are we missing some previous commit here ?

To apply cleanly, this patch needs the pin validity patch applied first. However, it will apply without it, yet not cleanly.

[U-Boot] [PATCH v2] M28: Added pin name support in GPIO driver

[Marek Vasut]

> >> +
> >> +	if (name[0])
> >> +		return (-EINVAL);
> > 
> > Why the parenthesis ?
> 
> Because you asked for them? But OK, I'll remove them again...

I asked you to add braces around the else part of the branch!
if { ... } else { ... } ... this stuff. Sorry for the confusion!

> 
> >> +
> >> +	return ( ((bank << MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK) |
> >> +		 ((pin  << MXS_PAD_PIN_SHIFT ) & MXS_PAD_PIN_MASK ) );
> >> +}
> >> +
> >> int gpio_invalid(int gp)
> >> {
> >> 
> >> 	if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||
> > 
> > This was there before or are we missing some previous commit here ?
> 
> To apply cleanly, this patch needs the pin validity patch applied first.
> However, it will apply without it, yet not cleanly.

I think I didn't see the pin validity patch then.

[U-Boot] [PATCH v2] M28: Added pin name support in GPIO driver

[Robert Deliën]

> I asked you to add braces around the else part of the branch!
> if { ... } else { ... } ... this stuff. Sorry for the confusion!

Ah; That makes sense now. No worries.

> I think I didn't see the pin validity patch then.

Hm, perhaps I neglected to CC you...