AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

PATCH New Albin Tonnerre 3 participants 11 messages Aug 12, 2009 – Sep 5, 2009 Arm/Microchip (AT91) SoC support

Share & tools

Albin Tonnerre

author

2009-08-12 16:10 UTC #0 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

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 <[email protected]>

---
 cpu/arm926ejs/at91/led.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

cpu/arm926ejs/at91/led.c +32 -0

index be68f59..f6b1912 100644
@@ -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

Signature · 1 line

1.6.0.4

Jean-Christophe PLAGNIOL-VILLARD

2009-08-12 21:15 UTC #1 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 7 lines · 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.

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.

Albin Tonnerre

author

2009-08-12 21:39 UTC #2 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 10 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 7 lines · 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.
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,

Signature · 4 lines

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

Jean-Christophe PLAGNIOL-VILLARD

2009-08-17 22:51 UTC #3 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 12 lines · Albin Tonnerre

Quoted 10 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · 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.
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

Quoted 6 lines

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.

Albin Tonnerre

author

2009-08-18 08:49 UTC #4 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD

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

Quoted 2 lines

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,

Signature · 11 lines

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

Jean-Christophe PLAGNIOL-VILLARD

2009-08-20 00:00 UTC #5 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 7 lines · Albin Tonnerre

Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD
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

Quoted 9 lines

Quoted 3 lines
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.

Albin Tonnerre

author

2009-08-20 08:36 UTC #6 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 7 lines · Albin Tonnerre
Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD
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,

Signature · 11 lines

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

Wolfgang Denk

2009-08-21 21:41 UTC #7 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Dear Albin Tonnerre,

Quoted 5 lines · In message <[email protected]> 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

Signature · 5 lines

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_

Jean-Christophe PLAGNIOL-VILLARD

2009-09-04 23:47 UTC #8 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

In reply to Albin Tonnerre

Quoted 9 lines · Albin Tonnerre

Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · Albin Tonnerre
Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD
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 ...

Quoted 8 lines

Quoted 3 lines
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.

Albin Tonnerre

author

2009-09-05 11:20 UTC #9 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 35 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Albin Tonnerre
Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · Albin Tonnerre
Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD
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 ...
Quoted 8 lines
Quoted 3 lines
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)
{
	....
}

Quoted 7 lines

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,

Signature · 11 lines

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

Jean-Christophe PLAGNIOL-VILLARD

2009-09-05 13:31 UTC #10 lore raw

PATCH 1/1 AT91: Add support for blue_LED_* and add coloured_LED_init to at91/led.c

Quoted 47 lines · Albin Tonnerre

Quoted 35 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 9 lines · Albin Tonnerre
Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · Albin Tonnerre
Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD
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 ...
Quoted 8 lines
Quoted 3 lines
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)
{
	....
}
Quoted 7 lines
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:

Quoted 6 lines

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.