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

This patch adds adds pin name support in the GPIO driver. With this patch applied, the gpio command supports pins to be addressed with friendly names.

The user's manual refers to pins by the bank number they're in and their number within that bank. With this patch the friendly naming convention to address pin number 5 in bank 3 is b3p5. But names like B00000003p005 are interpreted correctly too.

Signed-off-by: Robert Deliën <robert at delien.nl>

> This patch adds adds pin name support in the GPIO driver. With this patch
> applied, the gpio command supports pins to be addressed with friendly
> names.
> 
> The user's manual refers to pins by the bank number they're in and their
> number within that bank. With this patch the friendly naming convention to
> address pin number 5 in bank 3 is b3p5. But names like B00000003p005 are
> interpreted correctly too.
> 
> Signed-off-by: Robert Deliën <robert at delien.nl>
> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h
> b/arch/arm/include/asm/arch-mx28/gpio.h index be1c944..5ae66e6 100644
> --- a/arch/arm/include/asm/arch-mx28/gpio.h
> +++ b/arch/arm/include/asm/arch-mx28/gpio.h

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

> @@ -23,8 +23,44 @@
>  #ifndef        __MX28_GPIO_H__
>  #define        __MX28_GPIO_H__
> 
> +#include <linux/ctype.h>
> +#include <asm/errno.h>
> +#include <asm/arch/iomux.h>
> +
>  #ifdef CONFIG_MXS_GPIO
>  void mxs_gpio_init(void);
> +
> +static inline int name_to_gpio(const char *name)
> +{
> +       int     pin_nr;
> +
> +       if (name[0] >= '0' && name[0] <= '9') {

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

> +               pin_nr = simple_strtoul(name, (char **)&name, 10);
> +               if (name[0])
> +                       return -EINVAL;
> +               else
> +                       return pin_nr;
> +       }
> +
> +       if (tolower(name[0]) == 'b') {
> +               name++;
> +               pin_nr = (simple_strtoul(name, (char **)&name, 10) <<
> MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK; +       } else
> +               return -EINVAL;

Braces missing around this return statement. 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.

> +
> +       if (tolower(name[0]) == 'p') {
> +               name++;
> +               pin_nr |= (simple_strtoul(name, (char **)&name, 10) <<
> MXS_PAD_PIN_SHIFT) & MXS_PAD_PIN_MASK; +       } else
> +               return -EINVAL;
> +
> +       if (name[0])
> +               return -EINVAL;
> +
> +       return pin_nr;
> +}
> +#define name_to_gpio(n) name_to_gpio(n)

What's this define for ?

> +
>  #else
>  inline void mxs_gpio_init(void) {}
>  #endif

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.

M

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

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

> I don't think I follow ... don't you only have to define the function and that's 
> it?

No, it's a bit more complicated than that. It wouldn't be my choice either, but now I'm familiar with it, I find it quite charming. Let me see if I can explain:

Check out common/cmd_gpio.c.
The first part pulls in platform specific headers and checks if the macro name_to_gpio is defined. In case of Blackfin (and with my patch for i.MX28) it is defined by the macro trailing the static function implementation. For all other platforms it's not defined and in that case a it will be defined with a generic strtoul implementation:
...
#include <asm/gpio.h>

#ifndef name_to_gpio
#define name_to_gpio(name) simple_strtoul(name, NULL, 10)
#endif
...

The (static) function do_gpio executing the gpio command uses the macro name_to_gpio to convert the 3rd argument string into a pin number, allowing the platform specific header to define a static inline for alternative translations.
...
static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
…
	/* turn the gpio name into a gpio number */
	gpio = name_to_gpio(str_gpio);
	if (gpio < 0)
		goto show_usage;
…

Of course name_to_gpio could be a non-static function, allowing it to be used by other parts of the code too. And for that reason do_gpio could be non-static too.  But that would still require an ifdef-orgy like this to make it work for different diversities:
…
	/* turn the gpio name into a gpio number */
#if defined (CONFIG_MX28) || defined (CONFIG_BLACKFIN)
	gpio = name_to_gpio(str_gpio);
#else
	gpio = simple_strtoul(str_gpio, NULL, 10);
#endif
	if (gpio < 0)
		goto show_usage;
…

I suppose it's all a matter of taste. And even though we would have made a different choice, I find the blackfin approach rather charming. Since they were the first ones introducing friendly pin names, they have set the standard and I'm merely adhering to 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.

I don't see much use for this function outside the scope of cmd_gpio.c. And somebody who does need it elsewhere is always free to make it non-static.
But I can agree with you here.

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

I think the difference between both solutions is just taste. And since the blackfin solution was already there, I just adhered to it. I think having two different standards is not desirable, and changing the blackfin implementation may trigger that custodian (Mike Frysinger CC'd). I wonder if making changes to working code for a matter of taste is worth it

Robert.

please fix your client to properly wrap long lines in the non-patch area
-mike

On Tuesday 22 November 2011 10:40:24 Robert Deliën wrote:
> +static inline int name_to_gpio(const char *name)
> +{
> +       int     pin_nr;

single space after that "int"

> +       if (name[0] >= '0' && name[0] <= '9') {
> +               pin_nr = simple_strtoul(name, (char **)&name, 10);

NAK: that 2nd arg is wrong.  if you want to check the endp value, then declare 
a local variable and pass that in.

same goes to all other calls to simple_strtoul() in this func
-mike

On Tuesday 22 November 2011 15:49:06 Robert Deliën wrote:
> Of course name_to_gpio could be a non-static function, allowing it to be
> used by other parts of the code too. And for that reason do_gpio could be
> non-static too.  But that would still require an ifdef-orgy like this to
> make it work for different diversities:

no it doesn't.  you can have it be an external function in the gpio.c file like 
Marek wants, and then the header simply does:
	extern int name_to_gpio(unsigned gpio);
	#define name_to_gpio(gpio) name_to_gpio(gpio)
-mike

> On Tuesday 22 November 2011 15:49:06 Robert Deliën wrote:
> > Of course name_to_gpio could be a non-static function, allowing it to be
> > used by other parts of the code too. And for that reason do_gpio could be
> > non-static too.  But that would still require an ifdef-orgy like this to
> 
> > make it work for different diversities:
> no it doesn't.  you can have it be an external function in the gpio.c file
> like Marek wants, and then the header simply does:
> 	extern int name_to_gpio(unsigned gpio);
> 	#define name_to_gpio(gpio) name_to_gpio(gpio)
> -mike

Man, this is really ... urgh. But fine, it's better than it was anyway.

M

On 22/11/2011 16:40, Robert Deliën wrote:
> This patch adds adds pin name support in the GPIO driver. With this patch applied, the gpio command supports pins to be addressed with friendly names.
> 
> The user's manual refers to pins by the bank number they're in and their number within that bank. With this patch the friendly naming convention to address pin number 5 in bank 3 is b3p5. But names like B00000003p005 are interpreted correctly too.
> 
> Signed-off-by: Robert Deliën <robert at delien.nl>
> diff --git a/arch/arm/include/asm/arch-mx28/gpio.h b/arch/arm/include/asm/arch-mx28/gpio.h
> index be1c944..5ae66e6 100644
> --- a/arch/arm/include/asm/arch-mx28/gpio.h
> +++ b/arch/arm/include/asm/arch-mx28/gpio.h
> @@ -23,8 +23,44 @@
>  #ifndef        __MX28_GPIO_H__
>  #define        __MX28_GPIO_H__
>  
> +#include <linux/ctype.h>
> +#include <asm/errno.h>
> +#include <asm/arch/iomux.h>
> +
>  #ifdef CONFIG_MXS_GPIO
>  void mxs_gpio_init(void);
> +
> +static inline int name_to_gpio(const char *name)
> +{
> +       int     pin_nr;
> +
> +       if (name[0] >= '0' && name[0] <= '9') {
> +               pin_nr = simple_strtoul(name, (char **)&name, 10);
> +               if (name[0])
> +                       return -EINVAL;

Why do we need this check ? We have already copied / converted the
number in pin_nr, we do not need to check in the string again. And you
do not need to update name, because you are returning in any case.

But still you should check for (name != NULL) before accessing with
(name[0] >= '0' && name[0] <= '9')

> +
> +       if (tolower(name[0]) == 'b') {
> +               name++;
> +               pin_nr = (simple_strtoul(name, (char **)&name, 10) << MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK;

Does the name come from the user with the "gpio" command, right ? Then
the user can set any possible value here. Should we not check for the
maximum accepted value for MX28 GPIO before shifting ?

> +       } else
> +               return -EINVAL;
> +
> +       if (tolower(name[0]) == 'p') {
> +               name++;
> +               pin_nr |= (simple_strtoul(name, (char **)&name, 10) << MXS_PAD_PIN_SHIFT) & MXS_PAD_PIN_MASK;
> +       } else
> +               return -EINVAL;

Ditto

Best regards,
Stefano Babic

+       if (name[0] >= '0' && name[0] <= '9') {
+               pin_nr = simple_strtoul(name, (char **)&name, 10);
+               if (name[0])
+                       return -EINVAL;

Why do we need this check ? We have already copied / converted the
number in pin_nr, we do not need to check in the string again. And you
do not need to update name, because you are returning in any case.

This check checks if name pointer - after it's updated by simple_strtoul - points to the terminating NULL character. If if does, name contained a valid number. If it doesn't, it's pointing to trailing garbage and hence name didn't contain a valid number.
In other words; This checks prevents a string like 3kjh;khji being interpreted as 3.

But still you should check for (name != NULL) before accessing with
(name[0] >= '0' && name[0] <= '9')

Didn't do that because it's a static function. But since we're going to change it to a non-static, this check will be added.

+       if (tolower(name[0]) == 'b') {
+               name++;
+               pin_nr = (simple_strtoul(name, (char **)&name, 10) << MXS_PAD_BANK_SHIFT) & MXS_PAD_BANK_MASK;

Does the name come from the user with the "gpio" command, right ? Then
the user can set any possible value here. Should we not check for the
maximum accepted value for MX28 GPIO before shifting ?

Correct; name the third gpio command line argument.
I don't see things going horribly wrong here. The ul value is shifted up and masked with an identically shifted up mask of 0x1F. So any value above 31 will be stripped of it's more significant bits and actual value will wrap around. In other words, a value of 34 will end of a a value of 2, hence without being caught as an invalid number. To me this is expected behavior on a 32-bit system, but opinions may vary. I can check it here too, but then it'd be a duplicate of what's already checked in my patch on gpio_request.

Perhaps it's best to move the pin validity check name_to_gpio. I've put int in gpio_request because Blackfin put it there, but name_to_gpio really makes more sense. Marek suggested a separate function checking pin validity, but I would like to suggest to do this check in name_to_gpio, because this translation produces multiple parameters that need to be checked, in this case a bank and a pin number. And to perform a proper check, both of these paremeters must be preserved in full and be parse to the validity check. On other platforms these may be two different - incompatible - parameters, so I think validity checking during the conversion is a good trade-off.

+       if (tolower(name[0]) == 'p') {
+               name++;
+               pin_nr |= (simple_strtoul(name, (char **)&name, 10) << MXS_PAD_PIN_SHIFT) & MXS_PAD_PIN_MASK;

Ditto

Ditto; The bank number is trimmed down to it's intended width of 3 bits. A value of 5 and higher is translated, but later on rejected by the validity check in gpio_request.

> Man, this is really ... urgh. But fine, it's better than it was anyway.

I'm getting the impression that the "better than it was"-standard isn't applied consistently.

On Wednesday 23 November 2011 04:51:45 Robert Deliën wrote:
> +       if (name[0] >= '0' && name[0] <= '9') {
> +               pin_nr = simple_strtoul(name, (char **)&name, 10);
> +               if (name[0])
> +                       return -EINVAL;
> 
> Why do we need this check ? We have already copied / converted the
> number in pin_nr, we do not need to check in the string again. And you
> do not need to update name, because you are returning in any case.
> 
> This check checks if name pointer - after it's updated by simple_strtoul -
> points to the terminating NULL character. If if does, name contained a
> valid number. If it doesn't, it's pointing to trailing garbage and hence
> name didn't contain a valid number. In other words; This checks prevents a
> string like 3kjh;khji being interpreted as 3.

your client apparently lacks proper quoting.  please do something about this.  
it's fairly difficult to figure out what you're saying and what you're quoting.
-mike

On Tuesday 22 November 2011 17:37:49 Marek Vasut wrote:
> > like Marek wants, and then the header simply does:
> > 	extern int name_to_gpio(unsigned gpio);
> > 	#define name_to_gpio(gpio) name_to_gpio(gpio)
> 
> Man, this is really ... urgh. But fine, it's better than it was anyway.

this is the SOP in many Linux APIs, and is simple for keeping down duplication 
while allowing customizations of a tiny subset

if you have a better proposal then let's hear it
-mike