[U-Boot] [PATCH 1/6] led: add get_status support for DM LED support

[U-Boot] [PATCH 1/6] led: add get_status support for DM LED support

[techping.chan at gmail.com]

From: Ziping Chen <techping.chan@gmail.com>

Sometimes we need to read back the status of a LED.

Add a led_get_status function for DM LED support, and add a get_status
function for the driver to implement this function.

Signed-off-by: Ziping Chen <techping.chan@gmail.com>
---
 drivers/led/led-uclass.c | 10 ++++++++++
 include/led.h            | 15 +++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 784ac87..304b92a 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -42,6 +42,16 @@ int led_set_on(struct udevice *dev, int on)
 	return ops->set_on(dev, on);
 }
 
+int led_get_status(struct udevice *dev)
+{
+	struct led_ops *ops = led_get_ops(dev);
+
+	if (!ops->get_status)
+		return -ENOSYS;
+
+	return ops->get_status(dev);
+}
+
 UCLASS_DRIVER(led) = {
 	.id		= UCLASS_LED,
 	.name		= "led",
diff --git a/include/led.h b/include/led.h
index b929d0c..cd6fe98 100644
--- a/include/led.h
+++ b/include/led.h
@@ -26,6 +26,13 @@ struct led_ops {
 	 * @return 0 if OK, -ve on error
 	 */
 	int (*set_on)(struct udevice *dev, int on);
+	/**
+	 * led_get_status() - get the state of an LED
+	 *
+	 * @dev:	LED device to query
+	 * @return 0 if LED off, 1 if LED on, -ve on error
+	 */
+	int (*get_status)(struct udevice *dev);
 };
 
 #define led_get_ops(dev)	((struct led_ops *)(dev)->driver->ops)
@@ -48,4 +55,12 @@ int led_get_by_label(const char *label, struct udevice **devp);
  */
 int led_set_on(struct udevice *dev, int on);
 
+/**
+ * led_get_status() - get the state of an LED
+ *
+ * @dev:	LED device to query
+ * @return 0 if LED off, 1 if LED on, -ve on error
+ */
+int led_get_status(struct udevice *dev);
+
 #endif
-- 
2.7.4

[U-Boot] [PATCH 2/6] led: gpio: add support for get_status function

[techping.chan at gmail.com]

From: Ziping Chen <techping.chan@gmail.com>

The status of a GPIO-connected LED can be read back by reading the GPO
value.

Add the support for get_status function in led_gpio driver.

Signed-off-by: Ziping Chen <techping.chan@gmail.com>
---
 drivers/led/led_gpio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c
index 5b11990..b64721e 100644
--- a/drivers/led/led_gpio.c
+++ b/drivers/led/led_gpio.c
@@ -28,6 +28,16 @@ static int gpio_led_set_on(struct udevice *dev, int on)
 	return dm_gpio_set_value(&priv->gpio, on);
 }
 
+static int gpio_led_get_status(struct udevice *dev)
+{
+	struct led_gpio_priv *priv = dev_get_priv(dev);
+
+	if (!dm_gpio_is_valid(&priv->gpio))
+		return -EREMOTEIO;
+
+	return dm_gpio_get_value(&priv->gpio);
+}
+
 static int led_gpio_probe(struct udevice *dev)
 {
 	struct led_uclass_plat *uc_plat = dev_get_uclass_platdata(dev);
@@ -88,6 +98,7 @@ static int led_gpio_bind(struct udevice *parent)
 
 static const struct led_ops gpio_led_ops = {
 	.set_on		= gpio_led_set_on,
+	.get_status	= gpio_led_get_status,
 };
 
 static const struct udevice_id led_gpio_ids[] = {
-- 
2.7.4

[U-Boot] [PATCH 3/6] cmd: led: rename command enum value

[techping.chan at gmail.com]

From: Ziping Chen <techping.chan@gmail.com>

The "LED_OFF" constant conflicts with the constant with the same name in
include/linux/compat.h.

Rename all command constants' name prefix from LED_ to LED_CMD_.

Signed-off-by: Ziping Chen <techping.chan@gmail.com>
---
 cmd/led.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/cmd/led.c b/cmd/led.c
index 951a5e2..ef0ab1a 100644
--- a/cmd/led.c
+++ b/cmd/led.c
@@ -62,18 +62,18 @@ static const led_tbl_t led_commands[] = {
 	{ NULL, 0, NULL, NULL, NULL }
 };
 
-enum led_cmd { LED_ON, LED_OFF, LED_TOGGLE, LED_BLINK };
+enum led_cmd { LED_CMD_ON, LED_CMD_OFF, LED_CMD_TOGGLE, LED_CMD_BLINK };
 
 enum led_cmd get_led_cmd(char *var)
 {
 	if (strcmp(var, "off") == 0)
-		return LED_OFF;
+		return LED_CMD_OFF;
 	if (strcmp(var, "on") == 0)
-		return LED_ON;
+		return LED_CMD_ON;
 	if (strcmp(var, "toggle") == 0)
-		return LED_TOGGLE;
+		return LED_CMD_TOGGLE;
 	if (strcmp(var, "blink") == 0)
-		return LED_BLINK;
+		return LED_CMD_BLINK;
 
 	return -1;
 }
@@ -106,27 +106,27 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		    (strcmp(led_commands[i].string, argv[1]) == 0)) {
 			match = 1;
 			switch (cmd) {
-			case LED_ON:
+			case LED_CMD_ON:
 				if (led_commands[i].on)
 					led_commands[i].on();
 				else
 					__led_set(led_commands[i].mask,
 							  CONFIG_LED_STATUS_ON);
 				break;
-			case LED_OFF:
+			case LED_CMD_OFF:
 				if (led_commands[i].off)
 					led_commands[i].off();
 				else
 					__led_set(led_commands[i].mask,
 						  CONFIG_LED_STATUS_OFF);
 				break;
-			case LED_TOGGLE:
+			case LED_CMD_TOGGLE:
 				if (led_commands[i].toggle)
 					led_commands[i].toggle();
 				else
 					__led_toggle(led_commands[i].mask);
 				break;
-			case LED_BLINK:
+			case LED_CMD_BLINK:
 				if (argc != 4)
 					return CMD_RET_USAGE;
 
-- 
2.7.4

[U-Boot] [PATCH 4/6] cmd: led: add enum led_cmd member to support error code return

[techping.chan at gmail.com]

From: Ziping Chen <techping.chan@gmail.com>

Add enum led_cmd member LED_CMD_ERROR, so that the enum can contain
the error code -1.

Signed-off-by: Ziping Chen <techping.chan@gmail.com>
---
 cmd/led.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/cmd/led.c b/cmd/led.c
index ef0ab1a..d50938a 100644
--- a/cmd/led.c
+++ b/cmd/led.c
@@ -62,7 +62,13 @@ static const led_tbl_t led_commands[] = {
 	{ NULL, 0, NULL, NULL, NULL }
 };
 
-enum led_cmd { LED_CMD_ON, LED_CMD_OFF, LED_CMD_TOGGLE, LED_CMD_BLINK };
+enum led_cmd {
+	LED_CMD_ERROR = -1,
+	LED_CMD_ON,
+	LED_CMD_OFF,
+	LED_CMD_TOGGLE,
+	LED_CMD_BLINK
+};
 
 enum led_cmd get_led_cmd(char *var)
 {
-- 
2.7.4

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

[techping.chan at gmail.com]

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
 
+#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
-- 
2.7.4

[U-Boot] [PATCH 6/6] cmd: led: add command led list

[techping.chan at gmail.com]

From: Ziping Chen <techping.chan@gmail.com>

Add command "led list" to list all led(s) can be operated.

Signed-off-by: Ziping Chen <techping.chan@gmail.com>
---
 cmd/led.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/cmd/led.c b/cmd/led.c
index 3849a79..3f70666 100644
--- a/cmd/led.c
+++ b/cmd/led.c
@@ -230,6 +230,30 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		}
 		if (error_count != 0)
 			return CMD_RET_FAILURE;
+	} else if (strcmp("list", argv[1]) == 0) {
+		int node, len;
+		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;
+		}
+		printf(" led_name\n");
+		printf("----------------------------------------\n");
+		printf(" %s\n", fdt_getprop(gd->fdt_blob, node,
+					    "label", &len));
+		while (1) {
+			node = fdt_next_subnode(gd->fdt_blob, node);
+			if (node < 0)
+				break;
+			printf(" %s\n", fdt_getprop(gd->fdt_blob, node,
+						    "label", &len));
+		}
 	} else if (argc > 2) {
 		match = 1;
 		if (dm_led_set(argv[1], cmd) < 0)
@@ -287,6 +311,7 @@ U_BOOT_CMD(
 U_BOOT_CMD(
 	led, 4, 1, do_led,
 	"operate led(s)",
-	"[all|led_name] [on|off|toggle] - sets or clears led(s)"
+	"[all|led_name] [on|off|toggle] - sets or clears led(s)\n"
+	"led list - list all led(s) can be operated"
 );
 #endif
-- 
2.7.4

[U-Boot] [PATCH 3/6] cmd: led: rename command enum value

[Simon Glass]

On 27 March 2017 at 08:38,  <techping.chan@gmail.com> wrote:
> From: Ziping Chen <techping.chan@gmail.com>
>
> The "LED_OFF" constant conflicts with the constant with the same name in
> include/linux/compat.h.
>
> Rename all command constants' name prefix from LED_ to LED_CMD_.
>
> Signed-off-by: Ziping Chen <techping.chan@gmail.com>
> ---
>  cmd/led.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 4/6] cmd: led: add enum led_cmd member to support error code return

[Simon Glass]

On 27 March 2017 at 08:38,  <techping.chan@gmail.com> wrote:
> From: Ziping Chen <techping.chan@gmail.com>
>
> Add enum led_cmd member LED_CMD_ERROR, so that the enum can contain
> the error code -1.
>
> Signed-off-by: Ziping Chen <techping.chan@gmail.com>
> ---
>  cmd/led.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

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

[U-Boot] [PATCH 1/6] led: add get_status support for DM LED support

[Simon Glass]

Hi,

On 27 March 2017 at 08:38,  <techping.chan@gmail.com> wrote:
> From: Ziping Chen <techping.chan@gmail.com>
>
> Sometimes we need to read back the status of a LED.
>
> Add a led_get_status function for DM LED support, and add a get_status
> function for the driver to implement this function.
>
> Signed-off-by: Ziping Chen <techping.chan@gmail.com>
> ---
>  drivers/led/led-uclass.c | 10 ++++++++++
>  include/led.h            | 15 +++++++++++++++
>  2 files changed, 25 insertions(+)

I'm very sorry to say I just duplicated some of your work in my
attempt at cleaning up board_f :-(

Anyway could you please look at my patches which I think go a little
further than yours in some areas?

>
> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> index 784ac87..304b92a 100644
> --- a/drivers/led/led-uclass.c
> +++ b/drivers/led/led-uclass.c
> @@ -42,6 +42,16 @@ int led_set_on(struct udevice *dev, int on)
>         return ops->set_on(dev, on);
>  }
>
> +int led_get_status(struct udevice *dev)
> +{
> +       struct led_ops *ops = led_get_ops(dev);
> +
> +       if (!ops->get_status)
> +               return -ENOSYS;
> +
> +       return ops->get_status(dev);
> +}
> +
>  UCLASS_DRIVER(led) = {
>         .id             = UCLASS_LED,
>         .name           = "led",
> diff --git a/include/led.h b/include/led.h
> index b929d0c..cd6fe98 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -26,6 +26,13 @@ struct led_ops {
>          * @return 0 if OK, -ve on error
>          */
>         int (*set_on)(struct udevice *dev, int on);
> +       /**
> +        * led_get_status() - get the state of an LED
> +        *
> +        * @dev:        LED device to query
> +        * @return 0 if LED off, 1 if LED on, -ve on error
> +        */
> +       int (*get_status)(struct udevice *dev);
>  };
>
>  #define led_get_ops(dev)       ((struct led_ops *)(dev)->driver->ops)
> @@ -48,4 +55,12 @@ int led_get_by_label(const char *label, struct udevice **devp);
>   */
>  int led_set_on(struct udevice *dev, int on);
>
> +/**
> + * led_get_status() - get the state of an LED
> + *
> + * @dev:       LED device to query
> + * @return 0 if LED off, 1 if LED on, -ve on error
> + */
> +int led_get_status(struct udevice *dev);
> +
>  #endif
> --
> 2.7.4
>

Regards,
Simon

[U-Boot] [PATCH 2/6] led: gpio: add support for get_status function

[Simon Glass]

On 27 March 2017 at 08:38,  <techping.chan@gmail.com> wrote:
> From: Ziping Chen <techping.chan@gmail.com>
>
> The status of a GPIO-connected LED can be read back by reading the GPO
> value.
>
> Add the support for get_status function in led_gpio driver.
>
> Signed-off-by: Ziping Chen <techping.chan@gmail.com>
> ---
>  drivers/led/led_gpio.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

(although I have a version of this patch which expands status to
return the blink state also)

[U-Boot] [PATCH 1/6] led: add get_status support for DM LED support

[Ziping Chen]

2017-04-01 12:23 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>
> >
> > Sometimes we need to read back the status of a LED.
> >
> > Add a led_get_status function for DM LED support, and add a get_status
> > function for the driver to implement this function.
> >
> > Signed-off-by: Ziping Chen <techping.chan@gmail.com>
> > ---
> >  drivers/led/led-uclass.c | 10 ++++++++++
> >  include/led.h            | 15 +++++++++++++++
> >  2 files changed, 25 insertions(+)
>
> I'm very sorry to say I just duplicated some of your work in my
> attempt at cleaning up board_f :-(
>
> Anyway could you please look at my patches which I think go a little
> further than yours in some areas?
>
> >
> > diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> > index 784ac87..304b92a 100644
> > --- a/drivers/led/led-uclass.c
> > +++ b/drivers/led/led-uclass.c
> > @@ -42,6 +42,16 @@ int led_set_on(struct udevice *dev, int on)
> >         return ops->set_on(dev, on);
> >  }
> >
> > +int led_get_status(struct udevice *dev)
> > +{
> > +       struct led_ops *ops = led_get_ops(dev);
> > +
> > +       if (!ops->get_status)
> > +               return -ENOSYS;
> > +
> > +       return ops->get_status(dev);
> > +}
> > +
> >  UCLASS_DRIVER(led) = {
> >         .id             = UCLASS_LED,
> >         .name           = "led",
> > diff --git a/include/led.h b/include/led.h
> > index b929d0c..cd6fe98 100644
> > --- a/include/led.h
> > +++ b/include/led.h
> > @@ -26,6 +26,13 @@ struct led_ops {
> >          * @return 0 if OK, -ve on error
> >          */
> >         int (*set_on)(struct udevice *dev, int on);
> > +       /**
> > +        * led_get_status() - get the state of an LED
> > +        *
> > +        * @dev:        LED device to query
> > +        * @return 0 if LED off, 1 if LED on, -ve on error
> > +        */
> > +       int (*get_status)(struct udevice *dev);
> >  };
> >
> >  #define led_get_ops(dev)       ((struct led_ops *)(dev)->driver->ops)
> > @@ -48,4 +55,12 @@ int led_get_by_label(const char *label, struct
> udevice **devp);
> >   */
> >  int led_set_on(struct udevice *dev, int on);
> >
> > +/**
> > + * led_get_status() - get the state of an LED
> > + *
> > + * @dev:       LED device to query
> > + * @return 0 if LED off, 1 if LED on, -ve on error
> > + */
> > +int led_get_status(struct udevice *dev);
> > +
> >  #endif
> > --
> > 2.7.4
> >
>
> Regards,
> Simon
>

Hi, Simon

I have seen your code, which have more features.

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

[Ziping Chen]

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.

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

[Simon Glass]

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

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

[Ziping Chen]

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
>

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

[Simon Glass]

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
>
>