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

[Patrice CHOTARD <patrice.chotard@foss.st.com>]

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