Re: [PATCH v2 3/7] media: uvcvideo: Probe the PLF characteristics

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

Hi Ricardo,

Thank you for the patch.

On Mon, Jun 10, 2024 at 11:09:54PM +0000, Ricardo Ribalda wrote:
> The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
> 60Hz and Auto. But it does not clearly define if all the values must be
> implemented or not.
> 
> Instead of just using the UVC version to determine what the PLF control
> can do, probe it.
> 
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index d82cfc56dfd5..5b77ac308c84 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -495,18 +495,60 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
>  				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
>  };
>  
> -static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> +static int __uvc_ctrl_add_mapping_to_list(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping);
>  
>  static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
>  {
> +	const struct uvc_control_mapping *out_mapping =
> +					&uvc_ctrl_power_line_mapping_uvc11;
> +	u8 *buf __free(kfree) = NULL;
> +	u8 init_val;
> +	int ret;
> +
> +	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Save the default PLF value, so we can restore it. */
> +	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> +			     chain->dev->intfnum, ctrl->info.selector,
> +			     buf, sizeof(*buf));
> +	/* If we cannot read the control skip it. */
> +	if (ret)
> +		return ret;
> +	init_val = *buf;
> +
> +	/* If PLF value cannot be set to off, it is limited. */
> +	*buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
> +	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> +			     chain->dev->intfnum, ctrl->info.selector,
> +			     buf, sizeof(*buf));
> +	if (ret)
> +		return __uvc_ctrl_add_mapping_to_list(chain, ctrl,
> +					&uvc_ctrl_power_line_mapping_limited);
> +
> +	/* UVC 1.1 does not define auto, we can exit. */
>  	if (chain->dev->uvc_version < 0x150)
> -		return __uvc_ctrl_add_mapping(chain, ctrl,
> -					      &uvc_ctrl_power_line_mapping_uvc11);
> +		goto end;
> +
> +	/* Check if the device supports auto. */
> +	*buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
> +	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> +			     chain->dev->intfnum, ctrl->info.selector,
> +			     buf, sizeof(*buf));
> +	if (!ret)
> +		out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
> +
> +end:
> +	/* Restore initial value and add mapping. */
> +	*buf = init_val;
> +	uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> +		       chain->dev->intfnum, ctrl->info.selector,
> +		       buf, sizeof(*buf));
>  
> -	return __uvc_ctrl_add_mapping(chain, ctrl,
> -				      &uvc_ctrl_power_line_mapping_uvc15);
> +	return __uvc_ctrl_add_mapping_to_list(chain, ctrl, out_mapping);
>  }
>  
>  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {

-- 
Regards,

Laurent Pinchart