Adding u-boot list.... seem to have missed it in my reply. On Thu, Apr 21, 2011 at 9:16 AM, Jason Kridner <jkrid...@beagleboard.org>wrote:
> On Wed, Apr 20, 2011 at 6:04 PM, Wolfgang Denk <w...@denx.de> wrote: > >> Dear Jason Kridner, >> >> In message <1299013329-29931-1-git-send-email-jkrid...@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 turning >> LED on */ >> > + void (*off)(void); /* Optional fucntion for turning >> LED 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: w...@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 beaglebo...@googlegroups.com. >> To unsubscribe from this group, send email to >> beagleboard+unsubscr...@googlegroups.com. >> For more options, visit this group at >> http://groups.google.com/group/beagleboard?hl=en. >> >> >
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot