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

[Simon Glass]

Hi,

On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
<ulf_samuelsson at telia.com> wrote:
> On 2011-12-14 20:11, Simon Glass wrote:
>>
>> 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.
>
> The reason is simple.
> The API was developed to figure out why u-boot never reached the prompt.
> The API should be so simple to use in assembler that it did not introduce
> further bugs.
>
> In assembler the call maps to something like (IIRC):
>    bl    green_LED_on
>
> Much easier than passing parameters in registers.

Yes I see what you mean

mov r0, #LED_GREEN
mov r1, #1
bl led_set_state

presumably you have r0 and r1 used for other things. Still it seems
painful to subject board.c to this very assembler-specific API.
Perhaps the  current API could be relegated to a driver somewhere.

> There are other advantages of doing it your way of course.
>
> If you want to change, make sure you don't break any boards.

I am cleaning this up as part of the new board.c effort, waiting on
some responses on relocation still.

Regards,
Simon

>
> BR
> Ulf Samuelsson
>
>
>> 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
>
>
>
> --
> Best Regards
> Ulf Samuelsson
> +46 722 427437
>