* [U-Boot] [PATCH 2/6] led: gpio: add support for get_status function
2017-03-27 14:38 [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support techping.chan at gmail.com
@ 2017-03-27 14:38 ` techping.chan at gmail.com
2017-04-01 4:23 ` Simon Glass
2017-03-27 14:38 ` [U-Boot] [PATCH 3/6] cmd: led: rename command enum value techping.chan at gmail.com
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: techping.chan at gmail.com @ 2017-03-27 14:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 2/6] led: gpio: add support for get_status function
2017-03-27 14:38 ` [U-Boot] [PATCH 2/6] led: gpio: add support for get_status function techping.chan at gmail.com
@ 2017-04-01 4:23 ` Simon Glass
0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2017-04-01 4:23 UTC (permalink / raw)
To: u-boot
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)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 3/6] cmd: led: rename command enum value
2017-03-27 14:38 [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support techping.chan at gmail.com
2017-03-27 14:38 ` [U-Boot] [PATCH 2/6] led: gpio: add support for get_status function techping.chan at gmail.com
@ 2017-03-27 14:38 ` techping.chan at gmail.com
2017-04-01 4:22 ` Simon Glass
2017-03-27 14:38 ` [U-Boot] [PATCH 4/6] cmd: led: add enum led_cmd member to support error code return techping.chan at gmail.com
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: techping.chan at gmail.com @ 2017-03-27 14:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 3/6] cmd: led: rename command enum value
2017-03-27 14:38 ` [U-Boot] [PATCH 3/6] cmd: led: rename command enum value techping.chan at gmail.com
@ 2017-04-01 4:22 ` Simon Glass
0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2017-04-01 4:22 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 4/6] cmd: led: add enum led_cmd member to support error code return
2017-03-27 14:38 [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support techping.chan at gmail.com
2017-03-27 14:38 ` [U-Boot] [PATCH 2/6] led: gpio: add support for get_status function techping.chan at gmail.com
2017-03-27 14:38 ` [U-Boot] [PATCH 3/6] cmd: led: rename command enum value techping.chan at gmail.com
@ 2017-03-27 14:38 ` techping.chan at gmail.com
2017-04-01 4:22 ` Simon Glass
2017-03-27 14:38 ` [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation techping.chan at gmail.com
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: techping.chan at gmail.com @ 2017-03-27 14:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation
2017-03-27 14:38 [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support techping.chan at gmail.com
` (2 preceding siblings ...)
2017-03-27 14:38 ` [U-Boot] [PATCH 4/6] cmd: led: add enum led_cmd member to support error code return techping.chan at gmail.com
@ 2017-03-27 14:38 ` techping.chan at gmail.com
2017-04-01 4:22 ` Simon Glass
2017-03-27 14:38 ` [U-Boot] [PATCH 6/6] cmd: led: add command led list techping.chan at gmail.com
2017-04-01 4:23 ` [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support Simon Glass
5 siblings, 1 reply; 16+ messages in thread
From: techping.chan at gmail.com @ 2017-03-27 14:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation
2017-03-27 14:38 ` [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation techping.chan at gmail.com
@ 2017-04-01 4:22 ` Simon Glass
2017-04-05 13:24 ` Ziping Chen
0 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2017-04-01 4:22 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation
2017-04-01 4:22 ` Simon Glass
@ 2017-04-05 13:24 ` Ziping Chen
2017-04-09 19:27 ` Simon Glass
0 siblings, 1 reply; 16+ messages in thread
From: Ziping Chen @ 2017-04-05 13:24 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation
2017-04-05 13:24 ` Ziping Chen
@ 2017-04-09 19:27 ` Simon Glass
2017-04-10 15:06 ` Ziping Chen
0 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2017-04-09 19:27 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation
2017-04-09 19:27 ` Simon Glass
@ 2017-04-10 15:06 ` Ziping Chen
2017-04-10 17:35 ` Simon Glass
0 siblings, 1 reply; 16+ messages in thread
From: Ziping Chen @ 2017-04-10 15:06 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation
2017-04-10 15:06 ` Ziping Chen
@ 2017-04-10 17:35 ` Simon Glass
0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2017-04-10 17:35 UTC (permalink / raw)
To: u-boot
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
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH 6/6] cmd: led: add command led list
2017-03-27 14:38 [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support techping.chan at gmail.com
` (3 preceding siblings ...)
2017-03-27 14:38 ` [U-Boot] [PATCH 5/6] cmd: led: add DM-based implementation techping.chan at gmail.com
@ 2017-03-27 14:38 ` techping.chan at gmail.com
2017-04-01 4:23 ` [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support Simon Glass
5 siblings, 0 replies; 16+ messages in thread
From: techping.chan at gmail.com @ 2017-03-27 14:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support
2017-03-27 14:38 [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support techping.chan at gmail.com
` (4 preceding siblings ...)
2017-03-27 14:38 ` [U-Boot] [PATCH 6/6] cmd: led: add command led list techping.chan at gmail.com
@ 2017-04-01 4:23 ` Simon Glass
2017-04-05 13:20 ` Ziping Chen
5 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2017-04-01 4:23 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 16+ messages in thread* [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support
2017-04-01 4:23 ` [U-Boot] [PATCH 1/6] led: add get_status support for DM LED support Simon Glass
@ 2017-04-05 13:20 ` Ziping Chen
0 siblings, 0 replies; 16+ messages in thread
From: Ziping Chen @ 2017-04-05 13:20 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 16+ messages in thread