Re: [PATCH v4] [media] uvcvideo: Add iFunction or iInterface to device names.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Peter,

Thank you for the patch.

On Wednesday 19 Apr 2017 16:45:27 Peter Boström wrote:
> Permits distinguishing between two /dev/videoX entries from the same
> physical UVC device (that naturally share the same iProduct name).
> 
> This change matches current Windows behavior by prioritizing iFunction
> over iInterface, but unlike Windows it displays both iProduct and
> iFunction/iInterface strings when both are available.
> 
> Signed-off-by: Peter Boström <pbos@google.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 04bf35063c4c..ae22fcf0a529
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1998,6 +1998,7 @@ static int uvc_probe(struct usb_interface *intf,
>  {
>  	struct usb_device *udev = interface_to_usbdev(intf);
>  	struct uvc_device *dev;
> +	int function;
>  	int ret;
> 
>  	if (id->idVendor && id->idProduct)
> @@ -2029,9 +2030,26 @@ static int uvc_probe(struct usb_interface *intf,
>  		strlcpy(dev->name, udev->product, sizeof dev->name);
>  	else
>  		snprintf(dev->name, sizeof dev->name,
> -			"UVC Camera (%04x:%04x)",
> -			le16_to_cpu(udev->descriptor.idVendor),
> -			le16_to_cpu(udev->descriptor.idProduct));
> +			 "UVC Camera (%04x:%04x)",
> +			 le16_to_cpu(udev->descriptor.idVendor),
> +			 le16_to_cpu(udev->descriptor.idProduct));
> +
> +	/*
> +	 * Add iFunction or iInterface to names when available as additional
> +	 * distinguishers between interfaces. iFunction is prioritized over
> +	 * iInterface which matches Windows behavior at the point of writing.
> +	 */
> +	function = intf->cur_altsetting->desc.iInterface;
> +	if (intf->intf_assoc && intf->intf_assoc->iFunction != 0)
> +		function = intf->intf_assoc->iFunction;

Nitpicking, I'd prefer writing this as

	if (intf->intf_assoc && intf->intf_assoc->iFunction != 0)
		function = intf->intf_assoc->iFunction;
	else
		function = intf->cur_altsetting->desc.iInterface;

to clearly show what is the preferred case and what is the fallback. I'll fix 
that when applying, no need to resubmit the patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (function != 0) {
> +		size_t len;
> +
> +		strlcat(dev->name, ": ", sizeof(dev->name));
> +		len = strlen(dev->name);
> +		usb_string(udev, function, dev->name + len,
> +			   sizeof(dev->name) - len);
> +	}
> 
>  	/* Parse the Video Class control descriptor. */
>  	if (uvc_parse_control(dev) < 0) {

-- 
Regards,

Laurent Pinchart