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

Marek Vasut
2011-11-22T19:32:32Z

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