M28: GPIO pin validity check added

PATCH Superseded Robert Deliën 3 participants 5 messages Nov 23, 2011 – Nov 24, 2011

Subsystems (3)

Arm (32-bit) Arm i.MX and Layerscape GPIO

lore

Share & tools

lore

Robert Deliën

author

2011-11-23 20:50 UTC #0 lore raw

PATCH v3 1/1 M28: GPIO pin validity check added

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 <[email protected]<http://delien.nl/>>

arch/arm/include/asm/arch-mx28/gpio.h +4 -0

index be1c944..5abd7b1 100644
@@ -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

arch/arm/include/asm/arch-mx28/iomux.h +10 -0

index 7abdf58..2326fdb 100644
@@ -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

common/cmd_gpio.c +10 -0

index 9cc790a..51e11e7 100644
@@ -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);

drivers/gpio/mxs_gpio.c +25 -6

index b7e9591..dabee90 100644
@@ -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;
 }

Mike Frysinger

2011-11-23 22:19 UTC #1 lore raw

PATCH v3 1/1 M28: GPIO pin validity check added

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>

Marek Vasut

2011-11-24 00:15 UTC #2 lore raw

PATCH v3 1/1 M28: GPIO pin validity check added

In reply to Robert Deliën

Quoted 11 lines

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 <[email protected]<http://delien.nl/>>

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

Cc Marek Vasut <[email protected]>

Cc Stefano Babic <[email protected]>

!!!!

That way, you won't have to care for the Cc, which you forgot again!

arch/arm/include/asm/arch-mx28/gpio.h line 31

Quoted 13 lines

(quoted diff) +0 -0
b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644
arch/arm/include/asm/arch-mx28/gpio.h +4 -0
@@ -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.

Quoted 12 lines

arch/arm/include/asm/arch-mx28/gpio.h +0 -0
 #else
 inline void mxs_gpio_init(void) {}
 #endif
(quoted diff) +0 -0
b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644
arch/arm/include/asm/arch-mx28/iomux.h +0 -0
@@ -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)
 

arch/arm/include/asm/arch-mx28/iomux.h typedef u32 iomux_cfg_t; · line 69

Quoted 10 lines

arch/arm/include/asm/arch-mx28/iomux.h +10 -0

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

Quoted 19 lines

arch/arm/include/asm/arch-mx28/iomux.h +0 -0

 #define PAD_MUXSEL_0 0
 #define PAD_MUXSEL_1 1

#define PAD_MUXSEL_2 2

common/cmd_gpio.c +4 -0

index 9cc790a..51e11e7 100644
@@ -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,

common/cmd_gpio.c static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, · line 68

Quoted 9 lines

common/cmd_gpio.c +6 -0

@@ -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 ?

Quoted 29 lines

common/cmd_gpio.c +0 -0

  /* grab the pin before we tweak it */
  if (gpio_request(gpio, "cmd_gpio")) {
  printf("gpio: requesting pin %u failed\n", gpio);

drivers/gpio/mxs_gpio.c +2 -2

index b7e9591..dabee90 100644
@@ -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)

drivers/gpio/mxs_gpio.c line 69

Quoted 10 lines

drivers/gpio/mxs_gpio.c +10 -0

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

Quoted 29 lines

drivers/gpio/mxs_gpio.c +13 -1

+};
+
 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);

drivers/gpio/mxs_gpio.c int gpio_direction_output(int gp, int value) · line 146

Quoted 8 lines

drivers/gpio/mxs_gpio.c +0 -3

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

Robert Deliën

author

2011-11-24 08:21 UTC #3 lore raw

PATCH v3 1/1 M28: GPIO pin validity check added

Sorry guys; I'm bailing.

Out.

Quoted 190 lines · Marek Vasut

Quoted 11 lines
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 <[email protected]<http://delien.nl/>>
AARGH! Please add the following lines to the commit message:
Cc Marek Vasut <[email protected]>
Cc Stefano Babic <[email protected]>
!!!!
That way, you won't have to care for the Cc, which you forgot again!
arch/arm/include/asm/arch-mx28/gpio.h line 31
Quoted 13 lines
(quoted diff) +0 -0
b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644
arch/arm/include/asm/arch-mx28/gpio.h +4 -0
@@ -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.
Quoted 12 lines
arch/arm/include/asm/arch-mx28/gpio.h +0 -0
 #else
 inline void mxs_gpio_init(void) {}
 #endif
