[U-Boot] [PATCH v3] M28: GPIO pin validity check added

[U-Boot] [PATCH v3] M28: GPIO pin validity check added

[Robert Deliën]

This patch adds a validity check for GPIO pins to prevent clobbering reserved bit or even registers, per suggestion of Marek Vasut.

Please note:
1. gpio_request no longer checks validity. Pin validity must be checked explicitly use gpio_invalid prior to request and hence use. Previous validity check in gpio_request was incomplete anyway.
2. MX233 is not supported yet. Until it is, macros defining the number of pins for each bank reside in arch/arm/include/asm/arch-mx28/iomux.h

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

diff --git a/arch/arm/include/asm/arch-mx28/gpio.h b/arch/arm/include/asm/arch-mx28/gpio.h
index be1c944..5abd7b1 100644
--- a/arch/arm/include/asm/arch-mx28/gpio.h
+++ b/arch/arm/include/asm/arch-mx28/gpio.h
@@ -25,6 +25,10 @@

 #ifdef CONFIG_MXS_GPIO
 void mxs_gpio_init(void);
+
+extern int gpio_invalid(int gp);
+#define gpio_invalid(gpio) gpio_invalid(gpio)
+
 #else
 inline void mxs_gpio_init(void) {}
 #endif
diff --git a/arch/arm/include/asm/arch-mx28/iomux.h b/arch/arm/include/asm/arch-mx28/iomux.h
index 7abdf58..2326fdb 100644
--- a/arch/arm/include/asm/arch-mx28/iomux.h
+++ b/arch/arm/include/asm/arch-mx28/iomux.h
@@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t;
 #define MXS_PAD_PULL_VALID_SHIFT 16
 #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << MXS_PAD_PULL_VALID_SHIFT)

+#define MX28_BANK0_PINS 29
+#define MX28_BANK1_PINS 32
+#define MX28_BANK2_PINS 28
+#define MX28_BANK3_PINS 31
+#define MX28_BANK4_PINS 21
+
+#define MX23_BANK0_PINS 32
+#define MX23_BANK1_PINS 31
+#define MX23_BANK2_PINS 32
+
 #define PAD_MUXSEL_0 0
 #define PAD_MUXSEL_1 1
 #define PAD_MUXSEL_2 2
diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
index 9cc790a..51e11e7 100644
--- a/common/cmd_gpio.c
+++ b/common/cmd_gpio.c
@@ -15,6 +15,10 @@
 #define name_to_gpio(name) simple_strtoul(name, NULL, 10)
 #endif

+#ifndef gpio_invalid
+#define gpio_invalid(gpio) (0)
+#endif
+
 enum gpio_cmd {
  GPIO_INPUT,
  GPIO_SET,
@@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  if (gpio < 0)
  goto show_usage;

+ /* check bank and pin number for validity */
+ if (gpio_invalid(gpio)) {
+ printf("gpio: invalid pin %u\n", gpio);
+ return -1;
+ }
+
  /* grab the pin before we tweak it */
  if (gpio_request(gpio, "cmd_gpio")) {
  printf("gpio: requesting pin %u failed\n", gpio);
diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
index b7e9591..dabee90 100644
--- a/drivers/gpio/mxs_gpio.c
+++ b/drivers/gpio/mxs_gpio.c
@@ -31,7 +31,6 @@
 #include <asm/arch/imx-regs.h>

 #if defined(CONFIG_MX23)
-#define PINCTRL_BANKS 3
 #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10))
 #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10))
 #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10))
@@ -39,7 +38,6 @@
 #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10))
 #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10))
 #elif defined(CONFIG_MX28)
-#define PINCTRL_BANKS 5
 #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10))
 #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10))
 #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10))
@@ -57,11 +55,25 @@
 #define GPIO_INT_LEV_MASK (1 << 0)
 #define GPIO_INT_POL_MASK (1 << 1)

+static const int mxs_bank_pins[] = {
+#if defined(CONFIG_MX23)
+ MX23_BANK0_PINS,
+ MX23_BANK1_PINS,
+ MX23_BANK2_PINS
+#elif defined(CONFIG_MX28)
+ MX28_BANK0_PINS,
+ MX28_BANK1_PINS,
+ MX28_BANK2_PINS,
+ MX28_BANK3_PINS,
+ MX28_BANK4_PINS
+#endif
+};
+
 void mxs_gpio_init(void)
 {
  int i;

- for (i = 0; i < PINCTRL_BANKS; i++) {
+ for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) {
  writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i));
  writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i));
  /* Use SCT address here to clear the IRQSTAT bits */
