[PATCH v1 0/2] cmd: bind: Fix driver binding

[PATCH v1 0/2] cmd: bind: Fix driver binding

[Patrice Chotard]

This series is fixing issues reported by Herbert Poetzl when trying to
bind Ethernet gadget over USB on STM32MP1 platform.
2 issues have been found:
   - fix the bind command
   - add dwc2 bcdDevice in USB gadget controller


Patrice Chotard (2):
  cmd: bind: Fix driver binding on a device
  usb: gadget: Add bcdDevice for the DWC2 USB Gadget Controller

 cmd/bind.c                        |  2 +-
 drivers/core/device.c             |  2 +-
 drivers/core/lists.c              | 11 ++++++++---
 drivers/core/root.c               |  2 +-
 drivers/misc/imx8/scu.c           |  2 +-
 drivers/serial/serial-uclass.c    |  2 +-
 drivers/timer/timer-uclass.c      |  2 +-
 drivers/usb/gadget/gadget_chips.h |  8 ++++++++
 include/dm/lists.h                |  3 ++-
 test/dm/nop.c                     |  2 +-
 test/dm/test-fdt.c                |  2 +-
 11 files changed, 26 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Patrice Chotard]

Fix a regression brings by commit 84f8e36f03fa ("cmd: bind: allow to
bind driver with driver data")

As example, the following bind command doesn't work:

   bind /soc/usb-otg at 49000000 usb_ether

As usb_ether driver has no compatible string, it can't be find by
lists_bind_fdt(). In bind_by_node_path(), which called lists_bind_fdt(),
the driver entry is known, pass it to lists_bind_fdt() to force the driver
entry selection.

For this, add a new parameter struct *driver to lists_bind_fdt().
Fix also all lists_bind_fdt() callers.

Fixes: 84f8e36f03fa ("cmd: bind: allow to bind driver with driver data")

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
Reported-by: Herbert Poetzl <herbert@13thfloor.at>
Cc: Marek Vasut <marex@denx.de>
Cc: Herbert Poetzl <herbert@13thfloor.at>
---

 cmd/bind.c                     |  2 +-
 drivers/core/device.c          |  2 +-
 drivers/core/lists.c           | 11 ++++++++---
 drivers/core/root.c            |  2 +-
 drivers/misc/imx8/scu.c        |  2 +-
 drivers/serial/serial-uclass.c |  2 +-
 drivers/timer/timer-uclass.c   |  2 +-
 include/dm/lists.h             |  3 ++-
 test/dm/nop.c                  |  2 +-
 test/dm/test-fdt.c             |  2 +-
 10 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/cmd/bind.c b/cmd/bind.c
index af2f22cc4c..d8f610943c 100644
--- a/cmd/bind.c
+++ b/cmd/bind.c
@@ -152,7 +152,7 @@ static int bind_by_node_path(const char *path, const char *drv_name)
 	}
 
 	ofnode = ofnode_path(path);
-	ret = lists_bind_fdt(parent, ofnode, &dev, false);
+	ret = lists_bind_fdt(parent, ofnode, &dev, drv, false);
 
 	if (!dev || ret) {
 		printf("Unable to bind. err:%d\n", ret);
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 81f6880eac..3abd89aca6 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -1133,6 +1133,6 @@ int dev_enable_by_path(const char *path)
 	if (ret)
 		return ret;
 
-	return lists_bind_fdt(parent, node, NULL, false);
+	return lists_bind_fdt(parent, node, NULL, NULL, false);
 }
 #endif
diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index e214306b90..2eb808ce2d 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -182,7 +182,7 @@ static int driver_check_compatible(const struct udevice_id *of_match,
 }
 
 int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
