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

[Robert Deliën]

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