[U-Boot] [PATCH v3 3/5] dm: led: move default state support in led uclass

[Patrick Brünn]

Hi Patrick,
sorry, for responding so late, I am in the middle of a vacation at the moment.
>From: Patrick Delaunay [mailto:patrick.delaunay at st.com]
>Sent: Dienstag, 24. Juli 2018 10:59
>Subject: [PATCH v3 3/5] dm: led: move default state support in led uclass
>
>This patch save common LED property "default-state" value
>in post bind of LED uclass.
>The configuration for this default state is only performed when
>led_default_state() is called;
>It can be called in your board_init()
>or it could added in init_sequence_r[] in future.
>
>Reviewed-by: Simon Glass <sjg at chromium.org>
>Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
>---
>
>Changes in v3: None
>Changes in v2: None
>
> drivers/led/led-uclass.c | 54
>++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/led/led_gpio.c   |  8 -------
> include/led.h            | 23 +++++++++++++++++++++
> 3 files changed, 77 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>index 2f4d69e..141401d 100644
>--- a/drivers/led/led-uclass.c
>+++ b/drivers/led/led-uclass.c
>@@ -8,6 +8,7 @@
> #include <dm.h>
> #include <errno.h>
> #include <led.h>
>+#include <dm/device-internal.h>
> #include <dm/root.h>
> #include <dm/uclass-internal.h>
>
>@@ -63,8 +64,61 @@ int led_set_period(struct udevice *dev, int period_ms)
> }
> #endif
>
>+static int led_post_bind(struct udevice *dev)
>+{
>+      struct led_uc_plat *uc_pdata;
>+      const char *default_state;
>+
>+      uc_pdata = dev_get_uclass_platdata(dev);
>+
>+      /* common optional properties */
>+      uc_pdata->default_state = LED_DEF_NO;
>+      default_state = dev_read_string(dev, "default-state");
>+      if (default_state) {
>+              if (!strncmp(default_state, "on", 2))
>+                      uc_pdata->default_state = LED_DEF_ON;
>+              else if (!strncmp(default_state, "off", 3))
>+                      uc_pdata->default_state = LED_DEF_OFF;
>+              else if (!strncmp(default_state, "keep", 4))
>+                      uc_pdata->default_state = LED_DEF_KEEP;
>+      }
>+
>+      return 0;
>+}
>+
>+int led_default_state(void)
>+{
>+      struct udevice *dev;
>+      struct uclass *uc;
>+      struct led_uc_plat *uc_pdata;
>+      int ret;
>+
>+      ret = uclass_get(UCLASS_LED, &uc);
>+      if (ret)
>+              return ret;
>+      for (uclass_find_first_device(UCLASS_LED, &dev);
>+           dev;
>+           uclass_find_next_device(&dev)) {
>+              uc_pdata = dev_get_uclass_platdata(dev);
>+              if (!uc_pdata || uc_pdata->default_state == LED_DEF_NO)
>+                      continue;
>+              ret = device_probe(dev);
>+              if (ret)
>+                      return ret;
>+              if (uc_pdata->default_state == LED_DEF_ON)
>+                      led_set_state(dev, LEDST_ON);
>+              else if (uc_pdata->default_state == LED_DEF_OFF)
>+                      led_set_state(dev, LEDST_OFF);
>+              printf("%s: default_state=%d\n",
>+                     uc_pdata->label, uc_pdata->default_state);
Do you really want to keep this printf()?
>+      }
>+
>+      return ret;
>+}
>+
> UCLASS_DRIVER(led) = {
>       .id             = UCLASS_LED,
>       .name           = "led",
>+      .post_bind      = led_post_bind,
>       .per_device_platdata_auto_alloc_size = sizeof(struct led_uc_plat),
> };
>diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c
>index 533587d..93f6b91 100644
>--- a/drivers/led/led_gpio.c
>+++ b/drivers/led/led_gpio.c
>@@ -57,7 +57,6 @@ static int led_gpio_probe(struct udevice *dev)
> {
>       struct led_uc_plat *uc_plat = dev_get_uclass_platdata(dev);
>       struct led_gpio_priv *priv = dev_get_priv(dev);
>-      const char *default_state;
>       int ret;
>
>       /* Ignore the top-level LED node */
>@@ -68,13 +67,6 @@ static int led_gpio_probe(struct udevice *dev)
>       if (ret)
>               return ret;
>
>-      default_state = dev_read_string(dev, "default-state");
>-      if (default_state) {
>-              if (!strncmp(default_state, "on", 2))
>-                      gpio_led_set_state(dev, LEDST_ON);
>-              else if (!strncmp(default_state, "off", 3))
>-                      gpio_led_set_state(dev, LEDST_OFF);
>-      }
Is it necessary to move this code out of led_gpio_probe()?
If possible I would keep it here and implement led_default_state()
similar to mmc_initialize(->mmc_probe()). Than we could avoid
enum led_def_state_t and the double evaluation of default_state.

>       return 0;
> }
>
>diff --git a/include/led.h b/include/led.h
>index 940b97f..ff45f03 100644
>--- a/include/led.h
>+++ b/include/led.h
>@@ -8,12 +8,27 @@
> #define __LED_H
>
> /**
>+ * enum led_default_state - The initial state of the LED.
>+ * see Documentation/devicetree/bindings/leds/common.txt
>+ */
>+enum led_def_state_t {
>+      LED_DEF_NO,
>+      LED_DEF_ON,
>+      LED_DEF_OFF,
>+      LED_DEF_KEEP
>+};
>+
>+/**
>  * struct led_uc_plat - Platform data the uclass stores about each device
>  *
>  * @label:    LED label
>+ * @default_state* - The initial state of the LED.
>+  see Documentation/devicetree/bindings/leds/common.txt
>+ * * - set automatically on device bind by the uclass's '.post_bind' method.
>  */
> struct led_uc_plat {
>       const char *label;
>+      enum led_def_state_t default_state;
> };
>
> /**
>@@ -106,4 +121,12 @@ enum led_state_t led_get_state(struct udevice
>*dev);
>  */
> int led_set_period(struct udevice *dev, int period_ms);
>
>+/**
>+ * led_default_state() - set the default state for all the LED
>+ *
>+ * This enables all leds which have default state.
>+ *
>+ */
>+int led_default_state(void);
>+
> #endif
>--
>2.7.4
>
However, this whole v3 series was:
Tested-by: Patrick Bruenn <p.bruenn at beckhoff.com>
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075