[U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation

[Simon Glass <sjg@chromium.org>]

Hi,

On 27 March 2017 at 08:38,  <techping.chan@gmail.com> wrote:
> From: Ziping Chen <techping.chan@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.chan@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