Hi, On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson <ulf_samuels...@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_samuels...@telia.com> wrote: >>> >>> On 2010-11-05 13:21, Wolfgang Denk wrote: >>>> >>>> Dear Jason Kridner, >>>> >>>> In message<1288936236-30603-1-git-send-email-jkrid...@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<jkrid...@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@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot > > > > -- > Best Regards > Ulf Samuelsson > +46 722 427437 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot