[U-Boot] [PATCH v3 0/5] dm: led: remove auto probe in binding function

[U-Boot] [PATCH v3 0/5] dm: led: remove auto probe in binding function

[Patrick Delaunay]

Hi,

The commit bc882f5d5c7b4d6ed5e927bf838863af43c786e7
introduce auto probe of LED in binding function
but that cause issue on my board.

This first patch of this patchset activateis the LED on my board
to explain the issue, the second patch revert this commit and
the third one propose an other solution.

For me, this commit is a error because in README of doc/driver-model/
it is indicated:

  The device's bind() method is permitted to perform simple actions, but
  should not scan the device tree node, not initialise hardware, nor set up
  structures or allocate memory. All of these tasks should be left for
  the probe() method.

And in the patch the LED driver is probed during the binding scan.

When I activated the LED in my ARM target with the patch
"stm32mp1: add gpio led support", I have the sequence:

dm_init_and_scan() :

1/ dm_scan_fdt_node()
	=> loop on all the node

2/ scan for LED node
	=> probe of LED driver is forced by "default-state" detection
		LED1 - "red"
        => probe of father of "red" node
		A - led
		B - soc
		C - root
	=> probe of node needed by GPIO
		1 - pin-controller
		2 - gpio at 50002000
		3 - rcc-clk at 50000000
		4 - rcc at 50000000

	=> probe forced by default state for other led
		LED2 - green
		LED3 - orange

	=> probe of node needed by GPIO (other bank)
		5 - gpio at 50009000

3/ dm_extended_scan_fdt scan continue...
   scan node "fixed-clock" under "/clocks"
	clk-hse
	clk-hsi
	clk-lse
	clk-lsi
	clk-csi

4/ probe of all the used devices.... after dm_extended_scan_fdt()

So many driver are probed before the end of the scan binding loop !

And that cause issue in my board (boot failed) because the rcc-clk clock
driver found the input frequency with these fixed-clock, which are binded
only after the stm32mp1 clock driver probe.

For me probe in forbidden in binding function and
thus uclass_get_device_tail() is not allowed in the commit
bc882f5d5c7b4d6ed5e927bf838863af43c786e7 which need to be reverted.

In the third patch I proposed an other solution based
on the existing solution in u-class regulator used to enable
regulator with "boot on" property (see regulators_enable_boot_on function).

I think that adding a function is the  better solution in the driver model
to force probe for some node according binding information
(after dm_init_and_scan call).

This new function should be called in board_init function for each board
but it could be also added in init_sequence_r[] in future.


Changes in v3:
- add motivation in revert commit
- minor update after Simon review
- include led.h to avoid compilation warning on stm32mp1 board

Changes in v2:
  - add sandbox impact and test update

Patrick Delaunay (5):
  stm32mp1: add gpio led support
  Revert "dm: led: auto probe() LEDs with "default-state""
  dm: led: move default state support in led uclass
  stm32mp1: use new function led default state
  sandbox: led: use new function to configure default state

 arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi | 24 ++++++++++++++
 board/sandbox/sandbox.c                  |  9 ++++++
 board/st/stm32mp1/stm32mp1.c             |  4 +++
 common/board_r.c                         |  3 +-
 configs/stm32mp15_basic_defconfig        |  2 ++
 drivers/led/led-uclass.c                 | 54 ++++++++++++++++++++++++++++++++
 drivers/led/led_gpio.c                   | 17 ----------
 include/led.h                            | 23 ++++++++++++++
 test/dm/led.c                            |  3 ++
 9 files changed, 121 insertions(+), 18 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH v3 1/5] stm32mp1: add gpio led support

[Patrick Delaunay]

This patch add the 4 LED available on the ED1 board and activated
gpio led driver.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v3: None
Changes in v2: None

 arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi | 24 ++++++++++++++++++++++++
 configs/stm32mp15_basic_defconfig        |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
index 39a0ebc..4898483 100644
--- a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
+++ b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
@@ -13,6 +13,30 @@
 		mmc1 = &sdmmc2;
 		i2c3 = &i2c4;
 	};
