| Message ID | 1434127598-11719-5-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Laurent, Can you review/ack this since it touches on uvc? Thanks! Hans On 06/12/2015 06:46 PM, 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); > + 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; > +} > + > 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, > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
On 07/15/15 23:05, Laurent Pinchart wrote: > 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) 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. 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. > >> 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, > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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.
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@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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. > > 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 ?
On 07/16/15 10:27, Laurent Pinchart wrote: > 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@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. >> >> 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. I don't want to add code to the core to cater to exceptions. >> 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 ? Brrr. pvrusb2 exports controls (and a bunch of other things) to /sys allowing scripts to get/set controls using cat and echo. I have no idea how to switch pvrusb2 to the control framework while still keeping this non-standard API. I guess one of these days I will need to look at it, but I've been postponing it for years now :-) There are still a few other drivers that do not use the control framework: saa7164, zoran, fsl-viu, omap_vout (although I hope that one can be removed), usbvision and bcm2048 (that's a staging driver, so I can probably ignore that one). pvrusb2 is actually fairly important: v4l2-subdev still has legacy ops for handling controls and as long as pvrusb2 is not converted I cannot remove those ops. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Laurent On Thu, Jul 16, 2015 at 10:27 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > I'd argue that even just two drivers would be enough :-) Especially given that > the proposed implementation for uvcvideo is wrong. This is why we have the review process :P. I do my best, but you are the expert on your driver. A core implementation cannot be completely correct, as it will not support array controls. I will resend this patch ASAP. Thanks for your comments!
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); + 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; +} + 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,
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(+)