public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [U-Boot] [RFC] Add 'led' command
@ 2010-11-05  5:50 Jason Kridner
  2010-11-05 12:21 ` Wolfgang Denk
  2010-11-09 13:52 ` Mike Frysinger
  0 siblings, 2 replies; 13+ messages in thread
From: Jason Kridner @ 2010-11-05  5:50 UTC (permalink / raw)
  To: u-boot

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 at 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 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
+ */
+
+/*
+ * 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"
+);
+
-- 
1.5.6.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-05  5:50 [U-Boot] [RFC] Add 'led' command Jason Kridner
@ 2010-11-05 12:21 ` Wolfgang Denk
  2010-11-05 13:13   ` Reinhard Meyer
  2011-12-13 23:55   ` Ulf Samuelsson
  2010-11-09 13:52 ` Mike Frysinger
  1 sibling, 2 replies; 13+ messages in thread
From: Wolfgang Denk @ 2010-11-05 12:21 UTC (permalink / raw)
  To: u-boot

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 at 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

-- 
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
Dear Lord: I just want *one* one-armed manager so  I  never  have  to
hear "On the other hand", again.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-05 12:21 ` Wolfgang Denk
@ 2010-11-05 13:13   ` Reinhard Meyer
  2010-11-05 17:04     ` Jason Kridner
  2011-12-13 23:55   ` Ulf Samuelsson
  1 sibling, 1 reply; 13+ messages in thread
From: Reinhard Meyer @ 2010-11-05 13:13 UTC (permalink / raw)
  To: u-boot

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 at 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-05 13:13   ` Reinhard Meyer
@ 2010-11-05 17:04     ` Jason Kridner
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Kridner @ 2010-11-05 17:04 UTC (permalink / raw)
  To: u-boot

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 at 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-05  5:50 [U-Boot] [RFC] Add 'led' command Jason Kridner
  2010-11-05 12:21 ` Wolfgang Denk
@ 2010-11-09 13:52 ` Mike Frysinger
  2010-11-12 14:42   ` Jason Kridner
  1 sibling, 1 reply; 13+ messages in thread
From: Mike Frysinger @ 2010-11-09 13:52 UTC (permalink / raw)
  To: 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101109/4f9a2022/attachment.pgp 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-09 13:52 ` Mike Frysinger
@ 2010-11-12 14:42   ` Jason Kridner
  2010-11-13 23:31     ` Mike Frysinger
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Kridner @ 2010-11-12 14:42 UTC (permalink / raw)
  To: u-boot

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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-12 14:42   ` Jason Kridner
@ 2010-11-13 23:31     ` Mike Frysinger
  2010-11-18 10:37       ` Mike Frysinger
  0 siblings, 1 reply; 13+ messages in thread
From: Mike Frysinger @ 2010-11-13 23:31 UTC (permalink / raw)
  To: u-boot

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101113/e66ae631/attachment.pgp 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-13 23:31     ` Mike Frysinger
@ 2010-11-18 10:37       ` Mike Frysinger
  0 siblings, 0 replies; 13+ messages in thread
From: Mike Frysinger @ 2010-11-18 10:37 UTC (permalink / raw)
  To: u-boot

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101118/6f40acc3/attachment.pgp 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2010-11-05 12:21 ` Wolfgang Denk
  2010-11-05 13:13   ` Reinhard Meyer
@ 2011-12-13 23:55   ` Ulf Samuelsson
  2011-12-14 19:11     ` Simon Glass
  1 sibling, 1 reply; 13+ messages in thread
From: Ulf Samuelsson @ 2011-12-13 23:55 UTC (permalink / raw)
  To: u-boot

On 2010-11-05 13:21, Wolfgang Denk wrote:
> 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 at 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
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.

-- 
Best Regards
Ulf Samuelsson

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2011-12-13 23:55   ` Ulf Samuelsson
@ 2011-12-14 19:11     ` Simon Glass
       [not found]       ` <4EE8FB51.8010108@telia.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2011-12-14 19:11 UTC (permalink / raw)
  To: u-boot

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 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 at 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
>
> 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
       [not found]       ` <4EE8FB51.8010108@telia.com>
@ 2011-12-14 21:31         ` Simon Glass
  2011-12-15 18:17           ` Jason Kridner
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2011-12-14 21:31 UTC (permalink / raw)
  To: u-boot

Hi,

On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
<ulf_samuelsson@telia.com> wrote:
> On 2011-12-14 20:11, Simon Glass wrote:
>>
>> 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 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 at 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
>>>
>>> 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.
>
> The reason is simple.
> The API was developed to figure out why u-boot never reached the prompt.
> The API should be so simple to use in assembler that it did not introduce
> further bugs.
>
> In assembler the call maps to something like (IIRC):
> ? ?bl ? ?green_LED_on
>
> Much easier than passing parameters in registers.

Yes I see what you mean

mov r0, #LED_GREEN
mov r1, #1
bl led_set_state

presumably you have r0 and r1 used for other things. Still it seems
painful to subject board.c to this very assembler-specific API.
Perhaps the  current API could be relegated to a driver somewhere.

> There are other advantages of doing it your way of course.
>
> If you want to change, make sure you don't break any boards.

I am cleaning this up as part of the new board.c effort, waiting on
some responses on relocation still.

Regards,
Simon

>
> BR
> Ulf Samuelsson
>
>
>> 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 at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
>
> --
> Best Regards
> Ulf Samuelsson
> +46 722 427437
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2011-12-14 21:31         ` Simon Glass
@ 2011-12-15 18:17           ` Jason Kridner
  2011-12-16  7:58             ` Ulf Samuelsson
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Kridner @ 2011-12-15 18:17 UTC (permalink / raw)
  To: u-boot

I hope I'm not too far off from understanding the comments made at the
end of this thread...

On Wed, Dec 14, 2011 at 4:31 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
> <ulf_samuelsson@telia.com> wrote:
>> On 2011-12-14 20:11, Simon Glass wrote:
>>>
>>> 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 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 at 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
>>>>
>>>> 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);

The concept of selecting an LED by color seems broken to me.  Boards
may have many LEDs of the same color.

There is value in having additional meta-data regarding an LED, such
as its color, location, silk-screen text or anticipated use.  It seems
to me all of that should be ancillary to the index of the LED.  The
information to control the LED, such as the GPIO pin or associated
register, could further be stored in the data structure associated
with an LED index.

The only reason I implemented a function with the existing API was
that the API was established across several platforms.  Trying to find
time to dedicate to cleaning up those several implementations without
having any of the hardware (meaning lots of communications to other
folks rather than just making the code work) is difficult, but I'm
happy to contribute code fragments/patches if there is interest.

My approach would be to provide an array of structures to the command
that is initialized by the board file that contains the meta data for
the LED and pointers to the functions that perform the required
manipulation.  Default functions would be the platform GPIO functions.
 The array would also have the required arguments for those
manipulation functions, ie. the pin numbers to pass to the GPIO
functions.

The base definition is the easy part.  The hard part is following up
with all of the platforms and still making sure that the super-simple
assembly functions can work efficiently.


>>>
>>> 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.
>>
>> The reason is simple.
>> The API was developed to figure out why u-boot never reached the prompt.
>> The API should be so simple to use in assembler that it did not introduce
>> further bugs.
>>
>> In assembler the call maps to something like (IIRC):
>> ? ?bl ? ?green_LED_on
>>
>> Much easier than passing parameters in registers.
>
> Yes I see what you mean
>
> mov r0, #LED_GREEN
> mov r1, #1
> bl led_set_state
>
> presumably you have r0 and r1 used for other things. Still it seems
> painful to subject board.c to this very assembler-specific API.
> Perhaps the ?current API could be relegated to a driver somewhere.

In order to enable integrating such simple, well-optimized functions
into an LED command, would it work to pass pointers to those functions
in through a data structure configured by the board.c file?

I see the other big challenge being low-level reuse of the LED
functions.  Managing allocation of the LEDs to those functions, such
as network activity or heartbeats, and away from the LED command seems
like a necessary task for the board.c file as well.  Is there a better
resource management concept we can work from?

>
>> There are other advantages of doing it your way of course.
>>
>> If you want to change, make sure you don't break any boards.
>
> I am cleaning this up as part of the new board.c effort, waiting on
> some responses on relocation still.
>
> Regards,
> Simon
>
>>
>> BR
>> Ulf Samuelsson
>>
>>
>>> 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 at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>>
>>
>> --
>> Best Regards
>> Ulf Samuelsson
>> +46 722 427437
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [U-Boot] [RFC] Add 'led' command
  2011-12-15 18:17           ` Jason Kridner
@ 2011-12-16  7:58             ` Ulf Samuelsson
  0 siblings, 0 replies; 13+ messages in thread
From: Ulf Samuelsson @ 2011-12-16  7:58 UTC (permalink / raw)
  To: u-boot

I think that before we go ahead and do changes, we should ask ourselves:
Why does a bootloader need LED support?
Just because its on the board?

Do we need to support all LEDs because they are there?

i implemented the LED command because it seemed to have some application,
even though I could not think of any specific use at the time.

Would be curious to find out what people would use the LED function for, except for debugging.
Before that is known, it is hard to Judge if a change is warranted.

Admit that using green_LED_on for my second red LED on the bord
has some drawbacks, but how do you make a generalized command.
"led 1 on" sucks as a user interface.
"led red on" is much much better.

Implementing a change which results in broken boards support is a no no.

IMHO it also has very little incremental value to do the modifications.

Best Regards
Ulf Samuelsson
ulf_samuelsson at telia.com
+46  (722) 427 437


15 dec 2011 kl. 19:17 skrev Jason Kridner <jkridner@beagleboard.org>:

> I hope I'm not too far off from understanding the comments made at the
> end of this thread...
> 
> On Wed, Dec 14, 2011 at 4:31 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>> 
>> On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
>> <ulf_samuelsson@telia.com> wrote:
>>> On 2011-12-14 20:11, Simon Glass wrote:
>>>> 
>>>> 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 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 at 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
>>>>> 
>>>>> 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);
> 
> The concept of selecting an LED by color seems broken to me.  Boards
> may have many LEDs of the same color.
> 
> There is value in having additional meta-data regarding an LED, such
> as its color, location, silk-screen text or anticipated use.  It seems
> to me all of that should be ancillary to the index of the LED.  The
> information to control the LED, such as the GPIO pin or associated
> register, could further be stored in the data structure associated
> with an LED index.
> 
> The only reason I implemented a function with the existing API was
> that the API was established across several platforms.  Trying to find
> time to dedicate to cleaning up those several implementations without
> having any of the hardware (meaning lots of communications to other
> folks rather than just making the code work) is difficult, but I'm
> happy to contribute code fragments/patches if there is interest.
> 
> My approach would be to provide an array of structures to the command
> that is initialized by the board file that contains the meta data for
> the LED and pointers to the functions that perform the required
> manipulation.  Default functions would be the platform GPIO functions.
> The array would also have the required arguments for those
> manipulation functions, ie. the pin numbers to pass to the GPIO
> functions.
> 
> The base definition is the easy part.  The hard part is following up
> with all of the platforms and still making sure that the super-simple
> assembly functions can work efficiently.
> 
> 
>>>> 
>>>> 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.
>>> 
>>> The reason is simple.
>>> The API was developed to figure out why u-boot never reached the prompt.
>>> The API should be so simple to use in assembler that it did not introduce
>>> further bugs.
>>> 
>>> In assembler the call maps to something like (IIRC):
>>>    bl    green_LED_on
>>> 
>>> Much easier than passing parameters in registers.
>> 
>> Yes I see what you mean
>> 
>> mov r0, #LED_GREEN
>> mov r1, #1
>> bl led_set_state
>> 
>> presumably you have r0 and r1 used for other things. Still it seems
>> painful to subject board.c to this very assembler-specific API.
>> Perhaps the  current API could be relegated to a driver somewhere.
> 
> In order to enable integrating such simple, well-optimized functions
> into an LED command, would it work to pass pointers to those functions
> in through a data structure configured by the board.c file?
> 
> I see the other big challenge being low-level reuse of the LED
> functions.  Managing allocation of the LEDs to those functions, such
> as network activity or heartbeats, and away from the LED command seems
> like a necessary task for the board.c file as well.  Is there a better
> resource management concept we can work from?
> 
>> 
>>> There are other advantages of doing it your way of course.
>>> 
>>> If you want to change, make sure you don't break any boards.
>> 
>> I am cleaning this up as part of the new board.c effort, waiting on
>> some responses on relocation still.
>> 
>> Regards,
>> Simon
>> 
>>> 
>>> BR
>>> Ulf Samuelsson
>>> 
>>> 
>>>> 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 at lists.denx.de
>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>> 
>>> 
>>> 
>>> --
>>> Best Regards
>>> Ulf Samuelsson
>>> +46 722 427437
>>> 
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-12-16  7:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-05  5:50 [U-Boot] [RFC] Add 'led' command Jason Kridner
2010-11-05 12:21 ` Wolfgang Denk
2010-11-05 13:13   ` Reinhard Meyer
2010-11-05 17:04     ` Jason Kridner
2011-12-13 23:55   ` Ulf Samuelsson
2011-12-14 19:11     ` Simon Glass
     [not found]       ` <4EE8FB51.8010108@telia.com>
2011-12-14 21:31         ` Simon Glass
2011-12-15 18:17           ` Jason Kridner
2011-12-16  7:58             ` Ulf Samuelsson
2010-11-09 13:52 ` Mike Frysinger
2010-11-12 14:42   ` Jason Kridner
2010-11-13 23:31     ` Mike Frysinger
2010-11-18 10:37       ` Mike Frysinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox