led: add get_status support for DM LED support

From: Ziping Chen <[email protected]>

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 <[email protected]>

---
 drivers/led/led_gpio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

From: Ziping Chen <[email protected]>

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 <[email protected]>

---
 cmd/led.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

From: Ziping Chen <[email protected]>

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

Signed-off-by Ziping Chen <[email protected]>

---
 cmd/led.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

From: Ziping Chen <[email protected]>

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 <[email protected]>

---
 cmd/Kconfig  |   6 ++++
 cmd/Makefile |   4 +++
 cmd/led.c    | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 116 insertions(+), 7 deletions(-)

cmd/Kconfig +6 -0

index 25e3b78..66c94de 100644
@@ -511,6 +511,12 @@ config CMD_GPIO
        help
          GPIO support.
 
+config CMD_LED
+       bool "led"
+       depends on LED
+       help
+         LED support.
+
 endmenu
 
 

cmd/Makefile +4 -0

index f13bb8c..0817de5 100644
@@ -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

cmd/led.c +0 -0

index d50938a..3849a79 100644

cmd/led.c +36 -6

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

cmd/led.c +14 -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

cmd/led.c +31 -1

+               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

Hi,

Quoted 6 lines · On 27 March 2017 at 08:38, <[email protected]> wrote:

From: Ziping Chen <[email protected]>

Currently the "led" command only supports the old API without DM.

Add DM-based implementation of this command.

Quoted 7 lines

Also allow this command to be select with Kconfig.

Signed-off-by Ziping Chen <[email protected]>

---
 cmd/Kconfig  |   6 ++++
 cmd/Makefile |   4 +++
 cmd/led.c    | 113 ++++++++++++++++++++++++++++++

+++++++++++++++++++++++++----

Quoted 40 lines

 3 files changed, 116 insertions(+), 7 deletions(-)

cmd/Kconfig +6 -0

index 25e3b78..66c94de 100644
@@ -511,6 +511,12 @@ config CMD_GPIO
        help
          GPIO support.
 
+config CMD_LED
+       bool "led"
+       depends on LED
+       help
+         LED support.
+
 endmenu
 
 

cmd/Makefile +4 -0

index f13bb8c..0817de5 100644
@@ -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

cmd/led.c +0 -0

index d50938a..3849a79 100644

cmd/led.c line 21

Quoted 9 lines

cmd/led.c +5 -0

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

cmd/led.c

Quoted 4 lines

cmd/led.c +1 -0

+#ifndef CONFIG_LED
 struct led_tbl_s {
        char            *string;        /* String for use in the command

*/

cmd/led.c line 25

Quoted 1 line

cmd/led.c +0 -0

        led_id_t        mask;           /* Mask used for calling

__led_set() */

Quoted 58 lines

cmd/led.c +35 -6

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

cmd/led.c enum led_cmd get_led_cmd(char *var) · line 137

Quoted 10 lines

cmd/led.c +10 -0

+               }
+               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.

cmd/led.c int do_led (cmd_tbl_t *cmdtp, int flag, int argc,

Quoted 9 lines

cmd/led.c +4 -0

+               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[])

cmd/led.c int do_led (cmd_tbl_t *cmdtp, int flag, int argc,

Quoted 24 lines

cmd/led.c +9 -0

        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[])

Quoted 5 lines

cmd/led.c +1 -1

                                break;
                }
        }
-
+#else

cmd/led.c int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 215

Quoted 10 lines

cmd/led.c +10 -0

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

cmd/led.c int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

Quoted 12 lines

cmd/led.c +12 -0

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

cmd/led.c int do_led (cmd_tbl_t *cmdtp, int flag, int argc,

Quoted 15 lines

cmd/led.c +11 -0

+                       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[])

Quoted 16 lines

cmd/led.c +6 -0

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

Quoted 5 lines

cmd/led.c +2 -1

+);
+#endif
--

2.7.4

Regards,
Simon

2017-04-01 12:22 GMT+08:00 Simon Glass <[email protected]>:

Quoted 2 lines

Hi,

Hi, Simon

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

Hi,

Quoted 9 lines · Ziping Chen

2017-04-01 12:22 GMT+08:00 Simon Glass <[email protected]>:

Quoted 2 lines

Hi,

Quoted 4 lines

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

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 <[email protected]>:

Quoted 22 lines

Hi,

Quoted 9 lines · Ziping Chen

2017-04-01 12:22 GMT+08:00 Simon Glass <[email protected]>:

Quoted 2 lines

Hi,

Quoted 4 lines

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

From: Ziping Chen <[email protected]>

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 <[email protected]>

---
 drivers/led/led-uclass.c | 10 ++++++++++
 include/led.h            | 15 +++++++++++++++
 2 files changed, 25 insertions(+)

drivers/led/led-uclass.c +10 -0

index 784ac87..304b92a 100644
@@ -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",

include/led.h +15 -1

index b929d0c..cd6fe98 100644
@@ -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

Hi,

Quoted 4 lines · On 27 March 2017 at 08:38, <[email protected]> wrote:

From: Ziping Chen <[email protected]>

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

Quoted 8 lines

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 <[email protected]>

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

include/led.h int led_get_by_label(const char *label, struct

Quoted 41 lines

drivers/led/led-uclass.c +10 -0

index 784ac87..304b92a 100644
@@ -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",

include/led.h +7 -0

index b929d0c..cd6fe98 100644
@@ -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);

Quoted 3 lines

include/led.h +0 -0

  */
 int led_set_on(struct udevice *dev, int on);
 

Quoted 10 lines

include/led.h +8 -1

+/**
+ * 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
--

Quoted 2 lines

2.7.4

Regards,
Simon