> 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 at delien.nl<http://delien.nl/>>
AARGH! Please add the following lines to the commit message:
Cc: Marek Vasut <marek.va...@gmail.com>
Cc: Stefano Babic <sba...@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@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
