[U-Boot] [PATCH v2 18/26] dm: usb: Remove inactive children after a bus scan

[Hans de Goede]

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

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

>   	/* if we were not able to find at least one working bus, bail out */
>   	if (!count)
>   		printf("No controllers found\n");
>

Regards,

Hans