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

[Marek Vasut <marek.vasut@gmail.com>]

> 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>
> 
> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h
> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..36f3be8 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
> +

Do we need this in the header file?

> #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..f81c7d8 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)) {

gpio_is_valid() might be a better name?

> +		printf("gpio: invalid pin %u\n", gpio);
> +		return (-1);

return -EINVAL;

> +	}
> +
> 	/* 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..d2e9bac 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
> +};

Why not move it above, since the ifdef CONFIG_MX28 etc. are already there and 
define it twice. It'd be clearer. Also, it's quite obvious those are pin counts, 
so why introduce these defines and not put only plain numbers here. The 
MX28_BANK0_PINS etc are used only here anyway.

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

M