[v10,03/11] media: uvcvideo: introduce __uvc_ctrl_get_std()
Commit Message
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(-)
Comments
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
@@ -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;