diff mbox

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

Message ID 1490625523-11594-5-git-send-email-techping.chan@gmail.com
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

techping.chan@gmail.com March 27, 2017, 2:38 p.m. UTC 
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(-)

Comments

techping.chan@gmail.com April 5, 2017, 1:24 p.m. UTC | #1 
2017-04-01 12:22 GMT+08:00 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
>

Hi, Simon

I have seen your version, and I deem your code is more appropriate.
I'll learn from your code.

Thanks.
Simon Glass April 9, 2017, 7:27 p.m. UTC | #2 
Hi,

On 5 April 2017 at 07:24, Ziping Chen <techping.chan@gmail.com> wrote:
>
>
> 2017-04-01 12:22 GMT+08:00 Simon Glass <sjg@chromium.org>:
>>
>> Hi,


>
>
> Hi, Simon
>
> I have seen your version, and I deem your code is more appropriate.
> I'll learn from your code.

That is very kind of you. I'm sorry for the duplication.

Regards,
Simon
techping.chan@gmail.com April 10, 2017, 3:06 p.m. UTC | #3 
This illustrates the vitality of the developer. I'll work much harder. :-)

Regards,
Ziping Chen

2017-04-10 3:27 GMT+08:00 Simon Glass <sjg@chromium.org>:

> Hi,
>
> On 5 April 2017 at 07:24, Ziping Chen <techping.chan@gmail.com> wrote:
> >
> >
> > 2017-04-01 12:22 GMT+08:00 Simon Glass <sjg@chromium.org>:
> >>
> >> Hi,
>
>
> >
> >
> > Hi, Simon
> >
> > I have seen your version, and I deem your code is more appropriate.
> > I'll learn from your code.
>
> That is very kind of you. I'm sorry for the duplication.
>
> Regards,
> Simon
>
Simon Glass April 10, 2017, 5:35 p.m. UTC | #4 
Hi Ziping,

On 10 April 2017 at 09:06, Ziping Chen <techping.chan@gmail.com> wrote:
>
> This illustrates the vitality of the developer. I'll work much harder. :-)

:-)

OK, I've just sent v2.

Regards,
Simon

>
> Regards,
> Ziping Chen
>
> 2017-04-10 3:27 GMT+08:00 Simon Glass <sjg@chromium.org>:
>>
>> Hi,
>>
>> On 5 April 2017 at 07:24, Ziping Chen <techping.chan@gmail.com> wrote:
>> >
>> >
>> > 2017-04-01 12:22 GMT+08:00 Simon Glass <sjg@chromium.org>:
>> >>
>> >> Hi,
>>
>>
>> >
>> >
>> > Hi, Simon
>> >
>> > I have seen your version, and I deem your code is more appropriate.
>> > I'll learn from your code.
>>
>> That is very kind of you. I'm sorry for the duplication.
>>
>> Regards,
>> Simon
>
>
diff mbox

Patch

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
 
+#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;
+		}
+		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);
+		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