[v3] media: uvcvideo: use entity get_cur in uvc_ctrl_set

Entity controls should get_cur using an entity-defined function
instead of via a query. Fix this in uvc_ctrl_set.

Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")

Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v2:
-Move the logic of setting ctrl_data to 0 into load_cur.
-Do not initialize ret=0.
-Fix __uvc_ctrl_get() spacing.
-Fix typo in the title.

Changelog since v1:
-Factored out common logic into __uvc_ctrl_load_cur().

 drivers/media/usb/uvc/uvc_ctrl.c | 85 ++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 37 deletions(-)
  

Hi Yunke,

Thank you for the patch.

On Thu, Jul 07, 2022 at 05:53:31PM +0900, Yunke Cao wrote:
> Entity controls should get_cur using an entity-defined function
> instead of via a query. Fix this in uvc_ctrl_set.
> 
> Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> 
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v2:
> -Move the logic of setting ctrl_data to 0 into load_cur.
> -Do not initialize ret=0.
> -Fix __uvc_ctrl_get() spacing.
> -Fix typo in the title.
> 
> Changelog since v1:
> -Factored out common logic into __uvc_ctrl_load_cur().
> 
>  drivers/media/usb/uvc/uvc_ctrl.c | 85 ++++++++++++++++++--------------
>  1 file changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 0e78233fc8a0..181ce4b5db1e 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -963,35 +963,57 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
>  	return value;
>  }
>  
> +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> +			       struct uvc_control *ctrl)
> +{
> +	int ret;
> +
> +	if (ctrl->loaded)
> +		return 0;
> +
> +	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) {
> +		memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> +		       0, ctrl->info.size);
> +		ctrl->loaded = 1;
> +
> +		return 0;
> +	}
> +
> +	if (ctrl->entity->get_cur) {
> +		ret = ctrl->entity->get_cur(chain->dev,
> +			ctrl->entity,
> +			ctrl->info.selector,
> +			uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> +			ctrl->info.size);

I'll take this as an opportunity to realign the code:

	u8 *data;

	...

	data = uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT);

	if (ctrl->entity->get_cur)
		ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
					    ctrl->info.selector, data,
					    ctrl->info.size);
	else
		...

I'll send a v4 for your review :-)

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

> +	} else {
> +		ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> +				     ctrl->entity->id, chain->dev->intfnum,
> +				     ctrl->info.selector,
> +				     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> +				     ctrl->info.size);
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ctrl->loaded = 1;
> +
> +	return ret;
> +}
> +
>  static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> -	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> -	s32 *value)
> +			  struct uvc_control *ctrl,
> +			  struct uvc_control_mapping *mapping,
> +			  s32 *value)
>  {
>  	int ret;
>  
>  	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
>  		return -EACCES;
>  
> -	if (!ctrl->loaded) {
> -		if (ctrl->entity->get_cur) {
> -			ret = ctrl->entity->get_cur(chain->dev,
> -				ctrl->entity,
> -				ctrl->info.selector,
> -				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> -				ctrl->info.size);
> -		} else {
> -			ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> -				ctrl->entity->id,
> -				chain->dev->intfnum,
> -				ctrl->info.selector,
> -				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> -				ctrl->info.size);
> -		}
> -		if (ret < 0)
> -			return ret;
> -
> -		ctrl->loaded = 1;
> -	}
> +	ret = __uvc_ctrl_load_cur(chain, ctrl);
> +	if (ret < 0)
> +		return ret;
>  
>  	*value = __uvc_ctrl_get_value(mapping,
>  				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> @@ -1783,21 +1805,10 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>  	 * needs to be loaded from the device to perform the read-modify-write
>  	 * operation.
>  	 */
> -	if (!ctrl->loaded && (ctrl->info.size * 8) != mapping->size) {
> -		if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) {
> -			memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> -				0, ctrl->info.size);
> -		} else {
> -			ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> -				ctrl->entity->id, chain->dev->intfnum,
> -				ctrl->info.selector,
> -				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> -				ctrl->info.size);
> -			if (ret < 0)
> -				return ret;
> -		}
> -
> -		ctrl->loaded = 1;
> +	if ((ctrl->info.size * 8) != mapping->size) {
> +		ret = __uvc_ctrl_load_cur(chain, ctrl);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	/* Backup the current value in case we need to rollback later. */