On 07/16/15 10:11, Laurent Pinchart wrote:
> 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.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.
One of these days I should sit down and convert saa7164 to the control framework.
That shouldn't be too difficult.
Regards,
Hans
>
>>> 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.
>
--
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
