[U-Boot] [PATCH] M28: Added pin name support in GPIO driver

[Robert Deliën]

> No, move this to mxs_gpio.c and simply make the function not static.

I too find the construction a little strange, but I copied it from the Blackfin implementation.

But after taking a second look at it, it made sense: It makes the file pulling including it, define it statically locally. The macro below exports it to the macro name space and cmd_gpio checks for the existence of that makro. If it doesn't exist there, cmd_gpio defines it as simple_strtoul. I suppose this is created that way to allow diversity between platforms with and without name support.

Currently, Blackfin is the only platform supporting named pins. I'd be happy to change it, but then Blackfin needs some work too and I don't have the hardware to test it.

> So if I pass name == NULL, we're done for here ;-)

We would, but it's a static function, so it's unavailable to the rest of the world. And name won't be NULL unless the command line argument parsing is borked. I know what you mean, but if even all static functions start schizophrenically checking all their parameters we'd be doing that half the CPU cycles.

> Braces missing around this return statement.

Will do! I actually do that for all of my code, but this is how it was in Blackfin  (and in drivers/gpio/mxs_gpio.c, and in common/cmd_gpio.c, for that matter).

> Also, why not do something like:
> 
> if (tolower(name[0]) != 'b')
> 	return -EINVAL;
> 
> name++;
> pinnr = ...
> 
> if (tolower(name[0]) != 'p')
> 	return -EINVAL;
> 
> name++;
> ...
> 
> It seems more explicit to me that way.

Agreed; Will do!

> What's this define for ?

See above.

> Do you even need this if CONFIG_CMD_GPIO is undefined? Move the function to 
> mxs_gpio.c and make it not static should work for you.

Nope, I don't, but I didn't put it there. It was already there, so somebody must have approved (of overlooked) it ;-)