[U-Boot] [PATCH] M28: GPIO pin validity check added

[Robert Deliën]

>> +#define MX28_BANK0_PINS		29
>> +#define MX28_BANK1_PINS		32
>> +#define MX28_BANK2_PINS		28
>> +#define MX28_BANK3_PINS		31
>> +#define MX28_BANK4_PINS		21
>> +
>> +#define MX23_BANK0_PINS		32
>> +#define MX23_BANK1_PINS		31
>> +#define MX23_BANK2_PINS		32
>> +
> 
> Do we need this in the header file?

Not really, but I hate magic numbers and macros are free.

>> +	/* check bank and pin number for validity */
>> +	if (gpio_invalid(gpio)) {
> 
> gpio_is_valid() might be a better name?

Yes, it is. In fact, I used that name at first. But it's non-statice, accessible externally, so it should return -EINVAL in case the pin is not valid and 0 in case it is. That would lead to this weird construction when actually using it:
…
if (gpio_is_valid(gp)) {
	printf("Pin is invalid\n");
}
...

>> +		printf("gpio: invalid pin %u\n", gpio);
>> +		return (-1);
> 
> return -EINVAL;

Did that too initially, but U-Boot produced an error about not exiting the shell when returned that value. Since the rest of the code returns -1 on error everywhere, I decided to copy that.

> Why not move it above, since the ifdef CONFIG_MX28 etc. are already there and 
> define it twice. It'd be clearer.

Agreed; I'll probably slip that in later, with another patch. 

> Also, it's quite obvious those are pin counts, 
> so why introduce these defines and not put only plain numbers here. The 
> MX28_BANK0_PINS etc are used only here anyway.

You're right, but I just don't like magic numbers.

By the way: I started testing gpio on my evk board, but I didn't see any pins being toggled. Not even before all of my patches. Is this part finished/tested?

Robert.