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

[Jason Kridner]

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.
>
>
>>
>>
>> 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.
>>
>>
>