Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Tomasz Figa <tfiga@chromium.org>]

Hi Sakari,

On Fri, Sep 28, 2018 at 11:00 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> > Add support for exponential bases, prefixes as well as units for V4L2
> > controls. This makes it possible to convey information on the relation
> > between the control value and the hardware feature being controlled.
> >

Sorry for being late to the party.

Thanks for the series. I think it has a potential to be very useful.

Please see my comments below.

> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index ae083978988f1..23b02f2db85a1 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
> >       __u32                reserved[2];
> >  };
> >
> > +/* V4L2 control exponential bases */
> > +#define V4L2_CTRL_BASE_UNDEFINED     0
> > +#define V4L2_CTRL_BASE_LINEAR                1
>
> I'm not really sure you need BASE_LINEAR. That is effectively the same
> as UNDEFINED since what else can you do? It's also weird to have this
> as 'base' if the EXPONENTIAL flag is set.
>
> I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
> then you know the exponential base.

Or vice versa, we could remove UNDEFINED and LINEAR altogether and
have the EXPONENTIAL flag actually signify the presence of a valid
base? Besides that, "linear exponential base" just doesn't sound right
or am I missing some basic maths? ;)

Then we could actually have a LOGARITHMIC flag and it could reuse the
same bases enum.

>
> > +#define V4L2_CTRL_BASE_2             2
> > +#define V4L2_CTRL_BASE_10            10
> > +
> > +/* V4L2 control unit prefixes */
> > +#define V4L2_CTRL_PREFIX_NANO                -9
> > +#define V4L2_CTRL_PREFIX_MICRO               -6
> > +#define V4L2_CTRL_PREFIX_MILLI               -3
> > +#define V4L2_CTRL_PREFIX_1           0
>
> I would prefer PREFIX_NONE, since there is no prefix in this case.
>
> I assume this prefix is only valid if the unit is not UNDEFINED and not
> NONE?
>
> Is 'base' also dependent on a valid unit? (it doesn't appear to be)
>
> > +#define V4L2_CTRL_PREFIX_KILO                3
> > +#define V4L2_CTRL_PREFIX_MEGA                6
> > +#define V4L2_CTRL_PREFIX_GIGA                9
> > +
> > +/* V4L2 control units */
> > +#define V4L2_CTRL_UNIT_UNDEFINED     0
> > +#define V4L2_CTRL_UNIT_NONE          1

Hmm, what's the meaning of NONE? How does it differ from UNDEFINED?

> > +#define V4L2_CTRL_UNIT_SECOND                2
> > +#define V4L2_CTRL_UNIT_AMPERE                3
> > +#define V4L2_CTRL_UNIT_LINE          4
> > +#define V4L2_CTRL_UNIT_PIXEL         5
> > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC        6
> > +#define V4L2_CTRL_UNIT_HZ            7
> > +
> > +
> >  /*  Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
> >  struct v4l2_query_ext_ctrl {
> >       __u32                id;
> > @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
> >       __u32                elems;
> >       __u32                nr_of_dims;
> >       __u32                dims[V4L2_CTRL_MAX_DIMS];
> > -     __u32                reserved[32];
> > +     __u8                 base;
> > +     __s8                 prefix;

Should we make those bigger just in case, or leave some reserved
fields around so we can make them bigger when we need it?

Best regards,
Tomasz