dm: led: remove auto probe in binding function

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.

Patrick Delaunay (4):
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

 arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi | 24 ++++++++++++++
 board/st/stm32mp1/stm32mp1.c             |  4 +++
 configs/stm32mp15_basic_defconfig        |  2 ++
 drivers/led/led-uclass.c                 | 54 ++++++++++++++++++++++++++++++++
 drivers/led/led_gpio.c                   | 17 ----------
 include/led.h                            | 23 ++++++++++++++
 6 files changed, 107 insertions(+), 17 deletions(-)

2.7.4

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

---

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

index 5b8be93..e10e4b6 100644
@@ -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 {

index 3a94db5..dc38bcc 100644
@@ -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_SPL_PINCTRL_FULL is not set

2.7.4

This reverts commit bc882f5d5c7b4d6ed5e927bf838863af43c786e7.

---

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

index a36942b..533587d 100644
@@ -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

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.

---

 drivers/led/led-uclass.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/led/led_gpio.c   |  8 -------
 include/led.h            | 23 +++++++++++++++++++++
 3 files changed, 77 insertions(+), 8 deletions(-)

index 2f4d69e..141401d 100644
@@ -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),
 };

index 533587d..93f6b91 100644
@@ -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;
 }
 

index 940b97f..ff45f03 100644
@@ -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

Making the change like this breaks (and leaves broken) the default state test in test/dm/led.c so 'make tests' fails. Please rework the series to include fixing the test, thanks!

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/368f67ca/attachment.sig>

Hi Tom

Sorry for the disturbance...

I tried this compilation but I was blocked by lib SDL issues and other configuration on my PC and I push the patch without test execution. Today I take the time to solve my compilation issue and I reproduce the test error.

I will push a corrective version of this patch next Monday.

Today I add a board_init function for sandbox arch to call the added function... it is now working in sandbox:

	./u-boot -d ./arch/sandbox/dts/test.dtb

but I have still issue with test.... because board_init() is not called.

Patrick

Applied to u-boot/master, thanks!

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/456e3e86/attachment.sig>

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

---

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

index cc39fa6..db8c805 100644
@@ -22,5 +22,9 @@ int board_init(void)
 	/* address of boot parameters */
 	gd->bd->bi_boot_params = STM32_DDR_BASE + 0x100;
 
+#ifdef CONFIG_LED
+	led_default_state();
+#endif /* CONFIG_LED */
+
 	return 0;
 }

2.7.4

Applied to u-boot/master, thanks!

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/c004834a/attachment.sig>