| Message ID | 20221109060621.704531-4-yunkec@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: Implement UVC v1.5 ROI | expand |
Hi Yunke - thanks for the patches On 09/11/2022 06:06, Yunke Cao wrote: > Refactor uvc_ctrl to make adding compound control easier. > > Currently uvc_ctrl_get() only work for non-compound controls. > Move the logic into uvc_ctrl_std(), return error for compound > controls. s/uvc_ctrl_std/__uvc_ctrl_std/. This patch does a bit more than the commit message outlines, so I think it could do with some fleshing out. > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > Changelog since v9: > - No change. > Changelog since v8: > - No change. > Changelog since v7: > - Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7. > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +++++++++++++++++++++----------- > 1 file changed, 27 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index dfb9d1daece6..93ae7ba5d0cc 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1028,15 +1028,15 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > return ret; > } > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, > - struct uvc_control_mapping *mapping, > - s32 *value) > +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > + 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; Why is this check being dropped here? Won't it still be needed when the function's called for non-compound controls? > + if (uvc_ctrl_mapping_is_compound(mapping)) > + return -EINVAL; > > ret = __uvc_ctrl_load_cur(chain, ctrl); > if (ret < 0) > @@ -1153,8 +1153,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > __uvc_find_control(ctrl->entity, mapping->master_id, > &master_map, &master_ctrl, 0); > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > - s32 val; > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > + s32 val = 0; > + int ret; > + > + if (uvc_ctrl_mapping_is_compound(master_map)) > + return -EINVAL; > + > + ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val); > if (ret < 0) > return ret; > > @@ -1399,7 +1404,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > if (ctrl == NULL) > return; > > - if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0) > + if (uvc_ctrl_mapping_is_compound(mapping) || > + __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0) > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > @@ -1566,7 +1572,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > s32 val = 0; > > - if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0) > + if (uvc_ctrl_mapping_is_compound(mapping) || > + __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0) > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val, > @@ -1746,7 +1753,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > if (ctrl == NULL) > return -EINVAL; > > - return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value); > + if (uvc_ctrl_mapping_is_compound(mapping)) > + return -EINVAL; > + else > + return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > } > > static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, > @@ -1893,8 +1903,12 @@ int uvc_ctrl_set(struct uvc_fh *handle, > ctrl->info.size); > } > > - mapping->set(mapping, value, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > + if (!uvc_ctrl_mapping_is_compound(mapping)) > + mapping->set(mapping, value, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > + else > + return -EINVAL; > + > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > ctrl->handle = handle;
Hi Dan, Thanks for the review. On Thu, Dec 15, 2022 at 6:59 PM Dan Scally <dan.scally@ideasonboard.com> wrote: > > Hi Yunke - thanks for the patches > > On 09/11/2022 06:06, Yunke Cao wrote: > > Refactor uvc_ctrl to make adding compound control easier. > > > > Currently uvc_ctrl_get() only work for non-compound controls. > > Move the logic into uvc_ctrl_std(), return error for compound > > controls. > > > s/uvc_ctrl_std/__uvc_ctrl_std/. This patch does a bit more than the > commit message outlines, so I think it could do with some fleshing out. > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > --- > > Changelog since v9: > > - No change. > > Changelog since v8: > > - No change. > > Changelog since v7: > > - Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7. > > > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +++++++++++++++++++++----------- > > 1 file changed, 27 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index dfb9d1daece6..93ae7ba5d0cc 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1028,15 +1028,15 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > return ret; > > } > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > - struct uvc_control *ctrl, > > - struct uvc_control_mapping *mapping, > > - s32 *value) > > +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > > + 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; > > > Why is this check being dropped here? Won't it still be needed when the > function's called for non-compound controls? > I'm dropping it here because __uvc_ctrl_load_cur() checks the same thing. I think this is duplicated. > > + if (uvc_ctrl_mapping_is_compound(mapping)) > > + return -EINVAL; > > > > ret = __uvc_ctrl_load_cur(chain, ctrl); > > if (ret < 0) > > @@ -1153,8 +1153,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > __uvc_find_control(ctrl->entity, mapping->master_id, > > &master_map, &master_ctrl, 0); > > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > > - s32 val; > > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > > + s32 val = 0; > > + int ret; > > + > > + if (uvc_ctrl_mapping_is_compound(master_map)) > > + return -EINVAL; > > + > > + ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val); > > if (ret < 0) > > return ret; > > > > @@ -1399,7 +1404,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > if (ctrl == NULL) > > return; > > > > - if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0) > > + if (uvc_ctrl_mapping_is_compound(mapping) || > > + __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0) > > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > > @@ -1566,7 +1572,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > > s32 val = 0; > > > > - if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0) > > + if (uvc_ctrl_mapping_is_compound(mapping) || > > + __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0) > > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > > > uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val, > > @@ -1746,7 +1753,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > if (ctrl == NULL) > > return -EINVAL; > > > > - return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value); > > + if (uvc_ctrl_mapping_is_compound(mapping)) > > + return -EINVAL; > > + else > > + return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > > } > > > > static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, > > @@ -1893,8 +1903,12 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > ctrl->info.size); > > } > > > > - mapping->set(mapping, value, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > + if (!uvc_ctrl_mapping_is_compound(mapping)) > > + mapping->set(mapping, value, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > + else > > + return -EINVAL; > > + > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > ctrl->handle = handle; Best, Yunke
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index dfb9d1daece6..93ae7ba5d0cc 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1028,15 +1028,15 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, return ret; } -static int __uvc_ctrl_get(struct uvc_video_chain *chain, - struct uvc_control *ctrl, - struct uvc_control_mapping *mapping, - s32 *value) +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, + 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 (uvc_ctrl_mapping_is_compound(mapping)) + return -EINVAL; ret = __uvc_ctrl_load_cur(chain, ctrl); if (ret < 0) @@ -1153,8 +1153,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, __uvc_find_control(ctrl->entity, mapping->master_id, &master_map, &master_ctrl, 0); if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { - s32 val; - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); + s32 val = 0; + int ret; + + if (uvc_ctrl_mapping_is_compound(master_map)) + return -EINVAL; + + ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val); if (ret < 0) return ret; @@ -1399,7 +1404,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, if (ctrl == NULL) return; - if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0) + if (uvc_ctrl_mapping_is_compound(mapping) || + __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0) changes |= V4L2_EVENT_CTRL_CH_VALUE; uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); @@ -1566,7 +1572,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; s32 val = 0; - if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0) + if (uvc_ctrl_mapping_is_compound(mapping) || + __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0) changes |= V4L2_EVENT_CTRL_CH_VALUE; uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val, @@ -1746,7 +1753,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, if (ctrl == NULL) return -EINVAL; - return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value); + if (uvc_ctrl_mapping_is_compound(mapping)) + return -EINVAL; + else + return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); } static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain, @@ -1893,8 +1903,12 @@ int uvc_ctrl_set(struct uvc_fh *handle, ctrl->info.size); } - mapping->set(mapping, value, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); + if (!uvc_ctrl_mapping_is_compound(mapping)) + mapping->set(mapping, value, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); + else + return -EINVAL; + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) ctrl->handle = handle;
Refactor uvc_ctrl to make adding compound control easier. Currently uvc_ctrl_get() only work for non-compound controls. Move the logic into uvc_ctrl_std(), return error for compound controls. Signed-off-by: Yunke Cao <yunkec@google.com> --- Changelog since v9: - No change. Changelog since v8: - No change. Changelog since v7: - Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7. drivers/media/usb/uvc/uvc_ctrl.c | 40 +++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 13 deletions(-)