-		   bool pre_reloc_only)
+		   struct driver *drv, bool pre_reloc_only)
 {
 	struct driver *driver = ll_entry_start(struct driver, driver);
 	const int n_ents = ll_entry_count(struct driver, driver);
@@ -225,8 +225,13 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 		for (entry = driver; entry != driver + n_ents; entry++) {
 			ret = driver_check_compatible(entry->of_match, &id,
 						      compat);
-			if (!ret)
-				break;
+			if (drv) {
+				if (drv == entry)
+					break;
+			} else {
+				if (!ret)
+					break;
+			}
 		}
 		if (entry == driver + n_ents)
 			continue;
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 9bc682cffe..3c6fa3838d 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -236,7 +236,7 @@ static int dm_scan_fdt_node(struct udevice *parent, ofnode parent_node,
 			pr_debug("   - ignoring disabled device\n");
 			continue;
 		}
-		err = lists_bind_fdt(parent, node, NULL, pre_reloc_only);
+		err = lists_bind_fdt(parent, node, NULL, NULL, pre_reloc_only);
 		if (err && !ret) {
 			ret = err;
 			debug("%s: ret=%d\n", node_name, ret);
diff --git a/drivers/misc/imx8/scu.c b/drivers/misc/imx8/scu.c
index 035a600f71..4ab5cb4bf1 100644
--- a/drivers/misc/imx8/scu.c
+++ b/drivers/misc/imx8/scu.c
@@ -219,7 +219,7 @@ static int imx8_scu_bind(struct udevice *dev)
 
 	debug("%s(dev=%p)\n", __func__, dev);
 	ofnode_for_each_subnode(node, dev_ofnode(dev)) {
-		ret = lists_bind_fdt(dev, node, &child, true);
+		ret = lists_bind_fdt(dev, node, &child, NULL, true);
 		if (ret)
 			return ret;
 		debug("bind child dev %s\n", child->name);
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 8a87eed683..6d1c671efc 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -67,7 +67,7 @@ static int serial_check_stdout(const void *blob, struct udevice **devp)
 	 * anyway.
 	 */
 	if (node > 0 && !lists_bind_fdt(gd->dm_root, offset_to_ofnode(node),
-					devp, false)) {
+					devp, NULL, false)) {
 		if (!device_probe(*devp))
 			return 0;
 	}
diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
index 6f00a5d0db..b1ac604b5b 100644
--- a/drivers/timer/timer-uclass.c
+++ b/drivers/timer/timer-uclass.c
@@ -148,7 +148,7 @@ int notrace dm_timer_init(void)
 		 * If the timer is not marked to be bound before
 		 * relocation, bind it anyway.
 		 */
-		if (!lists_bind_fdt(dm_root(), node, &dev, false)) {
+		if (!lists_bind_fdt(dm_root(), node, &dev, NULL, false)) {
 			ret = device_probe(dev);
 			if (ret)
 				return ret;
diff --git a/include/dm/lists.h b/include/dm/lists.h
index 1a86552546..5896ae3658 100644
--- a/include/dm/lists.h
+++ b/include/dm/lists.h
@@ -53,13 +53,14 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only);
  * @parent: parent device (root)
  * @node: device tree node to bind
  * @devp: if non-NULL, returns a pointer to the bound device
+ * @drv: if non-NULL, force this driver to be bound
  * @pre_reloc_only: If true, bind only nodes with special devicetree properties,
  * or drivers with the DM_FLAG_PRE_RELOC flag. If false bind all drivers.
  * @return 0 if device was bound, -EINVAL if the device tree is invalid,
  * other -ve value on error
  */
 int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
-		   bool pre_reloc_only);
+		   struct driver *drv, bool pre_reloc_only);
 
 /**
  * device_bind_driver() - bind a device to a driver
diff --git a/test/dm/nop.c b/test/dm/nop.c
index 2cd92c5240..75b9e7b6cc 100644
--- a/test/dm/nop.c
+++ b/test/dm/nop.c
@@ -25,7 +25,7 @@ static int noptest_bind(struct udevice *parent)
 		const char *bind_flag = ofnode_read_string(ofnode, "bind");
 
 		if (bind_flag && (strcmp(bind_flag, "True") == 0))
-			lists_bind_fdt(parent, ofnode, &dev, false);
+			lists_bind_fdt(parent, ofnode, &dev, NULL, false);
 		ofnode = dev_read_next_subnode(ofnode);
 	}
 
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 6e83aeecd9..c6968b0d5f 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -592,7 +592,7 @@ static int zero_size_cells_bus_child_bind(struct udevice *dev)
 	int err;
 
 	ofnode_for_each_subnode(child, dev_ofnode(dev)) {
-		err = lists_bind_fdt(dev, child, NULL, false);
+		err = lists_bind_fdt(dev, child, NULL, NULL, false);
 		if (err) {
 			dev_err(dev, "%s: lists_bind_fdt, err=%d\n",
 				__func__, err);
-- 
2.17.1

[PATCH v1 2/2] usb: gadget: Add bcdDevice for the DWC2 USB Gadget Controller

[Patrice Chotard]

Add an entry in usb_gadget_controller_number() for the DWC2
gadget controller. It is used to bind the USB Ethernet driver.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
Reported-by: Herbert Poetzl <herbert@13thfloor.at>
Cc: Marek Vasut <marex@denx.de>
Cc: Herbert Poetzl <herbert@13thfloor.at>

---

 drivers/usb/gadget/gadget_chips.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
index 0cdf47c2dd..06e6a48949 100644
--- a/drivers/usb/gadget/gadget_chips.h
+++ b/drivers/usb/gadget/gadget_chips.h
@@ -167,6 +167,12 @@
 #define gadget_is_mtu3(g)        0
 #endif
 
+#ifdef CONFIG_USB_GADGET_DWC2_OTG
+#define gadget_is_dwc2(g)        (!strcmp("dwc2-udc", (g)->name))
+#else
+#define gadget_is_dwc2(g)        0
+#endif
+
 /**
  * usb_gadget_controller_number - support bcdDevice id convention
  * @gadget: the controller being driven
@@ -232,5 +238,7 @@ static inline int usb_gadget_controller_number(struct usb_gadget *gadget)
 		return 0x25;
 	else if (gadget_is_mtu3(gadget))
 		return 0x26;
+	else if (gadget_is_dwc2(gadget))
+		return 0x27;
 	return -ENOENT;
 }
-- 
2.17.1

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Andy Shevchenko]

On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
<patrice.chotard@foss.st.com> wrote:
>
> Fix a regression brings by commit 84f8e36f03fa ("cmd: bind: allow to
> bind driver with driver data")
>
> As example, the following bind command doesn't work:
>
>    bind /soc/usb-otg at 49000000 usb_ether
>
> As usb_ether driver has no compatible string, it can't be find by
> lists_bind_fdt(). In bind_by_node_path(), which called lists_bind_fdt(),
> the driver entry is known, pass it to lists_bind_fdt() to force the driver
> entry selection.
>
> For this, add a new parameter struct *driver to lists_bind_fdt().
> Fix also all lists_bind_fdt() callers.

With or without comments addressed
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Fixes: 84f8e36f03fa ("cmd: bind: allow to bind driver with driver data")

>

No blank line in the tag block.

> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> Reported-by: Herbert Poetzl <herbert@13thfloor.at>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Herbert Poetzl <herbert@13thfloor.at>
> ---
>
>  cmd/bind.c                     |  2 +-
>  drivers/core/device.c          |  2 +-
>  drivers/core/lists.c           | 11 ++++++++---
>  drivers/core/root.c            |  2 +-
>  drivers/misc/imx8/scu.c        |  2 +-
>  drivers/serial/serial-uclass.c |  2 +-
>  drivers/timer/timer-uclass.c   |  2 +-
>  include/dm/lists.h             |  3 ++-
>  test/dm/nop.c                  |  2 +-
>  test/dm/test-fdt.c             |  2 +-
>  10 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/cmd/bind.c b/cmd/bind.c
> index af2f22cc4c..d8f610943c 100644
> --- a/cmd/bind.c
> +++ b/cmd/bind.c
> @@ -152,7 +152,7 @@ static int bind_by_node_path(const char *path, const char *drv_name)
>         }
>
>         ofnode = ofnode_path(path);
> -       ret = lists_bind_fdt(parent, ofnode, &dev, false);
> +       ret = lists_bind_fdt(parent, ofnode, &dev, drv, false);
>
>         if (!dev || ret) {
>                 printf("Unable to bind. err:%d\n", ret);
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 81f6880eac..3abd89aca6 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -1133,6 +1133,6 @@ int dev_enable_by_path(const char *path)
>         if (ret)
>                 return ret;
>
> -       return lists_bind_fdt(parent, node, NULL, false);
> +       return lists_bind_fdt(parent, node, NULL, NULL, false);
>  }
>  #endif
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index e214306b90..2eb808ce2d 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -182,7 +182,7 @@ static int driver_check_compatible(const struct udevice_id *of_match,
>  }
>
>  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
> -                  bool pre_reloc_only)
> +                  struct driver *drv, bool pre_reloc_only)
>  {
>         struct driver *driver = ll_entry_start(struct driver, driver);
>         const int n_ents = ll_entry_count(struct driver, driver);
> @@ -225,8 +225,13 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                 for (entry = driver; entry != driver + n_ents; entry++) {
>                         ret = driver_check_compatible(entry->of_match, &id,
>                                                       compat);
> -                       if (!ret)
> -                               break;
> +                       if (drv) {
> +                               if (drv == entry)
> +                                       break;

> +                       } else {
> +                               if (!ret)
> +                                       break;
> +                       }

This can be simplified to
} else if (!ret)
  break;

>                 }
>                 if (entry == driver + n_ents)
>                         continue;
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index 9bc682cffe..3c6fa3838d 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -236,7 +236,7 @@ static int dm_scan_fdt_node(struct udevice *parent, ofnode parent_node,
>                         pr_debug("   - ignoring disabled device\n");
>                         continue;
>                 }
> -               err = lists_bind_fdt(parent, node, NULL, pre_reloc_only);
> +               err = lists_bind_fdt(parent, node, NULL, NULL, pre_reloc_only);
>                 if (err && !ret) {
>                         ret = err;
>                         debug("%s: ret=%d\n", node_name, ret);
> diff --git a/drivers/misc/imx8/scu.c b/drivers/misc/imx8/scu.c
> index 035a600f71..4ab5cb4bf1 100644
> --- a/drivers/misc/imx8/scu.c
> +++ b/drivers/misc/imx8/scu.c
> @@ -219,7 +219,7 @@ static int imx8_scu_bind(struct udevice *dev)
>
>         debug("%s(dev=%p)\n", __func__, dev);
>         ofnode_for_each_subnode(node, dev_ofnode(dev)) {
> -               ret = lists_bind_fdt(dev, node, &child, true);
> +               ret = lists_bind_fdt(dev, node, &child, NULL, true);
>                 if (ret)
>                         return ret;
>                 debug("bind child dev %s\n", child->name);
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 8a87eed683..6d1c671efc 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -67,7 +67,7 @@ static int serial_check_stdout(const void *blob, struct udevice **devp)
>          * anyway.
>          */
>         if (node > 0 && !lists_bind_fdt(gd->dm_root, offset_to_ofnode(node),
> -                                       devp, false)) {
> +                                       devp, NULL, false)) {
>                 if (!device_probe(*devp))
>                         return 0;
>         }
> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
> index 6f00a5d0db..b1ac604b5b 100644
> --- a/drivers/timer/timer-uclass.c
> +++ b/drivers/timer/timer-uclass.c
> @@ -148,7 +148,7 @@ int notrace dm_timer_init(void)
>                  * If the timer is not marked to be bound before
>                  * relocation, bind it anyway.
>                  */
> -               if (!lists_bind_fdt(dm_root(), node, &dev, false)) {
> +               if (!lists_bind_fdt(dm_root(), node, &dev, NULL, false)) {
>                         ret = device_probe(dev);
>                         if (ret)
>                                 return ret;
> diff --git a/include/dm/lists.h b/include/dm/lists.h
> index 1a86552546..5896ae3658 100644
> --- a/include/dm/lists.h
> +++ b/include/dm/lists.h
> @@ -53,13 +53,14 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only);
>   * @parent: parent device (root)
>   * @node: device tree node to bind
>   * @devp: if non-NULL, returns a pointer to the bound device
> + * @drv: if non-NULL, force this driver to be bound
>   * @pre_reloc_only: If true, bind only nodes with special devicetree properties,
>   * or drivers with the DM_FLAG_PRE_RELOC flag. If false bind all drivers.
>   * @return 0 if device was bound, -EINVAL if the device tree is invalid,
>   * other -ve value on error
>   */
>  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
> -                  bool pre_reloc_only);
> +                  struct driver *drv, bool pre_reloc_only);
>
>  /**
>   * device_bind_driver() - bind a device to a driver
> diff --git a/test/dm/nop.c b/test/dm/nop.c
> index 2cd92c5240..75b9e7b6cc 100644
> --- a/test/dm/nop.c
> +++ b/test/dm/nop.c
> @@ -25,7 +25,7 @@ static int noptest_bind(struct udevice *parent)
>                 const char *bind_flag = ofnode_read_string(ofnode, "bind");
>
>                 if (bind_flag && (strcmp(bind_flag, "True") == 0))
> -                       lists_bind_fdt(parent, ofnode, &dev, false);
> +                       lists_bind_fdt(parent, ofnode, &dev, NULL, false);
>                 ofnode = dev_read_next_subnode(ofnode);
>         }
>
> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> index 6e83aeecd9..c6968b0d5f 100644
> --- a/test/dm/test-fdt.c
> +++ b/test/dm/test-fdt.c
> @@ -592,7 +592,7 @@ static int zero_size_cells_bus_child_bind(struct udevice *dev)
>         int err;
>
>         ofnode_for_each_subnode(child, dev_ofnode(dev)) {
> -               err = lists_bind_fdt(dev, child, NULL, false);
> +               err = lists_bind_fdt(dev, child, NULL, NULL, false);
>                 if (err) {
>                         dev_err(dev, "%s: lists_bind_fdt, err=%d\n",
>                                 __func__, err);
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Patrice CHOTARD]

Hi Andy

On 4/9/21 11:16 AM, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
> <patrice.chotard@foss.st.com> wrote:
>>
>> Fix a regression brings by commit 84f8e36f03fa ("cmd: bind: allow to
>> bind driver with driver data")
>>
>> As example, the following bind command doesn't work:
>>
>>    bind /soc/usb-otg at 49000000 usb_ether
>>
>> As usb_ether driver has no compatible string, it can't be find by
>> lists_bind_fdt(). In bind_by_node_path(), which called lists_bind_fdt(),
>> the driver entry is known, pass it to lists_bind_fdt() to force the driver
>> entry selection.
>>
>> For this, add a new parameter struct *driver to lists_bind_fdt().
>> Fix also all lists_bind_fdt() callers.
> 
> With or without comments addressed
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
>>
>> Fixes: 84f8e36f03fa ("cmd: bind: allow to bind driver with driver data")
> 
>>
> 
> No blank line in the tag block.

ok will remove it

> 
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> Reported-by: Herbert Poetzl <herbert@13thfloor.at>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Herbert Poetzl <herbert@13thfloor.at>
>> ---
>>
>>  cmd/bind.c                     |  2 +-
>>  drivers/core/device.c          |  2 +-
>>  drivers/core/lists.c           | 11 ++++++++---
>>  drivers/core/root.c            |  2 +-
>>  drivers/misc/imx8/scu.c        |  2 +-
>>  drivers/serial/serial-uclass.c |  2 +-
>>  drivers/timer/timer-uclass.c   |  2 +-
>>  include/dm/lists.h             |  3 ++-
>>  test/dm/nop.c                  |  2 +-
>>  test/dm/test-fdt.c             |  2 +-
>>  10 files changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/cmd/bind.c b/cmd/bind.c
>> index af2f22cc4c..d8f610943c 100644
>> --- a/cmd/bind.c
>> +++ b/cmd/bind.c
>> @@ -152,7 +152,7 @@ static int bind_by_node_path(const char *path, const char *drv_name)
>>         }
>>
>>         ofnode = ofnode_path(path);
>> -       ret = lists_bind_fdt(parent, ofnode, &dev, false);
>> +       ret = lists_bind_fdt(parent, ofnode, &dev, drv, false);
>>
>>         if (!dev || ret) {
>>                 printf("Unable to bind. err:%d\n", ret);
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 81f6880eac..3abd89aca6 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -1133,6 +1133,6 @@ int dev_enable_by_path(const char *path)
>>         if (ret)
>>                 return ret;
>>
>> -       return lists_bind_fdt(parent, node, NULL, false);
>> +       return lists_bind_fdt(parent, node, NULL, NULL, false);
>>  }
>>  #endif
>> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>> index e214306b90..2eb808ce2d 100644
>> --- a/drivers/core/lists.c
>> +++ b/drivers/core/lists.c
>> @@ -182,7 +182,7 @@ static int driver_check_compatible(const struct udevice_id *of_match,
>>  }
>>
>>  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>> -                  bool pre_reloc_only)
>> +                  struct driver *drv, bool pre_reloc_only)
>>  {
>>         struct driver *driver = ll_entry_start(struct driver, driver);
>>         const int n_ents = ll_entry_count(struct driver, driver);
>> @@ -225,8 +225,13 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>>                 for (entry = driver; entry != driver + n_ents; entry++) {
>>                         ret = driver_check_compatible(entry->of_match, &id,
>>                                                       compat);
>> -                       if (!ret)
>> -                               break;
>> +                       if (drv) {
>> +                               if (drv == entry)
>> +                                       break;
> 
>> +                       } else {
>> +                               if (!ret)
>> +                                       break;
>> +                       }
> 
> This can be simplified to
> } else if (!ret)
>   break;

I know but checkpatch.pl requested it ;-)

Thanks
Patrice

> 
>>                 }
>>                 if (entry == driver + n_ents)
>>                         continue;
>> diff --git a/drivers/core/root.c b/drivers/core/root.c
>> index 9bc682cffe..3c6fa3838d 100644
>> --- a/drivers/core/root.c
>> +++ b/drivers/core/root.c
>> @@ -236,7 +236,7 @@ static int dm_scan_fdt_node(struct udevice *parent, ofnode parent_node,
>>                         pr_debug("   - ignoring disabled device\n");
>>                         continue;
>>                 }
>> -               err = lists_bind_fdt(parent, node, NULL, pre_reloc_only);
>> +               err = lists_bind_fdt(parent, node, NULL, NULL, pre_reloc_only);
>>                 if (err && !ret) {
>>                         ret = err;
>>                         debug("%s: ret=%d\n", node_name, ret);
>> diff --git a/drivers/misc/imx8/scu.c b/drivers/misc/imx8/scu.c
>> index 035a600f71..4ab5cb4bf1 100644
>> --- a/drivers/misc/imx8/scu.c
>> +++ b/drivers/misc/imx8/scu.c
>> @@ -219,7 +219,7 @@ static int imx8_scu_bind(struct udevice *dev)
>>
>>         debug("%s(dev=%p)\n", __func__, dev);
>>         ofnode_for_each_subnode(node, dev_ofnode(dev)) {
>> -               ret = lists_bind_fdt(dev, node, &child, true);
>> +               ret = lists_bind_fdt(dev, node, &child, NULL, true);
>>                 if (ret)
>>                         return ret;
>>                 debug("bind child dev %s\n", child->name);
>> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
>> index 8a87eed683..6d1c671efc 100644
>> --- a/drivers/serial/serial-uclass.c
>> +++ b/drivers/serial/serial-uclass.c
>> @@ -67,7 +67,7 @@ static int serial_check_stdout(const void *blob, struct udevice **devp)
>>          * anyway.
>>          */
>>         if (node > 0 && !lists_bind_fdt(gd->dm_root, offset_to_ofnode(node),
>> -                                       devp, false)) {
>> +                                       devp, NULL, false)) {
>>                 if (!device_probe(*devp))
>>                         return 0;
>>         }
>> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
>> index 6f00a5d0db..b1ac604b5b 100644
>> --- a/drivers/timer/timer-uclass.c
>> +++ b/drivers/timer/timer-uclass.c
>> @@ -148,7 +148,7 @@ int notrace dm_timer_init(void)
>>                  * If the timer is not marked to be bound before
>>                  * relocation, bind it anyway.
>>                  */
>> -               if (!lists_bind_fdt(dm_root(), node, &dev, false)) {
>> +               if (!lists_bind_fdt(dm_root(), node, &dev, NULL, false)) {
>>                         ret = device_probe(dev);
>>                         if (ret)
>>                                 return ret;
>> diff --git a/include/dm/lists.h b/include/dm/lists.h
>> index 1a86552546..5896ae3658 100644
>> --- a/include/dm/lists.h
>> +++ b/include/dm/lists.h
>> @@ -53,13 +53,14 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only);
>>   * @parent: parent device (root)
>>   * @node: device tree node to bind
>>   * @devp: if non-NULL, returns a pointer to the bound device
>> + * @drv: if non-NULL, force this driver to be bound
>>   * @pre_reloc_only: If true, bind only nodes with special devicetree properties,
>>   * or drivers with the DM_FLAG_PRE_RELOC flag. If false bind all drivers.
>>   * @return 0 if device was bound, -EINVAL if the device tree is invalid,
>>   * other -ve value on error
>>   */
>>  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>> -                  bool pre_reloc_only);
>> +                  struct driver *drv, bool pre_reloc_only);
>>
>>  /**
>>   * device_bind_driver() - bind a device to a driver
>> diff --git a/test/dm/nop.c b/test/dm/nop.c
>> index 2cd92c5240..75b9e7b6cc 100644
>> --- a/test/dm/nop.c
>> +++ b/test/dm/nop.c
>> @@ -25,7 +25,7 @@ static int noptest_bind(struct udevice *parent)
>>                 const char *bind_flag = ofnode_read_string(ofnode, "bind");
>>
>>                 if (bind_flag && (strcmp(bind_flag, "True") == 0))
>> -                       lists_bind_fdt(parent, ofnode, &dev, false);
>> +                       lists_bind_fdt(parent, ofnode, &dev, NULL, false);
>>                 ofnode = dev_read_next_subnode(ofnode);
>>         }
>>
>> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
>> index 6e83aeecd9..c6968b0d5f 100644
>> --- a/test/dm/test-fdt.c
>> +++ b/test/dm/test-fdt.c
>> @@ -592,7 +592,7 @@ static int zero_size_cells_bus_child_bind(struct udevice *dev)
>>         int err;
>>
>>         ofnode_for_each_subnode(child, dev_ofnode(dev)) {
>> -               err = lists_bind_fdt(dev, child, NULL, false);
>> +               err = lists_bind_fdt(dev, child, NULL, NULL, false);
>>                 if (err) {
>>                         dev_err(dev, "%s: lists_bind_fdt, err=%d\n",
>>                                 __func__, err);
>> --
>> 2.17.1
>>
> 
> 

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Andy Shevchenko]

On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
> On 4/9/21 11:16 AM, Andy Shevchenko wrote:
> > On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
> > <patrice.chotard@foss.st.com> wrote:

...

> >> +                       if (drv) {
> >> +                               if (drv == entry)
> >> +                                       break;
> >
> >> +                       } else {
> >> +                               if (!ret)
> >> +                                       break;
> >> +                       }
> >
> > This can be simplified to
> > } else if (!ret)
> >   break;
>
> I know but checkpatch.pl requested it ;-)

You mean it doesn't recognize 'else if' construction? Then it's a bug
there for sure.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Patrice CHOTARD]

On 4/9/21 11:48 AM, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
> <patrice.chotard@foss.st.com> wrote:
>> On 4/9/21 11:16 AM, Andy Shevchenko wrote:
>>> On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
>>> <patrice.chotard@foss.st.com> wrote:
> 
> ...
> 
>>>> +                       if (drv) {
>>>> +                               if (drv == entry)
>>>> +                                       break;
>>>
>>>> +                       } else {
>>>> +                               if (!ret)
>>>> +                                       break;
>>>> +                       }
>>>
>>> This can be simplified to
>>> } else if (!ret)
>>>   break;
>>
>> I know but checkpatch.pl requested it ;-)
> 
> You mean it doesn't recognize 'else if' construction? Then it's a bug
> there for sure.
> 

No, i mean checkpath.pl request to put {} on all statements as shown below :

./scripts/checkpatch.pl 0001-cmd-bind-Fix-driver-binding-on-a-device.patch 
CHECK: braces {} should be used on all arms of this statement
#83: FILE: drivers/core/lists.c:228:
+			if (drv) {
[...]
+			} else if (!ret)
[...]

total: 0 errors, 0 warnings, 1 checks, 100 lines checked

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Andy Shevchenko]

On Fri, Apr 9, 2021 at 1:32 PM Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
> On 4/9/21 11:48 AM, Andy Shevchenko wrote:
> > On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
> > <patrice.chotard@foss.st.com> wrote:
> >> On 4/9/21 11:16 AM, Andy Shevchenko wrote:
> >>> On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
> >>> <patrice.chotard@foss.st.com> wrote:

...

> >>>> +                       if (drv) {
> >>>> +                               if (drv == entry)
> >>>> +                                       break;
> >>>
> >>>> +                       } else {
> >>>> +                               if (!ret)
> >>>> +                                       break;
> >>>> +                       }
> >>>
> >>> This can be simplified to
> >>> } else if (!ret)
> >>>   break;
> >>
> >> I know but checkpatch.pl requested it ;-)
> >
> > You mean it doesn't recognize 'else if' construction? Then it's a bug
> > there for sure.
> >
>
> No, i mean checkpath.pl request to put {} on all statements as shown below :
>
> ./scripts/checkpatch.pl 0001-cmd-bind-Fix-driver-binding-on-a-device.patch
> CHECK: braces {} should be used on all arms of this statement
> #83: FILE: drivers/core/lists.c:228:
> +                       if (drv) {
> [...]
> +                       } else if (!ret)

So, you still can put else and if on one line, right?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Patrice CHOTARD]

On 4/9/21 1:01 PM, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 1:32 PM Patrice CHOTARD
> <patrice.chotard@foss.st.com> wrote:
>> On 4/9/21 11:48 AM, Andy Shevchenko wrote:
>>> On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
>>> <patrice.chotard@foss.st.com> wrote:
>>>> On 4/9/21 11:16 AM, Andy Shevchenko wrote:
>>>>> On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
>>>>> <patrice.chotard@foss.st.com> wrote:
> 
> ...
> 
>>>>>> +                       if (drv) {
>>>>>> +                               if (drv == entry)
>>>>>> +                                       break;
>>>>>
>>>>>> +                       } else {
>>>>>> +                               if (!ret)
>>>>>> +                                       break;
>>>>>> +                       }
>>>>>
>>>>> This can be simplified to
>>>>> } else if (!ret)
>>>>>   break;
>>>>
>>>> I know but checkpatch.pl requested it ;-)
>>>
>>> You mean it doesn't recognize 'else if' construction? Then it's a bug
>>> there for sure.
>>>
>>
>> No, i mean checkpath.pl request to put {} on all statements as shown below :
>>
>> ./scripts/checkpatch.pl 0001-cmd-bind-Fix-driver-binding-on-a-device.patch
>> CHECK: braces {} should be used on all arms of this statement
>> #83: FILE: drivers/core/lists.c:228:
>> +                       if (drv) {
>> [...]
>> +                       } else if (!ret)
> 
> So, you still can put else and if on one line, right?
> 

No, if i put else and if on one line as you suggested, checkpatch.pl is complaining as shown above.

Patrice

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Sean Anderson]

On 4/9/21 8:05 AM, Patrice CHOTARD wrote:
> 
> 
> On 4/9/21 1:01 PM, Andy Shevchenko wrote:
>> On Fri, Apr 9, 2021 at 1:32 PM Patrice CHOTARD
>> <patrice.chotard@foss.st.com> wrote:
>>> On 4/9/21 11:48 AM, Andy Shevchenko wrote:
>>>> On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
>>>> <patrice.chotard@foss.st.com> wrote:
>>>>> On 4/9/21 11:16 AM, Andy Shevchenko wrote:
>>>>>> On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
>>>>>> <patrice.chotard@foss.st.com> wrote:
>>
>> ...
>>
>>>>>>> +                       if (drv) {
>>>>>>> +                               if (drv == entry)
>>>>>>> +                                       break;
>>>>>>
>>>>>>> +                       } else {
>>>>>>> +                               if (!ret)
>>>>>>> +                                       break;
>>>>>>> +                       }
>>>>>>
>>>>>> This can be simplified to
>>>>>> } else if (!ret)
>>>>>>    break;
>>>>>
>>>>> I know but checkpatch.pl requested it ;-)
>>>>
>>>> You mean it doesn't recognize 'else if' construction? Then it's a bug
>>>> there for sure.
>>>>
>>>
>>> No, i mean checkpath.pl request to put {} on all statements as shown below :
>>>
>>> ./scripts/checkpatch.pl 0001-cmd-bind-Fix-driver-binding-on-a-device.patch
>>> CHECK: braces {} should be used on all arms of this statement
>>> #83: FILE: drivers/core/lists.c:228:
>>> +                       if (drv) {
>>> [...]
>>> +                       } else if (!ret)
>>
>> So, you still can put else and if on one line, right?
>>
> 
> No, if i put else and if on one line as you suggested, checkpatch.pl is complaining as shown above.
> 
> Patrice
> 

} else if (!ret) {
	break;
}

?

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Andy Shevchenko]

On Fri, Apr 09, 2021 at 09:13:12AM -0400, Sean Anderson wrote:
> On 4/9/21 8:05 AM, Patrice CHOTARD wrote:
> > On 4/9/21 1:01 PM, Andy Shevchenko wrote:
> > > On Fri, Apr 9, 2021 at 1:32 PM Patrice CHOTARD
> > > <patrice.chotard@foss.st.com> wrote:
> > > > On 4/9/21 11:48 AM, Andy Shevchenko wrote:
> > > > > On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
> > > > > <patrice.chotard@foss.st.com> wrote:
> > > > > > On 4/9/21 11:16 AM, Andy Shevchenko wrote:
> > > > > > > On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
> > > > > > > <patrice.chotard@foss.st.com> wrote:

...

> > > > > > > > +                       if (drv) {
> > > > > > > > +                               if (drv == entry)
> > > > > > > > +                                       break;
> > > > > > > 
> > > > > > > > +                       } else {
> > > > > > > > +                               if (!ret)
> > > > > > > > +                                       break;
> > > > > > > > +                       }
> > > > > > > 
> > > > > > > This can be simplified to
> > > > > > > } else if (!ret)
> > > > > > >    break;
> > > > > > 
> > > > > > I know but checkpatch.pl requested it ;-)
> > > > > 
> > > > > You mean it doesn't recognize 'else if' construction? Then it's a bug
> > > > > there for sure.
> > > > > 
> > > > 
> > > > No, i mean checkpath.pl request to put {} on all statements as shown below :
> > > > 
> > > > ./scripts/checkpatch.pl 0001-cmd-bind-Fix-driver-binding-on-a-device.patch
> > > > CHECK: braces {} should be used on all arms of this statement
> > > > #83: FILE: drivers/core/lists.c:228:
> > > > +                       if (drv) {
> > > > [...]
> > > > +                       } else if (!ret)
> > > 
> > > So, you still can put else and if on one line, right?
> > > 
> > 
> > No, if i put else and if on one line as you suggested, checkpatch.pl is complaining as shown above.
> > 
> > Patrice
> > 
> 
> } else if (!ret) {
> 	break;
> }
> 
> ?

Thanks, that's fine for me. Does checkpatch.pl complain on this?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Patrice CHOTARD]

On 4/9/21 3:41 PM, Andy Shevchenko wrote:
> On Fri, Apr 09, 2021 at 09:13:12AM -0400, Sean Anderson wrote:
>> On 4/9/21 8:05 AM, Patrice CHOTARD wrote:
>>> On 4/9/21 1:01 PM, Andy Shevchenko wrote:
>>>> On Fri, Apr 9, 2021 at 1:32 PM Patrice CHOTARD
>>>> <patrice.chotard@foss.st.com> wrote:
>>>>> On 4/9/21 11:48 AM, Andy Shevchenko wrote:
>>>>>> On Fri, Apr 9, 2021 at 12:28 PM Patrice CHOTARD
>>>>>> <patrice.chotard@foss.st.com> wrote:
>>>>>>> On 4/9/21 11:16 AM, Andy Shevchenko wrote:
>>>>>>>> On Fri, Apr 9, 2021 at 10:37 AM Patrice Chotard
>>>>>>>> <patrice.chotard@foss.st.com> wrote:
> 
> ...
> 
>>>>>>>>> +                       if (drv) {
>>>>>>>>> +                               if (drv == entry)
>>>>>>>>> +                                       break;
>>>>>>>>
>>>>>>>>> +                       } else {
>>>>>>>>> +                               if (!ret)
>>>>>>>>> +                                       break;
>>>>>>>>> +                       }
>>>>>>>>
>>>>>>>> This can be simplified to
>>>>>>>> } else if (!ret)
>>>>>>>>    break;
>>>>>>>
>>>>>>> I know but checkpatch.pl requested it ;-)
>>>>>>
>>>>>> You mean it doesn't recognize 'else if' construction? Then it's a bug
>>>>>> there for sure.
>>>>>>
>>>>>
>>>>> No, i mean checkpath.pl request to put {} on all statements as shown below :
>>>>>
>>>>> ./scripts/checkpatch.pl 0001-cmd-bind-Fix-driver-binding-on-a-device.patch
>>>>> CHECK: braces {} should be used on all arms of this statement
>>>>> #83: FILE: drivers/core/lists.c:228:
>>>>> +                       if (drv) {
>>>>> [...]
>>>>> +                       } else if (!ret)
>>>>
>>>> So, you still can put else and if on one line, right?
>>>>
>>>
>>> No, if i put else and if on one line as you suggested, checkpatch.pl is complaining as shown above.
>>>
>>> Patrice
>>>
>>
>> } else if (!ret) {
>> 	break;
>> }
>>
>> ?
> 
> Thanks, that's fine for me. Does checkpatch.pl complain on this?
> 

This implementation is OK too, checkpatch is happy with it.

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Simon Glass]

On Fri, 9 Apr 2021 at 08:36, Patrice Chotard
<patrice.chotard@foss.st.com> wrote:
>
> Fix a regression brings by commit 84f8e36f03fa ("cmd: bind: allow to
> bind driver with driver data")
>
> As example, the following bind command doesn't work:
>
>    bind /soc/usb-otg at 49000000 usb_ether
>
> As usb_ether driver has no compatible string, it can't be find by
> lists_bind_fdt(). In bind_by_node_path(), which called lists_bind_fdt(),
> the driver entry is known, pass it to lists_bind_fdt() to force the driver
> entry selection.
>
> For this, add a new parameter struct *driver to lists_bind_fdt().
> Fix also all lists_bind_fdt() callers.
>
> Fixes: 84f8e36f03fa ("cmd: bind: allow to bind driver with driver data")
>
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> Reported-by: Herbert Poetzl <herbert@13thfloor.at>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Herbert Poetzl <herbert@13thfloor.at>
> ---
>
>  cmd/bind.c                     |  2 +-
>  drivers/core/device.c          |  2 +-
>  drivers/core/lists.c           | 11 ++++++++---
>  drivers/core/root.c            |  2 +-
>  drivers/misc/imx8/scu.c        |  2 +-
>  drivers/serial/serial-uclass.c |  2 +-
>  drivers/timer/timer-uclass.c   |  2 +-
>  include/dm/lists.h             |  3 ++-
>  test/dm/nop.c                  |  2 +-
>  test/dm/test-fdt.c             |  2 +-
>  10 files changed, 18 insertions(+), 12 deletions(-)
>

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

Really this command needs a test.

Regards,
Simon

[PATCH v1 1/2] cmd: bind: Fix driver binding on a device

[Patrice CHOTARD]

Hi Simon

On 4/14/21 9:38 PM, Simon Glass wrote:
> On Fri, 9 Apr 2021 at 08:36, Patrice Chotard
> <patrice.chotard@foss.st.com> wrote:
>>
>> Fix a regression brings by commit 84f8e36f03fa ("cmd: bind: allow to
>> bind driver with driver data")
>>
>> As example, the following bind command doesn't work:
>>
>>    bind /soc/usb-otg at 49000000 usb_ether
>>
>> As usb_ether driver has no compatible string, it can't be find by
>> lists_bind_fdt(). In bind_by_node_path(), which called lists_bind_fdt(),
>> the driver entry is known, pass it to lists_bind_fdt() to force the driver
>> entry selection.
>>
>> For this, add a new parameter struct *driver to lists_bind_fdt().
>> Fix also all lists_bind_fdt() callers.
>>
>> Fixes: 84f8e36f03fa ("cmd: bind: allow to bind driver with driver data")
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> Reported-by: Herbert Poetzl <herbert@13thfloor.at>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Herbert Poetzl <herbert@13thfloor.at>
>> ---
>>
>>  cmd/bind.c                     |  2 +-
>>  drivers/core/device.c          |  2 +-
>>  drivers/core/lists.c           | 11 ++++++++---
>>  drivers/core/root.c            |  2 +-
>>  drivers/misc/imx8/scu.c        |  2 +-
>>  drivers/serial/serial-uclass.c |  2 +-
>>  drivers/timer/timer-uclass.c   |  2 +-
>>  include/dm/lists.h             |  3 ++-
>>  test/dm/nop.c                  |  2 +-
>>  test/dm/test-fdt.c             |  2 +-
>>  10 files changed, 18 insertions(+), 12 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Really this command needs a test.

Yes i will work on that even is there is already a bind test. 
In fact, this new use case was not covered by the existing implementation.

Patrice

> 
> Regards,
> Simon
>