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