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.

index be1c944..5ae66e6 100644
@@ -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;
+               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;
+
+       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)
+
 #else
 inline void mxs_gpio_init(void) {}
 #endif

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.

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

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.

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.

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

Agreed; Will do!

See above.

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

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

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.

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

M

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.

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.

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.

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
-------------- 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/20111122/05131e2c/attachment.pgp>

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

M

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

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
-------------- 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/20111123/cbe0d229/attachment.pgp>

please fix your client to properly wrap long lines in the non-patch area -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/20111122/727f5da7/attachment.pgp>

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

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
-------------- 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/20111123/d06133c8/attachment.pgp>