Re: [RFC v3 04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

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

Hi Ricardo,

Thank you for the patch.

On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> use the controller framework.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> void *fh, return uvc_ctrl_rollback(handle);
>  }
> 
> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> +				     struct v4l2_ext_controls *ctrls)
> +{
> +	struct uvc_fh *handle = fh;
> +	struct uvc_video_chain *chain = handle->chain;
> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> +	unsigned int i;
> +	int ret;
> +	struct v4l2_queryctrl qc;
> +
> +	ret = uvc_ctrl_begin(chain);

There's no need to call uvc_ctrl_begin() here (and if there was, you'd have to 
call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).

> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> +		qc.id = ctrl->id;
> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> +		if (ret < 0) {
> +			ctrls->error_idx = i;
> +			return ret;
> +		}
> +		ctrl->value = qc.default_value;
> +	}
> +
> +	ctrls->error_idx = 0;
> +
> +	return 0;
> +}

Instead of open-coding this in multiple drivers, how about adding a helper 
function to the core ? Something like (totally untested)

int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
                               struct v4l2_ext_controls *ctrls)
{
	struct video_device *vdev = video_devdata(file);
	unsigned int i;
	int ret;

	for (i = 0; i < ctrls->count; ++i) {
		struct v4l2_queryctrl qc;

		qc.id = ctrl->id;
		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
		if (ret < 0) {
			ctrls->error_idx = i;
			return ret;
		}

		ctrls->controls[i].value = qc.default_value;
	}

	ctrls->error_idx = 0;

	return 0;
}

The function could be called by v4l_g_def_ext_ctrls() when ops-
>vidioc_g_def_ext_ctrls is NULL.

>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  				     struct v4l2_ext_controls *ctrls,
>  				     bool commit)
> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>  	.vidioc_querymenu = uvc_ioctl_querymenu,

-- 
Regards,

Laurent Pinchart