+
+	led {
+		compatible = "gpio-leds";
+
+		red {
+			label = "stm32mp:red:status";
+			gpios = <&gpioa 13 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		green {
+			label = "stm32mp:green:user";
+			gpios = <&gpioa 14 GPIO_ACTIVE_LOW>;
+			default-state = "on";
+		};
+		orange {
+			label = "stm32mp:orange:status";
+			gpios = <&gpioh 7 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		blue {
+			label = "stm32mp:blue:user";
+			gpios = <&gpiod 11 GPIO_ACTIVE_HIGH>;
+		};
+	};
 };
 
 &uart4_pins_a {
diff --git a/configs/stm32mp15_basic_defconfig b/configs/stm32mp15_basic_defconfig
index c72a440..2cac114 100644
--- a/configs/stm32mp15_basic_defconfig
+++ b/configs/stm32mp15_basic_defconfig
@@ -29,6 +29,8 @@ CONFIG_CMD_EXT4_WRITE=y
 # CONFIG_SPL_DOS_PARTITION is not set
 CONFIG_DM_I2C=y
 CONFIG_SYS_I2C_STM32F7=y
+CONFIG_LED=y
+CONFIG_LED_GPIO=y
 CONFIG_DM_MMC=y
 CONFIG_STM32_SDMMC2=y
 # CONFIG_PINCTRL_FULL is not set
-- 
2.7.4

[U-Boot] [PATCH v3 2/5] Revert "dm: led: auto probe() LEDs with "default-state""

[Patrick Delaunay]

This reverts commit bc882f5d5c7b4d6ed5e927bf838863af43c786e7.
because this patch adds the probe of LED driver during the
binding phasis. It is not allowed in driver model because
the drivers (clock, pincontrol) needed by the LED driver can
be also probed before the binding of all the device and
it is a source of problems.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v3:
- add motivation in revert commit

Changes in v2: None

 drivers/led/led_gpio.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c
index a36942b..533587d 100644
--- a/drivers/led/led_gpio.c
+++ b/drivers/led/led_gpio.c
@@ -10,7 +10,6 @@
 #include <led.h>
 #include <asm/gpio.h>
 #include <dm/lists.h>
-#include <dm/uclass-internal.h>
 
 struct led_gpio_priv {
 	struct gpio_desc gpio;
@@ -118,14 +117,6 @@ static int led_gpio_bind(struct udevice *parent)
 			return ret;
 		uc_plat = dev_get_uclass_platdata(dev);
 		uc_plat->label = label;
-
-		if (ofnode_read_bool(node, "default-state")) {
-			struct udevice *devp;
-
-			ret = uclass_get_device_tail(dev, 0, &devp);
-			if (ret)
-				return ret;
-		}
 	}
 
 	return 0;
-- 
2.7.4

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

[Patrick Delaunay]

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@chromium.org>
Signed-off-by: Patrick Delaunay <patrick.delaunay@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);
+	}
+
+	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);
-	}
 	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

[U-Boot] [PATCH v3 4/5] stm32mp1: use new function led default state

[Patrick Delaunay]

Initialize the led with the default state defined in device tree.

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

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v3:
- minor update after Simon review
- include led.h to avoid compilation warning on stm32mp1 board

Changes in v2: None

 board/st/stm32mp1/stm32mp1.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index cc39fa6..bfc8ab6 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -4,6 +4,7 @@
  */
 #include <config.h>
 #include <common.h>
+#include <led.h>
 #include <asm/arch/stm32.h>
 
 /*
@@ -22,5 +23,8 @@ int board_init(void)
 	/* address of boot parameters */
 	gd->bd->bi_boot_params = STM32_DDR_BASE + 0x100;
 
+	if (IS_ENABLED(CONFIG_LED))
+		led_default_state();
+
 	return 0;
 }
-- 
2.7.4

[U-Boot] [PATCH v3 5/5] sandbox: led: use new function to configure default state

[Patrick Delaunay]

Initialize the led with the default state defined in device tree
in board_init and solve issue with test for led default state.

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


Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
Led default-state is correctly handle in Sandbox, tested with:
  ./u-boot -d ./arch/sandbox/dts/test.dtb
  => led list
  sandbox:red     <inactive>
  sandbox:green   <inactive>
  sandbox:default_on on
  sandbox:default_off off

This patch solve "make tests" issue introduced by
http://patchwork.ozlabs.org/patch/943651/

Changes in v3: None
Changes in v2:
  - add sandbox impact and test update

 board/sandbox/sandbox.c | 9 +++++++++
 common/board_r.c        | 3 ++-
 test/dm/led.c           | 3 +++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
index 195f620..66b5f24 100644
--- a/board/sandbox/sandbox.c
+++ b/board/sandbox/sandbox.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <cros_ec.h>
 #include <dm.h>
+#include <led.h>
 #include <os.h>
 #include <asm/test.h>
 #include <asm/u-boot-sandbox.h>
@@ -47,6 +48,14 @@ int dram_init(void)
 	return 0;
 }
 
