| Message ID | 1288936393-30650-1-git-send-email-jkridner@beagleboard.org |
|---|---|
| State | Superseded |
| Delegated to: | Sandeep Paulraj |
| Headers | show |
> -----Original Message----- > From: u-boot-bounces@lists.denx.de > [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Jason Kridner > Sent: Friday, November 05, 2010 11:23 AM > To: u-boot@lists.denx.de; beagleboard@googlegroups.com > Subject: [U-Boot] [PATCH] BeagleBoard: Added LED driver > > Added LED driver using status_led. USR0 is set to monitor the boot > status. USR1 is set to be the green LED. > (cherry picked from commit 048b526fd7cc0c642f27c674b3e235321c880b66) > > Signed-off-by: Jason Kridner <jkridner@beagleboard.org> > --- > board/ti/beagle/Makefile | 4 ++- > board/ti/beagle/beagle.c | 7 ++++ > board/ti/beagle/led.c | 91 > ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 101 insertions(+), 1 deletions(-) > create mode 100644 board/ti/beagle/led.c > > diff --git a/board/ti/beagle/Makefile b/board/ti/beagle/Makefile > index f797112..4cc675c 100644 > --- a/board/ti/beagle/Makefile > +++ b/board/ti/beagle/Makefile > @@ -25,8 +25,10 @@ include $(TOPDIR)/config.mk > > LIB = $(obj)lib$(BOARD).a > > -COBJS := beagle.o > +COBJS-y := $(BOARD).o > +COBJS-$(CONFIG_STATUS_LED) += led.o > > +COBJS := $(sort $(COBJS-y)) > SRCS := $(COBJS:.o=.c) > OBJS := $(addprefix $(obj),$(COBJS)) > > diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c > index 93c452e..dd7b6b5 100644 > --- a/board/ti/beagle/beagle.c > +++ b/board/ti/beagle/beagle.c > @@ -30,6 +30,9 @@ > * MA 02111-1307 USA > */ > #include <common.h> > +#ifdef CONFIG_STATUS_LED > +#include <status_led.h> > +#endif > #include <twl4030.h> > #include <asm/io.h> > #include <asm/arch/mmc_host_def.h> > @@ -78,6 +81,10 @@ int board_init(void) > /* boot param addr */ > gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100); > > +#if defined(CONFIG_STATUS_LED) && defined(STATUS_LED_BOOT) > + status_led_set (STATUS_LED_BOOT, STATUS_LED_ON); > +#endif > + > return 0; > } > > diff --git a/board/ti/beagle/led.c b/board/ti/beagle/led.c > new file mode 100644 > index 0000000..df26552 > --- /dev/null > +++ b/board/ti/beagle/led.c > @@ -0,0 +1,91 @@ > +/* > + * Copyright (c) 2010 Texas Instruments, Inc. > + * Jason Kridner <jkridner@beagleboard.org> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA > + */ > +#include <common.h> > +#include <status_led.h> > +#include <asm/arch/cpu.h> > +#include <asm/io.h> > +#include <asm/arch/sys_proto.h> > +#include <asm/arch/gpio.h> > + > +static unsigned int saved_state[2] = {STATUS_LED_OFF, > STATUS_LED_OFF}; > + > +/* GPIO pins for the LEDs */ > +#define BEAGLE_LED_USR0 149 > +#define BEAGLE_LED_USR1 150 > + > +#ifdef STATUS_LED_GREEN > +void green_LED_off (void) > +{ > + __led_set (STATUS_LED_GREEN, 0); > +} > + > +void green_LED_on (void) > +{ > + __led_set (STATUS_LED_GREEN, 1); > +} > +#endif > + > +void __led_init (led_id_t mask, int state) > +{ > + __led_set (mask, state); > +} > + > +void __led_toggle (led_id_t mask) > +{ > +#ifdef STATUS_LED_BIT > + if (STATUS_LED_BIT & mask) { > + if (STATUS_LED_ON == saved_state[0]) > + __led_set(STATUS_LED_BIT, 0); > + else > + __led_set(STATUS_LED_BIT, 1); > + } > +#endif > +#ifdef STATUS_LED_BIT1 > + if (STATUS_LED_BIT1 & mask) { > + if (STATUS_LED_ON == saved_state[1]) > + __led_set(STATUS_LED_BIT1, 0); > + else > + __led_set(STATUS_LED_BIT1, 1); > + } > +#endif > +} > + > +void __led_set (led_id_t mask, int state) > +{ > +#ifdef STATUS_LED_BIT > + if (STATUS_LED_BIT & mask) { > + if (!omap_request_gpio(BEAGLE_LED_USR0)) { > + omap_set_gpio_direction(BEAGLE_LED_USR0, 0); > + omap_set_gpio_dataout(BEAGLE_LED_USR0, state); > + } > + saved_state[0] = state; > + } > +#endif > +#ifdef STATUS_LED_BIT1 > + if (STATUS_LED_BIT1 & mask) { > + if (!omap_request_gpio(BEAGLE_LED_USR1)) { > + omap_set_gpio_direction(BEAGLE_LED_USR1, 0); > + omap_set_gpio_dataout(BEAGLE_LED_USR1, state); > + } > + saved_state[1] = state; > + } > +#endif > +} [sp] I see too many ifdef blocks in the code above. The also seems to be repetitive. Is user really expected to change the u-boot config for each LED bit/color? Can this be simplified by one function that takes an argument? Should reduce code and ifdefs - if at all - get confined to one function. ~sanjeev > + > -- > 1.5.6.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On Fri, Nov 5, 2010 at 8:02 AM, Premi, Sanjeev <premi@ti.com> wrote: >> -----Original Message----- >> From: u-boot-bounces@lists.denx.de >> [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Jason Kridner >> Sent: Friday, November 05, 2010 11:23 AM >> To: u-boot@lists.denx.de; beagleboard@googlegroups.com >> Subject: [U-Boot] [PATCH] BeagleBoard: Added LED driver >> >> Added LED driver using status_led. USR0 is set to monitor the boot >> status. USR1 is set to be the green LED. >> (cherry picked from commit 048b526fd7cc0c642f27c674b3e235321c880b66) >> >> Signed-off-by: Jason Kridner <jkridner@beagleboard.org> >> --- >> board/ti/beagle/Makefile | 4 ++- >> board/ti/beagle/beagle.c | 7 ++++ >> board/ti/beagle/led.c | 91 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 101 insertions(+), 1 deletions(-) >> create mode 100644 board/ti/beagle/led.c >> >> diff --git a/board/ti/beagle/Makefile b/board/ti/beagle/Makefile >> index f797112..4cc675c 100644 >> --- a/board/ti/beagle/Makefile >> +++ b/board/ti/beagle/Makefile >> @@ -25,8 +25,10 @@ include $(TOPDIR)/config.mk >> >> LIB = $(obj)lib$(BOARD).a >> >> -COBJS := beagle.o >> +COBJS-y := $(BOARD).o >> +COBJS-$(CONFIG_STATUS_LED) += led.o >> >> +COBJS := $(sort $(COBJS-y)) >> SRCS := $(COBJS:.o=.c) >> OBJS := $(addprefix $(obj),$(COBJS)) >> >> diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c >> index 93c452e..dd7b6b5 100644 >> --- a/board/ti/beagle/beagle.c >> +++ b/board/ti/beagle/beagle.c >> @@ -30,6 +30,9 @@ >> * MA 02111-1307 USA >> */ >> #include <common.h> >> +#ifdef CONFIG_STATUS_LED >> +#include <status_led.h> >> +#endif >> #include <twl4030.h> >> #include <asm/io.h> >> #include <asm/arch/mmc_host_def.h> >> @@ -78,6 +81,10 @@ int board_init(void) >> /* boot param addr */ >> gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100); >> >> +#if defined(CONFIG_STATUS_LED) && defined(STATUS_LED_BOOT) >> + status_led_set (STATUS_LED_BOOT, STATUS_LED_ON); >> +#endif >> + >> return 0; >> } >> >> diff --git a/board/ti/beagle/led.c b/board/ti/beagle/led.c >> new file mode 100644 >> index 0000000..df26552 >> --- /dev/null >> +++ b/board/ti/beagle/led.c >> @@ -0,0 +1,91 @@ >> +/* >> + * Copyright (c) 2010 Texas Instruments, Inc. >> + * Jason Kridner <jkridner@beagleboard.org> >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, >> + * MA 02111-1307 USA >> + */ >> +#include <common.h> >> +#include <status_led.h> >> +#include <asm/arch/cpu.h> >> +#include <asm/io.h> >> +#include <asm/arch/sys_proto.h> >> +#include <asm/arch/gpio.h> >> + >> +static unsigned int saved_state[2] = {STATUS_LED_OFF, >> STATUS_LED_OFF}; >> + >> +/* GPIO pins for the LEDs */ >> +#define BEAGLE_LED_USR0 149 >> +#define BEAGLE_LED_USR1 150 >> + >> +#ifdef STATUS_LED_GREEN >> +void green_LED_off (void) >> +{ >> + __led_set (STATUS_LED_GREEN, 0); >> +} >> + >> +void green_LED_on (void) >> +{ >> + __led_set (STATUS_LED_GREEN, 1); >> +} >> +#endif >> + >> +void __led_init (led_id_t mask, int state) >> +{ >> + __led_set (mask, state); >> +} >> + >> +void __led_toggle (led_id_t mask) >> +{ >> +#ifdef STATUS_LED_BIT >> + if (STATUS_LED_BIT & mask) { >> + if (STATUS_LED_ON == saved_state[0]) >> + __led_set(STATUS_LED_BIT, 0); >> + else >> + __led_set(STATUS_LED_BIT, 1); >> + } >> +#endif >> +#ifdef STATUS_LED_BIT1 >> + if (STATUS_LED_BIT1 & mask) { >> + if (STATUS_LED_ON == saved_state[1]) >> + __led_set(STATUS_LED_BIT1, 0); >> + else >> + __led_set(STATUS_LED_BIT1, 1); >> + } >> +#endif >> +} >> + >> +void __led_set (led_id_t mask, int state) >> +{ >> +#ifdef STATUS_LED_BIT >> + if (STATUS_LED_BIT & mask) { >> + if (!omap_request_gpio(BEAGLE_LED_USR0)) { >> + omap_set_gpio_direction(BEAGLE_LED_USR0, 0); >> + omap_set_gpio_dataout(BEAGLE_LED_USR0, state); >> + } >> + saved_state[0] = state; >> + } >> +#endif >> +#ifdef STATUS_LED_BIT1 >> + if (STATUS_LED_BIT1 & mask) { >> + if (!omap_request_gpio(BEAGLE_LED_USR1)) { >> + omap_set_gpio_direction(BEAGLE_LED_USR1, 0); >> + omap_set_gpio_dataout(BEAGLE_LED_USR1, state); >> + } >> + saved_state[1] = state; >> + } >> +#endif >> +} > > [sp] I see too many ifdef blocks in the code above. The also seems to be > repetitive. > > Is user really expected to change the u-boot config for each LED bit/color? This is the existing architecture in status_led.h. I want to make sure this code compiles no matter which of the defines have been set. > Can this be simplified by one function that takes an argument? Do you mean set vs. toggle? I think that would be great too, but again following an existing definition. > Should reduce code and ifdefs - if at all - get confined to one function. Do you have any proposal on how to do so? Personally, I'd be inclined to eliminate the colors all together and have 2 values that can be ignored/rejected by the functions if not supported: LED # (an ID) and an integral value (could be brightness or color). Also, I'd be happy with simply having a set value function and having the rest of the system monitor the state (rather than having a toggle method). Thoughts? > > ~sanjeev > >> + >> -- >> 1.5.6.4 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot >>
Dear Jason Kridner, In message <AANLkTim71JeaAkjbHNHfv8FqDvcc5L=ES1XFbwLBmt8r@mail.gmail.com> you wrote: > > > [sp] I see too many ifdef blocks in the code above. The also seems to be > > repetitive. > > > > Is user really expected to change the u-boot config for each LED > bit/color? > > This is the existing architecture in status_led.h. I want to make > sure this code compiles no matter which of the defines have been set. > > > Can this be simplified by one function that takes an argument? > > Do you mean set vs. toggle? I think that would be great too, but > again following an existing definition. Please don;t stick with bad examples just because they exist. Please feel free to come up ith an improvement. If you have time and resources, please feel free to clean up the status_led code as well. Best regards, Wolfgang Denk
diff --git a/board/ti/beagle/Makefile b/board/ti/beagle/Makefile index f797112..4cc675c 100644 --- a/board/ti/beagle/Makefile +++ b/board/ti/beagle/Makefile @@ -25,8 +25,10 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(BOARD).a -COBJS := beagle.o +COBJS-y := $(BOARD).o +COBJS-$(CONFIG_STATUS_LED) += led.o +COBJS := $(sort $(COBJS-y)) SRCS := $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(COBJS)) diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c index 93c452e..dd7b6b5 100644 --- a/board/ti/beagle/beagle.c +++ b/board/ti/beagle/beagle.c @@ -30,6 +30,9 @@ * MA 02111-1307 USA */ #include <common.h> +#ifdef CONFIG_STATUS_LED +#include <status_led.h> +#endif #include <twl4030.h> #include <asm/io.h> #include <asm/arch/mmc_host_def.h> @@ -78,6 +81,10 @@ int board_init(void) /* boot param addr */ gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100); +#if defined(CONFIG_STATUS_LED) && defined(STATUS_LED_BOOT) + status_led_set (STATUS_LED_BOOT, STATUS_LED_ON); +#endif + return 0; } diff --git a/board/ti/beagle/led.c b/board/ti/beagle/led.c new file mode 100644 index 0000000..df26552 --- /dev/null +++ b/board/ti/beagle/led.c @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2010 Texas Instruments, Inc. + * Jason Kridner <jkridner@beagleboard.org> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ +#include <common.h> +#include <status_led.h> +#include <asm/arch/cpu.h> +#include <asm/io.h> +#include <asm/arch/sys_proto.h> +#include <asm/arch/gpio.h> + +static unsigned int saved_state[2] = {STATUS_LED_OFF, STATUS_LED_OFF}; + +/* GPIO pins for the LEDs */ +#define BEAGLE_LED_USR0 149 +#define BEAGLE_LED_USR1 150 + +#ifdef STATUS_LED_GREEN +void green_LED_off (void) +{ + __led_set (STATUS_LED_GREEN, 0); +} + +void green_LED_on (void) +{ + __led_set (STATUS_LED_GREEN, 1); +} +#endif + +void __led_init (led_id_t mask, int state) +{ + __led_set (mask, state); +} + +void __led_toggle (led_id_t mask) +{ +#ifdef STATUS_LED_BIT + if (STATUS_LED_BIT & mask) { + if (STATUS_LED_ON == saved_state[0]) + __led_set(STATUS_LED_BIT, 0); + else + __led_set(STATUS_LED_BIT, 1); + } +#endif +#ifdef STATUS_LED_BIT1 + if (STATUS_LED_BIT1 & mask) { + if (STATUS_LED_ON == saved_state[1]) + __led_set(STATUS_LED_BIT1, 0); + else + __led_set(STATUS_LED_BIT1, 1); + } +#endif +} + +void __led_set (led_id_t mask, int state) +{ +#ifdef STATUS_LED_BIT + if (STATUS_LED_BIT & mask) { + if (!omap_request_gpio(BEAGLE_LED_USR0)) { + omap_set_gpio_direction(BEAGLE_LED_USR0, 0); + omap_set_gpio_dataout(BEAGLE_LED_USR0, state); + } + saved_state[0] = state; + } +#endif +#ifdef STATUS_LED_BIT1 + if (STATUS_LED_BIT1 & mask) { + if (!omap_request_gpio(BEAGLE_LED_USR1)) { + omap_set_gpio_direction(BEAGLE_LED_USR1, 0); + omap_set_gpio_dataout(BEAGLE_LED_USR1, state); + } + saved_state[1] = state; + } +#endif +} +
Added LED driver using status_led. USR0 is set to monitor the boot status. USR1 is set to be the green LED. (cherry picked from commit 048b526fd7cc0c642f27c674b3e235321c880b66) Signed-off-by: Jason Kridner <jkridner@beagleboard.org> --- board/ti/beagle/Makefile | 4 ++- board/ti/beagle/beagle.c | 7 ++++ board/ti/beagle/led.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 1 deletions(-) create mode 100644 board/ti/beagle/led.c