All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
@ 2009-05-06 14:21 Daniel Gorsulowski
  2009-05-06 20:37 ` Wolfgang Denk
  2009-05-07  6:31 ` Stefan Roese
  0 siblings, 2 replies; 10+ messages in thread
From: Daniel Gorsulowski @ 2009-05-06 14:21 UTC (permalink / raw)
  To: u-boot

This patch allows any at91 board, implementing the GPIO LED API,
to control the LEDs from the console.

led [ 1 | 2 | 3 | all ]  [ on | off ]

Adding configuration items CONFIG_AT91_LED and CONFIG_CMD_LED
enables the command.
Moreover the GPIO Pins have to be defined by CONFIG_USER1_LED ...
CONFIG_USER3_LED.
---
 common/Makefile                 |    1 +
 common/cmd_led.c                |   86 +++++++++++++++++++++++++++++++++++++++
 cpu/arm926ejs/at91/led.c        |   79 +++++++++++++++++++++++++++++++++++
 include/asm-arm/arch-at91/led.h |   52 +++++++++++++++++++++++
 4 files changed, 218 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_led.c
 create mode 100644 include/asm-arm/arch-at91/led.h

diff --git a/common/Makefile b/common/Makefile
index b9f4ca7..e0f571c 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -103,6 +103,7 @@ COBJS-$(CONFIG_CMD_IMMAP) += cmd_immap.o
 COBJS-$(CONFIG_CMD_IRQ) += cmd_irq.o
 COBJS-$(CONFIG_CMD_ITEST) += cmd_itest.o
 COBJS-$(CONFIG_CMD_JFFS2) += cmd_jffs2.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..f914d2d
--- /dev/null
+++ b/common/cmd_led.c
@@ -0,0 +1,86 @@
+/*
+ * (C) Copyright 2008
+ * Ulf Samuelsson <ulf.samuelsson@atmel.com>
+ *
+ * (C) Copyright 2009
+ * Daniel Gorsulowski <daniel.gorsulowski@esd.eu>
+ * esd electronic system design gmbh <www.esd.eu>
+ *
+ * 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 <asm/arch/led.h>
+
+int do_led(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	int led;
+
+	/* Validate arguments */
+	if ((argc != 3)) {
+		printf("Usage:\n%s\n", cmdtp->usage);
+		return 1;
+	}
+	if (strcmp(argv[1], "1") == 0) {
+		led = (1 << 0);
+	} else if (strcmp(argv[1], "2") == 0) {
+		led = (1 << 1);
+	} else if (strcmp(argv[1], "3") == 0) {
+		led = (1 << 2);
+	} else if (strcmp(argv[1], "all") == 0) {
+		led = 31;
+	} else {
+		printf ("Usage:\n%s\n", cmdtp->usage);
+		return 1;
+	}
+
+	if (strcmp(argv[2], "off") == 0) {
+#ifdef CONFIG_USER1_LED
+		if(led & 1) user1_led_off();
+#endif
+#ifdef CONFIG_USER2_LED
+		if(led & 2) user2_led_off();
+#endif
+#ifdef CONFIG_USER3_LED
+		if(led & 4) user3_led_off();
+#endif
+	} else if (strcmp(argv[2], "on") == 0) {
+#ifdef CONFIG_USER1_LED
+		if(led & 1) user1_led_on();
+#endif
+#ifdef CONFIG_USER2_LED
+		if(led & 2) user2_led_on();
+#endif
+#ifdef CONFIG_USER3_LED
+		if(led & 4) user3_led_on();
+#endif
+	} else {
+		printf ("Usage:\n%s\n", cmdtp->usage);
+		return 1;
+	}
+	return 0;
+}
+
+U_BOOT_CMD(
+	led, 3, 1, do_led,
+	"[1|2|3|all] [on|off]",
+	"[1|2|3|all] [on|off] sets/clears led 1,2,3"
+);
diff --git a/cpu/arm926ejs/at91/led.c b/cpu/arm926ejs/at91/led.c
index be68f59..46a74fe 100644
--- a/cpu/arm926ejs/at91/led.c
+++ b/cpu/arm926ejs/at91/led.c
@@ -3,6 +3,10 @@
  * Stelian Pop <stelian.pop@leadtechdesign.com>
  * Lead Tech Design <www.leadtechdesign.com>
  *
