Hi Hans,
On 9 November 2015 at 00:22, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 09-11-15 07:48, Simon Glass wrote:
Each scan of the USB bus may return different results. Existing driver-model devices are reused when found, but if a device no longer exists it will stay around, de-activated, but bound.
Detect these devices and remove them after the scan completes.
Signed-off-by: Simon Glass sjg@chromium.org
I wonder how this is better then my original: "dm: usb: Use device_unbind_children to clean up usb devs on stop"
Patch, the end result of both patches is the same and both are a NOP when DM_DEVICE_REMOVE is not set. Where as my code seems to be a much more KISS approach to the problem (my approach is just 3 lines vs 23 lines for yours).
I know we will need usb_find_child in the DM_DEVICE_REMOVE not set case, but why not only revert the:
"dm: usb: Rename usb_find_child to usb_find_emul_child"
commit, keep the other 2 you revert and drop this patch ?
This drops 3 patches from your patch-set and the end result is more clean IMHO.
I would like to avoid binding/unbinding things when nothing changes if possible. Also I'd like to support attaching device tree nodes/properties to USB devices as necessary, as we do with PCI, and removing things breaks that.
I still have to figure out one more test case, so I'll do that before commenting further.
Changes in v2: None
drivers/usb/host/usb-uclass.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 4aa92f8..50538e0 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -202,6 +202,20 @@ static void usb_scan_bus(struct udevice *bus, bool recurse) printf("%d USB Device(s) found\n", priv->next_addr); }
+static void remove_inactive_children(struct uclass *uc, struct udevice *bus) +{
uclass_foreach_dev(bus, uc) {struct udevice *dev, *next;if (!device_active(bus))continue;device_foreach_child_safe(dev, next, bus) {if (!device_active(dev))device_unbind(dev);}}+}
- int usb_init(void) { int controllers_initialized = 0;
@@ -270,6 +284,15 @@ int usb_init(void) }
debug("scan end\n");
/* Remove any devices that were not found on this scan */remove_inactive_children(uc, bus);ret = uclass_get(UCLASS_USB_HUB, &uc);if (ret)return ret;remove_inactive_children(uc, bus);Why do you need to call remove_inactive_children twice here? This seems worthy of a comment explaining why this is necessary.
One is removing the children of USB controllers, one is removing the children of USB hubs. I'll add a comment.
/* if we were not able to find at least one working bus, bail out*/ if (!count) printf("No controllers found\n");
Regards, Simon