| Message ID | 20190814202815.32491-4-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-ctrls: Add camera 'location' support | expand |
Hi Jacopo, Thank you for the patch. On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote: > Add support for the newly defined V4L2_CID_LOCATION read-only control > used to report the camera device mounting position. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ > include/uapi/linux/v4l2-controls.h | 4 ++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 7d3a33258748..8ab0857df59a 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; > case V4L2_CID_PAN_SPEED: return "Pan, Speed"; > case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; > + case V4L2_CID_LOCATION: return "Location"; Depending on what we decide to name the control (see review of 2/5), you should adjust the description accordingly. > > /* FM Radio Modulator controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > break; > case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: > *type = V4L2_CTRL_TYPE_FWHT_PARAMS; > + case V4L2_CID_LOCATION: > + *type = V4L2_CTRL_TYPE_INTEGER; > + *flags |= V4L2_CTRL_FLAG_READ_ONLY; > + *min = V4L2_LOCATION_FRONT; > + *max = V4L2_LOCATION_BACK; I don't think the control should have a min and a max different than the current value, as it's a fully static control. I'd drop those two lines here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx when creating the control. That why you should be able to collapse this with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. > + *step = 1; > break; > default: > *type = V4L2_CTRL_TYPE_INTEGER; > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index 37807f23231e..5c4c7b245921 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { > #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) > #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) > > +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) > +#define V4L2_LOCATION_FRONT (0 << 0) > +#define V4L2_LOCATION_BACK (1 << 0) Why not just 0 and 1 ? > + > /* FM Modulator class control IDs */ > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
Hi Laurent, On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote: > > Add support for the newly defined V4L2_CID_LOCATION read-only control > > used to report the camera device mounting position. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ > > include/uapi/linux/v4l2-controls.h | 4 ++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > index 7d3a33258748..8ab0857df59a 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; > > case V4L2_CID_PAN_SPEED: return "Pan, Speed"; > > case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; > > + case V4L2_CID_LOCATION: return "Location"; > > Depending on what we decide to name the control (see review of 2/5), you > should adjust the description accordingly. > > > > > /* FM Radio Modulator controls */ > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > > break; > > case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: > > *type = V4L2_CTRL_TYPE_FWHT_PARAMS; > > + case V4L2_CID_LOCATION: > > + *type = V4L2_CTRL_TYPE_INTEGER; > > + *flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + *min = V4L2_LOCATION_FRONT; > > + *max = V4L2_LOCATION_BACK; > > I don't think the control should have a min and a max different than the > current value, as it's a fully static control. I'd drop those two lines > here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx > when creating the control. That why you should be able to collapse this > with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. > Ah, I thought min/max should report the actual control values limits. Anyway, if we move this to be an integer menu control with an helper to parse the DT property and register the control on behalf of drivers, this will change. > > + *step = 1; > > break; > > default: > > *type = V4L2_CTRL_TYPE_INTEGER; > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > > index 37807f23231e..5c4c7b245921 100644 > > --- a/include/uapi/linux/v4l2-controls.h > > +++ b/include/uapi/linux/v4l2-controls.h > > @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { > > #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) > > #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) > > > > +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) > > +#define V4L2_LOCATION_FRONT (0 << 0) > > +#define V4L2_LOCATION_BACK (1 << 0) > > Why not just 0 and 1 ? Or why not BIT(). I saw that the (1 << x) style is the mostly used one in this header file when defining macros like this one so I went for consistency with the existing code. > > > + > > /* FM Modulator class control IDs */ > > > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) > > -- > Regards, > > Laurent Pinchart
On Thu, Aug 15, 2019 at 03:02:45PM +0200, Jacopo Mondi wrote: > Hi Laurent, > > On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote: > > > Add support for the newly defined V4L2_CID_LOCATION read-only control > > > used to report the camera device mounting position. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ > > > include/uapi/linux/v4l2-controls.h | 4 ++++ > > > 2 files changed, 11 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > > index 7d3a33258748..8ab0857df59a 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > > case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; > > > case V4L2_CID_PAN_SPEED: return "Pan, Speed"; > > > case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; > > > + case V4L2_CID_LOCATION: return "Location"; > > > > Depending on what we decide to name the control (see review of 2/5), you > > should adjust the description accordingly. > > > > > > > > /* FM Radio Modulator controls */ > > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > > > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > > > break; > > > case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: > > > *type = V4L2_CTRL_TYPE_FWHT_PARAMS; > > > + case V4L2_CID_LOCATION: > > > + *type = V4L2_CTRL_TYPE_INTEGER; > > > + *flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > + *min = V4L2_LOCATION_FRONT; > > > + *max = V4L2_LOCATION_BACK; > > > > I don't think the control should have a min and a max different than the > > current value, as it's a fully static control. I'd drop those two lines > > here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx > > when creating the control. That why you should be able to collapse this > > with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. > > > > Ah, I thought min/max should report the actual control values limits. > Anyway, if we move this to be an integer menu control with an helper > to parse the DT property and register the control on behalf of > drivers, this will change. > > > > + *step = 1; > > > break; > > > default: > > > *type = V4L2_CTRL_TYPE_INTEGER; > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > > > index 37807f23231e..5c4c7b245921 100644 > > > --- a/include/uapi/linux/v4l2-controls.h > > > +++ b/include/uapi/linux/v4l2-controls.h > > > @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { > > > #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) > > > #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) > > > > > > +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) > > > +#define V4L2_LOCATION_FRONT (0 << 0) > > > +#define V4L2_LOCATION_BACK (1 << 0) > > > > Why not just 0 and 1 ? > > Or why not BIT(). No, BIT(0) == (0 << 0) but BIT(1) != (1 << 0). > I saw that the (1 << x) style is the mostly used one in this header > file when defining macros like this one so I went for consistency with > the existing code. That's when you need a bitmask, which isn't the case here. > > > + > > > /* FM Modulator class control IDs */ > > > > > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
On 8/14/19 10:28 PM, Jacopo Mondi wrote: > Add support for the newly defined V4L2_CID_LOCATION read-only control > used to report the camera device mounting position. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ > include/uapi/linux/v4l2-controls.h | 4 ++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 7d3a33258748..8ab0857df59a 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; > case V4L2_CID_PAN_SPEED: return "Pan, Speed"; > case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; > + case V4L2_CID_LOCATION: return "Location"; > > /* FM Radio Modulator controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > break; > case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: > *type = V4L2_CTRL_TYPE_FWHT_PARAMS; Missing break! Regards, Hans > + case V4L2_CID_LOCATION: > + *type = V4L2_CTRL_TYPE_INTEGER; > + *flags |= V4L2_CTRL_FLAG_READ_ONLY; > + *min = V4L2_LOCATION_FRONT; > + *max = V4L2_LOCATION_BACK; > + *step = 1; > break; > default: > *type = V4L2_CTRL_TYPE_INTEGER; > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index 37807f23231e..5c4c7b245921 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { > #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) > #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) > > +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) > +#define V4L2_LOCATION_FRONT (0 << 0) > +#define V4L2_LOCATION_BACK (1 << 0) > + > /* FM Modulator class control IDs */ > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) >
On 8/15/19 3:02 PM, Jacopo Mondi wrote: > Hi Laurent, > > On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote: >> Hi Jacopo, >> >> Thank you for the patch. >> >> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote: >>> Add support for the newly defined V4L2_CID_LOCATION read-only control >>> used to report the camera device mounting position. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ >>> include/uapi/linux/v4l2-controls.h | 4 ++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >>> index 7d3a33258748..8ab0857df59a 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) >>> case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; >>> case V4L2_CID_PAN_SPEED: return "Pan, Speed"; >>> case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; >>> + case V4L2_CID_LOCATION: return "Location"; >> >> Depending on what we decide to name the control (see review of 2/5), you >> should adjust the description accordingly. >> >>> >>> /* FM Radio Modulator controls */ >>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */ >>> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, >>> break; >>> case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: >>> *type = V4L2_CTRL_TYPE_FWHT_PARAMS; >>> + case V4L2_CID_LOCATION: >>> + *type = V4L2_CTRL_TYPE_INTEGER; >>> + *flags |= V4L2_CTRL_FLAG_READ_ONLY; >>> + *min = V4L2_LOCATION_FRONT; >>> + *max = V4L2_LOCATION_BACK; >> >> I don't think the control should have a min and a max different than the >> current value, as it's a fully static control. I'd drop those two lines >> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx >> when creating the control. That why you should be able to collapse this >> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. >> > > Ah, I thought min/max should report the actual control values limits. > Anyway, if we move this to be an integer menu control with an helper > to parse the DT property and register the control on behalf of > drivers, this will change. > >>> + *step = 1; >>> break; >>> default: >>> *type = V4L2_CTRL_TYPE_INTEGER; >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h >>> index 37807f23231e..5c4c7b245921 100644 >>> --- a/include/uapi/linux/v4l2-controls.h >>> +++ b/include/uapi/linux/v4l2-controls.h >>> @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { >>> #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) >>> #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) >>> >>> +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) >>> +#define V4L2_LOCATION_FRONT (0 << 0) >>> +#define V4L2_LOCATION_BACK (1 << 0) >> >> Why not just 0 and 1 ? > > Or why not BIT(). I saw that the (1 << x) style is the mostly used one in > this header file when defining macros like this one so I went for > consistency with the existing code. Definitely not right. This is an enumeration, so just number from 0, 1, 2, ... Nothing to do with bits/bitmasks. Regards, Hans > >> >>> + >>> /* FM Modulator class control IDs */ >>> >>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) >> >> -- >> Regards, >> >> Laurent Pinchart
Hi Hans, On Thu, Aug 15, 2019 at 03:41:53PM +0200, Hans Verkuil wrote: > On 8/15/19 3:02 PM, Jacopo Mondi wrote: > > Hi Laurent, > > > > On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote: > >> Hi Jacopo, > >> > >> Thank you for the patch. > >> > >> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote: > >>> Add support for the newly defined V4L2_CID_LOCATION read-only control > >>> used to report the camera device mounting position. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ > >>> include/uapi/linux/v4l2-controls.h | 4 ++++ > >>> 2 files changed, 11 insertions(+) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > >>> index 7d3a33258748..8ab0857df59a 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > >>> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) > >>> case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; > >>> case V4L2_CID_PAN_SPEED: return "Pan, Speed"; > >>> case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; > >>> + case V4L2_CID_LOCATION: return "Location"; > >> > >> Depending on what we decide to name the control (see review of 2/5), you > >> should adjust the description accordingly. > >> > >>> > >>> /* FM Radio Modulator controls */ > >>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > >>> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > >>> break; > >>> case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: > >>> *type = V4L2_CTRL_TYPE_FWHT_PARAMS; > >>> + case V4L2_CID_LOCATION: > >>> + *type = V4L2_CTRL_TYPE_INTEGER; > >>> + *flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>> + *min = V4L2_LOCATION_FRONT; > >>> + *max = V4L2_LOCATION_BACK; > >> > >> I don't think the control should have a min and a max different than the > >> current value, as it's a fully static control. I'd drop those two lines > >> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx > >> when creating the control. That why you should be able to collapse this > >> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. > >> > > > > Ah, I thought min/max should report the actual control values limits. > > Anyway, if we move this to be an integer menu control with an helper > > to parse the DT property and register the control on behalf of > > drivers, this will change. > > > >>> + *step = 1; > >>> break; > >>> default: > >>> *type = V4L2_CTRL_TYPE_INTEGER; > >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > >>> index 37807f23231e..5c4c7b245921 100644 > >>> --- a/include/uapi/linux/v4l2-controls.h > >>> +++ b/include/uapi/linux/v4l2-controls.h > >>> @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { > >>> #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) > >>> #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) > >>> > >>> +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) > >>> +#define V4L2_LOCATION_FRONT (0 << 0) > >>> +#define V4L2_LOCATION_BACK (1 << 0) > >> > >> Why not just 0 and 1 ? > > > > Or why not BIT(). I saw that the (1 << x) style is the mostly used one in > > this header file when defining macros like this one so I went for > > consistency with the existing code. > > Definitely not right. This is an enumeration, so just number from 0, 1, 2, ... > > Nothing to do with bits/bitmasks. Aren't these enumerations too? #define V4L2_CID_3A_LOCK (V4L2_CID_CAMERA_CLASS_BASE+27) #define V4L2_LOCK_EXPOSURE (1 << 0) #define V4L2_LOCK_WHITE_BALANCE (1 << 1) #define V4L2_LOCK_FOCUS (1 << 2) #define V4L2_CID_AUTO_FOCUS_START (V4L2_CID_CAMERA_CLASS_BASE+28) #define V4L2_CID_AUTO_FOCUS_STOP (V4L2_CID_CAMERA_CLASS_BASE+29) #define V4L2_CID_AUTO_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+30) #define V4L2_AUTO_FOCUS_STATUS_IDLE (0 << 0) #define V4L2_AUTO_FOCUS_STATUS_BUSY (1 << 0) #define V4L2_AUTO_FOCUS_STATUS_REACHED (1 << 1) #define V4L2_AUTO_FOCUS_STATUS_FAILED (1 << 2) Anyway, I'm happy to change them to plain numbers. > > Regards, > > Hans > > > > >> > >>> + > >>> /* FM Modulator class control IDs */ > >>> > >>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) > >> > >> -- > >> Regards, > >> > >> Laurent Pinchart >
Hi Hans, On Thu, Aug 15, 2019 at 03:23:47PM +0200, Hans Verkuil wrote: > On 8/14/19 10:28 PM, Jacopo Mondi wrote: > > Add support for the newly defined V4L2_CID_LOCATION read-only control > > used to report the camera device mounting position. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ > > include/uapi/linux/v4l2-controls.h | 4 ++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > index 7d3a33258748..8ab0857df59a 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) > > case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; > > case V4L2_CID_PAN_SPEED: return "Pan, Speed"; > > case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; > > + case V4L2_CID_LOCATION: return "Location"; > > > > /* FM Radio Modulator controls */ > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > > break; > > case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: > > *type = V4L2_CTRL_TYPE_FWHT_PARAMS; > > Missing break! > Sorry, this was a trivial mistake. Thanks for spotting! > Regards, > > Hans > > > + case V4L2_CID_LOCATION: > > + *type = V4L2_CTRL_TYPE_INTEGER; > > + *flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + *min = V4L2_LOCATION_FRONT; > > + *max = V4L2_LOCATION_BACK; > > + *step = 1; > > break; > > default: > > *type = V4L2_CTRL_TYPE_INTEGER; > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > > index 37807f23231e..5c4c7b245921 100644 > > --- a/include/uapi/linux/v4l2-controls.h > > +++ b/include/uapi/linux/v4l2-controls.h > > @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { > > #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) > > #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) > > > > +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) > > +#define V4L2_LOCATION_FRONT (0 << 0) > > +#define V4L2_LOCATION_BACK (1 << 0) > > + > > /* FM Modulator class control IDs */ > > > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) > > >
On 8/15/19 3:50 PM, Jacopo Mondi wrote: > Hi Hans, > > On Thu, Aug 15, 2019 at 03:41:53PM +0200, Hans Verkuil wrote: >> On 8/15/19 3:02 PM, Jacopo Mondi wrote: >>> Hi Laurent, >>> >>> On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote: >>>> Hi Jacopo, >>>> >>>> Thank you for the patch. >>>> >>>> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote: >>>>> Add support for the newly defined V4L2_CID_LOCATION read-only control >>>>> used to report the camera device mounting position. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ >>>>> include/uapi/linux/v4l2-controls.h | 4 ++++ >>>>> 2 files changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >>>>> index 7d3a33258748..8ab0857df59a 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>>>> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) >>>>> case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; >>>>> case V4L2_CID_PAN_SPEED: return "Pan, Speed"; >>>>> case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; >>>>> + case V4L2_CID_LOCATION: return "Location"; >>>> >>>> Depending on what we decide to name the control (see review of 2/5), you >>>> should adjust the description accordingly. >>>> >>>>> >>>>> /* FM Radio Modulator controls */ >>>>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */ >>>>> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, >>>>> break; >>>>> case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: >>>>> *type = V4L2_CTRL_TYPE_FWHT_PARAMS; >>>>> + case V4L2_CID_LOCATION: >>>>> + *type = V4L2_CTRL_TYPE_INTEGER; >>>>> + *flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>> + *min = V4L2_LOCATION_FRONT; >>>>> + *max = V4L2_LOCATION_BACK; >>>> >>>> I don't think the control should have a min and a max different than the >>>> current value, as it's a fully static control. I'd drop those two lines >>>> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx >>>> when creating the control. That why you should be able to collapse this >>>> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. >>>> >>> >>> Ah, I thought min/max should report the actual control values limits. >>> Anyway, if we move this to be an integer menu control with an helper >>> to parse the DT property and register the control on behalf of >>> drivers, this will change. >>> >>>>> + *step = 1; >>>>> break; >>>>> default: >>>>> *type = V4L2_CTRL_TYPE_INTEGER; >>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h >>>>> index 37807f23231e..5c4c7b245921 100644 >>>>> --- a/include/uapi/linux/v4l2-controls.h >>>>> +++ b/include/uapi/linux/v4l2-controls.h >>>>> @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { >>>>> #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) >>>>> #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) >>>>> >>>>> +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) >>>>> +#define V4L2_LOCATION_FRONT (0 << 0) >>>>> +#define V4L2_LOCATION_BACK (1 << 0) >>>> >>>> Why not just 0 and 1 ? >>> >>> Or why not BIT(). I saw that the (1 << x) style is the mostly used one in >>> this header file when defining macros like this one so I went for >>> consistency with the existing code. >> >> Definitely not right. This is an enumeration, so just number from 0, 1, 2, ... >> >> Nothing to do with bits/bitmasks. > > Aren't these enumerations too? > > #define V4L2_CID_3A_LOCK (V4L2_CID_CAMERA_CLASS_BASE+27) > #define V4L2_LOCK_EXPOSURE (1 << 0) > #define V4L2_LOCK_WHITE_BALANCE (1 << 1) > #define V4L2_LOCK_FOCUS (1 << 2) > > #define V4L2_CID_AUTO_FOCUS_START (V4L2_CID_CAMERA_CLASS_BASE+28) > #define V4L2_CID_AUTO_FOCUS_STOP (V4L2_CID_CAMERA_CLASS_BASE+29) > #define V4L2_CID_AUTO_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+30) > #define V4L2_AUTO_FOCUS_STATUS_IDLE (0 << 0) > #define V4L2_AUTO_FOCUS_STATUS_BUSY (1 << 0) > #define V4L2_AUTO_FOCUS_STATUS_REACHED (1 << 1) > #define V4L2_AUTO_FOCUS_STATUS_FAILED (1 << 2) No, these are bitmasks for bitmask controls. So one or more status/lock bits can be 1. Regards, Hans > > Anyway, I'm happy to change them to plain numbers. > >> >> Regards, >> >> Hans >> >>> >>>> >>>>> + >>>>> /* FM Modulator class control IDs */ >>>>> >>>>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) >>>> >>>> -- >>>> Regards, >>>> >>>> Laurent Pinchart >>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 7d3a33258748..8ab0857df59a 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range"; case V4L2_CID_PAN_SPEED: return "Pan, Speed"; case V4L2_CID_TILT_SPEED: return "Tilt, Speed"; + case V4L2_CID_LOCATION: return "Location"; /* FM Radio Modulator controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, break; case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS: *type = V4L2_CTRL_TYPE_FWHT_PARAMS; + case V4L2_CID_LOCATION: + *type = V4L2_CTRL_TYPE_INTEGER; + *flags |= V4L2_CTRL_FLAG_READ_ONLY; + *min = V4L2_LOCATION_FRONT; + *max = V4L2_LOCATION_BACK; + *step = 1; break; default: *type = V4L2_CTRL_TYPE_INTEGER; diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index 37807f23231e..5c4c7b245921 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -889,6 +889,10 @@ enum v4l2_auto_focus_range { #define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32) #define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) +#define V4L2_CID_LOCATION (V4L2_CID_CAMERA_CLASS_BASE+34) +#define V4L2_LOCATION_FRONT (0 << 0) +#define V4L2_LOCATION_BACK (1 << 0) + /* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
Add support for the newly defined V4L2_CID_LOCATION read-only control used to report the camera device mounting position. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++ include/uapi/linux/v4l2-controls.h | 4 ++++ 2 files changed, 11 insertions(+)