Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Sakari Ailus]

Hi Ricardo,

On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> Move the query control error logic to its own function.
> There is no functional change introduced by this patch.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 853dfb7b5f7b..a57272a2c9e1 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
>  	}
>  }
>  
> -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> -			u8 intfnum, u8 cs, void *data, u16 size)
> +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
>  {
>  	int ret;
>  	u8 error;
>  	u8 tmp;
>  
> -	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> -				UVC_CTRL_CONTROL_TIMEOUT);
> -	if (likely(ret == size))
> -		return 0;
> -
> -	if (ret > 0 && ret < size) {
> -		memset(data + ret, 0, size - ret);
> -		return 0;
> -	}
> -
> -	if (ret != -EPIPE) {
> -		dev_err(&dev->udev->dev,
> -			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> -			uvc_query_name(query), cs, unit, ret, size);
> -		return ret ? ret : -EPIPE;
> -	}
> -
>  	/* Reuse data[0] to request the error code. */
>  	tmp = *(u8 *)data;
>  
> @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	return -EPIPE;
>  }
>  
> +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> +		   u8 intfnum, u8 cs, void *data, u16 size)
> +{
> +	int ret;
> +
> +	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> +			       UVC_CTRL_CONTROL_TIMEOUT);
> +	if (likely(ret == size))
> +		return 0;
> +
> +	if (ret == -EPIPE)
> +		return uvc_query_ctrl_error(dev, intfnum, data);
> +
> +	dev_err(&dev->udev->dev,
> +		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> +		uvc_query_name(query), cs, unit, ret, size);

This message should probably be printed after the check below.

I'd actually move the below check before the ret == -EPIPE check as it's a
successful case (and changing the condition to <= would make the ret ==
size check redundant).

> +
> +	if (ret > 0 && ret < size) {
> +		memset(data + ret, 0, size - ret);
> +		return 0;
> +	}
> +
> +	return ret ? ret : -EPIPE;
> +}
> +
>  static const struct usb_device_id elgato_cam_link_4k = {
>  	USB_DEVICE(0x0fd9, 0x0066)
>  };
> 

-- 
Kind regards,

Sakari Ailus