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

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 at delien.nl>

> 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 at 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

>> +
>> +	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.

> >> +
> >> +	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.

> 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...