Re: [PATCH v2 09/12] media: uvcvideo: Add uvc_ctrl_query_entity helper

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

Hi Ricardo,

On Thu, Jun 05, 2025 at 05:53:02PM +0000, Ricardo Ribalda wrote:
> Create a helper function to query a control. The new function reduces
> the number of arguments, calculates the length of the operation and
> redirects the operation to the hardware or to the entity private
> functions.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 81 ++++++++++++++++++++--------------------
>  1 file changed, 41 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b2768080c08aafa85acb9b7f318672c043d84e55..21ec7b978bc7aca21db7cb8fd5d135d876f3330c 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -576,6 +576,34 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
>  				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
>  };
>  
> +static int uvc_ctrl_query_entity(struct uvc_device *dev,
> +				 const struct uvc_control *ctrl, u8 query,
> +				 void *data)
> +{
> +	u16 len;
> +
> +	switch (query) {
> +	case UVC_GET_INFO:
> +		len = 1;
> +		break;
> +	case UVC_GET_LEN:
> +		len = 2;
> +		break;
> +	default:
> +		len = ctrl->info.size;

		break;

> +	}
> +
> +	if (query == UVC_GET_CUR && ctrl->entity->get_cur)
> +		return ctrl->entity->get_cur(dev, ctrl->entity,
> +					     ctrl->info.selector, data, len);
> +	if (query == UVC_GET_INFO && ctrl->entity->get_info)
> +		return ctrl->entity->get_info(dev, ctrl->entity,
> +					      ctrl->info.selector, data);
> +
> +	return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
> +			      ctrl->info.selector, data, len);
> +}
> +
>  static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping(
>  	struct uvc_video_chain *chain, struct uvc_control *ctrl)
>  {
> @@ -1222,35 +1250,27 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
>  	int ret;
>  
>  	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
> -		ret = uvc_query_ctrl(chain->dev, UVC_GET_DEF, ctrl->entity->id,
> -				     chain->dev->intfnum, ctrl->info.selector,
> -				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> -				     ctrl->info.size);
> +		ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_DEF,
> +					uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
>  		if (ret < 0)
>  			return ret;
>  	}
>  
>  	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
> -		ret = uvc_query_ctrl(chain->dev, UVC_GET_MIN, ctrl->entity->id,
> -				     chain->dev->intfnum, ctrl->info.selector,
> -				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
> -				     ctrl->info.size);
> +		ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MIN,
> +					uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
>  		if (ret < 0)
>  			return ret;
>  	}
>  	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) {
> -		ret = uvc_query_ctrl(chain->dev, UVC_GET_MAX, ctrl->entity->id,
> -				     chain->dev->intfnum, ctrl->info.selector,
> -				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
> -				     ctrl->info.size);
> +		ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MAX,
> +					uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
>  		if (ret < 0)
>  			return ret;
>  	}
>  	if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) {
> -		ret = uvc_query_ctrl(chain->dev, UVC_GET_RES, ctrl->entity->id,
> -				     chain->dev->intfnum, ctrl->info.selector,
> -				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES),
> -				     ctrl->info.size);
> +		ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_RES,
> +					uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
>  		if (ret < 0) {
>  			if (UVC_ENTITY_TYPE(ctrl->entity) !=
>  			    UVC_VC_EXTENSION_UNIT)
> @@ -1291,16 +1311,7 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
>  		return 0;
>  	}
>  
> -	if (ctrl->entity->get_cur)
> -		ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
> -					    ctrl->info.selector, data,
> -					    ctrl->info.size);
> -	else
> -		ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> -				     ctrl->entity->id, chain->dev->intfnum,
> -				     ctrl->info.selector, data,
> -				     ctrl->info.size);
> -
> +	ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_CUR, data);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -2164,11 +2175,8 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  			continue;
>  
>  		if (!rollback)
> -			ret = uvc_query_ctrl(dev, UVC_SET_CUR, ctrl->entity->id,
> -				dev->intfnum, ctrl->info.selector,
> -				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> -				ctrl->info.size);
> -
> +			ret = uvc_ctrl_query_entity(dev, ctrl, UVC_SET_CUR,
> +				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>  		if (!ret)
>  			processed_ctrls++;
>  
> @@ -2570,13 +2578,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
>  	if (data == NULL)
>  		return -ENOMEM;
>  
> -	if (ctrl->entity->get_info)
> -		ret = ctrl->entity->get_info(dev, ctrl->entity,
> -					     ctrl->info.selector, data);
> -	else
> -		ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> -				     dev->intfnum, info->selector, data, 1);

Here ctrl->info isn't filled yet, so replacing info->selector with
ctrl->info.selector won't work. Usage of ctrl->info.selector in the
->get_info() branch looks like a bug.

> -
> +	ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_INFO, data);
>  	if (!ret) {
>  		info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
>  				 UVC_CTRL_FLAG_SET_CUR |
> @@ -2654,8 +2656,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  	info->selector = ctrl->index + 1;
>  
>  	/* Query and verify the control length (GET_LEN) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_LEN, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 2);
> +	ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_LEN, data);

Same here.

>  	if (ret < 0) {
>  		uvc_dbg(dev, CONTROL,
>  			"GET_LEN failed on control %pUl/%u (%d)\n",

-- 
Regards,

Laurent Pinchart