Re: [PATCH v7 3/8] media: uvcvideo: Support minimum for V4L2_CTRL_TYPE_MENU

[Laurent Pinchart]

Hi Ricardo,

On Fri, Jun 17, 2022 at 03:55:52PM +0200, Ricardo Ribalda wrote:
> On Fri, 17 Jun 2022 at 15:50, Laurent Pinchart wrote:
> > On Fri, Jun 17, 2022 at 12:36:40PM +0200, Ricardo Ribalda wrote:
> > > Currently all mappings of type V4L2_CTRL_TYPE_MENU, have a minimum of 0,
> > > but there are some controls (limited powerline), that start with a value
> > > different than 0.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 5 +++--
> > >  drivers/media/usb/uvc/uvcvideo.h | 1 +
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 092decfdaa62..3b20b23abd1e 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1144,7 +1144,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > >
> > >       switch (mapping->v4l2_type) {
> > >       case V4L2_CTRL_TYPE_MENU:
> > > -             v4l2_ctrl->minimum = 0;
> > > +             v4l2_ctrl->minimum = mapping->menu_min;
> > >               v4l2_ctrl->maximum = mapping->menu_count - 1;
> > >               v4l2_ctrl->step = 1;
> > >
> > > @@ -1264,7 +1264,8 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > >               goto done;
> > >       }
> > >
> > > -     if (query_menu->index >= mapping->menu_count) {
> > > +     if (query_menu->index < mapping->menu_min ||
> > > +         query_menu->index >= mapping->menu_count) {
> > >               ret = -EINVAL;
> > >               goto done;
> > >       }
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index fff5c5c99a3d..6ceb7f7b964d 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -254,6 +254,7 @@ struct uvc_control_mapping {
> > >       u32 data_type;
> > >
> > >       const struct uvc_menu_info *menu_info;
> > > +     u32 menu_min;
> > >       u32 menu_count;
> >
> > That's a bit of a stop-gap measure, could we turn it into a bitmask
> > instead ?
> 
> Unfortunately that is uAPI :(
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/v4l2-controls.h#n101
> 
> We have to keep the control type and its values.

Sure, I didn't mean changing that, but replacing menu_min and menu_count
in uvc_control_mapping with a menu_mask that stores a bitmask of all
supported values. This will allow skipping the first value in the power
line frequency control case, but will also support skipping other menu
entries for other controls in the future.

> > >
> > >       u32 master_id;

-- 
Regards,

Laurent Pinchart