2017-04-01 12:22 GMT+08:00 Simon Glass <s...@chromium.org>:
> 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
>
Hi, Simon
I have seen your version, and I deem your code is more appropriate.
I'll learn from your code.
Thanks.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
