On Thursday 16 July 2015 10:23:03 Hans Verkuil wrote:
> On 07/16/15 10:11, Laurent Pinchart wrote:
> > 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.ribalda@xxxxxxxxx>
> >>>> ---
> >>>>
> >>>> 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.
>
> There are only three drivers that need to implement this manually: uvc,
> saa7164 and pvrusb2. That's not enough to warrant moving this into the
> core.
I'd argue that even just two drivers would be enough :-) Especially given that
the proposed implementation for uvcvideo is wrong.
> One of these days I should sit down and convert saa7164 to the control
> framework. That shouldn't be too difficult.
How about pvrusb2, is there a good reason not to use the control framework
there ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
