[U-Boot] [RFC] Add 'led' command

[Simon Glass]

Hi,

On Tue, Dec 13, 2011 at 3:55 PM, Ulf Samuelsson
<ulf_samuelsson at telia.com> wrote:
> On 2010-11-05 13:21, Wolfgang Denk wrote:
>>
>> Dear Jason Kridner,
>>
>> In message<1288936236-30603-1-git-send-email-jkridner at beagleboard.org>
>>  you wrote:
>>>
>>> It is desired to have the led command on the BeagleBoard to allow for
>>> some
>>> interaction in the scripts.
>>>
>>> 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 ]
>>>
>>> Adds configuration item CONFIG_CMD_LED enabling the command.
>>>
>>> Partially based on patch from Ulf Samuelsson:
>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
>>>
>>> Signed-off-by: Jason Kridner<jkridner at beagleboard.org>
>>> ---
>>>  common/Makefile  |    1 +
>>>  common/cmd_led.c |  207
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 208 insertions(+), 0 deletions(-)
>>>  create mode 100644 common/cmd_led.c
>>
>> I understand the requirement, but I think it is more than time to come
>> up with a common solution here instead of adding more and more copies
>> of very similar code.
>>
>> We already have:
>>
>>        arch/arm/cpu/arm920t/ep93xx/led.c
>
> The file below is mapping the led commands I.E: red_led_on
> to at91 I/O calls like at91_set_gpio_value.
> A merge, means that a common way of setting GPIO must be available
>
>>        arch/arm/cpu/arm926ejs/at91/led.c
>
>
>
>
> The only thing the files below do are to initialize the pins for the LED.
> While the actual pins are hidden in the configs,
> you have a variation of which LEDs are available.
> Also you need to enable different clocks.
> It is really an extension of the board init file.
> Maybe they should be merged with the board file instead.
> I personally don't see a problem with keeping the separate file.
>
> There is no commonality between these files and the led.c
> in "arch/arm/cpu/arm926ejs/at91/led.c"
>
>
>>        board/atmel/at91cap9adk/led.c
>>        board/atmel/at91rm9200dk/led.c
>>        board/atmel/at91rm9200ek/led.c
>>        board/atmel/at91sam9260ek/led.c
>>        board/atmel/at91sam9261ek/led.c
>>        board/atmel/at91sam9263ek/led.c
>>        board/atmel/at91sam9m10g45ek/led.c
>>        board/atmel/at91sam9rlek/led.c
>>        board/ronetix/pm9261/led.c
>>        board/ronetix/pm9263/led.c
>>        board/eukrea/cpu9260/led.c
>
>
>
>>        board/logicpd/zoom2/led.c
>>        board/ns9750dev/led.c
>>        board/psyent/pk1c20/led.c
>>
>> Please, let's unify these.
>>
>> Best regards,
>>
>> Wolfgang Denk
>>
>
> The proposed patch is adding a command, and
> which uses the coloured LED interface and there
> is no commonality between this code and
> the code in the board and cpu directories.

IMO this LED interface is not very nice. Is there a reason there is
not just one function? Perhaps like:

enum led_colour {
   LED_GREEN,
   LED_YELLOW,
   LED_RED,
   LED_BLUE,
};

void led_init(void);

/**
 * Set the state of a coloured LED
 *
 * @param led   LED to adjust
 * @param on    1 to turn it on, 0 to turn it off
 */
void led_set_state(enum led_colour led, int on);

instead of:

extern void	coloured_LED_init (void);
extern void	red_LED_on(void);
extern void	red_LED_off(void);
extern void	green_LED_on(void);
extern void	green_LED_off(void);
extern void	yellow_LED_on(void);
extern void	yellow_LED_off(void);
extern void	blue_LED_on(void);
extern void	blue_LED_off(void);

and associated weak symbols.

It may even be possible to tidy up the existing status_led.h file at
the same time.

I agree that the command is sort-of sideways, but really I think this
should be cleaned up before adding more code that uses the API. It
just makes the job harder.

Regards,
Simon

>
> --
> Best Regards
> Ulf Samuelsson
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot