Hi Dan,
Thanks for the review.
On Thu, Dec 15, 2022 at 6:59 PM Dan Scally <dan.scally@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
