[U-Boot] [beagleboard] Re: [PATCH] Add 'led' command

[Jason Kridner]

On Thu, Apr 21, 2011 at 9:17 AM, Jason Kridner <jkridner at 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 at beagleboard.org>wrote:
>
>> On Wed, Apr 20, 2011 at 6:04 PM, Wolfgang Denk <wd at denx.de> wrote:
>>
>>> Dear Jason Kridner,
>>>
>>> In message <1299013329-29931-1-git-send-email-jkridner at 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 at 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 at googlegroups.com.
>>> To unsubscribe from this group, send email to
>>> beagleboard+unsubscribe at googlegroups.com.
>>> For more options, visit this group at
>>> http://groups.google.com/group/beagleboard?hl=en.
>>>
>>>
>>
>