@@ -69,6 +81,16 @@ void mxs_gpio_init(void)
  }
 }

+int gpio_invalid(int gp)
+{
+ if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||
+     (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) ||
+     (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) )
+ return -EINVAL;
+
+ return 0;
+}
+
 int gpio_get_value(int gp)
 {
  uint32_t bank = PAD_BANK(gp);
@@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value)

 int gpio_request(int gp, const char *label)
 {
- if (PAD_BANK(gp) > PINCTRL_BANKS)
- return -EINVAL;
-
  return 0;
 }

[U-Boot] [PATCH v3] M28: GPIO pin validity check added

[Mike Frysinger]

this one is mangled as well
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111123/cd65f789/attachment.pgp>

[U-Boot] [PATCH v3] M28: GPIO pin validity check added

[Marek Vasut]

> This patch adds a validity check for GPIO pins to prevent clobbering
> reserved bit or even registers, per suggestion of Marek Vasut.
> 
> Please note:
> 1. gpio_request no longer checks validity. Pin validity must be checked
> explicitly use gpio_invalid prior to request and hence use. Previous
> validity check in gpio_request was incomplete anyway. 2. MX233 is not
> supported yet. Until it is, macros defining the number of pins for each
> bank reside in arch/arm/include/asm/arch-mx28/iomux.h
> 
> Signed-off-by: Robert Deli?n <robert@delien.nl<http://delien.nl/>>

AARGH! Please add the following lines to the commit message:

Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>

!!!!

That way, you won't have to care for the Cc, which you forgot again!
> 
> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h
> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644
> --- a/arch/arm/include/asm/arch-mx28/gpio.h
> +++ b/arch/arm/include/asm/arch-mx28/gpio.h
> @@ -25,6 +25,10 @@
> 
>  #ifdef CONFIG_MXS_GPIO
>  void mxs_gpio_init(void);
> +
> +extern int gpio_invalid(int gp);
> +#define gpio_invalid(gpio) gpio_invalid(gpio)
> +

Please, gpio_is_valid() and invert the logic.

>  #else
>  inline void mxs_gpio_init(void) {}
>  #endif
> diff --git a/arch/arm/include/asm/arch-mx28/iomux.h
> b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644
> --- a/arch/arm/include/asm/arch-mx28/iomux.h
> +++ b/arch/arm/include/asm/arch-mx28/iomux.h
> @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t;
>  #define MXS_PAD_PULL_VALID_SHIFT 16
>  #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 <<
> MXS_PAD_PULL_VALID_SHIFT)
> 
> +#define MX28_BANK0_PINS 29
> +#define MX28_BANK1_PINS 32
> +#define MX28_BANK2_PINS 28
> +#define MX28_BANK3_PINS 31
> +#define MX28_BANK4_PINS 21
> +
> +#define MX23_BANK0_PINS 32
> +#define MX23_BANK1_PINS 31
> +#define MX23_BANK2_PINS 32
> +

It's not used anywhere else than mxs_gpio.c, so define it there to reduce the 
scope. Also, I'd be just for using array of numbers with explanation comment to 
avoid making the code unnecessarily bigger.

