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. 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot