[U-Boot] [RFC] Add 'led' command
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridner jkridner@beagleboard.org --- common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
diff --git a/common/Makefile b/common/Makefile index 2c37073..40facb5 100644 --- a/common/Makefile +++ b/common/Makefile @@ -107,6 +107,7 @@ COBJS-$(CONFIG_CMD_IRQ) += cmd_irq.o COBJS-$(CONFIG_CMD_ITEST) += cmd_itest.o COBJS-$(CONFIG_CMD_JFFS2) += cmd_jffs2.o COBJS-$(CONFIG_CMD_CRAMFS) += cmd_cramfs.o +COBJS-$(CONFIG_CMD_LED) += cmd_led.o COBJS-$(CONFIG_CMD_LICENSE) += cmd_license.o COBJS-y += cmd_load.o COBJS-$(CONFIG_LOGBUFFER) += cmd_log.o diff --git a/common/cmd_led.c b/common/cmd_led.c new file mode 100644 index 0000000..3b7b534 --- /dev/null +++ b/common/cmd_led.c @@ -0,0 +1,207 @@ +/* + * (C) Copyright 2010 + * Jason Kridner jkridner@beagleboard.org + * + * Based on cmd_led.c patch from: + * http://www.mail-archive.com/u-boot@lists.denx.de/msg06873.html + * (C) Copyright 2008 + * Ulf Samuelsson ulf.samuelsson@atmel.com + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +/* + * This file provides a shell like 'test' function to return + * true/false from an integer or string compare of two memory + * locations or a location and a scalar/literal. + * A few parts were lifted from bash 'test' command + */ + +#include <common.h> +#include <config.h> +#include <command.h> +#include <status_led.h> + +int do_led ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[] ) +{ +#ifdef CONFIG_BOARD_SPECIFIC_LED + led_id_t mask; +#endif + int state; + + /* Validate arguments */ + if ((argc != 3)){ + printf("Usage:\n%s\n", cmdtp->usage); + return 1; + } + + if (strcmp(argv[2], "off") == 0) { + state = 0; + } else if (strcmp(argv[2], "on") == 0) { + state = 1; + } else { + printf ("Usage:\n%s\n", cmdtp->usage); + return 1; + } + +#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 +#ifdef STATUS_LED_RED + if (strcmp(argv[1], "red") == 0) { + if (state == 0) + red_LED_off(); + else + red_LED_on(); + } + else +#endif +#ifdef STATUS_LED_GREEN + if (strcmp(argv[1], "green") == 0) { + if (state == 0) + green_LED_off(); + else + green_LED_on(); + } + else +#endif +#ifdef STATUS_LED_YELLOW + if (strcmp(argv[1], "yellow") == 0) { + if (state == 0) + yellow_LED_off(); + else + yellow_LED_on(); + } + else +#endif +#ifdef STATUS_LED_BLUE + if (strcmp(argv[1], "blue") == 0) { + if (state == 0) + blue_LED_off(); + else + blue_LED_on(); + } + else +#endif + if (strcmp(argv[1], "all") == 0) { + mask = 0 +#if defined(STATUS_LED_BIT) && defined(CONFIG_BOARD_SPECIFIC_LED) + | STATUS_LED_BIT +#endif +#if defined(STATUS_LED_BIT1) && defined(CONFIG_BOARD_SPECIFIC_LED) + | STATUS_LED_BIT1 +#endif +#if defined(STATUS_LED_BIT2) && defined(CONFIG_BOARD_SPECIFIC_LED) + | STATUS_LED_BIT2 +#endif +#if defined(STATUS_LED_BIT3) && defined(CONFIG_BOARD_SPECIFIC_LED) + | STATUS_LED_BIT3 +#endif + ; +#ifdef CONFIG_BOARD_SPECIFIC_LED + __led_set(mask, state); +#endif +#ifdef STATUS_LED_RED + if (state == 0) + red_LED_off(); + else + red_LED_on(); +#endif +#ifdef STATUS_LED_GREEN + if (state == 0) + green_LED_off(); + else + green_LED_on(); +#endif +#ifdef STATUS_LED_YELLOW + if (state == 0) + yellow_LED_off(); + else + yellow_LED_on(); +#endif +#ifdef STATUS_LED_BLUE + if (state == 0) + blue_LED_off(); + else + blue_LED_on(); +#endif + } else { + printf ("Usage:\n%s\n", cmdtp->usage); + return 1; + } + + return 0; +} + +U_BOOT_CMD( + led, 3, 1, do_led, + "led\t- [" +#if defined(STATUS_LED_BIT) && defined(CONFIG_BOARD_SPECIFIC_LED) + "0|" +#endif +#if defined(STATUS_LED_BIT1) && defined(CONFIG_BOARD_SPECIFIC_LED) + "1|" +#endif +#if defined(STATUS_LED_BIT2) && defined(CONFIG_BOARD_SPECIFIC_LED) + "2|" +#endif +#if defined(STATUS_LED_BIT3) && defined(CONFIG_BOARD_SPECIFIC_LED) + "3|" +#endif +#ifdef STATUS_LED_GREEN + "green|" +#endif +#ifdef STATUS_LED_YELLOW + "yellow|" +#endif +#ifdef STATUS_LED_RED + "red|" +#endif +#ifdef STATUS_LED_BLUE + "blue|" +#endif + "all] [on|off]\n", + "led [led_name] [on|off] sets or clears led(s)\n" +); +
Dear Jason Kridner,
In message 1288936236-30603-1-git-send-email-jkridner@beagleboard.org you wrote:
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridner jkridner@beagleboard.org
common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
I understand the requirement, but I think it is more than time to come up with a common solution here instead of adding more and more copies of very similar code.
We already have:
arch/arm/cpu/arm920t/ep93xx/led.c arch/arm/cpu/arm926ejs/at91/led.c board/atmel/at91cap9adk/led.c board/atmel/at91rm9200dk/led.c board/atmel/at91rm9200ek/led.c board/atmel/at91sam9260ek/led.c board/atmel/at91sam9261ek/led.c board/atmel/at91sam9263ek/led.c board/atmel/at91sam9m10g45ek/led.c board/atmel/at91sam9rlek/led.c board/eukrea/cpu9260/led.c board/logicpd/zoom2/led.c board/ns9750dev/led.c board/psyent/pk1c20/led.c board/ronetix/pm9261/led.c board/ronetix/pm9263/led.c
Please, let's unify these.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridner jkridner@beagleboard.org
common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
I understand the requirement, but I think it is more than time to come up with a common solution here instead of adding more and more copies of very similar code.
We already have: ... arch/arm/cpu/arm926ejs/at91/led.c board/atmel/at91cap9adk/led.c board/atmel/at91rm9200dk/led.c board/atmel/at91rm9200ek/led.c board/atmel/at91sam9260ek/led.c board/atmel/at91sam9261ek/led.c board/atmel/at91sam9263ek/led.c board/atmel/at91sam9m10g45ek/led.c board/atmel/at91sam9rlek/led.c
At least the atmel stuff are functions to implement the control of the LEDs (via gpio, i2c, spi etc.) which inherently is board specific; but not a command interface to control them from u-boot prompt/scripts.
His patch tries to add a command, not a LED implementation. Such a command was on my mind for a while.
Best Regards, Reinhard
On Fri, Nov 5, 2010 at 9:13 AM, Reinhard Meyer u-boot@emk-elektronik.de wrote:
Dear Wolfgang Denk,
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridner jkridner@beagleboard.org
common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
I understand the requirement, but I think it is more than time to come up with a common solution here instead of adding more and more copies of very similar code.
We already have: ... arch/arm/cpu/arm926ejs/at91/led.c board/atmel/at91cap9adk/led.c board/atmel/at91rm9200dk/led.c board/atmel/at91rm9200ek/led.c board/atmel/at91sam9260ek/led.c board/atmel/at91sam9261ek/led.c board/atmel/at91sam9263ek/led.c board/atmel/at91sam9m10g45ek/led.c board/atmel/at91sam9rlek/led.c
At least the atmel stuff are functions to implement the control of the LEDs (via gpio, i2c, spi etc.) which inherently is board specific; but not a command interface to control them from u-boot prompt/scripts.
His patch tries to add a command, not a LED implementation. Such a command was on my mind for a while.
I tried to make it such that this command is enabled by the implementations on the other architectures by following the existing design. I don't know how they are making use of the LED functions, so it seems this command is required to make their implementations useful. I hope that is reason enough to at least get different maintainers to try this command out and give some additional feedback.
It would be great if we had a summary of how these LED functions are used. For the BeagleBoard, we are simply enabling scripts to use this command. I think others are using the LED functions to indicate boot status and other u-boot native operations. Does such a summary exist so that I can make any command implementation suitable?
Regards, Jason
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkridner@beagleboard.org you wrote:
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridnerjkridner@beagleboard.org
common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
I understand the requirement, but I think it is more than time to come up with a common solution here instead of adding more and more copies of very similar code.
We already have:
arch/arm/cpu/arm920t/ep93xx/led.c
The file below is mapping the led commands I.E: red_led_on to at91 I/O calls like at91_set_gpio_value. A merge, means that a common way of setting GPIO must be available
arch/arm/cpu/arm926ejs/at91/led.c
The only thing the files below do are to initialize the pins for the LED. While the actual pins are hidden in the configs, you have a variation of which LEDs are available. Also you need to enable different clocks. It is really an extension of the board init file. Maybe they should be merged with the board file instead. I personally don't see a problem with keeping the separate file.
There is no commonality between these files and the led.c in "arch/arm/cpu/arm926ejs/at91/led.c"
board/atmel/at91cap9adk/led.c board/atmel/at91rm9200dk/led.c board/atmel/at91rm9200ek/led.c board/atmel/at91sam9260ek/led.c board/atmel/at91sam9261ek/led.c board/atmel/at91sam9263ek/led.c board/atmel/at91sam9m10g45ek/led.c board/atmel/at91sam9rlek/led.c board/ronetix/pm9261/led.c board/ronetix/pm9263/led.c board/eukrea/cpu9260/led.c
board/logicpd/zoom2/led.c board/ns9750dev/led.c board/psyent/pk1c20/led.c
Please, let's unify these.
Best regards,
Wolfgang Denk
The proposed patch is adding a command, and which uses the coloured LED interface and there is no commonality between this code and the code in the board and cpu directories.
Hi,
On Tue, Dec 13, 2011 at 3:55 PM, Ulf Samuelsson ulf_samuelsson@telia.com wrote:
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkridner@beagleboard.org you wrote:
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridnerjkridner@beagleboard.org
common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
I understand the requirement, but I think it is more than time to come up with a common solution here instead of adding more and more copies of very similar code.
We already have:
arch/arm/cpu/arm920t/ep93xx/led.c
The file below is mapping the led commands I.E: red_led_on to at91 I/O calls like at91_set_gpio_value. A merge, means that a common way of setting GPIO must be available
arch/arm/cpu/arm926ejs/at91/led.c
The only thing the files below do are to initialize the pins for the LED. While the actual pins are hidden in the configs, you have a variation of which LEDs are available. Also you need to enable different clocks. It is really an extension of the board init file. Maybe they should be merged with the board file instead. I personally don't see a problem with keeping the separate file.
There is no commonality between these files and the led.c in "arch/arm/cpu/arm926ejs/at91/led.c"
board/atmel/at91cap9adk/led.c board/atmel/at91rm9200dk/led.c board/atmel/at91rm9200ek/led.c board/atmel/at91sam9260ek/led.c board/atmel/at91sam9261ek/led.c board/atmel/at91sam9263ek/led.c board/atmel/at91sam9m10g45ek/led.c board/atmel/at91sam9rlek/led.c board/ronetix/pm9261/led.c board/ronetix/pm9263/led.c board/eukrea/cpu9260/led.c
board/logicpd/zoom2/led.c board/ns9750dev/led.c board/psyent/pk1c20/led.c
Please, let's unify these.
Best regards,
Wolfgang Denk
The proposed patch is adding a command, and which uses the coloured LED interface and there is no commonality between this code and the code in the board and cpu directories.
IMO this LED interface is not very nice. Is there a reason there is not just one function? Perhaps like:
enum led_colour { LED_GREEN, LED_YELLOW, LED_RED, LED_BLUE, };
void led_init(void);
/** * Set the state of a coloured LED * * @param led LED to adjust * @param on 1 to turn it on, 0 to turn it off */ void led_set_state(enum led_colour led, int on);
instead of:
extern void coloured_LED_init (void); extern void red_LED_on(void); extern void red_LED_off(void); extern void green_LED_on(void); extern void green_LED_off(void); extern void yellow_LED_on(void); extern void yellow_LED_off(void); extern void blue_LED_on(void); extern void blue_LED_off(void);
and associated weak symbols.
It may even be possible to tidy up the existing status_led.h file at the same time.
I agree that the command is sort-of sideways, but really I think this should be cleaned up before adding more code that uses the API. It just makes the job harder.
Regards, Simon
-- Best Regards Ulf Samuelsson
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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" ...
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
also, these #ifdef trees scream for some sort of unification
- } else {
printf ("Usage:\n%s\n", cmdtp->usage);return 1;
return cmd_usage(cmptp);
files should not have trailing new lines -mike
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
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
On Saturday, November 13, 2010 18:31:39 Mike Frysinger wrote:
On Friday, November 12, 2010 09:42:52 Jason Kridner wrote:
On Tue, Nov 9, 2010 at 8:52 AM, Mike Frysinger 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".
ah, i found what i was thinking of. there is a "getenv_yesno" helper. obviously not the same as "on/off", but should be a good model for a new "str_onoff" helper. -mike
participants (6)
-
Jason Kridner -
Mike Frysinger -
Reinhard Meyer -
Simon Glass -
Ulf Samuelsson -
Wolfgang Denk