>  #define PAD_MUXSEL_0 0
>  #define PAD_MUXSEL_1 1
>  #define PAD_MUXSEL_2 2
> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
> index 9cc790a..51e11e7 100644
> --- a/common/cmd_gpio.c
> +++ b/common/cmd_gpio.c
> @@ -15,6 +15,10 @@
>  #define name_to_gpio(name) simple_strtoul(name, NULL, 10)
>  #endif
> 
> +#ifndef gpio_invalid
> +#define gpio_invalid(gpio) (0)
> +#endif
> +
>  enum gpio_cmd {
>   GPIO_INPUT,
>   GPIO_SET,
> @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[]) if (gpio < 0)
>   goto show_usage;
> 
> + /* check bank and pin number for validity */
> + if (gpio_invalid(gpio)) {
> + printf("gpio: invalid pin %u\n", gpio);
> + return -1;
> + }
> +

Please separate this cmd_gpio() part out! Are you ignoring my previous comments 
completely ?

>   /* grab the pin before we tweak it */
>   if (gpio_request(gpio, "cmd_gpio")) {
>   printf("gpio: requesting pin %u failed\n", gpio);
> diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
> index b7e9591..dabee90 100644
> --- a/drivers/gpio/mxs_gpio.c
> +++ b/drivers/gpio/mxs_gpio.c
> @@ -31,7 +31,6 @@
>  #include <asm/arch/imx-regs.h>
> 
>  #if defined(CONFIG_MX23)
> -#define PINCTRL_BANKS 3
>  #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10))
>  #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10))
>  #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10))
> @@ -39,7 +38,6 @@
>  #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10))
>  #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10))
>  #elif defined(CONFIG_MX28)
> -#define PINCTRL_BANKS 5
>  #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10))
>  #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10))
>  #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10))
> @@ -57,11 +55,25 @@
>  #define GPIO_INT_LEV_MASK (1 << 0)
>  #define GPIO_INT_POL_MASK (1 << 1)
> 
> +static const int mxs_bank_pins[] = {
> +#if defined(CONFIG_MX23)
> + MX23_BANK0_PINS,
> + MX23_BANK1_PINS,
> + MX23_BANK2_PINS
> +#elif defined(CONFIG_MX28)
> + MX28_BANK0_PINS,
> + MX28_BANK1_PINS,
> + MX28_BANK2_PINS,
> + MX28_BANK3_PINS,
> + MX28_BANK4_PINS
> +#endif

Ignoring? I asked you to define it above and don't duplicate the ifdefs.

> +};
> +
>  void mxs_gpio_init(void)
>  {
>   int i;
> 
> - for (i = 0; i < PINCTRL_BANKS; i++) {
> + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) {
>   writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i));
>   writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i));
>   /* Use SCT address here to clear the IRQSTAT bits */
> @@ -69,6 +81,16 @@ void mxs_gpio_init(void)
>   }
>  }
> 
> +int gpio_invalid(int gp)
> +{
> + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||
> +     (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) ||
> +     (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) )
> + return -EINVAL;
> +
> + return 0;
> +}
> +
>  int gpio_get_value(int gp)
>  {
>   uint32_t bank = PAD_BANK(gp);
> @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value)
> 
>  int gpio_request(int gp, const char *label)
>  {
> - if (PAD_BANK(gp) > PINCTRL_BANKS)
> - return -EINVAL;
> -
>   return 0;
>  }

Please make a checklist before resubmitting. Also, one more thing, submit this 
as a series to avoid this mess of patches:

