Patchwork [U-Boot] gpio: omap_gpio: Fix valid gpio range for AM33XX

login
register
mail settings
Submitter Axel Lin
Date June 20, 2013, 3:26 p.m.
Message ID <1371742016.8005.5.camel@phoenix>
Download mbox | patch
Permalink /patch/253004/
State Changes Requested
Delegated to: Marek Vasut
Headers show

Comments

Axel Lin - June 20, 2013, 3:26 p.m.
AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/gpio/omap_gpio.c | 4 ++++
 1 file changed, 4 insertions(+)
Tom Rini - June 20, 2013, 3:35 p.m.
On Thu, Jun 20, 2013 at 11:26:56PM +0800, Axel Lin wrote:

> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Reviewed-by: Tom Rini <trini@ti.com>
Stefan Roese - June 20, 2013, 3:47 p.m.
On 20.06.2013 17:26, Axel Lin wrote:
> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/gpio/omap_gpio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
> index a30d7f0..9aa6d41 100644
> --- a/drivers/gpio/omap_gpio.c
> +++ b/drivers/gpio/omap_gpio.c
> @@ -55,7 +55,11 @@ static inline int get_gpio_index(int gpio)
>  
>  int gpio_is_valid(int gpio)
>  {
> +#if defined(CONFIG_AM33XX)
> +	return (gpio >= 0) && (gpio < 128);
> +#else
>  	return (gpio >= 0) && (gpio < 192);
> +#endif
>  }

Those ifdef's in the code really ugly and frowned upon. Better would be
to move this into a define in the top of the C file:

#if defined(CONFIG_AM33XX)
#define CONFIG_OMAP_MAX_GPIO	128
#else
#define CONFIG_OMAP_MAX_GPIO	192
#endif

And then use this define in the code:

	return (gpio >= 0) && (gpio < CONFIG_OMAP_MAX_GPIO);

Thanks,
Stefan
Marek Vasut - June 20, 2013, 7:42 p.m.
Dear Stefan Roese,

> On 20.06.2013 17:26, Axel Lin wrote:
> > AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
> > 
> > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > ---
> > 
> >  drivers/gpio/omap_gpio.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
> > index a30d7f0..9aa6d41 100644
> > --- a/drivers/gpio/omap_gpio.c
> > +++ b/drivers/gpio/omap_gpio.c
> > @@ -55,7 +55,11 @@ static inline int get_gpio_index(int gpio)
> > 
> >  int gpio_is_valid(int gpio)
> >  {
> > 
> > +#if defined(CONFIG_AM33XX)
> > +	return (gpio >= 0) && (gpio < 128);
> > +#else
> > 
> >  	return (gpio >= 0) && (gpio < 192);
> > 
> > +#endif
> > 
> >  }
> 
> Those ifdef's in the code really ugly and frowned upon. Better would be
> to move this into a define in the top of the C file:
> 
> #if defined(CONFIG_AM33XX)
> #define CONFIG_OMAP_MAX_GPIO	128
> #else
> #define CONFIG_OMAP_MAX_GPIO	192
> #endif
> 
> And then use this define in the code:
> 
> 	return (gpio >= 0) && (gpio < CONFIG_OMAP_MAX_GPIO);
> 
> Thanks,
> Stefan

You were faster ;-) and I agree with this method.

Best regards,
Marek Vasut

Patch

diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
index a30d7f0..9aa6d41 100644
--- a/drivers/gpio/omap_gpio.c
+++ b/drivers/gpio/omap_gpio.c
@@ -55,7 +55,11 @@  static inline int get_gpio_index(int gpio)
 
 int gpio_is_valid(int gpio)
 {
+#if defined(CONFIG_AM33XX)
+	return (gpio >= 0) && (gpio < 128);
+#else
 	return (gpio >= 0) && (gpio < 192);
+#endif
 }
 
 static int check_gpio(int gpio)