Message ID | 1299013329-29931-1-git-send-email-jkridner@beagleboard.org |
---|---|
State | Changes Requested |
Headers | show |
Dear Jason Kridner, In message <1299013329-29931-1-git-send-email-jkridner@beagleboard.org> you wrote: > 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 ] I still wonder if such a patch will help to get rid of the ton of LEd drivers we already have in U-Boot, or if it just adds another such interface? Which drivers ("led.c" files) will be obsoleted by this patch? If it is intended to be a generic interface - how can this then be combined with the status_led.c driver? The configuration "green", "yellow", "red" seems to be very specific to me - I guess this applies just to very few boards? ... > +struct led_tbl_s { > + char *string; /* String for use in the command */ > + led_id_t mask; /* Mask used for calling __led_set() */ > + void (*on)(void); /* Optional fucntion for turning LED on */ > + void (*off)(void); /* Optional fucntion for turning LED on */ > +}; > + > +typedef struct led_tbl_s led_tbl_t; > + > +static const led_tbl_t led_commands[] = { > +#ifdef CONFIG_BOARD_SPECIFIC_LED > +#ifdef STATUS_LED_BIT > + { "0", STATUS_LED_BIT, NULL, NULL }, > +#endif > +#ifdef STATUS_LED_BIT1 > + { "1", STATUS_LED_BIT1, NULL, NULL }, > +#endif > +#ifdef STATUS_LED_BIT2 > + { "2", STATUS_LED_BIT2, NULL, NULL }, > +#endif > +#ifdef STATUS_LED_BIT3 > + { "3", STATUS_LED_BIT3, NULL, NULL }, > +#endif What are these "status bits" good for when they have no actual handlers attached? Where are they actually used? > +#ifdef STATUS_LED_GREEN > + { "green", STATUS_LED_GREEN, green_LED_off, green_LED_on }, > +#endif > +#ifdef STATUS_LED_YELLOW > + { "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on }, > +#endif > +#ifdef STATUS_LED_RED > + { "red", STATUS_LED_RED, red_LED_off, red_LED_on }, > +#endif > +#ifdef STATUS_LED_BLUE > + { "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on }, > +#endif We do not allow CamelCase identifiers (like "green_LED_off"). Best regards, Wolfgang Denk
Adding u-boot list.... seem to have missed it in my reply. On Thu, Apr 21, 2011 at 9:16 AM, Jason Kridner <jkridner@beagleboard.org>wrote: > On Wed, Apr 20, 2011 at 6:04 PM, Wolfgang Denk <wd@denx.de> wrote: > >> Dear Jason Kridner, >> >> In message <1299013329-29931-1-git-send-email-jkridner@beagleboard.org> >> you wrote: >> > 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 ] >> >> I still wonder if such a patch will help to get rid of the ton of LEd >> drivers we already have in U-Boot, or if it just adds another such >> interface? >> > > It neither adds nor subtracts. > > >> >> Which drivers ("led.c" files) will be obsoleted by this patch? >> > > None. The objective is simply to expose led.c functionality to a > command-line function. > > >> >> >> If it is intended to be a generic interface - how can this then be >> combined with the status_led.c driver? >> > > It is intended to utilize status_led.h and therefore to be complementary to > status_led.c. Looking at it right now, it looks like I can better reuse > some functions in that driver, so I will modify the code to do that. My > apologies for missing it. > > >> >> The configuration "green", "yellow", "red" seems to be very specific >> to me - I guess this applies just to very few boards? >> > > Yes, but it is in status_led.h, so I wanted to include the support for it. > > >> >> ... >> > +struct led_tbl_s { >> > + char *string; /* String for use in the command >> */ >> > + led_id_t mask; /* Mask used for calling >> __led_set() */ >> > + void (*on)(void); /* Optional fucntion for turning >> LED on */ >> > + void (*off)(void); /* Optional fucntion for turning >> LED on */ >> > +}; >> > + >> > +typedef struct led_tbl_s led_tbl_t; >> > + >> > +static const led_tbl_t led_commands[] = { >> > +#ifdef CONFIG_BOARD_SPECIFIC_LED >> > +#ifdef STATUS_LED_BIT >> > + { "0", STATUS_LED_BIT, NULL, NULL }, >> > +#endif >> > +#ifdef STATUS_LED_BIT1 >> > + { "1", STATUS_LED_BIT1, NULL, NULL }, >> > +#endif >> > +#ifdef STATUS_LED_BIT2 >> > + { "2", STATUS_LED_BIT2, NULL, NULL }, >> > +#endif >> > +#ifdef STATUS_LED_BIT3 >> > + { "3", STATUS_LED_BIT3, NULL, NULL }, >> > +#endif >> >> What are these "status bits" good for when they have no actual >> handlers attached? Where are they actually used? >> > > If no LED specific handler is provided, __led_set is used. I'm going to > switch this to status_led_set() based on your feedback. > > >> >> >> > +#ifdef STATUS_LED_GREEN >> > + { "green", STATUS_LED_GREEN, green_LED_off, green_LED_on }, >> > +#endif >> > +#ifdef STATUS_LED_YELLOW >> > + { "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on }, >> > +#endif >> > +#ifdef STATUS_LED_RED >> > + { "red", STATUS_LED_RED, red_LED_off, red_LED_on }, >> > +#endif >> > +#ifdef STATUS_LED_BLUE >> > + { "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on }, >> > +#endif >> >> We do not allow CamelCase identifiers (like "green_LED_off"). >> > > Easy enough to fix. > > >> >> >> Best regards, >> >> Wolfgang Denk >> >> -- >> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de >> Sometimes a man will tell his bartender things he'll never tell his >> doctor. >> -- Dr. Phillip Boyce, "The Menagerie" ("The Cage"), >> stardate unknown. >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Beagle Board" group. >> To post to this group, send email to beagleboard@googlegroups.com. >> To unsubscribe from this group, send email to >> beagleboard+unsubscribe@googlegroups.com. >> For more options, visit this group at >> http://groups.google.com/group/beagleboard?hl=en. >> >> >
On Thu, Apr 21, 2011 at 9:17 AM, Jason Kridner <jkridner@beagleboard.org>wrote: > Adding u-boot list.... seem to have missed it in my reply. > > > On Thu, Apr 21, 2011 at 9:16 AM, Jason Kridner <jkridner@beagleboard.org>wrote: > >> On Wed, Apr 20, 2011 at 6:04 PM, Wolfgang Denk <wd@denx.de> wrote: >> >>> Dear Jason Kridner, >>> >>> In message <1299013329-29931-1-git-send-email-jkridner@beagleboard.org> >>> you wrote: >>> > 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 ] >>> >>> I still wonder if such a patch will help to get rid of the ton of LEd >>> drivers we already have in U-Boot, or if it just adds another such >>> interface? >>> >> >> It neither adds nor subtracts. >> >> >>> >>> Which drivers ("led.c" files) will be obsoleted by this patch? >>> >> >> None. The objective is simply to expose led.c functionality to a >> command-line function. >> >> >>> >>> >>> If it is intended to be a generic interface - how can this then be >>> combined with the status_led.c driver? >>> >> >> It is intended to utilize status_led.h and therefore to be complementary >> to status_led.c. Looking at it right now, it looks like I can better reuse >> some functions in that driver, so I will modify the code to do that. My >> apologies for missing it. >> >> >>> >>> The configuration "green", "yellow", "red" seems to be very specific >>> to me - I guess this applies just to very few boards? >>> >> >> Yes, but it is in status_led.h, so I wanted to include the support for it. >> >> >>> >>> ... >>> > +struct led_tbl_s { >>> > + char *string; /* String for use in the command >>> */ >>> > + led_id_t mask; /* Mask used for calling >>> __led_set() */ >>> > + void (*on)(void); /* Optional fucntion for turning >>> LED on */ >>> > + void (*off)(void); /* Optional fucntion for turning >>> LED on */ >>> > +}; >>> > + >>> > +typedef struct led_tbl_s led_tbl_t; >>> > + >>> > +static const led_tbl_t led_commands[] = { >>> > +#ifdef CONFIG_BOARD_SPECIFIC_LED >>> > +#ifdef STATUS_LED_BIT >>> > + { "0", STATUS_LED_BIT, NULL, NULL }, >>> > +#endif >>> > +#ifdef STATUS_LED_BIT1 >>> > + { "1", STATUS_LED_BIT1, NULL, NULL }, >>> > +#endif >>> > +#ifdef STATUS_LED_BIT2 >>> > + { "2", STATUS_LED_BIT2, NULL, NULL }, >>> > +#endif >>> > +#ifdef STATUS_LED_BIT3 >>> > + { "3", STATUS_LED_BIT3, NULL, NULL }, >>> > +#endif >>> >>> What are these "status bits" good for when they have no actual >>> handlers attached? Where are they actually used? >>> >> >> If no LED specific handler is provided, __led_set is used. I'm going to >> switch this to status_led_set() based on your feedback. >> >> >>> >>> >>> > +#ifdef STATUS_LED_GREEN >>> > + { "green", STATUS_LED_GREEN, green_LED_off, green_LED_on }, >>> > +#endif >>> > +#ifdef STATUS_LED_YELLOW >>> > + { "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on }, >>> > +#endif >>> > +#ifdef STATUS_LED_RED >>> > + { "red", STATUS_LED_RED, red_LED_off, red_LED_on }, >>> > +#endif >>> > +#ifdef STATUS_LED_BLUE >>> > + { "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on }, >>> > +#endif >>> >>> We do not allow CamelCase identifiers (like "green_LED_off"). >>> >> >> Easy enough to fix. >> > I might have spoken too soon. Those identifiers come straight out of status_led.h. Would you like me to run a script across the entire codebase to switch them? I am doing so with: for file in `find . | grep '\.[ch]$'`; do perl -i -pe 's/(green|yellow|red|blue)_LED_(on|off)/$1_led_$2/g' $file; done Eventually, maybe I'll have my head wrapped around how all of this led.c stuff works well enough to really give you a global clean-up. I still feel like I'm being suckered into something when all I want is a command to access the functions that are already there. I guess that is the price of open source. :-) > >> >>> >>> >>> Best regards, >>> >>> Wolfgang Denk >>> >>> -- >>> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel >>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >>> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de >>> Sometimes a man will tell his bartender things he'll never tell his >>> doctor. >>> -- Dr. Phillip Boyce, "The Menagerie" ("The Cage"), >>> stardate unknown. >>> >>> -- >>> You received this message because you are subscribed to the Google Groups >>> "Beagle Board" group. >>> To post to this group, send email to beagleboard@googlegroups.com. >>> To unsubscribe from this group, send email to >>> beagleboard+unsubscribe@googlegroups.com. >>> For more options, visit this group at >>> http://groups.google.com/group/beagleboard?hl=en. >>> >>> >> >
diff --git a/common/Makefile b/common/Makefile index abea91c..762aba4 100644 --- a/common/Makefile +++ b/common/Makefile @@ -105,6 +105,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..f1e8a62 --- /dev/null +++ b/common/cmd_led.c @@ -0,0 +1,153 @@ +/* + * (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 + */ + +#include <common.h> +#include <config.h> +#include <command.h> +#include <status_led.h> + +struct led_tbl_s { + char *string; /* String for use in the command */ + led_id_t mask; /* Mask used for calling __led_set() */ + void (*on)(void); /* Optional fucntion for turning LED on */ + void (*off)(void); /* Optional fucntion for turning LED on */ +}; + +typedef struct led_tbl_s led_tbl_t; + +static const led_tbl_t led_commands[] = { +#ifdef CONFIG_BOARD_SPECIFIC_LED +#ifdef STATUS_LED_BIT + { "0", STATUS_LED_BIT, NULL, NULL }, +#endif +#ifdef STATUS_LED_BIT1 + { "1", STATUS_LED_BIT1, NULL, NULL }, +#endif +#ifdef STATUS_LED_BIT2 + { "2", STATUS_LED_BIT2, NULL, NULL }, +#endif +#ifdef STATUS_LED_BIT3 + { "3", STATUS_LED_BIT3, NULL, NULL }, +#endif +#endif +#ifdef STATUS_LED_GREEN + { "green", STATUS_LED_GREEN, green_LED_off, green_LED_on }, +#endif +#ifdef STATUS_LED_YELLOW + { "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on }, +#endif +#ifdef STATUS_LED_RED + { "red", STATUS_LED_RED, red_LED_off, red_LED_on }, +#endif +#ifdef STATUS_LED_BLUE + { "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on }, +#endif + { NULL, 0, NULL, NULL } +}; + +int str_onoff (char *var) +{ + if (strcmp(var, "off") == 0) { + return 0; + } + if (strcmp(var, "on") == 0) { + return 1; + } + return -1; +} + +int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + int state, i; + + /* Validate arguments */ + if ((argc != 3)) { + return cmd_usage(cmdtp); + } + + state = str_onoff(argv[2]); + if (state < 0) { + return cmd_usage(cmdtp); + } + + for (i = 0; led_commands[i].string; i++) { + if ((strcmp("all", argv[1]) == 0) || + (strcmp(led_commands[i].string, argv[1]) == 0)) { + if (led_commands[i].on) { + if (state) { + led_commands[i].on(); + } else { + led_commands[i].off(); + } + } else { + __led_set(led_commands[i].mask, state); + } + break; + } + } + + /* If we ran out of matches, print Usage */ + if (!led_commands[i].string && !(strcmp("all", argv[1]) == 0)) { + return cmd_usage(cmdtp); + } + + return 0; +} + +U_BOOT_CMD( + led, 3, 1, do_led, + "led\t- [" +#ifdef CONFIG_BOARD_SPECIFIC_LED +#ifdef STATUS_LED_BIT + "0|" +#endif +#ifdef STATUS_LED_BIT1 + "1|" +#endif +#ifdef STATUS_LED_BIT2 + "2|" +#endif +#ifdef STATUS_LED_BIT3 + "3|" +#endif +#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" +);
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. Updated based on feedback: http://www.mail-archive.com/u-boot@lists.denx.de/msg41847.html https://groups.google.com/d/topic/beagleboard/8Wf1HiK_QBo/discussion * Fixed a handful of style issues. * Significantly reduced the number of #ifdefs and redundant code * Converted redundant code into loops test against a structure * Made use of cmd_usage() * Introduced a str_onoff() function, but haven't yet put it in common * Eliminated trailing newline Signed-off-by: Jason Kridner <jkridner@beagleboard.org> --- common/Makefile | 1 + common/cmd_led.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c