[U-Boot] [RFC] Add 'led' command

[Mike Frysinger <vapier@gentoo.org>]

On Friday, November 12, 2010 09:42:52 Jason Kridner wrote:
> On Tue, Nov 9, 2010 at 8:52 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Friday, November 05, 2010 01:50:36 Jason Kridner wrote:
> >> +     if (strcmp(argv[2], "off") == 0) {
> >> +             state = 0;
> >> +     } else if (strcmp(argv[2], "on") == 0) {
> >> +             state = 1;
> > 
> > i could have sworn we had a helper somewhere to handle "boolean strings"
> > ...
> 
> common/cmd_cache.c has an internal on_off function.  All other places
> seem to do individual strcmp.  Let me know if you find such a helper.
> Is there value to putting this in a function like the one in
> cmd_cache.c?

i think there's value in moving this to generalizing and moving to common 
code.  there are other places where we handle env vars which could have the 
value "on" or "off".

> >> +#if defined(STATUS_LED_BIT) && defined(CONFIG_BOARD_SPECIFIC_LED)
> >> +     if (strcmp(argv[1], "0") == 0) {
> >> +             mask = STATUS_LED_BIT;
> >> +             __led_set(mask, state);
> >> +     }
> >> +     else
> >> +#endif
> >> +#if defined(STATUS_LED_BIT1) && defined(CONFIG_BOARD_SPECIFIC_LED)
> >> +     if (strcmp(argv[1], "1") == 0) {
> >> +             mask = STATUS_LED_BIT1;
> >> +             __led_set(mask, state);
> >> +     }
> >> +     else
> >> +#endif
> >> +#if defined(STATUS_LED_BIT2) && defined(CONFIG_BOARD_SPECIFIC_LED)
> >> +     if (strcmp(argv[1], "2") == 0) {
> >> +             mask = STATUS_LED_BIT2;
> >> +             __led_set(mask, state);
> >> +     }
> >> +     else
> >> +#endif
> >> +#if defined(STATUS_LED_BIT3) && defined(CONFIG_BOARD_SPECIFIC_LED)
> >> +     if (strcmp(argv[1], "3") == 0) {
> >> +             mask = STATUS_LED_BIT3;
> >> +             __led_set(mask, state);
> >> +     }
> >> +     else
> >> +#endif
> > 
> > i dont know why you need the mask variable here at all
> 
> It is an ugly hack at avoiding definition of the bit pattern to be
> passed into __led_set(), to keep that function lean as defined by the
> platform.

i dont follow.  why are these two things different ?
...
	mask = STATUS_LED_BIT3;
	__led_set(mask, state);
...
	__led_set(STATUS_LED_BIT3, state);
...

> > also, these #ifdef trees scream for some sort of unification
> 
> It impacts performance, but what do you think if I just put them into
> a data structure and loop, like what I'm suggesting above with my
> functions?

i mean at least create a single define that expands into the duplicated code
-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/20101113/e66ae631/attachment.pgp