+int board_init(void)
+{
+#ifdef CONFIG_LED
+	led_default_state();
+#endif /* CONFIG_LED */
+	return 0;
+}
+
 #ifdef CONFIG_BOARD_LATE_INIT
 int board_late_init(void)
 {
diff --git a/common/board_r.c b/common/board_r.c
index 64f2574..9402c0e 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -690,7 +690,8 @@ static init_fnc_t init_sequence_r[] = {
 #ifdef CONFIG_DM
 	initr_dm,
 #endif
-#if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV)
+#if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
+	defined(CONFIG_SANDBOX)
 	board_init,	/* Setup chipselects */
 #endif
 	/*
diff --git a/test/dm/led.c b/test/dm/led.c
index 0071f21..00de7b3 100644
--- a/test/dm/led.c
+++ b/test/dm/led.c
@@ -32,6 +32,9 @@ static int dm_test_led_default_state(struct unit_test_state *uts)
 {
 	struct udevice *dev;
 
+	/* configure the default state (auto-probe) */
+	led_default_state();
+
 	/* Check that we handle the default-state property correctly. */
 	ut_assertok(led_get_by_label("sandbox:default_on", &dev));
 	ut_asserteq(LEDST_ON, led_get_state(dev));
-- 
2.7.4

[U-Boot] [PATCH v3 2/5] Revert "dm: led: auto probe() LEDs with "default-state""

[Simon Glass]

On 24 July 2018 at 02:58, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> This reverts commit bc882f5d5c7b4d6ed5e927bf838863af43c786e7.
> because this patch adds the probe of LED driver during the
> binding phasis. It is not allowed in driver model because
> the drivers (clock, pincontrol) needed by the LED driver can
> be also probed before the binding of all the device and
> it is a source of problems.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
> Changes in v3:
> - add motivation in revert commit
>
> Changes in v2: None
>
>  drivers/led/led_gpio.c | 9 ---------
>  1 file changed, 9 deletions(-)
>

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

[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@chromium.org>
>Signed-off-by: Patrick Delaunay <patrick.delaunay@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@beckhoff.com>
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

[U-Boot] [PATCH v3 5/5] sandbox: led: use new function to configure default state

[Patrick Brünn]

>From: Patrick Delaunay [mailto:patrick.delaunay at st.com]
>Sent: Dienstag, 24. Juli 2018 10:59
>Subject: [PATCH v3 5/5] sandbox: led: use new function to configure default
>state
>
>Initialize the led with the default state defined in device tree
>in board_init and solve issue with test for led default state.
>
>Reviewed-by: Simon Glass <sjg@chromium.org>
>
>
>Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>---
>Led default-state is correctly handle in Sandbox, tested with:
>  ./u-boot -d ./arch/sandbox/dts/test.dtb
>  => led list
>  sandbox:red     <inactive>
>  sandbox:green   <inactive>
>  sandbox:default_on on
>  sandbox:default_off off
>
>This patch solve "make tests" issue introduced by
>http://patchwork.ozlabs.org/patch/943651/
>
>Changes in v3: None
>Changes in v2:
>  - add sandbox impact and test update
>
> board/sandbox/sandbox.c | 9 +++++++++
> common/board_r.c        | 3 ++-
> test/dm/led.c           | 3 +++
> 3 files changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
>index 195f620..66b5f24 100644
>--- a/board/sandbox/sandbox.c
>+++ b/board/sandbox/sandbox.c
>@@ -6,6 +6,7 @@
> #include <common.h>
> #include <cros_ec.h>
> #include <dm.h>
>+#include <led.h>
> #include <os.h>
> #include <asm/test.h>
> #include <asm/u-boot-sandbox.h>
>@@ -47,6 +48,14 @@ int dram_init(void)
>       return 0;
> }
>
>+int board_init(void)
>+{
>+#ifdef CONFIG_LED
>+      led_default_state();
>+#endif /* CONFIG_LED */

I think you forgot to apply Simons suggestion here, like you did for 4/5

>+      return 0;
>+}
>...

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

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

[Patrick DELAUNAY]

Hi Patrick,

> From: Patrick Brünn <P.Bruenn@beckhoff.com>
> Sent: jeudi 26 juillet 2018 12:40
> 
> Hi Patrick,
> sorry, for responding so late, I am in the middle of a vacation at the moment.

It is normal in summer time, 
I will be also in holiday at end of next week.

> >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
> >
...
> >@@ -63,8 +64,61 @@ int led_set_period(struct udevice *dev, int
> >period_ms)  }  #endif
...
> >+int led_default_state(void)
> >+{
...
> >+                      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()?

No it is debug message keep; I need to remove it

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

No really needed but better I think.
I add this parsing during bind to avoid  probing of LED drivers
(and so configuration of the associated device as pincontrol, clock, ...)
when the "default-state" node is not present;  That avoid potential issue and it is  faster.

In next function, device_probe() is not called when uc_pdata->default_state = LED_DEF_NO to avoid it.

So I need the information before the probe...

I choose the u_class bind function, because uclass have no ofdata_to_platdata.

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

I want to have only probed driver in "dm tree" only the LED driver really initiliazed
And mmc_initialaze which probe all the mmc drivers

Class      Probed  Driver      Name
----------------------------------------
 led        [ + ]   gpio_led    |-- leds
 led        [   ]   gpio_led    |   |-- iracibble
 led        [   ]   gpio_led    |   |-- martinet
 led        [ + ]   gpio_led    |   |-- default_on
 led        [ + ]   gpio_led    |   `-- default_off

But after double check, I can move all the treatment in led_default_state()
Without changing the behavior and it is is more simple (no enum and double evaluation).

> 
> >       return 0;
> > }

> >
> However, this whole v3 series was:
> Tested-by: Patrick Bruenn <p.bruenn@beckhoff.com> Beckhoff Automation
> GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered
> office: Verl, Germany | Register court: Guetersloh HRA 7075
> 

Thanks for the tests, I will push a v4 with the proposed code simplification.

Patrick