Hi Hans,
On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
> On 07/15/15 23:05, Laurent Pinchart wrote:
> > 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.riba...@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)
>
> It's only open-coded in drivers that do not use the control framework. For
> drivers that use the control framework it is completely transparent.
Sure, but still, the same function is implemented several times while a single
implementation could do. I'd prefer having it in the core until all (or all
but one) drivers are converted to the control framework.
> > 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.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