(quoted diff) +0 -0
b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644
arch/arm/include/asm/arch-mx28/iomux.h +0 -0
@@ -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)
 
arch/arm/include/asm/arch-mx28/iomux.h typedef u32 iomux_cfg_t; · line 69
Quoted 10 lines
arch/arm/include/asm/arch-mx28/iomux.h +10 -0
+#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.
Quoted 19 lines
arch/arm/include/asm/arch-mx28/iomux.h +0 -0
 #define PAD_MUXSEL_0 0
 #define PAD_MUXSEL_1 1
#define PAD_MUXSEL_2 2
common/cmd_gpio.c +4 -0
index 9cc790a..51e11e7 100644
@@ -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,
common/cmd_gpio.c static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, · line 68
Quoted 9 lines
common/cmd_gpio.c +6 -0
@@ -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 ?
Quoted 29 lines
common/cmd_gpio.c +0 -0
 /* grab the pin before we tweak it */
 if (gpio_request(gpio, "cmd_gpio")) {
 printf("gpio: requesting pin %u failed\n", gpio);
drivers/gpio/mxs_gpio.c +2 -2
index b7e9591..dabee90 100644
@@ -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)
drivers/gpio/mxs_gpio.c line 69
Quoted 10 lines
drivers/gpio/mxs_gpio.c +10 -0
+ 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.
Quoted 29 lines
drivers/gpio/mxs_gpio.c +13 -1
+};
+
 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);
drivers/gpio/mxs_gpio.c int gpio_direction_output(int gp, int value) · line 146
Quoted 8 lines
drivers/gpio/mxs_gpio.c +0 -3
@@ -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

Marek Vasut

2011-11-24 14:15 UTC #4 lore raw

PATCH v3 1/1 M28: GPIO pin validity check added

Quoted 3 lines

Sorry guys; I'm bailing.
Out.

Well ... it can't be helped.

Quoted 205 lines · Marek Vasut

Quoted 203 lines
Quoted 11 lines
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 <[email protected]<http://delien.nl/>>
AARGH! Please add the following lines to the commit message:
Cc Marek Vasut <[email protected]>
Cc Stefano Babic <[email protected]>
!!!!
That way, you won't have to care for the Cc, which you forgot again!
arch/arm/include/asm/arch-mx28/gpio.h line 31
Quoted 12 lines
(quoted diff) +0 -0
b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5abd7b1 100644
arch/arm/include/asm/arch-mx28/gpio.h +4 -0
@@ -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.
Quoted 12 lines
arch/arm/include/asm/arch-mx28/gpio.h +0 -0
 #else
 inline void mxs_gpio_init(void) {}
 #endif
(quoted diff) +0 -0
b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..2326fdb 100644
arch/arm/include/asm/arch-mx28/iomux.h +0 -0
@@ -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)
 
arch/arm/include/asm/arch-mx28/iomux.h typedef u32 iomux_cfg_t; · line 69
Quoted 10 lines
arch/arm/include/asm/arch-mx28/iomux.h +10 -0
+#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.
Quoted 21 lines
arch/arm/include/asm/arch-mx28/iomux.h +0 -0
 #define PAD_MUXSEL_0 0
 #define PAD_MUXSEL_1 1
#define PAD_MUXSEL_2 2
common/cmd_gpio.c +4 -0
index 9cc790a..51e11e7 100644
@@ -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,
common/cmd_gpio.c static int do_gpio(cmd_tbl_t *cmdtp, int flag, int · line 69
Quoted 10 lines
common/cmd_gpio.c +6 -0
@@ -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 ?
Quoted 30 lines
common/cmd_gpio.c +0 -0
 /* grab the pin before we tweak it */
 if (gpio_request(gpio, "cmd_gpio")) {
printf("gpio: requesting pin %u failed\n", gpio);
drivers/gpio/mxs_gpio.c +2 -2
index b7e9591..dabee90 100644
@@ -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)
drivers/gpio/mxs_gpio.c line 69
Quoted 10 lines
drivers/gpio/mxs_gpio.c +10 -0
+ 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.
Quoted 45 lines
drivers/gpio/mxs_gpio.c +3 -1
+};
+
 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 */
drivers/gpio/mxs_gpio.c +10 -0
@@ -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);
drivers/gpio/mxs_gpio.c +0 -3
@@ -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;
 
Quoted 1 line
}
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