+ * (C) Copyright 2009
+ * Daniel Gorsulowski <daniel.gorsulowski@esd.eu>
+ * esd electronic system design gmbh <www.esd.eu>
+ *
  * See file CREDITS for list of people who contributed to this
  * project.
  *
@@ -27,6 +31,81 @@
 #include <asm/arch/gpio.h>
 #include <asm/arch/io.h>
 
+#ifdef CONFIG_AT91_LED
+
+#if defined(CONFIG_AT91RM9200)
+#include <asm/arch/at91rm9200.h>
+#elif defined(CONFIG_AT91SAM9260) || defined(CONFIG_AT91SAM9G20)
+#include <asm/arch/at91sam9260.h>
+#elif defined(CONFIG_AT91SAM9261)
+#include <asm/arch/at91sam9261.h>
+#elif defined(CONFIG_AT91SAM9263)
+#include <asm/arch/at91sam9263.h>
+#elif defined(CONFIG_AT91SAM9RL)
+#include <asm/arch/at91sam9rl.h>
+#elif defined(CONFIG_AT91CAP9)
+#include <asm/arch/at91cap9.h>
+#endif
+
+#include <asm/arch/led.h>
+
+void at91_led_init(void)
+{
+	/* Enable clock */
+	at91_sys_write(AT91_PMC_PCER, 1 << AT91SAM9263_ID_PIOB |
+				      1 << AT91SAM9263_ID_PIOCDE);
+
+#ifdef CONFIG_USER1_LED
+	at91_set_gpio_output(CONFIG_USER1_LED, 1);
+	at91_set_gpio_value(CONFIG_USER1_LED, 1);
+#endif
+#ifdef CONFIG_USER2_LED
+	at91_set_gpio_output(CONFIG_USER2_LED, 1);
+	at91_set_gpio_value(CONFIG_USER2_LED, 1);
+#endif
+#ifdef CONFIG_USER3_LED
+	at91_set_gpio_output(CONFIG_USER3_LED, 1);
+	at91_set_gpio_value(CONFIG_USER3_LED, 1);
+#endif
+}
+#ifdef CONFIG_USER1_LED
+void user1_led_on(void)
+{
+	at91_set_gpio_value(CONFIG_USER1_LED, 0);
+}
+
+void user1_led_off(void)
+{
+	at91_set_gpio_value(CONFIG_USER1_LED, 1);
+}
+#endif
+
+#ifdef CONFIG_USER2_LED
+void user2_led_on(void)
+{
+	at91_set_gpio_value(CONFIG_USER2_LED, 0);
+}
+
+void user2_led_off(void)
+{
+	at91_set_gpio_value(CONFIG_USER2_LED, 1);
+}
+#endif
+
+#ifdef CONFIG_USER3_LED
+void user3_led_on(void)
+{
+	at91_set_gpio_value(CONFIG_USER3_LED, 0);
+}
+
+void user3_led_off(void)
+{
+	at91_set_gpio_value(CONFIG_USER3_LED, 1);
+}
+#endif
+
+#endif /* CONFIG_AT91_LED */
+
 #ifdef CONFIG_RED_LED
 void red_LED_on(void)
 {
diff --git a/include/asm-arm/arch-at91/led.h b/include/asm-arm/arch-at91/led.h
new file mode 100644
index 0000000..878b2cf
--- /dev/null
+++ b/include/asm-arm/arch-at91/led.h
@@ -0,0 +1,52 @@
+/*
+ * (C) Copyright 2000-2004
+ * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+ *
+ * (C) Copyright 2009
+ * Daniel Gorsulowski <daniel.gorsulowski@esd.eu>
+ * esd electronic system design gmbh <www.esd.eu>
+ *
+ * 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
+ */
+
+#ifndef __ASM_ARCH_LED_H
+#define __ASM_ARCH_LED_H
+
+/*
+ * User LEDs API
+ */
+#ifndef	__ASSEMBLY__
+extern void	at91_led_init(void);
+extern void	user1_led_on(void);
+extern void	user1_led_off(void);
+extern void	user2_led_on(void);
+extern void	user2_led_off(void);
+extern void	user3_led_on(void);
+extern void	user3_led_off(void);
+#else
+	.extern at91_led_init
+	.extern user1_led_on
+	.extern user1_led_off
+	.extern user2_led_on
+	.extern user2_led_off
+	.extern user3_led_on
+	.extern user3_led_off
+#endif
+
+#endif /* __ASM_ARCH_LED_H */
-- 
1.6.1

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-06 14:21 [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console Daniel Gorsulowski
@ 2009-05-06 20:37 ` Wolfgang Denk
  2009-05-06 20:40   ` Mike Frysinger
  2009-05-07  6:01   ` Daniel Gorsulowski
  2009-05-07  6:31 ` Stefan Roese
  1 sibling, 2 replies; 10+ messages in thread
From: Wolfgang Denk @ 2009-05-06 20:37 UTC (permalink / raw)
  To: u-boot

Dear Daniel Gorsulowski,

In message <1241619669338-git-send-email-Daniel.Gorsulowski@esd.eu> you wrote:
> This patch allows any at91 board, implementing the GPIO LED API,
> to control the LEDs from the console.
> 
> led [ 1 | 2 | 3 | all ]  [ on | off ]
> 
> Adding configuration items CONFIG_AT91_LED and CONFIG_CMD_LED
> enables the command.
> Moreover the GPIO Pins have to be defined by CONFIG_USER1_LED ...
> CONFIG_USER3_LED.

Signed-off-by: line missing.

> ---
>  common/Makefile                 |    1 +
>  common/cmd_led.c                |   86 +++++++++++++++++++++++++++++++++++++++
>  cpu/arm926ejs/at91/led.c        |   79 +++++++++++++++++++++++++++++++++++
>  include/asm-arm/arch-at91/led.h |   52 +++++++++++++++++++++++
>  4 files changed, 218 insertions(+), 0 deletions(-)
>  create mode 100644 common/cmd_led.c
>  create mode 100644 include/asm-arm/arch-at91/led.h
> 
> diff --git a/common/Makefile b/common/Makefile
> index b9f4ca7..e0f571c 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -103,6 +103,7 @@ COBJS-$(CONFIG_CMD_IMMAP) += cmd_immap.o
>  COBJS-$(CONFIG_CMD_IRQ) += cmd_irq.o
>  COBJS-$(CONFIG_CMD_ITEST) += cmd_itest.o
>  COBJS-$(CONFIG_CMD_JFFS2) += cmd_jffs2.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

Ummm... common is for, well, for >>common<< stuff. If this code is
specific to AT91 only, it should not go into common.


...
> +U_BOOT_CMD(
> +	led, 3, 1, do_led,
> +	"[1|2|3|all] [on|off]",
> +	"[1|2|3|all] [on|off] sets/clears led 1,2,3"

Do you really say "I set a LED? I clear a LED?"  I don't think so.

> diff --git a/include/asm-arm/arch-at91/led.h b/include/asm-arm/arch-at91/led.h
> new file mode 100644
> index 0000000..878b2cf
> --- /dev/null
> +++ b/include/asm-arm/arch-at91/led.h
> @@ -0,0 +1,52 @@
> +/*
> + * (C) Copyright 2000-2004
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.

You claim that I have written any of this code?  I decline.


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
Superior ability breeds superior ambition.
	-- Spock, "Space Seed", stardate 3141.9

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-06 20:37 ` Wolfgang Denk
@ 2009-05-06 20:40   ` Mike Frysinger
  2009-05-07  6:01   ` Daniel Gorsulowski
  1 sibling, 0 replies; 10+ messages in thread
From: Mike Frysinger @ 2009-05-06 20:40 UTC (permalink / raw)
  To: u-boot

On Wednesday 06 May 2009 16:37:28 Wolfgang Denk wrote:
> In message Daniel Gorsulowski wrote:
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > +COBJS-$(CONFIG_CMD_LED) += cmd_led.o
>
> Ummm... common is for, well, for >>common<< stuff. If this code is
> specific to AT91 only, it should not go into common.

well, how common is common ?  ive put commands in there that are common to all 
Blackfin parts, but that's it ... should i be relocating them to lib_blackfin/ 
or something ?
-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/20090506/08bf0730/attachment.pgp 

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-06 20:37 ` Wolfgang Denk
  2009-05-06 20:40   ` Mike Frysinger
@ 2009-05-07  6:01   ` Daniel Gorsulowski
  2009-05-07  7:32     ` Wolfgang Denk
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Gorsulowski @ 2009-05-07  6:01 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear Daniel Gorsulowski,
> 
> In message <1241619669338-git-send-email-Daniel.Gorsulowski@esd.eu> you wrote:
>> This patch allows any at91 board, implementing the GPIO LED API,
>> to control the LEDs from the console.
>>
>> led [ 1 | 2 | 3 | all ]  [ on | off ]
>>
>> Adding configuration items CONFIG_AT91_LED and CONFIG_CMD_LED
>> enables the command.
>> Moreover the GPIO Pins have to be defined by CONFIG_USER1_LED ...
>> CONFIG_USER3_LED.
> 
> Signed-off-by: line missing.
> 
Sorry, I'll fix that by the next patch.
>> ---
>>  common/Makefile                 |    1 +
>>  common/cmd_led.c                |   86 +++++++++++++++++++++++++++++++++++++++
>>  cpu/arm926ejs/at91/led.c        |   79 +++++++++++++++++++++++++++++++++++
>>  include/asm-arm/arch-at91/led.h |   52 +++++++++++++++++++++++
>>  4 files changed, 218 insertions(+), 0 deletions(-)
>>  create mode 100644 common/cmd_led.c
>>  create mode 100644 include/asm-arm/arch-at91/led.h
>>
>> diff --git a/common/Makefile b/common/Makefile
>> index b9f4ca7..e0f571c 100644
>> --- a/common/Makefile
>> +++ b/common/Makefile
>> @@ -103,6 +103,7 @@ COBJS-$(CONFIG_CMD_IMMAP) += cmd_immap.o
>>  COBJS-$(CONFIG_CMD_IRQ) += cmd_irq.o
>>  COBJS-$(CONFIG_CMD_ITEST) += cmd_itest.o
>>  COBJS-$(CONFIG_CMD_JFFS2) += cmd_jffs2.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
> 
> Ummm... common is for, well, for >>common<< stuff. If this code is
> specific to AT91 only, it should not go into common.
> 
IMHO this code is not specific to AT91 only. Well, other architectures does not
support the CONFIG_CMD_LED yet, but they could be implemented later.
> 
> ...
>> +U_BOOT_CMD(
>> +	led, 3, 1, do_led,
>> +	"[1|2|3|all] [on|off]",
>> +	"[1|2|3|all] [on|off] sets/clears led 1,2,3"
> 
> Do you really say "I set a LED? I clear a LED?"  I don't think so.
> 
Ok, I'll write "turns led 1,2,3 on/off".
>> diff --git a/include/asm-arm/arch-at91/led.h b/include/asm-arm/arch-at91/led.h
>> new file mode 100644
>> index 0000000..878b2cf
>> --- /dev/null
>> +++ b/include/asm-arm/arch-at91/led.h
>> @@ -0,0 +1,52 @@
>> +/*
>> + * (C) Copyright 2000-2004
>> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> 
> You claim that I have written any of this code?  I decline.
> 
I took that structure from include/status_led.h, so i picked you up to the list.
But if you mean, I'll remove these lines.
> 
> Best regards,
> 
> Wolfgang Denk
>
Best Regards,
Daniel Gorsulowski

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-06 14:21 [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console Daniel Gorsulowski
  2009-05-06 20:37 ` Wolfgang Denk
@ 2009-05-07  6:31 ` Stefan Roese
  2009-05-08  5:28   ` Daniel Gorsulowski
  1 sibling, 1 reply; 10+ messages in thread
From: Stefan Roese @ 2009-05-07  6:31 UTC (permalink / raw)
  To: u-boot

Hi Daniel,

On Wednesday 06 May 2009, Daniel Gorsulowski wrote:
> This patch allows any at91 board, implementing the GPIO LED API,
> to control the LEDs from the console.
>
> led [ 1 | 2 | 3 | all ]  [ on | off ]

Why limit this to a max of 3 LED's? If this is a generic command (which I like 
btw) then we should support a user/board defined number of LED's. In your case 
it's 3, but the infrastructure should support any number.

More comments below.

<snip>

> diff --git a/common/cmd_led.c b/common/cmd_led.c
> new file mode 100644
> index 0000000..f914d2d
> --- /dev/null
> +++ b/common/cmd_led.c
> @@ -0,0 +1,86 @@
> +/*
> + * (C) Copyright 2008
> + * Ulf Samuelsson <ulf.samuelsson@atmel.com>
> + *
> + * (C) Copyright 2009
> + * Daniel Gorsulowski <daniel.gorsulowski@esd.eu>
> + * esd electronic system design gmbh <www.esd.eu>
> + *
> + * 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 <asm/arch/led.h>
> +
> +int do_led(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	int led;
> +
> +	/* Validate arguments */
> +	if ((argc != 3)) {
> +		printf("Usage:\n%s\n", cmdtp->usage);
> +		return 1;
> +	}
> +	if (strcmp(argv[1], "1") == 0) {
> +		led = (1 << 0);
> +	} else if (strcmp(argv[1], "2") == 0) {
> +		led = (1 << 1);
> +	} else if (strcmp(argv[1], "3") == 0) {
> +		led = (1 << 2);
> +	} else if (strcmp(argv[1], "all") == 0) {
> +		led = 31;
> +	} else {
> +		printf ("Usage:\n%s\n", cmdtp->usage);
> +		return 1;
> +	}

Here we have the problem with max of 3 again. Why not just scan the 2nd 
parameter as an int and use it as parameter for the following function calls 
(see below)?

> +
> +	if (strcmp(argv[2], "off") == 0) {
> +#ifdef CONFIG_USER1_LED
> +		if(led & 1) user1_led_off();
> +#endif
> +#ifdef CONFIG_USER2_LED
> +		if(led & 2) user2_led_off();
> +#endif
> +#ifdef CONFIG_USER3_LED
> +		if(led & 4) user3_led_off();
> +#endif
> +	} else if (strcmp(argv[2], "on") == 0) {
> +#ifdef CONFIG_USER1_LED
> +		if(led & 1) user1_led_on();
> +#endif
> +#ifdef CONFIG_USER2_LED
> +		if(led & 2) user2_led_on();
> +#endif
> +#ifdef CONFIG_USER3_LED
> +		if(led & 4) user3_led_on();
> +#endif

I suggest to use something like this here:

	led_nr = simple_strtoul(argv[1], NULL, 10);
	if (led_nr > CONFIG_LED_MAX) {
		printf ("Usage:\n%s\n", cmdtp->usage);
		return 1;
	}

	if (strcmp(argv[2], "off") == 0) {
		on = 1;
	} else if (strcmp(argv[2], "on") == 0) {
		on = 0;
	} else {
		printf ("Usage:\n%s\n", cmdtp->usage);
		return 1;
	}

	user_led(led_nr, on);

No ugly #ifdef's in this case. What do you think?

Best regards,
Stefan

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-07  6:01   ` Daniel Gorsulowski
@ 2009-05-07  7:32     ` Wolfgang Denk
  2009-05-08  5:37       ` Daniel Gorsulowski
  0 siblings, 1 reply; 10+ messages in thread
From: Wolfgang Denk @ 2009-05-07  7:32 UTC (permalink / raw)
  To: u-boot

Dear Daniel Gorsulowski,

In message <4A02792B.8060102@esd.eu> you wrote:
> 
> > Ummm... common is for, well, for >>common<< stuff. If this code is
> > specific to AT91 only, it should not go into common.
> > 
> IMHO this code is not specific to AT91 only. Well, other architectures does not
> support the CONFIG_CMD_LED yet, but they could be implemented later.

This is not quite correct. Actually several boards already support
this, or very similar functions:

board/amcc/taihu/taihu.c:static int do_led_ctl(cmd_tbl_t* cmd_tp, int flags, int argc, char *argv[])
board/amcc/taishan/lcd.c:static int do_led_test_off(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
board/amcc/taishan/lcd.c:static int do_led_test_on(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
board/cm5200/cmd_cm5200.c:int do_led(char *argv[])
board/pcs440ep/pcs440ep.c:int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
board/pn62/cmd_pn62.c:int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
board/tqc/tqm5200/cmd_stk52xx.c:int do_led(char *argv[])
board/trab/trab_fkt.c:int do_led (char **);
board/trab/trab_fkt.c:int do_led (char **argv)


If the code is really generic, then probably it should merge some (or
all?) of the existing implementations, too.

> >> +++ b/include/asm-arm/arch-at91/led.h
> >> @@ -0,0 +1,52 @@
> >> +/*
> >> + * (C) Copyright 2000-2004
> >> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > 
> > You claim that I have written any of this code?  I decline.
> > 
> I took that structure from include/status_led.h, so i picked you up to the list.
> But if you mean, I'll remove these lines.

I did not write this code:

-> git-blame include/status_led.h
...
de74b9ee (Wolfgang Denk    2007-10-13 21:15:39 +0200 389) /*
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 390)  * Coloured LEDs API
de74b9ee (Wolfgang Denk    2007-10-13 21:15:39 +0200 391)  */
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 392) #ifndef       __ASSEMBLY__
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 393) extern void   coloured_LED_init (void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 394) extern void   red_LED_on(void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 395) extern void   red_LED_off(void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 396) extern void   green_LED_on(void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 397) extern void   green_LED_off(void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 398) extern void   yellow_LED_on(void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 399) extern void   yellow_LED_off(void);
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 400) #else
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 401)       .extern LED_init
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 402)       .extern red_LED_on
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 403)       .extern red_LED_off
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 404)       .extern yellow_LED_on
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 405)       .extern yellow_LED_off
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 406)       .extern green_LED_on
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 407)       .extern green_LED_off
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 408) #endif
bd86220f (Peter Pearse     2007-09-18 13:07:54 +0100 409) 
c609719b (wdenk            2002-11-03 00:24:07 +0000 410) #endif        /* CONFIG_STATUS_LED    */
...

As you can see, this stuff was added in commit bd86220f by Peter Pearse.


But... if this is already present in include/status_led.h, then why do
you have to copy the code? Don't do that! Use the existing include
file instead.

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
Making files is easy under  the  UNIX  operating  system.  Therefore,
users  tend  to  create  numerous  files  using large amounts of file
space. It has been said that the only standard thing about  all  UNIX
systems  is  the  message-of-the-day  telling users to clean up their
files.                             - System V.2 administrator's guide

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-07  6:31 ` Stefan Roese
@ 2009-05-08  5:28   ` Daniel Gorsulowski
  2009-05-08  6:21     ` Stefan Roese
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Gorsulowski @ 2009-05-08  5:28 UTC (permalink / raw)
  To: u-boot

Hi Stefan,

Stefan Roese wrote:
> Hi Daniel,
> 
> On Wednesday 06 May 2009, Daniel Gorsulowski wrote:
>> This patch allows any at91 board, implementing the GPIO LED API,
>> to control the LEDs from the console.
>>
>> led [ 1 | 2 | 3 | all ]  [ on | off ]
> 
> Why limit this to a max of 3 LED's? If this is a generic command (which I like 
> btw) then we should support a user/board defined number of LED's. In your case 
> it's 3, but the infrastructure should support any number.
> 
> More comments below.
> 
> <snip>
> 
...
> 
> I suggest to use something like this here:
> 
> 	led_nr = simple_strtoul(argv[1], NULL, 10);
> 	if (led_nr > CONFIG_LED_MAX) {
> 		printf ("Usage:\n%s\n", cmdtp->usage);
> 		return 1;
> 	}
> 
> 	if (strcmp(argv[2], "off") == 0) {
> 		on = 1;
> 	} else if (strcmp(argv[2], "on") == 0) {
> 		on = 0;
> 	} else {
> 		printf ("Usage:\n%s\n", cmdtp->usage);
> 		return 1;
> 	}
> 
> 	user_led(led_nr, on);
> 
> No ugly #ifdef's in this case. What do you think?
> 
> Best regards,
> Stefan
> 
I agree with you.
Please give me some days, to implement your basic approaches.
I've many other things to do and it's not that easy (for me)
to create a tidy patch.

Best regards,
Daniel

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-07  7:32     ` Wolfgang Denk
@ 2009-05-08  5:37       ` Daniel Gorsulowski
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Gorsulowski @ 2009-05-08  5:37 UTC (permalink / raw)
  To: u-boot

Wolfgang Denk schrieb:
> Dear Daniel Gorsulowski,
> 
> In message <4A02792B.8060102@esd.eu> you wrote:
>>> Ummm... common is for, well, for >>common<< stuff. If this code is
>>> specific to AT91 only, it should not go into common.
>>>
>> IMHO this code is not specific to AT91 only. Well, other architectures does not
>> support the CONFIG_CMD_LED yet, but they could be implemented later.
> 
> This is not quite correct. Actually several boards already support
> this, or very similar functions:
> 
> board/amcc/taihu/taihu.c:static int do_led_ctl(cmd_tbl_t* cmd_tp, int flags, int argc, char *argv[])
> board/amcc/taishan/lcd.c:static int do_led_test_off(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
> board/amcc/taishan/lcd.c:static int do_led_test_on(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
> board/cm5200/cmd_cm5200.c:int do_led(char *argv[])
> board/pcs440ep/pcs440ep.c:int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> board/pn62/cmd_pn62.c:int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> board/tqc/tqm5200/cmd_stk52xx.c:int do_led(char *argv[])
> board/trab/trab_fkt.c:int do_led (char **);
> board/trab/trab_fkt.c:int do_led (char **argv)
> 
Can you tell me a better place for at91 specific code?
I think lib_arm is not the proper place for it. I meant to place it in
arm926ejs/at91, what do you mean?
> 
...
> 
> Best regards,
> 
> Wolfgang Denk
> 
Best regards,
Daniel Gorsulowski

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-08  5:28   ` Daniel Gorsulowski
@ 2009-05-08  6:21     ` Stefan Roese
  2009-05-14  8:49       ` Daniel Gorsulowski
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Roese @ 2009-05-08  6:21 UTC (permalink / raw)
  To: u-boot

Hi Daniel,

On Friday 08 May 2009, Daniel Gorsulowski wrote:
> > I suggest to use something like this here:
> >
> > 	led_nr = simple_strtoul(argv[1], NULL, 10);
> > 	if (led_nr > CONFIG_LED_MAX) {
> > 		printf ("Usage:\n%s\n", cmdtp->usage);
> > 		return 1;
> > 	}
> >
> > 	if (strcmp(argv[2], "off") == 0) {
> > 		on = 1;
> > 	} else if (strcmp(argv[2], "on") == 0) {
> > 		on = 0;
> > 	} else {
> > 		printf ("Usage:\n%s\n", cmdtp->usage);
> > 		return 1;
> > 	}
> >
> > 	user_led(led_nr, on);
> >
> > No ugly #ifdef's in this case. What do you think?
> >
> > Best regards,
> > Stefan
>
> I agree with you.

Good. :)

> Please give me some days, to implement your basic approaches.
> I've many other things to do and it's not that easy (for me)
> to create a tidy patch.

Sure. Take your time. 

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

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

* [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console
  2009-05-08  6:21     ` Stefan Roese
@ 2009-05-14  8:49       ` Daniel Gorsulowski
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Gorsulowski @ 2009-05-14  8:49 UTC (permalink / raw)
  To: u-boot

The folowing patch tries to fix all defects of the previous patch.
The new led command is _not_ limited to 3 LEDs any more, but it
is still restricted to at91 architectures.

Best regards,
Daniel Gorsulowski

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

end of thread, other threads:[~2009-05-14  8:49 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-06 14:21 [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO LEDs from the console Daniel Gorsulowski
2009-05-06 20:37 ` Wolfgang Denk
2009-05-06 20:40   ` Mike Frysinger
2009-05-07  6:01   ` Daniel Gorsulowski
2009-05-07  7:32     ` Wolfgang Denk
2009-05-08  5:37       ` Daniel Gorsulowski
2009-05-07  6:31 ` Stefan Roese
2009-05-08  5:28   ` Daniel Gorsulowski
2009-05-08  6:21     ` Stefan Roese
2009-05-14  8:49       ` Daniel Gorsulowski

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.