Hi Marek, On Wed, 2 Aug 2023 at 06:47, Marek Vasut <ma...@denx.de> wrote: > > Extend the driver core to perform lookup by both OF node and driver > bound to the node. Use this to look up specific device instances to > unbind from nodes in the unbind command. One example where this is > needed is USB peripheral controller, which may have multiple gadget > drivers bound to it. The unbind command has to select that specific > gadget driver instance to unbind from the controller, not unbind the > controller driver itself from the controller. > > USB ethernet gadget usage looks as follows with this change. Notice > the extra 'usb_ether' addition in the 'unbind' command at the end. > " > bind /soc/usb-otg@49000000 usb_ether > setenv ethact usb_ether > setenv loadaddr 0xc2000000 > setenv ipaddr 10.0.0.2 > setenv serverip 10.0.0.1 > setenv netmask 255.255.255.0 > tftpboot 0xc2000000 10.0.0.1:test.file > unbind /soc/usb-otg@49000000 usb_ether > " > > Signed-off-by: Marek Vasut <ma...@denx.de> > --- > Cc: Kevin Hilman <khil...@baylibre.com> > Cc: Lukasz Majewski <lu...@denx.de> > Cc: Miquel Raynal <miquel.ray...@bootlin.com> > Cc: Simon Glass <s...@chromium.org> > --- > V2: No change > V3: No change > V4: No change > --- > cmd/bind.c | 10 +++++----- > drivers/core/device.c | 20 +++++++++++++++----- > include/dm/device.h | 17 +++++++++++++++++ > 3 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/cmd/bind.c b/cmd/bind.c > index 4d1b7885e60..3137cdf6cb5 100644 > --- a/cmd/bind.c > +++ b/cmd/bind.c > @@ -162,7 +162,7 @@ static int bind_by_node_path(const char *path, const char > *drv_name) > return 0; > } > > -static int unbind_by_node_path(const char *path) > +static int unbind_by_node_path(const char *path, const char *drv_name)
Can you add a function comment? I am wondering what drm_name is for > { > struct udevice *dev; > int ret; > @@ -174,7 +174,7 @@ static int unbind_by_node_path(const char *path) > return -EINVAL; > } > > - ret = device_find_global_by_ofnode(ofnode, &dev); > + ret = device_find_global_by_ofnode_driver(ofnode, drv_name, &dev); > > if (!dev || ret) { > printf("Cannot find a device with path %s\n", path); > @@ -214,9 +214,9 @@ static int do_bind_unbind(struct cmd_tbl *cmdtp, int > flag, int argc, > return CMD_RET_USAGE; > ret = bind_by_node_path(argv[1], argv[2]); > } else if (by_node && !bind) { > - if (argc != 2) > + if (argc != 2 && argc != 3) how about: argv < 2 > return CMD_RET_USAGE; > - ret = unbind_by_node_path(argv[1]); > + ret = unbind_by_node_path(argv[1], argv[2]); but if argc is 2, how can we access argv[2]? Is it because we accept NULL? In that case, please add a comment. > } else if (!by_node && bind) { > int index = (argc > 2) ? dectoul(argv[2], NULL) : 0; > > @@ -251,7 +251,7 @@ U_BOOT_CMD( > U_BOOT_CMD( > unbind, 4, 0, do_bind_unbind, > "Unbind a device from a driver", > - "<node path>\n" > + "<node path> [<driver>]\n" > "unbind <class> <index>\n" > "unbind <class> <index> <driver>\n" > ); > diff --git a/drivers/core/device.c b/drivers/core/device.c > index 6e26b64fb81..52fceb69341 100644 > --- a/drivers/core/device.c > +++ b/drivers/core/device.c > @@ -877,15 +877,17 @@ int device_get_child_by_of_offset(const struct udevice > *parent, int node, > } > > static struct udevice *_device_find_global_by_ofnode(struct udevice *parent, > - ofnode ofnode) > + ofnode ofnode, > + const char *drv) > { > struct udevice *dev, *found; > > - if (ofnode_equal(dev_ofnode(parent), ofnode)) > + if (ofnode_equal(dev_ofnode(parent), ofnode) && > + (!drv || (drv && !strcmp(parent->driver->name, drv)))) > return parent; > > device_foreach_child(dev, parent) { > - found = _device_find_global_by_ofnode(dev, ofnode); > + found = _device_find_global_by_ofnode(dev, ofnode, drv); > if (found) > return found; > } > @@ -895,7 +897,15 @@ static struct udevice > *_device_find_global_by_ofnode(struct udevice *parent, > > int device_find_global_by_ofnode(ofnode ofnode, struct udevice **devp) > { > - *devp = _device_find_global_by_ofnode(gd->dm_root, ofnode); > + *devp = _device_find_global_by_ofnode(gd->dm_root, ofnode, NULL); > + > + return *devp ? 0 : -ENOENT; > +} > + > +int device_find_global_by_ofnode_driver(ofnode ofnode, const char *drv, > + struct udevice **devp) > +{ > + *devp = _device_find_global_by_ofnode(gd->dm_root, ofnode, drv); > > return *devp ? 0 : -ENOENT; > } > @@ -904,7 +914,7 @@ int device_get_global_by_ofnode(ofnode ofnode, struct > udevice **devp) > { > struct udevice *dev; > > - dev = _device_find_global_by_ofnode(gd->dm_root, ofnode); > + dev = _device_find_global_by_ofnode(gd->dm_root, ofnode, NULL); > return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); > } > > diff --git a/include/dm/device.h b/include/dm/device.h > index b86bf90609b..5f05ae0924f 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -748,6 +748,23 @@ int device_get_child_by_of_offset(const struct udevice > *parent, int of_offset, > > int device_find_global_by_ofnode(ofnode node, struct udevice **devp); > > +/** > + * device_find_global_by_ofnode_driver() - Get a device based on ofnode and > driver > + * > + * Locates a device by its device tree ofnode and driver currently bound to > + * it, searching globally throughout the all driver model devices. > + * > + * The device is NOT probed > + * > + * @node: Device tree ofnode to find > + * @drv: Driver name bound to device > + * @devp: Returns pointer to device if found, otherwise this is set to NULL > + * Return: 0 if OK, -ve on error > + */ > + > +int device_find_global_by_ofnode_driver(ofnode node, const char *drv, > + struct udevice **devp); > + > /** > * device_get_global_by_ofnode() - Get a device based on ofnode > * > -- > 2.40.1 > I suppose the test is in another patch, but it is often better to put it in the same patch. Regards, Simon