From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Tue, 22 Nov 2011 19:28:10 +0100
Subject: [U-Boot] [PATCH] M28: Added pin name support in GPIO driver
In-Reply-To: <B0658F55E67EDE4A914644632835B2CAF7F1E5@DEUTERIUM.delien.local>
References: <B0658F55E67EDE4A914644632835B2CAF7F1E5@DEUTERIUM.delien.local>
Message-ID: <201111221928.11095.marek.vasut@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

> 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.
> 
> The user's manual refers to pins by the bank number they're in and their
> number within that bank. With this patch the friendly naming convention to
> address pin number 5 in bank 3 is b3p5. But names like B00000003p005 are
> interpreted correctly too.
> 
> 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 be1c944..5ae66e6 100644
> --- a/arch/arm/include/asm/arch-mx28/gpio.h
> +++ b/arch/arm/include/asm/arch-mx28/gpio.h

No, move this to mxs_gpio.c and simply make the function not static.

> @@ -23,8 +23,44 @@
>  #ifndef        __MX28_GPIO_H__
>  #define        __MX28_GPIO_H__
> 
> +#include <linux/ctype.h>
> +#include <asm/errno.h>
> +#include <asm/arch/iomux.h>
> +
>  #ifdef CONFIG_MXS_GPIO
>  void mxs_gpio_init(void);
> +
> +static inline int name_to_gpio(const char *name)
> +{
> +       int     pin_nr;
> +
> +       if (name[0] >= '0' && name[0] <= '9') {

So if I pass name == NULL, we're done for here ;-)

> +               pin_nr = simple_strtoul(name, (char **)&name, 10);
> +               if (name[0])
> +                       return -EINVAL;
> +               else
> +                       return pin_nr;
> +       }
> +
> +       if (tolower(name[0]) == 'b') {
> +               name++;
> +               pin_nr = (simple_strtoul(name, (char **)&name, 10) <<
> MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK; +       } else
> +               return -EINVAL;

Braces missing around this return statement. Also, why not do something like:

if (tolower(name[0]) != 'b')
	return -EINVAL;

name++;
pinnr = ...

if (tolower(name[0]) != 'p')
	return -EINVAL;

name++;
...

It seems more explicit to me that way.

> +
> +       if (tolower(name[0]) == 'p') {
> +               name++;
> +               pin_nr |= (simple_strtoul(name, (char **)&name, 10) <<
> MXS_PAD_PIN_SHIFT) & MXS_PAD_PIN_MASK; +       } else
> +               return -EINVAL;
> +
> +       if (name[0])
> +               return -EINVAL;
> +
> +       return pin_nr;
> +}
> +#define name_to_gpio(n) name_to_gpio(n)

What's this define for ?

> +
>  #else
>  inline void mxs_gpio_init(void) {}
>  #endif

Do you even need this if CONFIG_CMD_GPIO is undefined? Move the function to 
mxs_gpio.c and make it not static should work for you.

M