Mike,
Thanks for the feedback (ack to all of it). I didn't fix-up the style from the original patch given that I figured there'd be enough comments to deal with regarding the overall architecture of it, but it seems there is a need for such a generic command and this really is the easiest way to move along the process of getting something. It isn't like this is the calling of my career to fix the LED command, but I need it and maybe I can provide some better ideas along the way without this taking an eternity...
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:
+int do_led ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[] )
much of the style of this code is broken. and i cant imagine this code compiling warning free with current master.
no spaces around the paren, and the argv has been constified.
also, this should be marked static
- if ((argc != 3)){
space before the brace and useless set of paren here
- printf("Usage:\n%s\n", cmdtp->usage);
- return 1;
return cmd_usage(cmdtp);
- 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?
static int on_off (const char *s) { if (strcmp(s, "on") == 0) { return (1); } else if (strcmp(s, "off") == 0) { return (0); } return (-1); }
- printf ("Usage:\n%s\n", cmdtp->usage);
- return 1;
return cmd_usage(cmdtp);
+#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 think a more practical approach would be to define an led_set(led_no, state) function that only ever set the state of a single LED at a time (with LEDs having an integral state, in case they want to encode color or brightness in the integer) and then to create query functions: int led_get_state(led_no) - to return the current LED state const char * led_get_description(led_no) - to return the pointer to a constant character string that could be used in prompts and a looped strcmp <--- or maybe just have a single constant string with a known separator, |, that can be reused in the usage output. int led_last_no() - to return the index number of the last LED on the board (I could stand to have it be a LED_LAST_NO definition)
In the led command, "on" would always be the value 1 and 0 would always be off, but passing an integer would be fine. I'm sure most implementations will simply be a GPIO, but I can imagine someone using the command to adjust the state of a back-light.
I'm sure a better scheme could be dreamed, but that is my simple attempt with a minimal set of functions. I think such a change would require some good cooperation with many maintainers to make sure I'm not breaking their systems.
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?
It would be possible to simply create something like const char * char my_leds = "red|yellow|green|top|bottom|backlight";
For legacy purposes, in status_led.h I could have something like I've done for the usage command today and create a "generic" driver using the existing function definitions (blue_LED_*, red_LED_*, etc.). It would mean that a structure of ifdefs would still be there, but that most implementors wouldn't need to use them and I'd replace this particular set of ifdefs with a for loop that incremented per character and used a strncmp when it found a | or \0, exiting after finding the \0.
Sound like an improvement? Shall I give the suggestion in code?
- } else {
- printf ("Usage:\n%s\n", cmdtp->usage);
- return 1;
return cmd_usage(cmptp);
files should not have trailing new lines -mike