Re: [U-Boot] [beagleboard] Re: [PATCH] Add 'led' command
Adding u-boot list.... seem to have missed it in my reply.
On Thu, Apr 21, 2011 at 9:16 AM, Jason Kridner jkridner@beagleboard.orgwrote:
On Wed, Apr 20, 2011 at 6:04 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jason Kridner,
In message 1299013329-29931-1-git-send-email-jkridner@beagleboard.org you wrote:
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
I still wonder if such a patch will help to get rid of the ton of LEd drivers we already have in U-Boot, or if it just adds another such interface?
It neither adds nor subtracts.
Which drivers ("led.c" files) will be obsoleted by this patch?
None. The objective is simply to expose led.c functionality to a command-line function.
If it is intended to be a generic interface - how can this then be combined with the status_led.c driver?
It is intended to utilize status_led.h and therefore to be complementary to status_led.c. Looking at it right now, it looks like I can better reuse some functions in that driver, so I will modify the code to do that. My apologies for missing it.
The configuration "green", "yellow", "red" seems to be very specific to me - I guess this applies just to very few boards?
Yes, but it is in status_led.h, so I wanted to include the support for it.
...
+struct led_tbl_s {
char *string; /* String for use in the command*/
led_id_t mask; /* Mask used for calling__led_set() */
void (*on)(void); /* Optional fucntion for turningLED on */
void (*off)(void); /* Optional fucntion for turningLED on */
+};
+typedef struct led_tbl_s led_tbl_t;
+static const led_tbl_t led_commands[] = { +#ifdef CONFIG_BOARD_SPECIFIC_LED +#ifdef STATUS_LED_BIT
{ "0", STATUS_LED_BIT, NULL, NULL },+#endif +#ifdef STATUS_LED_BIT1
{ "1", STATUS_LED_BIT1, NULL, NULL },+#endif +#ifdef STATUS_LED_BIT2
{ "2", STATUS_LED_BIT2, NULL, NULL },+#endif +#ifdef STATUS_LED_BIT3
{ "3", STATUS_LED_BIT3, NULL, NULL },+#endif
What are these "status bits" good for when they have no actual handlers attached? Where are they actually used?
If no LED specific handler is provided, __led_set is used. I'm going to switch this to status_led_set() based on your feedback.
+#ifdef STATUS_LED_GREEN
{ "green", STATUS_LED_GREEN, green_LED_off, green_LED_on },+#endif +#ifdef STATUS_LED_YELLOW
{ "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on },+#endif +#ifdef STATUS_LED_RED
{ "red", STATUS_LED_RED, red_LED_off, red_LED_on },+#endif +#ifdef STATUS_LED_BLUE
{ "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on },+#endif
We do not allow CamelCase identifiers (like "green_LED_off").
Easy enough to fix.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Sometimes a man will tell his bartender things he'll never tell his doctor. -- Dr. Phillip Boyce, "The Menagerie" ("The Cage"), stardate unknown.
-- You received this message because you are subscribed to the Google Groups "Beagle Board" group. To post to this group, send email to beagleboard@googlegroups.com. To unsubscribe from this group, send email to beagleboard+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/beagleboard?hl=en.
On Thu, Apr 21, 2011 at 9:17 AM, Jason Kridner jkridner@beagleboard.orgwrote:
Adding u-boot list.... seem to have missed it in my reply.
On Thu, Apr 21, 2011 at 9:16 AM, Jason Kridner jkridner@beagleboard.orgwrote:
On Wed, Apr 20, 2011 at 6:04 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jason Kridner,
In message 1299013329-29931-1-git-send-email-jkridner@beagleboard.org you wrote:
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
I still wonder if such a patch will help to get rid of the ton of LEd drivers we already have in U-Boot, or if it just adds another such interface?
It neither adds nor subtracts.
Which drivers ("led.c" files) will be obsoleted by this patch?
None. The objective is simply to expose led.c functionality to a command-line function.
If it is intended to be a generic interface - how can this then be combined with the status_led.c driver?
It is intended to utilize status_led.h and therefore to be complementary to status_led.c. Looking at it right now, it looks like I can better reuse some functions in that driver, so I will modify the code to do that. My apologies for missing it.
The configuration "green", "yellow", "red" seems to be very specific to me - I guess this applies just to very few boards?
Yes, but it is in status_led.h, so I wanted to include the support for it.
...
+struct led_tbl_s {
char *string; /* String for use in the command*/
led_id_t mask; /* Mask used for calling__led_set() */
void (*on)(void); /* Optional fucntion for turningLED on */
void (*off)(void); /* Optional fucntion for turningLED on */
+};
+typedef struct led_tbl_s led_tbl_t;
+static const led_tbl_t led_commands[] = { +#ifdef CONFIG_BOARD_SPECIFIC_LED +#ifdef STATUS_LED_BIT
{ "0", STATUS_LED_BIT, NULL, NULL },+#endif +#ifdef STATUS_LED_BIT1
{ "1", STATUS_LED_BIT1, NULL, NULL },+#endif +#ifdef STATUS_LED_BIT2
{ "2", STATUS_LED_BIT2, NULL, NULL },+#endif +#ifdef STATUS_LED_BIT3
{ "3", STATUS_LED_BIT3, NULL, NULL },+#endif
What are these "status bits" good for when they have no actual handlers attached? Where are they actually used?
If no LED specific handler is provided, __led_set is used. I'm going to switch this to status_led_set() based on your feedback.
+#ifdef STATUS_LED_GREEN
{ "green", STATUS_LED_GREEN, green_LED_off, green_LED_on },+#endif +#ifdef STATUS_LED_YELLOW
{ "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on },+#endif +#ifdef STATUS_LED_RED
{ "red", STATUS_LED_RED, red_LED_off, red_LED_on },+#endif +#ifdef STATUS_LED_BLUE
{ "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on },+#endif
We do not allow CamelCase identifiers (like "green_LED_off").
Easy enough to fix.
I might have spoken too soon. Those identifiers come straight out of status_led.h. Would you like me to run a script across the entire codebase to switch them?
I am doing so with: for file in `find . | grep '.[ch]$'`; do perl -i -pe 's/(green|yellow|red|blue)_LED_(on|off)/$1_led_$2/g' $file; done
Eventually, maybe I'll have my head wrapped around how all of this led.c stuff works well enough to really give you a global clean-up. I still feel like I'm being suckered into something when all I want is a command to access the functions that are already there. I guess that is the price of open source. :-)
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Sometimes a man will tell his bartender things he'll never tell his doctor. -- Dr. Phillip Boyce, "The Menagerie" ("The Cage"), stardate unknown.
-- You received this message because you are subscribed to the Google Groups "Beagle Board" group. To post to this group, send email to beagleboard@googlegroups.com. To unsubscribe from this group, send email to beagleboard+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/beagleboard?hl=en.
participants (1)
-
Jason Kridner