Hi Hans, On 17 June 2015 at 13:33, Hans de Goede <hdego...@redhat.com> wrote: > On an usb stop instead of leaving orphan usb devices behind simply remove
On a usb_stop() or On a 'usb stop' command ? > them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build > usb_stop() when that is set. This seems a little unfortunate. I can see the reasoning, but do you think this is necessary? I suspect people chasing code size may remove that option and still want to use USB properly. > > The result of this commit is best seen in the output of "dm tree" after > plugging out an usb hub with 2 devices plugges in and plugging in a keyb. > instead, before this commit the output would be: > > usb [ + ] `-- sunxi-musb > usb_hub [ ] |-- usb_hub > usb_mass_st [ ] | |-- usb_mass_storage > usb_dev_gen [ ] | `-- generic_bus_0_dev_3 > usb_dev_gen [ + ] `-- generic_bus_0_dev_1 > > Notice the non active usb_hub child and its 2 non active children. The > first child being non-active as in this example also causes usb_get_dev_index > to return NULL when probing the first child, which results in the usb kbd > code not binding to the keyboard. Although I suspect that could be fixed. > > With this commit in place the output after swapping and "usb reset" is: > > usb [ + ] `-- sunxi-musb > usb_dev_gen [ + ] `-- generic_bus_0_dev_1 > > As expected, and usb_get_dev_index works properly and the keyboard works. > > After this commit usb_find_child() is only necessary for emulated usb > devices, so make its body #ifdef CONFIG_USB_EMUL. > > Signed-off-by: Hans de Goede <hdego...@redhat.com> > --- > drivers/usb/host/usb-uclass.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c > index bce6cec..8f26e35 100644 > --- a/drivers/usb/host/usb-uclass.c > +++ b/drivers/usb/host/usb-uclass.c > @@ -128,6 +128,7 @@ int usb_alloc_device(struct usb_device *udev) > return ops->alloc_device(bus, udev); > } > > +#ifdef CONFIG_DM_DEVICE_REMOVE > int usb_stop(void) > { > struct udevice *bus; > @@ -143,6 +144,12 @@ int usb_stop(void) > uc_priv = uc->priv; > > uclass_foreach_dev(bus, uc) { > + ret = device_chld_remove(bus); > + if (ret && !err) > + err = ret; > + ret = device_chld_unbind(bus); > + if (ret && !err) > + err = ret; > ret = device_remove(bus); > if (ret && !err) > err = ret; > @@ -166,6 +173,7 @@ int usb_stop(void) > > return err; > } > +#endif > > static void usb_scan_bus(struct udevice *bus, bool recurse) > { > @@ -491,6 +499,7 @@ static int usb_find_child(struct udevice *parent, > struct usb_interface_descriptor *iface, > struct udevice **devp) > { > +#ifdef CONFIG_USB_EMUL /* Emulated devices are explictily bound */ explicitly Can you add a comment about this? It seems that we should rename this function to usb_find_emul_child() and have it present only when CONFIG_USB_EMUL is around? Also, why bother with the #ifdef if this function is never called outside sandbox? > struct udevice *dev; > > *devp = NULL; > @@ -509,6 +518,7 @@ static int usb_find_child(struct udevice *parent, > return 0; > } > } > +#endif > > return -ENOENT; > } > -- > 2.4.3 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot