On Thu, Apr 21, 2011 at 9:17 AM, Jason Kridner <jkrid...@beagleboard.org>wrote:
> 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. >> > 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: 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