* [U-Boot] [PATCH] Add 'led' command
@ 2011-03-01 21:02 Jason Kridner
2011-04-20 22:04 ` Wolfgang Denk
0 siblings, 1 reply; 4+ messages in thread
From: Jason Kridner @ 2011-03-01 21:02 UTC (permalink / raw)
To: u-boot
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 at lists.denx.de/msg09593.html.
Updated based on feedback:
http://www.mail-archive.com/u-boot at 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
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 at 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"
+);
--
1.5.6.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] Add 'led' command
2011-03-01 21:02 [U-Boot] [PATCH] Add 'led' command Jason Kridner
@ 2011-04-20 22:04 ` Wolfgang Denk
[not found] ` <BANLkTinRwxu7FxdB33sJaF=F=EuYk+pbFg@mail.gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Wolfgang Denk @ 2011-04-20 22:04 UTC (permalink / raw)
To: u-boot
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
--
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 at 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [beagleboard] Re: [PATCH] Add 'led' command
[not found] ` <BANLkTinRwxu7FxdB33sJaF=F=EuYk+pbFg@mail.gmail.com>
@ 2011-04-21 13:17 ` Jason Kridner
2011-04-21 14:49 ` Jason Kridner
0 siblings, 1 reply; 4+ messages in thread
From: Jason Kridner @ 2011-04-21 13:17 UTC (permalink / raw)
To: u-boot
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 at 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 at googlegroups.com.
>> To unsubscribe from this group, send email to
>> beagleboard+unsubscribe at googlegroups.com.
>> For more options, visit this group at
>> http://groups.google.com/group/beagleboard?hl=en.
>>
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [beagleboard] Re: [PATCH] Add 'led' command
2011-04-21 13:17 ` [U-Boot] [beagleboard] " Jason Kridner
@ 2011-04-21 14:49 ` Jason Kridner
0 siblings, 0 replies; 4+ messages in thread
From: Jason Kridner @ 2011-04-21 14:49 UTC (permalink / raw)
To: u-boot
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 at 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 at googlegroups.com.
>>> To unsubscribe from this group, send email to
>>> beagleboard+unsubscribe at googlegroups.com.
>>> For more options, visit this group at
>>> http://groups.google.com/group/beagleboard?hl=en.
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-04-21 14:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-01 21:02 [U-Boot] [PATCH] Add 'led' command Jason Kridner
2011-04-20 22:04 ` Wolfgang Denk
[not found] ` <BANLkTinRwxu7FxdB33sJaF=F=EuYk+pbFg@mail.gmail.com>
2011-04-21 13:17 ` [U-Boot] [beagleboard] " Jason Kridner
2011-04-21 14:49 ` Jason Kridner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.