| Message ID | 20220707085331.283402-1-yunkec@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] media: uvcvideo: use entity get_cur in uvc_ctrl_set | expand |
Hi Yunke, Thank you for the patch. On Thu, Jul 07, 2022 at 05:53:31PM +0900, Yunke Cao wrote: > Entity controls should get_cur using an entity-defined function > instead of via a query. Fix this in uvc_ctrl_set. > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > Changelog since v2: > -Move the logic of setting ctrl_data to 0 into load_cur. > -Do not initialize ret=0. > -Fix __uvc_ctrl_get() spacing. > -Fix typo in the title. > > Changelog since v1: > -Factored out common logic into __uvc_ctrl_load_cur(). > > drivers/media/usb/uvc/uvc_ctrl.c | 85 ++++++++++++++++++-------------- > 1 file changed, 48 insertions(+), 37 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 0e78233fc8a0..181ce4b5db1e 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -963,35 +963,57 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > return value; > } > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > + struct uvc_control *ctrl) > +{ > + int ret; > + > + if (ctrl->loaded) > + return 0; > + > + if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) { > + memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > + 0, ctrl->info.size); > + ctrl->loaded = 1; > + > + return 0; > + } > + > + if (ctrl->entity->get_cur) { > + ret = ctrl->entity->get_cur(chain->dev, > + ctrl->entity, > + ctrl->info.selector, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > + ctrl->info.size); I'll take this as an opportunity to realign the code: u8 *data; ... data = uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT); if (ctrl->entity->get_cur) ret = ctrl->entity->get_cur(chain->dev, ctrl->entity, ctrl->info.selector, data, ctrl->info.size); else ... I'll send a v4 for your review :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } else { > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > + ctrl->entity->id, chain->dev->intfnum, > + ctrl->info.selector, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > + ctrl->info.size); > + } > + > + if (ret < 0) > + return ret; > + > + ctrl->loaded = 1; > + > + return ret; > +} > + > static int __uvc_ctrl_get(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > - s32 *value) > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping, > + s32 *value) > { > int ret; > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > return -EACCES; > > - if (!ctrl->loaded) { > - if (ctrl->entity->get_cur) { > - ret = ctrl->entity->get_cur(chain->dev, > - ctrl->entity, > - ctrl->info.selector, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > - ctrl->info.size); > - } else { > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > - ctrl->entity->id, > - chain->dev->intfnum, > - ctrl->info.selector, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > - ctrl->info.size); > - } > - if (ret < 0) > - return ret; > - > - ctrl->loaded = 1; > - } > + ret = __uvc_ctrl_load_cur(chain, ctrl); > + if (ret < 0) > + return ret; > > *value = __uvc_ctrl_get_value(mapping, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > @@ -1783,21 +1805,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, > * needs to be loaded from the device to perform the read-modify-write > * operation. > */ > - if (!ctrl->loaded && (ctrl->info.size * 8) != mapping->size) { > - if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) { > - memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > - 0, ctrl->info.size); > - } else { > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > - ctrl->entity->id, chain->dev->intfnum, > - ctrl->info.selector, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > - ctrl->info.size); > - if (ret < 0) > - return ret; > - } > - > - ctrl->loaded = 1; > + if ((ctrl->info.size * 8) != mapping->size) { > + ret = __uvc_ctrl_load_cur(chain, ctrl); > + if (ret < 0) > + return ret; > } > > /* Backup the current value in case we need to rollback later. */
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0e78233fc8a0..181ce4b5db1e 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -963,35 +963,57 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, return value; } +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, + struct uvc_control *ctrl) +{ + int ret; + + if (ctrl->loaded) + return 0; + + if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) { + memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), + 0, ctrl->info.size); + ctrl->loaded = 1; + + return 0; + } + + if (ctrl->entity->get_cur) { + ret = ctrl->entity->get_cur(chain->dev, + ctrl->entity, + ctrl->info.selector, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), + ctrl->info.size); + } else { + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, + ctrl->entity->id, chain->dev->intfnum, + ctrl->info.selector, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), + ctrl->info.size); + } + + if (ret < 0) + return ret; + + ctrl->loaded = 1; + + return ret; +} + static int __uvc_ctrl_get(struct uvc_video_chain *chain, - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, - s32 *value) + struct uvc_control *ctrl, + struct uvc_control_mapping *mapping, + s32 *value) { int ret; if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) return -EACCES; - if (!ctrl->loaded) { - if (ctrl->entity->get_cur) { - ret = ctrl->entity->get_cur(chain->dev, - ctrl->entity, - ctrl->info.selector, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - ctrl->info.size); - } else { - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, - ctrl->entity->id, - chain->dev->intfnum, - ctrl->info.selector, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - ctrl->info.size); - } - if (ret < 0) - return ret; - - ctrl->loaded = 1; - } + ret = __uvc_ctrl_load_cur(chain, ctrl); + if (ret < 0) + return ret; *value = __uvc_ctrl_get_value(mapping, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); @@ -1783,21 +1805,10 @@ int uvc_ctrl_set(struct uvc_fh *handle, * needs to be loaded from the device to perform the read-modify-write * operation. */ - if (!ctrl->loaded && (ctrl->info.size * 8) != mapping->size) { - if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) { - memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - 0, ctrl->info.size); - } else { - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, - ctrl->entity->id, chain->dev->intfnum, - ctrl->info.selector, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - ctrl->info.size); - if (ret < 0) - return ret; - } - - ctrl->loaded = 1; + if ((ctrl->info.size * 8) != mapping->size) { + ret = __uvc_ctrl_load_cur(chain, ctrl); + if (ret < 0) + return ret; } /* Backup the current value in case we need to rollback later. */
Entity controls should get_cur using an entity-defined function instead of via a query. Fix this in uvc_ctrl_set. Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> Signed-off-by: Yunke Cao <yunkec@google.com> --- Changelog since v2: -Move the logic of setting ctrl_data to 0 into load_cur. -Do not initialize ret=0. -Fix __uvc_ctrl_get() spacing. -Fix typo in the title. Changelog since v1: -Factored out common logic into __uvc_ctrl_load_cur(). drivers/media/usb/uvc/uvc_ctrl.c | 85 ++++++++++++++++++-------------- 1 file changed, 48 insertions(+), 37 deletions(-)