On Friday, November 12, 2010 09:42:52 Jason Kridner wrote: > On Tue, Nov 9, 2010 at 8:52 AM, Mike Frysinger <vap...@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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot