[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Albin Tonnerre]

Currently, at91/led.c only provides _on and _off functions for green,
yellow and red LEDs. This patch provides a generic coloured_LED_init
function, which is a first step towards getting rid of the
board-specific (and duplicated) board/*/*/led.c files on at91
This patch alos adds similar support for blue LEDs, mostly for the sake
of completeness.

Signed-off-by: Albin Tonnerre <albin.tonnerre@free-electrons.com>
---
 cpu/arm926ejs/at91/led.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/cpu/arm926ejs/at91/led.c b/cpu/arm926ejs/at91/led.c
index be68f59..f6b1912 100644
--- a/cpu/arm926ejs/at91/led.c
+++ b/cpu/arm926ejs/at91/led.c
@@ -27,6 +27,26 @@
 #include <asm/arch/gpio.h>
 #include <asm/arch/io.h>
 
+void coloured_LED_init(void)
+{
+#ifdef CONFIG_RED_LED
+	at91_set_gpio_output(CONFIG_RED_LED, 0);
+	red_LED_off();
+#endif
+#ifdef CONFIG_GREEN_LED
+	at91_set_gpio_output(CONFIG_GREEN_LED, 0);
+	green_LED_off();
+#endif
+#ifdef CONFIG_YELLOW_LED
+	at91_set_gpio_output(CONFIG_YELLOW_LED, 0);
+	yellow_LED_off();
+#endif
+#ifdef CONFIG_BLUE_LED
+	at91_set_gpio_output(CONFIG_BLUE_LED, 0);
+	blue_LED_off();
+#endif
+}
+
 #ifdef CONFIG_RED_LED
 void red_LED_on(void)
 {
@@ -62,3 +82,15 @@ void yellow_LED_off(void)
 	at91_set_gpio_value(CONFIG_YELLOW_LED, 1);
 }
 #endif
+
+#ifdef CONFIG_BLUE_LED
+void blue_LED_on(void)
+{
+	at91_set_gpio_value(CONFIG_BLUE_LED, 0);
+}
+
+void blue_LED_off(void)
+{
+	at91_set_gpio_value(CONFIG_BLUE_LED, 1);
+}
+#endif
-- 
1.6.0.4

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:10 Wed 12 Aug     , Albin Tonnerre wrote:
> Currently, at91/led.c only provides _on and _off functions for green,
> yellow and red LEDs. This patch provides a generic coloured_LED_init
> function, which is a first step towards getting rid of the
> board-specific (and duplicated) board/*/*/led.c files on at91
> This patch alos adds similar support for blue LEDs, mostly for the sake
> of completeness.
> 
we do need to stop adding more and more specific led api
we do need to have a common api that we can use on any arch for c & asm

Best Regards,
J.

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Albin Tonnerre]

On Wed, Aug 12, 2009 at 11:15:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> On 18:10 Wed 12 Aug     , Albin Tonnerre wrote:
> > Currently, at91/led.c only provides _on and _off functions for green,
> > yellow and red LEDs. This patch provides a generic coloured_LED_init
> > function, which is a first step towards getting rid of the
> > board-specific (and duplicated) board/*/*/led.c files on at91
> > This patch alos adds similar support for blue LEDs, mostly for the sake
> > of completeness.
> > 
> we do need to stop adding more and more specific led api
> we do need to have a common api that we can use on any arch for c & asm

This does not *add* specific LED API, it merely implements the existing one.
There /is/ a common led API, which currently has no support for ARM, as you
probably know.
Does you message mean you'd accept a patch which drops the coloured-LED API
(whose only user is currently ARM) and use the one described in
include/status_led.h ?

Regards,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:39 Wed 12 Aug     , Albin Tonnerre wrote:
> On Wed, Aug 12, 2009 at 11:15:26PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > On 18:10 Wed 12 Aug     , Albin Tonnerre wrote:
> > > Currently, at91/led.c only provides _on and _off functions for green,
> > > yellow and red LEDs. This patch provides a generic coloured_LED_init
> > > function, which is a first step towards getting rid of the
> > > board-specific (and duplicated) board/*/*/led.c files on at91
> > > This patch alos adds similar support for blue LEDs, mostly for the sake
> > > of completeness.
> > > 
> > we do need to stop adding more and more specific led api
> > we do need to have a common api that we can use on any arch for c & asm
> 
> This does not *add* specific LED API, it merely implements the existing one.
> There /is/ a common led API, which currently has no support for ARM, as you
> probably know.
> Does you message mean you'd accept a patch which drops the coloured-LED API
> (whose only user is currently ARM) and use the one described in
> include/status_led.h ?
no please take a look on the other LED thread

I want to hape coloured-LED api and numbered led handle by the same api
with the less size impact

for c & assembly

Best Regards,
J.

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Albin Tonnerre]

