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

[Marek Vasut]

> > 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.

I don't think I follow ... don't you only have to define the function and that's 
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.

What? Which one is?

The name_to_gpio_number() function, which I assume is used by the cmd_gpio must 
exactly be NON-static! Or prove me wrong please.

> 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 ;-)

Don't rely on other code too much, improvise and bring progress :)

M