git format-patch --cover-letter HEAD~3 -o my_awesome_series
vim my_awesome_series/0000-*
<create some sane cover letter, add Cc: lines there too!!!>
git send-email --annotate --to="u-boot at lists.denx.de" my_awesome_series/*

Then when someone tells you it's completely wrong ;-)

git format-patch etc.
git send-email only the patches which you changed, and use the Message-ID for 
in-reply-to to particular patches, so the mail threading is right.

M

[U-Boot] [PATCH v3] M28: GPIO pin validity check added

[Robert Deliën]

Sorry guys; I'm bailing.

Out.


On Nov 24, 2011, at 1:15, Marek Vasut wrote:

>> This patch adds a validity check for GPIO pins to prevent clobbering
>> reserved bit or even registers, per suggestion of Marek Vasut.
>> 
>> Please note:
>> 1. gpio_request no longer checks validity. Pin validity must be checked
>> explicitly use gpio_invalid prior to request and hence use. Previous
>> validity check in gpio_request was incomplete anyway. 2. MX233 is not
>> supported yet. Until it is, macros defining the number of pins for each
>> bank reside in arch/arm/include/asm/arch-mx28/iomux.h
>> 
>> Signed-off-by: Robert Deli?n <robert@delien.nl<http://delien.nl/>>
> 
> AARGH! Please add the following lines to the commit message:
> 
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> 
> !!!!
> 
> That way, you won't have to care for the Cc, which you forgot again!
>> 
>> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h
>> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644
>> --- a/arch/arm/include/asm/arch-mx28/gpio.h
>> +++ b/arch/arm/include/asm/arch-mx28/gpio.h
>> @@ -25,6 +25,10 @@
>> 
>> #ifdef CONFIG_MXS_GPIO
>> void mxs_gpio_init(void);
>> +
>> +extern int gpio_invalid(int gp);
>> +#define gpio_invalid(gpio) gpio_invalid(gpio)
>> +
> 
> Please, gpio_is_valid() and invert the logic.
> 
>> #else
>> inline void mxs_gpio_init(void) {}
>> #endif
>> diff --git a/arch/arm/include/asm/arch-mx28/iomux.h
>> b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644
>> --- a/arch/arm/include/asm/arch-mx28/iomux.h
>> +++ b/arch/arm/include/asm/arch-mx28/iomux.h
>> @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t;
>> #define MXS_PAD_PULL_VALID_SHIFT 16
>> #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 <<
>> MXS_PAD_PULL_VALID_SHIFT)
>> 
>> +#define MX28_BANK0_PINS 29
>> +#define MX28_BANK1_PINS 32
>> +#define MX28_BANK2_PINS 28
>> +#define MX28_BANK3_PINS 31
>> +#define MX28_BANK4_PINS 21
>> +
>> +#define MX23_BANK0_PINS 32
>> +#define MX23_BANK1_PINS 31
>> +#define MX23_BANK2_PINS 32
>> +
> 
> It's not used anywhere else than mxs_gpio.c, so define it there to reduce the 
> scope. Also, I'd be just for using array of numbers with explanation comment to 
> avoid making the code unnecessarily bigger.
> 
>> #define PAD_MUXSEL_0 0
>> #define PAD_MUXSEL_1 1
>> #define PAD_MUXSEL_2 2
>> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
>> index 9cc790a..51e11e7 100644
>> --- a/common/cmd_gpio.c
>> +++ b/common/cmd_gpio.c
>> @@ -15,6 +15,10 @@
>> #define name_to_gpio(name) simple_strtoul(name, NULL, 10)
>> #endif
>> 
>> +#ifndef gpio_invalid
>> +#define gpio_invalid(gpio) (0)
>> +#endif
>> +
>> enum gpio_cmd {
>>  GPIO_INPUT,
>>  GPIO_SET,
>> @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc,
>> char * const argv[]) if (gpio < 0)
>>  goto show_usage;
>> 
>> + /* check bank and pin number for validity */
>> + if (gpio_invalid(gpio)) {
>> + printf("gpio: invalid pin %u\n", gpio);
>> + return -1;
>> + }
>> +
> 
> Please separate this cmd_gpio() part out! Are you ignoring my previous comments 
> completely ?
> 
>>  /* grab the pin before we tweak it */
>>  if (gpio_request(gpio, "cmd_gpio")) {
>>  printf("gpio: requesting pin %u failed\n", gpio);
>> diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
>> index b7e9591..dabee90 100644
>> --- a/drivers/gpio/mxs_gpio.c
>> +++ b/drivers/gpio/mxs_gpio.c
>> @@ -31,7 +31,6 @@
>> #include <asm/arch/imx-regs.h>
>> 
>> #if defined(CONFIG_MX23)
>> -#define PINCTRL_BANKS 3
>> #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10))
>> #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10))
>> #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10))
>> @@ -39,7 +38,6 @@
>> #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10))
>> #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10))
>> #elif defined(CONFIG_MX28)
>> -#define PINCTRL_BANKS 5
>> #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10))
>> #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10))
>> #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10))
>> @@ -57,11 +55,25 @@
>> #define GPIO_INT_LEV_MASK (1 << 0)
>> #define GPIO_INT_POL_MASK (1 << 1)
>> 
>> +static const int mxs_bank_pins[] = {
>> +#if defined(CONFIG_MX23)
>> + MX23_BANK0_PINS,
>> + MX23_BANK1_PINS,
>> + MX23_BANK2_PINS
>> +#elif defined(CONFIG_MX28)
>> + MX28_BANK0_PINS,
>> + MX28_BANK1_PINS,
>> + MX28_BANK2_PINS,
>> + MX28_BANK3_PINS,
>> + MX28_BANK4_PINS
>> +#endif
> 
> Ignoring? I asked you to define it above and don't duplicate the ifdefs.
> 
>> +};
>> +
>> void mxs_gpio_init(void)
>> {
>>  int i;
>> 
>> - for (i = 0; i < PINCTRL_BANKS; i++) {
>> + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) {
>>  writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i));
>>  writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i));
>>  /* Use SCT address here to clear the IRQSTAT bits */
>> @@ -69,6 +81,16 @@ void mxs_gpio_init(void)
>>  }
>> }
>> 
>> +int gpio_invalid(int gp)
>> +{
>> + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||
>> +     (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) ||
>> +     (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) )
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> int gpio_get_value(int gp)
>> {
>>  uint32_t bank = PAD_BANK(gp);
>> @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value)
>> 
>> int gpio_request(int gp, const char *label)
>> {
>> - if (PAD_BANK(gp) > PINCTRL_BANKS)
>> - return -EINVAL;
>> -
>>  return 0;
>> }
> 
> Please make a checklist before resubmitting. Also, one more thing, submit this 
> as a series to avoid this mess of patches:
> 
> git format-patch --cover-letter HEAD~3 -o my_awesome_series
> vim my_awesome_series/0000-*
> <create some sane cover letter, add Cc: lines there too!!!>
> git send-email --annotate --to="u-boot at lists.denx.de" my_awesome_series/*
> 
> Then when someone tells you it's completely wrong ;-)
> 
> git format-patch etc.
> git send-email only the patches which you changed, and use the Message-ID for 
> in-reply-to to particular patches, so the mail threading is right.
> 
> M

[U-Boot] [PATCH v3] M28: GPIO pin validity check added

[Marek Vasut]

> Sorry guys; I'm bailing.
> 
> Out.

Well ... it can't be helped.

M
> 
> On Nov 24, 2011, at 1:15, Marek Vasut wrote:
> >> This patch adds a validity check for GPIO pins to prevent clobbering
> >> reserved bit or even registers, per suggestion of Marek Vasut.
> >> 
> >> Please note:
> >> 1. gpio_request no longer checks validity. Pin validity must be checked
> >> explicitly use gpio_invalid prior to request and hence use. Previous
> >> validity check in gpio_request was incomplete anyway. 2. MX233 is not
> >> supported yet. Until it is, macros defining the number of pins for each
> >> bank reside in arch/arm/include/asm/arch-mx28/iomux.h
> >> 
> >> Signed-off-by: Robert Deli?n <robert@delien.nl<http://delien.nl/>>
> > 
> > AARGH! Please add the following lines to the commit message:
> > 
> > Cc: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > 
> > !!!!
> > 
> > That way, you won't have to care for the Cc, which you forgot again!
> > 
> >> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h
> >> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644
> >> --- a/arch/arm/include/asm/arch-mx28/gpio.h
> >> +++ b/arch/arm/include/asm/arch-mx28/gpio.h
> >> @@ -25,6 +25,10 @@
> >> 
> >> #ifdef CONFIG_MXS_GPIO
> >> void mxs_gpio_init(void);
> >> +
> >> +extern int gpio_invalid(int gp);
> >> +#define gpio_invalid(gpio) gpio_invalid(gpio)
> >> +
> > 
> > Please, gpio_is_valid() and invert the logic.
> > 
> >> #else
> >> inline void mxs_gpio_init(void) {}
> >> #endif
> >> diff --git a/arch/arm/include/asm/arch-mx28/iomux.h
> >> b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644
> >> --- a/arch/arm/include/asm/arch-mx28/iomux.h
> >> +++ b/arch/arm/include/asm/arch-mx28/iomux.h
> >> @@ -56,6 +56,16 @@ typedef u32 iomux_cfg_t;
> >> #define MXS_PAD_PULL_VALID_SHIFT 16
> >> #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 <<
> >> MXS_PAD_PULL_VALID_SHIFT)
> >> 
> >> +#define MX28_BANK0_PINS 29
> >> +#define MX28_BANK1_PINS 32
> >> +#define MX28_BANK2_PINS 28
> >> +#define MX28_BANK3_PINS 31
> >> +#define MX28_BANK4_PINS 21
> >> +
> >> +#define MX23_BANK0_PINS 32
> >> +#define MX23_BANK1_PINS 31
> >> +#define MX23_BANK2_PINS 32
> >> +
> > 
> > It's not used anywhere else than mxs_gpio.c, so define it there to reduce
> > the scope. Also, I'd be just for using array of numbers with explanation
> > comment to avoid making the code unnecessarily bigger.
> > 
> >> #define PAD_MUXSEL_0 0
> >> #define PAD_MUXSEL_1 1
> >> #define PAD_MUXSEL_2 2
> >> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
> >> index 9cc790a..51e11e7 100644
> >> --- a/common/cmd_gpio.c
> >> +++ b/common/cmd_gpio.c
> >> @@ -15,6 +15,10 @@
> >> #define name_to_gpio(name) simple_strtoul(name, NULL, 10)
> >> #endif
> >> 
> >> +#ifndef gpio_invalid
> >> +#define gpio_invalid(gpio) (0)
> >> +#endif
> >> +
> >> enum gpio_cmd {
> >> 
> >>  GPIO_INPUT,
> >>  GPIO_SET,
> >> 
> >> @@ -56,6 +60,12 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int
> >> argc, char * const argv[]) if (gpio < 0)
> >> 
> >>  goto show_usage;
> >> 
> >> + /* check bank and pin number for validity */
> >> + if (gpio_invalid(gpio)) {
> >> + printf("gpio: invalid pin %u\n", gpio);
> >> + return -1;
> >> + }
> >> +
> > 
> > Please separate this cmd_gpio() part out! Are you ignoring my previous
> > comments completely ?
> > 
> >>  /* grab the pin before we tweak it */
> >>  if (gpio_request(gpio, "cmd_gpio")) {
> >>  printf("gpio: requesting pin %u failed\n", gpio);
> >> 
> >> diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
> >> index b7e9591..dabee90 100644
> >> --- a/drivers/gpio/mxs_gpio.c
> >> +++ b/drivers/gpio/mxs_gpio.c
> >> @@ -31,7 +31,6 @@
> >> #include <asm/arch/imx-regs.h>
> >> 
> >> #if defined(CONFIG_MX23)
> >> -#define PINCTRL_BANKS 3
> >> #define PINCTRL_DOUT(n) (0x0500 + ((n) * 0x10))
> >> #define PINCTRL_DIN(n) (0x0600 + ((n) * 0x10))
> >> #define PINCTRL_DOE(n) (0x0700 + ((n) * 0x10))
> >> @@ -39,7 +38,6 @@
> >> #define PINCTRL_IRQEN(n) (0x0900 + ((n) * 0x10))
> >> #define PINCTRL_IRQSTAT(n) (0x0c00 + ((n) * 0x10))
> >> #elif defined(CONFIG_MX28)
> >> -#define PINCTRL_BANKS 5
> >> #define PINCTRL_DOUT(n) (0x0700 + ((n) * 0x10))
> >> #define PINCTRL_DIN(n) (0x0900 + ((n) * 0x10))
> >> #define PINCTRL_DOE(n) (0x0b00 + ((n) * 0x10))
> >> @@ -57,11 +55,25 @@
> >> #define GPIO_INT_LEV_MASK (1 << 0)
> >> #define GPIO_INT_POL_MASK (1 << 1)
> >> 
> >> +static const int mxs_bank_pins[] = {
> >> +#if defined(CONFIG_MX23)
> >> + MX23_BANK0_PINS,
> >> + MX23_BANK1_PINS,
> >> + MX23_BANK2_PINS
> >> +#elif defined(CONFIG_MX28)
> >> + MX28_BANK0_PINS,
> >> + MX28_BANK1_PINS,
> >> + MX28_BANK2_PINS,
> >> + MX28_BANK3_PINS,
> >> + MX28_BANK4_PINS
> >> +#endif
> > 
> > Ignoring? I asked you to define it above and don't duplicate the ifdefs.
> > 
> >> +};
> >> +
> >> void mxs_gpio_init(void)
> >> {
> >> 
> >>  int i;
> >> 
> >> - for (i = 0; i < PINCTRL_BANKS; i++) {
> >> + for (i = 0; i < ARRAY_SIZE(mxs_bank_pins); i++) {
> >> 
> >>  writel(0, MXS_PINCTRL_BASE + PINCTRL_PIN2IRQ(i));
> >>  writel(0, MXS_PINCTRL_BASE + PINCTRL_IRQEN(i));
> >>  /* Use SCT address here to clear the IRQSTAT bits */
> >> 
> >> @@ -69,6 +81,16 @@ void mxs_gpio_init(void)
> >> 
> >>  }
> >> 
> >> }
> >> 
> >> +int gpio_invalid(int gp)
> >> +{
> >> + if ( (gp & ~(MXS_PAD_BANK_MASK | MXS_PAD_PIN_MASK)) ||
> >> +     (PAD_BANK(gp) >= ARRAY_SIZE(mxs_bank_pins)) ||
> >> +     (PAD_PIN(gp) >= mxs_bank_pins[PAD_BANK(gp)]) )
> >> + return -EINVAL;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> int gpio_get_value(int gp)
> >> {
> >> 
> >>  uint32_t bank = PAD_BANK(gp);
> >> 
> >> @@ -120,9 +142,6 @@ int gpio_direction_output(int gp, int value)
> >> 
> >> int gpio_request(int gp, const char *label)
> >> {
> >> - if (PAD_BANK(gp) > PINCTRL_BANKS)
> >> - return -EINVAL;
> >> -
> >> 
> >>  return 0;
> >> 
> >> }
> > 
> > Please make a checklist before resubmitting. Also, one more thing, submit
> > this as a series to avoid this mess of patches:
> > 
> > git format-patch --cover-letter HEAD~3 -o my_awesome_series
> > vim my_awesome_series/0000-*
> > <create some sane cover letter, add Cc: lines there too!!!>
> > git send-email --annotate --to="u-boot at lists.denx.de" my_awesome_series/*
> > 
> > Then when someone tells you it's completely wrong ;-)
> > 
> > git format-patch etc.
> > git send-email only the patches which you changed, and use the Message-ID
> > for in-reply-to to particular patches, so the mail threading is right.
> > 
> > M