On Tue, Aug 18, 2009 at 12:51:48AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> no please take a look on the other LED thread

Would you please provide a pointer to this thread ? THe only one remotely
related I can find is
http://lists.denx.de/pipermail/u-boot/2009-May/052160.html, and you did not
participate in this one ...

> I want to hape coloured-LED api and numbered led handle by the same api
> with the less size impact

When you have this same API, you'll still need code to turn LEDs on and off, and
you'll still have to beat the LED code out of cards-specific code because it
really ought to be in a common file. My patch does exactly that, does *not*
impact size, and is an actual step towards whatever you plan next. I'd prefer if
you were rejecting this on actual, justified grounds.

Regards,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090818/341f80a9/attachment.pgp 

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:49 Tue 18 Aug     , Albin Tonnerre wrote:
> On Tue, Aug 18, 2009 at 12:51:48AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > no please take a look on the other LED thread
> 
> Would you please provide a pointer to this thread ? THe only one remotely
> related I can find is
> http://lists.denx.de/pipermail/u-boot/2009-May/052160.html, and you did not
> participate in this one ...
I've as I'm the one who ask Daniel Gorsulowski to base his new code (this patch)
against Ulf precedent patch and as other people have done the same comment as I will do
no need to repeat it
> 
> > I want to hape coloured-LED api and numbered led handle by the same api
> > with the less size impact
> 
> When you have this same API, you'll still need code to turn LEDs on and off, and
> you'll still have to beat the LED code out of cards-specific code because it
> really ought to be in a common file. My patch does exactly that, does *not*
> impact size, and is an actual step towards whatever you plan next. I'd prefer if
> you were rejecting this on actual, justified grounds.
your patch does not do it
1) you force the init led to be common which must be board specific as every one can do it
how he want

2) you do brake all at91 boards too

3) as already said multiple time we need to cleanup it and stop adding new color support
for eachi board or arch specialy when no board use it

Best Regards,
J.

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Albin Tonnerre]

On Thu, Aug 20, 2009 at 02:00:45AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> On 10:49 Tue 18 Aug     , Albin Tonnerre wrote:
> > On Tue, Aug 18, 2009 at 12:51:48AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > no please take a look on the other LED thread
> > 
> > Would you please provide a pointer to this thread ? THe only one remotely
> > related I can find is
> > http://lists.denx.de/pipermail/u-boot/2009-May/052160.html, and you did not
> > participate in this one ...
> I've as I'm the one who ask Daniel Gorsulowski to base his new code (this patch)
> against Ulf precedent patch and as other people have done the same comment as I will do
> no need to repeat it

Ok, so I'm really confused now. This patch does exactly what you're arguing
against in the rest of your mail, and you still don't provide a pointer to Ulf's
patch.
Would you mind *explaining* to me what your plan is? I just can't get it.

Regards,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090820/8f5b9d58/attachment.pgp 

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Wolfgang Denk]

Dear Albin Tonnerre,

In message <20090820083619.GB5086@pc-ras4041.res.insa> you wrote:
> 
> Ok, so I'm really confused now. This patch does exactly what you're arguing
> against in the rest of your mail, and you still don't provide a pointer to Ulf's
> patch.
> Would you mind *explaining* to me what your plan is? I just can't get it.

I know this doesn't help you, but maybe it solaces you: I'm confused
as well. I completely los track of what might be needed to get changed
to make this patch acceptable.

Jean-Christophe, would you please be so kind and elucidate?

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
Don't you know anything? I should have thought anyone knows that  who
knows anything about anything...      - Terry Pratchett, _Soul Music_

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:36 Thu 20 Aug     , Albin Tonnerre wrote:
> On Thu, Aug 20, 2009 at 02:00:45AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > On 10:49 Tue 18 Aug     , Albin Tonnerre wrote:
> > > On Tue, Aug 18, 2009 at 12:51:48AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > > no please take a look on the other LED thread
> > > 
> > > Would you please provide a pointer to this thread ? THe only one remotely
> > > related I can find is
> > > http://lists.denx.de/pipermail/u-boot/2009-May/052160.html, and you did not
> > > participate in this one ...
> > I've as I'm the one who ask Daniel Gorsulowski to base his new code (this patch)
> > against Ulf precedent patch and as other people have done the same comment as I will do
> > no need to repeat it
> 
> Ok, so I'm really confused now. This patch does exactly what you're arguing
> against in the rest of your mail, and you still don't provide a pointer to Ulf's
> patch.
> Would you mind *explaining* to me what your plan is? I just can't get it.
something like this for assembly

.macro set_led \num, \state
	call (c or assembly) set_led_state num state
.endm

.macore set_led_red \state
	call (c or assembly) set_led_state CONFIG_SYS_LED_RED state
.endm

and for c

void set_led(int num, int state)
{
	....
}

void set_reg_led(int state)
{
	set_led(CONFIG_SYS_LED_RED, state);
}

