Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Jacopo Mondi]

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@xxxxxxxxxx>
> > ---
> >  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

Attachment: signature.asc
Description: PGP signature