>> +#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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