etc... for all colour

maybe an array could help

struct leds [] = {
#ifdef CONFIG_SYS_LED_RED
	{
		.name = "red",
		.num = CONFIG_SYS_LED_RED,
	},
#endif
#ifdef CONFIG_SYS_LED_GREEN
	{
		.name = "green",
		.num = CONFIG_SYS_LED_GREEN,
	},
#endif
};

so we can create a command to manage them

static int do_set_led(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
{
	int i;

	if argc[1] is num && < array_size of leds
	then
		set_led(num, argv[2]);
	else if argc[1] is a colour led
	then
		set_led(get_led_num(argc[1]), argc[2]);
	fi

}

so this will not be arm specific anymore

Best Regards,
J.

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Albin Tonnerre]

On Sat, 05 Sep 2009 01:47 +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> On 10:36 Thu 20 Aug     , Albin Tonnerre wrote:
> > On Thu, Aug 20, 2009 at 02:00:45AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > On 10:49 Tue 18 Aug     , Albin Tonnerre wrote:
> > > > On Tue, Aug 18, 2009 at 12:51:48AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > > > no please take a look on the other LED thread
> > > > 
> > > > Would you please provide a pointer to this thread ? THe only one remotely
> > > > related I can find is
> > > > http://lists.denx.de/pipermail/u-boot/2009-May/052160.html, and you did not
> > > > participate in this one ...
> > > I've as I'm the one who ask Daniel Gorsulowski to base his new code (this patch)
> > > against Ulf precedent patch and as other people have done the same comment as I will do
> > > no need to repeat it
> > 
> > Ok, so I'm really confused now. This patch does exactly what you're arguing
> > against in the rest of your mail, and you still don't provide a pointer to Ulf's
> > patch.
> > Would you mind *explaining* to me what your plan is? I just can't get it.
> something like this for assembly
> 
> .macro set_led \num, \state
> 	call (c or assembly) set_led_state num state
> .endm
> 
> .macore set_led_red \state
> 	call (c or assembly) set_led_state CONFIG_SYS_LED_RED state
> .endm
> 
> and for c
> 
> void set_led(int num, int state)
> {
> 	....
> }
> 
> void set_reg_led(int state)
> {
> 	set_led(CONFIG_SYS_LED_RED, state);
> }
> 
> etc... for all colour
> 

Ok. So what about implementing the current, existing LED API ?
you'd get:

void status_led_set  (int led, int state);
that sound a lot like your set_led function. And then

status_led_set(STATUS_LED_{YELLOW,BLUE,RED}, state)

which is basically the same as your set_reg_led?

Cheers,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090905/65c9c1bb/attachment.pgp 

[U-Boot] [PATCH] AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:20 Sat 05 Sep     , Albin Tonnerre wrote:
> On Sat, 05 Sep 2009 01:47 +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > On 10:36 Thu 20 Aug     , Albin Tonnerre wrote:
> > > On Thu, Aug 20, 2009 at 02:00:45AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > > On 10:49 Tue 18 Aug     , Albin Tonnerre wrote:
> > > > > On Tue, Aug 18, 2009 at 12:51:48AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > > > > no please take a look on the other LED thread
> > > > > 
> > > > > Would you please provide a pointer to this thread ? THe only one remotely
> > > > > related I can find is
> > > > > http://lists.denx.de/pipermail/u-boot/2009-May/052160.html, and you did not
> > > > > participate in this one ...
> > > > I've as I'm the one who ask Daniel Gorsulowski to base his new code (this patch)
> > > > against Ulf precedent patch and as other people have done the same comment as I will do
> > > > no need to repeat it
> > > 
> > > Ok, so I'm really confused now. This patch does exactly what you're arguing
> > > against in the rest of your mail, and you still don't provide a pointer to Ulf's
> > > patch.
> > > Would you mind *explaining* to me what your plan is? I just can't get it.
> > something like this for assembly
> > 
> > .macro set_led \num, \state
> > 	call (c or assembly) set_led_state num state
> > .endm
> > 
> > .macore set_led_red \state
> > 	call (c or assembly) set_led_state CONFIG_SYS_LED_RED state
> > .endm
> > 
> > and for c
> > 
> > void set_led(int num, int state)
> > {
> > 	....
> > }
> > 
> > void set_reg_led(int state)
> > {
> > 	set_led(CONFIG_SYS_LED_RED, state);
> > }
> > 
> > etc... for all colour
> > 
> 
> Ok. So what about implementing the current, existing LED API ?
> you'd get:
> 
> void status_led_set  (int led, int state);
> that sound a lot like your set_led function. And then
> 
> status_led_set(STATUS_LED_{YELLOW,BLUE,RED}, state)
> 
> which is basically the same as your set_reg_led?
so you use this one instead

Best Regards,
J.