Re: [PATCH v15 05/19] media: uvcvideo: Handle uvc menu translation inside uvc_get_le_value

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 14-Nov-24 8:10 PM, Ricardo Ribalda wrote:
> map->get() gets a value from an uvc_control in "UVC format" and converts
> it to a value that can be consumed by v4l2.
> 
> Instead of using a special get function for V4L2_CTRL_TYPE_MENU, we
> were converting from uvc_get_le_value in two different places.
> 
> Move the conversion to uvc_get_le_value().
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 77 +++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index bab9fdac98e6..77f7058ec966 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -862,6 +862,25 @@ static inline void uvc_clear_bit(u8 *data, int bit)
>  	data[bit >> 3] &= ~(1 << (bit & 7));
>  }
>  
> +static s32 uvc_menu_to_v4l2_menu(struct uvc_control_mapping *mapping, s32 val)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
> +		u32 menu_value;
> +
> +		if (!test_bit(i, &mapping->menu_mask))
> +			continue;
> +
> +		menu_value = uvc_mapping_get_menu_value(mapping, i);
> +
> +		if (menu_value == val)
> +			return i;
> +	}
> +
> +	return val;
> +}
> +
>  /*
>   * Extract the bit string specified by mapping->offset and mapping->size
>   * from the little-endian data stored at 'data' and return the result as
> @@ -896,6 +915,16 @@ static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
>  	if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)
>  		value |= -(value & (1 << (mapping->size - 1)));
>  
> +	/* If it is a menu, convert from uvc to v4l2. */
> +	if (mapping->v4l2_type != V4L2_CTRL_TYPE_MENU)
> +		return value;
> +
> +	switch (query) {
> +	case UVC_GET_CUR:
> +	case UVC_GET_DEF:
> +		return uvc_menu_to_v4l2_menu(mapping, value);
> +	}
> +
>  	return value;
>  }
>  
> @@ -1060,32 +1089,6 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
>  	return 0;
>  }
>  
> -static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> -				const u8 *data)
> -{
> -	s32 value = mapping->get(mapping, UVC_GET_CUR, data);
> -
> -	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> -		unsigned int i;
> -
> -		for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
> -			u32 menu_value;
> -
> -			if (!test_bit(i, &mapping->menu_mask))
> -				continue;
> -
> -			menu_value = uvc_mapping_get_menu_value(mapping, i);
> -
> -			if (menu_value == value) {
> -				value = i;
> -				break;
> -			}
> -		}
> -	}
> -
> -	return value;
> -}
> -
>  static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
>  			       struct uvc_control *ctrl)
>  {
> @@ -1136,8 +1139,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
>  	if (ret < 0)
>  		return ret;
>  
> -	*value = __uvc_ctrl_get_value(mapping,
> -				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> +	*value = mapping->get(mapping, UVC_GET_CUR,
> +			      uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>  
>  	return 0;
>  }
> @@ -1287,7 +1290,6 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  {
>  	struct uvc_control_mapping *master_map = NULL;
>  	struct uvc_control *master_ctrl = NULL;
> -	unsigned int i;
>  
>  	memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
>  	v4l2_ctrl->id = mapping->id;
> @@ -1330,21 +1332,6 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  		v4l2_ctrl->minimum = ffs(mapping->menu_mask) - 1;
>  		v4l2_ctrl->maximum = fls(mapping->menu_mask) - 1;
>  		v4l2_ctrl->step = 1;
> -
> -		for (i = 0; BIT(i) <= mapping->menu_mask; ++i) {
> -			u32 menu_value;
> -
> -			if (!test_bit(i, &mapping->menu_mask))
> -				continue;
> -
> -			menu_value = uvc_mapping_get_menu_value(mapping, i);
> -
> -			if (menu_value == v4l2_ctrl->default_value) {
> -				v4l2_ctrl->default_value = i;
> -				break;
> -			}
> -		}
> -
>  		return 0;
>  
>  	case V4L2_CTRL_TYPE_BOOLEAN:
> @@ -1592,7 +1579,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>  	ctrl->handle = NULL;
>  
>  	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> -		s32 value = __uvc_ctrl_get_value(mapping, data);
> +		s32 value = mapping->get(mapping, UVC_GET_CUR, data);
>  
>  		/*
>  		 * handle may be NULL here if the device sends auto-update
>