Hi, On 27 March 2017 at 08:38, <techping.c...@gmail.com> wrote: > From: Ziping Chen <techping.c...@gmail.com> > > Currently the "led" command only supports the old API without DM. > > Add DM-based implementation of this command. > > Also allow this command to be select with Kconfig. > > Signed-off-by: Ziping Chen <techping.c...@gmail.com> > --- > cmd/Kconfig | 6 ++++ > cmd/Makefile | 4 +++ > cmd/led.c | 113 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 116 insertions(+), 7 deletions(-) > > diff --git a/cmd/Kconfig b/cmd/Kconfig > index 25e3b78..66c94de 100644 > --- a/cmd/Kconfig > +++ b/cmd/Kconfig > @@ -511,6 +511,12 @@ config CMD_GPIO > help > GPIO support. > > +config CMD_LED > + bool "led" > + depends on LED > + help > + LED support. > + > endmenu > > > diff --git a/cmd/Makefile b/cmd/Makefile > index f13bb8c..0817de5 100644 > --- a/cmd/Makefile > +++ b/cmd/Makefile > @@ -79,7 +79,11 @@ obj-$(CONFIG_CMD_ITEST) += itest.o > obj-$(CONFIG_CMD_JFFS2) += jffs2.o > obj-$(CONFIG_CMD_CRAMFS) += cramfs.o > obj-$(CONFIG_CMD_LDRINFO) += ldrinfo.o > +ifdef CONFIG_LED > +obj-$(CONFIG_CMD_LED) += led.o > +else > obj-$(CONFIG_LED_STATUS_CMD) += led.o > +endif > obj-$(CONFIG_CMD_LICENSE) += license.o > obj-y += load.o > obj-$(CONFIG_LOGBUFFER) += log.o > diff --git a/cmd/led.c b/cmd/led.c > index d50938a..3849a79 100644 > --- a/cmd/led.c > +++ b/cmd/led.c > @@ -13,8 +13,14 @@ > #include <common.h> > #include <config.h> > #include <command.h> > +#ifndef CONFIG_LED > #include <status_led.h> > +#else > +#include <dm.h> > +DECLARE_GLOBAL_DATA_PTR; > +#endif
I think you can drop those two #ifdefs. > > +#ifndef CONFIG_LED > struct led_tbl_s { > char *string; /* String for use in the command */ > led_id_t mask; /* Mask used for calling __led_set() > */ > @@ -62,6 +68,15 @@ static const led_tbl_t led_commands[] = { > { NULL, 0, NULL, NULL, NULL } > }; > > +/* > + * LED drivers providing a blinking LED functionality, like the > + * PCA9551, can override this empty weak function > + */ > +void __weak __led_blink(led_id_t mask, int freq) > +{ > +} > +#endif > + > enum led_cmd { > LED_CMD_ERROR = -1, > LED_CMD_ON, > @@ -78,19 +93,53 @@ enum led_cmd get_led_cmd(char *var) > return LED_CMD_ON; > if (strcmp(var, "toggle") == 0) > return LED_CMD_TOGGLE; > +#ifndef CONFIG_LED > if (strcmp(var, "blink") == 0) > return LED_CMD_BLINK; > - > +#endif > return -1; > } > > -/* > - * LED drivers providing a blinking LED functionality, like the > - * PCA9551, can override this empty weak function > - */ > -void __weak __led_blink(led_id_t mask, int freq) > +#ifdef CONFIG_LED > +int dm_led_set(char *label, enum led_cmd cmd) > { > + struct udevice *dev; > + char status; > + if (led_get_by_label(label, &dev)) { > + printf("Warning: led [ %s ] not found\n", > + label); > + return -1; > + } > + switch (cmd) { > + case LED_CMD_ON: > + led_set_on(dev, 1); > + if (led_get_status(dev) != 1) { > + printf("Warning: status of [ %s ] is still 0\n", > + label); > + return -1; > + } > + break; > + case LED_CMD_OFF: > + led_set_on(dev, 0); > + if (led_get_status(dev) != 0) { > + printf("Warning: status of [ %s ] is still 1\n", > + label); > + return -1; > + } > + break; > + case LED_CMD_TOGGLE: > + status = led_get_status(dev); > + led_set_on(dev, !status); > + if (led_get_status(dev) == status) { > + printf("Warning: status of [ %s ] is still %d\n", > + label, status); > + return -1; > + } Funny, in my version I moved this down into the uclass...but this seems reasonable also. > + break; > + } > + return 0; > } > +#endif > > int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > { > @@ -99,14 +148,23 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * > const argv[]) > int freq; > > /* Validate arguments */ > +#ifndef CONFIG_LED > if ((argc < 3) || (argc > 4)) > +#else > + if ((argc < 2) || (argc > 4)) > +#endif > return CMD_RET_USAGE; > > cmd = get_led_cmd(argv[2]); > +#ifndef CONFIG_LED > if (cmd < 0) { > +#else > + if (argc > 2 && cmd < 0) { > +#endif > return CMD_RET_USAGE; > } > > +#ifndef CONFIG_LED > for (i = 0; led_commands[i].string; i++) { > if ((strcmp("all", argv[1]) == 0) || > (strcmp(led_commands[i].string, argv[1]) == 0)) { > @@ -144,7 +202,40 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * > const argv[]) > break; > } > } > - > +#else > + if (strcmp("all", argv[1]) == 0) { > + char *label = ""; > + int node, len, error_count = 0; > + match = 1; > + node = fdt_path_offset(gd->fdt_blob, "/leds"); > + if (node < 0) { > + printf("led: null led found\n"); > + return CMD_RET_FAILURE; > + } > + node = fdt_first_subnode(gd->fdt_blob, node); Why are you checking the DT here - can this information not come from the uclass? Please see my led command patch. I might be missing a reason. > + if (node < 0) { > + printf("led: null led found\n"); > + return CMD_RET_FAILURE; > + } > + label = fdt_getprop(gd->fdt_blob, node, "label", &len); > + if (dm_led_set(label, cmd) < 0) > + error_count++; > + while (1) { > + node = fdt_next_subnode(gd->fdt_blob, node); > + if (node < 0) > + break; > + label = fdt_getprop(gd->fdt_blob, node, "label", > &len); > + if (dm_led_set(label, cmd) < 0) > + error_count++; > + } > + if (error_count != 0) > + return CMD_RET_FAILURE; > + } else if (argc > 2) { > + match = 1; > + if (dm_led_set(argv[1], cmd) < 0) > + return CMD_RET_FAILURE; > + } > +#endif > /* If we ran out of matches, print Usage */ > if (!match) { > return CMD_RET_USAGE; > @@ -153,6 +244,7 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * > const argv[]) > return 0; > } > > +#ifndef CONFIG_LED > U_BOOT_CMD( > led, 4, 1, do_led, > "[" > @@ -191,3 +283,10 @@ U_BOOT_CMD( > "all] [on|off|toggle|blink] [blink-freq in ms]", > "[led_name] [on|off|toggle|blink] sets or clears led(s)" > ); > +#else > +U_BOOT_CMD( > + led, 4, 1, do_led, > + "operate led(s)", > + "[all|led_name] [on|off|toggle] - sets or clears led(s)" > +); > +#endif > -- > 2.7.4 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot