| Message ID | 20210409073617.16045-2-patrice.chotard@foss.st.com |
|---|---|
| State | Superseded |
| Delegated to: | Lukasz Majewski |
| Headers | show |
| Series | cmd: bind: Fix driver binding | expand |
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@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 >
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@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 >> > >
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.
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
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?
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
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; } ?
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?
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.
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@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
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@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 >
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);
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@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(-)