[U-Boot] [PATCH V3 1/2] GPIO: Tegra2: add GPIO driver for Tegra2

[Mike Frysinger]

On Wednesday, June 08, 2011 18:08:12 Tom Warren wrote:
> --- a/common/cmd_gpio.c
> +++ b/common/cmd_gpio.c
> 
> +#ifdef	CONFIG_TEGRA2_GPIO
> +#include <asm/arch/gpio.h>
> +#else
>  #include <asm/gpio.h>
> +#endif

sorry, but no.  create an asm/gpio.h in the arch/arm/ subtree that includes 
asm/arch/gpio.h.  you could probably even move some of the common stuff (like 
the gpio func prototypes) into that header.

if we get enough people picking this up in u-boot, we can probably eventually 
unify further in asm-generic/gpio.h.  but that day isnt today :P.

> --- /dev/null
> +++ b/drivers/gpio/tegra2_gpio.c
>
> +int gpio_num = 0;
> +char gpio_label[20] = "";

this doesnt make much sense.  the gpio code is not 1-pin at a time, it allows 
for any number of pins to be requested at a time.  i'd suggest you just drop 
these two variables and their users.

> +/* Config pin 'gp' as GPIO or SFPIO, based on 'type' */
> +void __set_config(int gp, int type)

probably want all these internal funcs to be marked static

> +int gpio_request(int gp, const char *label)
> +{
> +	/* Configure as a GPIO */
> +	__set_config(gp, 1);

you probably want to do error checking on gp.  if someone does 
gpio_request(10000), i'm pretty sure your current code is going to crash and 
burn.

> +int gpio_direction_input(int gp)
> +{
> ...
> +	/* Configure as a GPIO */
> +	__set_config(gp, 1);
> ...
> +int gpio_direction_output(int gp, int value)
> +{
> ...
> +	/* Configure as a GPIO */
> +	__set_config(gp, 1);

this should be unnecessary.  code that calls any gpio_* funcs on a pin that 
first did not call gpio_request (and succeed) is broken.  dont work around 
that in the core code.
-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/20110608/35681e6a/attachment.pgp