[PATCH] v4l2-compat-ioctl32.c: add missing controls to, ctrl_is_pointer

[PATCH] v4l2-compat-ioctl32.c: add missing controls to, ctrl_is_pointer

[Hans Verkuil]

We need to find a better method for this. But for now just add
the missing pointer controls to this list.

Also properly mask the id. The high flag bits shouldn't be used
with these ioctls, but it certainly doesn't hurt.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 90827073066f..afae914b8099 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -674,9 +674,14 @@ struct v4l2_ext_control32 {
    type STRING is a pointer type. */
 static inline int ctrl_is_pointer(u32 id)
 {
-	switch (id) {
+	switch (id & V4L2_CTRL_ID_MASK) {
 	case V4L2_CID_RDS_TX_PS_NAME:
 	case V4L2_CID_RDS_TX_RADIO_TEXT:
+	case V4L2_CID_RDS_RX_PS_NAME:
+	case V4L2_CID_RDS_RX_RADIO_TEXT:
+	case V4L2_CID_DETECT_MD_REGION_GRID:
+	case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
+	case V4L2_CID_RDS_TX_ALT_FREQS:
 		return 1;
 	default:
 		return 0;
-- 
2.13.2

[PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Mauro Carvalho Chehab]

In the past, only string controls were pointers. That changed when compounded
types got added, but the compat32 code was not updated.

We could just add those controls there, but maintaining it is flaw, as we
often forget about the compat code. So, instead, rely on the control type,
as this is always updated when new controls are added.

As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
move the ctrl_is_pointer() helper function to v4l2-ctrl.c.

---

Re-sending this patch series, as it was c/c to the linux-doc ML by mistake.

Mauro Carvalho Chehab (3):
  media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
  media: v4l2-ctrls: prepare the function to be used by compat32 code
  media: compat32: reimplement ctrl_is_pointer()

 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +---------
 drivers/media/v4l2-core/v4l2-ctrls.c          | 49 +++++++++++++++++++++++++--
 include/media/v4l2-ctrls.h                    | 28 ++++++++++-----
 3 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.13.3

[PATCH 1/3] media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill

[Mauro Carvalho Chehab]

The arguments for this function are pointers. Make it clear at
its documentation.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 include/media/v4l2-ctrls.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 2d2aed56922f..6ba30acf06aa 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -339,18 +339,18 @@ struct v4l2_ctrl_config {
 /**
  * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
  *
- * @id: ID of the control
- * @name: name of the control
- * @type: type of the control
- * @min: minimum value for the control
- * @max: maximum value for the control
- * @step: control step
- * @def: default value for the control
- * @flags: flags to be used on the control
+ * @id: pointer for storing the ID of the control
+ * @name: pointer for storing the name of the control
+ * @type: pointer for storing the type of the control
+ * @min: pointer for storing the minimum value for the control
+ * @max: pointer for storing the maximum value for the control
+ * @step: pointer for storing the control step
+ * @def: pointer for storing the default value for the control
+ * @flags: pointer for storing the flags to be used on the control
  *
  * This works for all standard V4L2 controls.
  * For non-standard controls it will only fill in the given arguments
- * and @name will be %NULL.
+ * and @name content will be filled with %NULL.
  *
  * This function will overwrite the contents of @name, @type and @flags.
  * The contents of @min, @max, @step and @def may be modified depending on
-- 
2.13.3

[PATCH 2/3] media: v4l2-ctrls: prepare the function to be used by compat32 code

[Mauro Carvalho Chehab]

Right now, both v4l2_ctrl_fill() and compat32 code need to know
the type of the control. As new controls are added, this cause
troubles at compat32, as it won't be able to discover what
functions are pointers or not.

Change v4l2_ctrl_fill() function for it to be called with just
one argument: the control type. This way, the compat32 code can
use it internally.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 31 +++++++++++++++++++++++++++++--
 include/media/v4l2-ctrls.h           |  2 ++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index dd1db678718c..c512b7539077 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -939,8 +939,10 @@ EXPORT_SYMBOL(v4l2_ctrl_get_name);
 void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		    s64 *min, s64 *max, u64 *step, s64 *def, u32 *flags)
 {
-	*name = v4l2_ctrl_get_name(id);
-	*flags = 0;
+	if (name) {
+		*name = v4l2_ctrl_get_name(id);
+		*flags = 0;
+	}
 
 	switch (id) {
 	case V4L2_CID_AUDIO_MUTE:
@@ -996,11 +998,15 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM:
 	case V4L2_CID_RDS_RX_MUSIC_SPEECH:
 		*type = V4L2_CTRL_TYPE_BOOLEAN;
+		if (!name)
+			break;
 		*min = 0;
 		*max = *step = 1;
 		break;
 	case V4L2_CID_ROTATE:
 		*type = V4L2_CTRL_TYPE_INTEGER;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 		break;
 	case V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE:
@@ -1015,6 +1021,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_AUTO_FOCUS_START:
 	case V4L2_CID_AUTO_FOCUS_STOP:
 		*type = V4L2_CTRL_TYPE_BUTTON;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
 			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 		*min = *max = *step = *def = 0;
@@ -1099,12 +1107,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RF_TUNER_CLASS:
 	case V4L2_CID_DETECT_CLASS:
 		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
+		if (!name)
+			break;
 		/* You can neither read not write these */
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
 		*min = *max = *step = *def = 0;
 		break;
 	case V4L2_CID_BG_COLOR:
 		*type = V4L2_CTRL_TYPE_INTEGER;
+		if (!name)
+			break;
 		*step = 1;
 		*min = 0;
 		/* Max is calculated as RGB888 that is 2^24 */
@@ -1123,10 +1135,14 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 		*type = V4L2_CTRL_TYPE_INTEGER;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
 	case V4L2_CID_MPEG_VIDEO_DEC_PTS:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
 		*min = *def = 0;
 		*max = 0x1ffffffffLL;
@@ -1134,6 +1150,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MPEG_VIDEO_DEC_FRAME:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
+		if (!name)
+			break;
+
 		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
 		*min = *def = 0;
 		*max = 0x7fffffffffffffffLL;
@@ -1141,6 +1160,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_PIXEL_RATE:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
 	case V4L2_CID_DETECT_MD_REGION_GRID:
@@ -1156,6 +1177,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
 	}
+
+	if (!name)
+		return;
+
+	/* Update flags for some controls */
+
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_ENCODING:
 	case V4L2_CID_MPEG_AUDIO_MODE:
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 6ba30acf06aa..e22dea218a4c 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -352,6 +352,8 @@ struct v4l2_ctrl_config {
  * For non-standard controls it will only fill in the given arguments
  * and @name content will be filled with %NULL.
  *
+ * if @name is NULL, only the @type will be filled.
+ *
  * This function will overwrite the contents of @name, @type and @flags.
  * The contents of @min, @max, @step and @def may be modified depending on
  * the type.
-- 
2.13.3

[PATCH 3/3] media: compat32: reimplement ctrl_is_pointer()

[Mauro Carvalho Chehab]

The current way that this function works is subject to problems
as new controls gets added. Move it to v4l2-ctrls and use the
knowledge that v4l2_ctrl_fill() has about controls, in order to
detect if a given control is a pointer.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +-----------------
 drivers/media/v4l2-core/v4l2-ctrls.c          | 18 ++++++++++++++++++
 include/media/v4l2-ctrls.h                    |  8 ++++++++
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index af8b4c5b0efa..0adcc37280c8 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -19,6 +19,7 @@
 #include <linux/v4l2-subdev.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -668,23 +669,6 @@ struct v4l2_ext_control32 {
 	};
 } __attribute__ ((packed));
 
-/* The following function really belong in v4l2-common, but that causes
-   a circular dependency between modules. We need to think about this, but
-   for now this will do. */
-
-/* Return non-zero if this control is a pointer type. Currently only
-   type STRING is a pointer type. */
-static inline int ctrl_is_pointer(u32 id)
-{
-	switch (id) {
-	case V4L2_CID_RDS_TX_PS_NAME:
-	case V4L2_CID_RDS_TX_RADIO_TEXT:
-		return 1;
-	default:
-		return 0;
-	}
-}
-
 static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext_controls32 __user *up)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index c512b7539077..0d5dab485723 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1254,6 +1254,24 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 }
 EXPORT_SYMBOL(v4l2_ctrl_fill);
 
+bool ctrl_is_pointer(u32 id)
+{
+	enum v4l2_ctrl_type type;
+
+	v4l2_ctrl_fill(id, NULL, &type, NULL, NULL, NULL, NULL, NULL);
+
+	switch (type) {
+	case V4L2_CTRL_TYPE_STRING:
+	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_U32:
+		return true;
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL(ctrl_is_pointer);
+
 static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 changes)
 {
 	memset(ev->reserved, 0, sizeof(ev->reserved));
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index e22dea218a4c..bc6772f50956 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -367,6 +367,14 @@ struct v4l2_ctrl_config {
 void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		    s64 *min, s64 *max, u64 *step, s64 *def, u32 *flags);
 
+/**
+ * ctrl_is_pointer - Returns non-zero if this control is a pointer type.
+ *
+ * @id: ID of the control
+ *
+ * Currently only STRING and compound types are pointers.
+ */
+bool ctrl_is_pointer(u32 id);
 
 /**
  * v4l2_ctrl_handler_init_class() - Initialize the control handler.
-- 
2.13.3

Re: [PATCH 1/3] media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill

[Hans Verkuil]

On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> The arguments for this function are pointers. Make it clear at
> its documentation.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  include/media/v4l2-ctrls.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 2d2aed56922f..6ba30acf06aa 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -339,18 +339,18 @@ struct v4l2_ctrl_config {
>  /**
>   * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
>   *
> - * @id: ID of the control
> - * @name: name of the control
> - * @type: type of the control
> - * @min: minimum value for the control
> - * @max: maximum value for the control
> - * @step: control step
> - * @def: default value for the control
> - * @flags: flags to be used on the control
> + * @id: pointer for storing the ID of the control

id isn't a pointer, all other arguments are.

> + * @name: pointer for storing the name of the control

This is a pointer to a pointer.

> + * @type: pointer for storing the type of the control
> + * @min: pointer for storing the minimum value for the control
> + * @max: pointer for storing the maximum value for the control
> + * @step: pointer for storing the control step
> + * @def: pointer for storing the default value for the control
> + * @flags: pointer for storing the flags to be used on the control
>   *
>   * This works for all standard V4L2 controls.
>   * For non-standard controls it will only fill in the given arguments
> - * and @name will be %NULL.
> + * and @name content will be filled with %NULL.

I'd say: 'set to %NULL'.

>   *
>   * This function will overwrite the contents of @name, @type and @flags.
>   * The contents of @min, @max, @step and @def may be modified depending on
> 

Regards,

	Hans

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Hans Verkuil]

On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> In the past, only string controls were pointers. That changed when compounded
> types got added, but the compat32 code was not updated.
> 
> We could just add those controls there, but maintaining it is flaw, as we
> often forget about the compat code. So, instead, rely on the control type,
> as this is always updated when new controls are added.
> 
> As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> move the ctrl_is_pointer() helper function to v4l2-ctrl.c.

This series doesn't really solve anything:

- it introduces a circular dependency between two modules
- it doesn't handle driver-custom controls (the old code didn't either). For
  example vivid has custom pointer controls.
- it replaces a list of control IDs with a list of type IDs, which also has to
  be kept up to date.

I thought this over and I have a better and much simpler idea. I'll post a
patch for that.

Regards,

	Hans

> 
> ---
> 
> Re-sending this patch series, as it was c/c to the linux-doc ML by mistake.
> 
> Mauro Carvalho Chehab (3):
>   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
>   media: v4l2-ctrls: prepare the function to be used by compat32 code
>   media: compat32: reimplement ctrl_is_pointer()
> 
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +---------
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 49 +++++++++++++++++++++++++--
>  include/media/v4l2-ctrls.h                    | 28 ++++++++++-----
>  3 files changed, 67 insertions(+), 28 deletions(-)
> 

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Mauro Carvalho Chehab]

Em Fri, 11 Aug 2017 08:05:03 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> > In the past, only string controls were pointers. That changed when compounded
> > types got added, but the compat32 code was not updated.
> > 
> > We could just add those controls there, but maintaining it is flaw, as we
> > often forget about the compat code. So, instead, rely on the control type,
> > as this is always updated when new controls are added.
> > 
> > As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> > move the ctrl_is_pointer() helper function to v4l2-ctrl.c.  
> 
> This series doesn't really solve anything:
> 
> - it introduces a circular dependency between two modules

What two modules? both v4l2-ctrl and compat32 belong to the *same* module.
See the Makefile:

videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
			v4l2-async.o
ifeq ($(CONFIG_COMPAT),y)
  videodev-objs += v4l2-compat-ioctl32.o
endif

Both belong to videodev module. IMHO, the best is to move whatever
control check logic it might need to v4l2-ctrls.

> - it doesn't handle driver-custom controls (the old code didn't either). For
>   example vivid has custom pointer controls.

True.

> - it replaces a list of control IDs with a list of type IDs, which also has to
>   be kept up to date.

True, but at least after the patch, the ancillary function is together
with the code that handles the controls. Also, we don't introduce new
types too often.

> 
> I thought this over and I have a better and much simpler idea. I'll post a
> patch for that.

OK.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > ---
> > 
> > Re-sending this patch series, as it was c/c to the linux-doc ML by mistake.
> > 
> > Mauro Carvalho Chehab (3):
> >   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
> >   media: v4l2-ctrls: prepare the function to be used by compat32 code
> >   media: compat32: reimplement ctrl_is_pointer()
> > 
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +---------
> >  drivers/media/v4l2-core/v4l2-ctrls.c          | 49 +++++++++++++++++++++++++--
> >  include/media/v4l2-ctrls.h                    | 28 ++++++++++-----
> >  3 files changed, 67 insertions(+), 28 deletions(-)
> >   
> 



Thanks,
Mauro

Re: [PATCH 1/3] media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill

[Mauro Carvalho Chehab]

Em Fri, 11 Aug 2017 08:01:58 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> > The arguments for this function are pointers. Make it clear at
> > its documentation.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  include/media/v4l2-ctrls.h | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 2d2aed56922f..6ba30acf06aa 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -339,18 +339,18 @@ struct v4l2_ctrl_config {
> >  /**
> >   * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
> >   *
> > - * @id: ID of the control
> > - * @name: name of the control
> > - * @type: type of the control
> > - * @min: minimum value for the control
> > - * @max: maximum value for the control
> > - * @step: control step
> > - * @def: default value for the control
> > - * @flags: flags to be used on the control
> > + * @id: pointer for storing the ID of the control  
> 
> id isn't a pointer, all other arguments are.
> 
> > + * @name: pointer for storing the name of the control  
> 
> This is a pointer to a pointer.

IMO, it is better to say that it is a pointer to a string.

> 
> > + * @type: pointer for storing the type of the control
> > + * @min: pointer for storing the minimum value for the control
> > + * @max: pointer for storing the maximum value for the control
> > + * @step: pointer for storing the control step
> > + * @def: pointer for storing the default value for the control
> > + * @flags: pointer for storing the flags to be used on the control
> >   *
> >   * This works for all standard V4L2 controls.
> >   * For non-standard controls it will only fill in the given arguments
> > - * and @name will be %NULL.
> > + * and @name content will be filled with %NULL.  
> 
> I'd say: 'set to %NULL'.
> 
> >   *
> >   * This function will overwrite the contents of @name, @type and @flags.
> >   * The contents of @min, @max, @step and @def may be modified depending on
> >   
> 
> Regards,
> 
> 	Hans

Thanks for reviewing. Version 2 follows.

---

[PATCH v2] media: v4l2-ctrls.h: better document the arguments for
 v4l2_ctrl_fill

The arguments for this function are pointers. Make it clear at
its documentation.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 2d2aed56922f..044ea9bc83a8 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -340,17 +340,19 @@ struct v4l2_ctrl_config {
  * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
  *
  * @id: ID of the control
- * @name: name of the control
- * @type: type of the control
- * @min: minimum value for the control
- * @max: maximum value for the control
- * @step: control step
- * @def: default value for the control
- * @flags: flags to be used on the control
+ * @name: pointer to be filled with a string with the name of the control
+ * @type: pointer for storing the type of the control
+ * @min: pointer for storing the minimum value for the control
+ * @max: pointer for storing the maximum value for the control
+ * @step: pointer for storing the control step
+ * @def: pointer for storing the default value for the control
+ * @flags: pointer for storing the flags to be used on the control
  *
  * This works for all standard V4L2 controls.
  * For non-standard controls it will only fill in the given arguments
- * and @name will be %NULL.
+ * and @name content will be set to %NULL.
+ *
+ * if @name is NULL, only the @type will be filled.
  *
  * This function will overwrite the contents of @name, @type and @flags.
  * The contents of @min, @max, @step and @def may be modified depending on


Thanks,
Mauro

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Hans Verkuil]

On 11/08/17 11:00, Mauro Carvalho Chehab wrote:
> Em Fri, 11 Aug 2017 08:05:03 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
>>> In the past, only string controls were pointers. That changed when compounded
>>> types got added, but the compat32 code was not updated.
>>>
>>> We could just add those controls there, but maintaining it is flaw, as we
>>> often forget about the compat code. So, instead, rely on the control type,
>>> as this is always updated when new controls are added.
>>>
>>> As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
>>> move the ctrl_is_pointer() helper function to v4l2-ctrl.c.  
>>
>> This series doesn't really solve anything:
>>
>> - it introduces a circular dependency between two modules
> 
> What two modules? both v4l2-ctrl and compat32 belong to the *same* module.
> See the Makefile:
> 
> videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> 			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
> 			v4l2-async.o
> ifeq ($(CONFIG_COMPAT),y)
>   videodev-objs += v4l2-compat-ioctl32.o
> endif

My fault, I should have checked. The 'circular dependency' comment in the code
referred to the bad old days when v4l2_ctrl_fill was in v4l2-common.c and that
was a separate module. All that has long since changed.

> Both belong to videodev module. IMHO, the best is to move whatever
> control check logic it might need to v4l2-ctrls.
> 
>> - it doesn't handle driver-custom controls (the old code didn't either). For
>>   example vivid has custom pointer controls.
> 
> True.
> 
>> - it replaces a list of control IDs with a list of type IDs, which also has to
>>   be kept up to date.
> 
> True, but at least after the patch, the ancillary function is together
> with the code that handles the controls. Also, we don't introduce new
> types too often.
> 
>>
>> I thought this over and I have a better and much simpler idea. I'll post a
>> patch for that.
> 
> OK.

I have the patch ready, but I need to test it first with vivid.

Regards,

	Hans

Re: [PATCH 1/3] media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill

[Hans Verkuil]

On 11/08/17 11:10, Mauro Carvalho Chehab wrote:
> Em Fri, 11 Aug 2017 08:01:58 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
>>> The arguments for this function are pointers. Make it clear at
>>> its documentation.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>>> ---
>>>  include/media/v4l2-ctrls.h | 18 +++++++++---------
>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>> index 2d2aed56922f..6ba30acf06aa 100644
>>> --- a/include/media/v4l2-ctrls.h
>>> +++ b/include/media/v4l2-ctrls.h
>>> @@ -339,18 +339,18 @@ struct v4l2_ctrl_config {
>>>  /**
>>>   * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
>>>   *
>>> - * @id: ID of the control
>>> - * @name: name of the control
>>> - * @type: type of the control
>>> - * @min: minimum value for the control
>>> - * @max: maximum value for the control
>>> - * @step: control step
>>> - * @def: default value for the control
>>> - * @flags: flags to be used on the control
>>> + * @id: pointer for storing the ID of the control  
>>
>> id isn't a pointer, all other arguments are.
>>
>>> + * @name: pointer for storing the name of the control  
>>
>> This is a pointer to a pointer.
> 
> IMO, it is better to say that it is a pointer to a string.
> 
>>
>>> + * @type: pointer for storing the type of the control
>>> + * @min: pointer for storing the minimum value for the control
>>> + * @max: pointer for storing the maximum value for the control
>>> + * @step: pointer for storing the control step
>>> + * @def: pointer for storing the default value for the control
>>> + * @flags: pointer for storing the flags to be used on the control
>>>   *
>>>   * This works for all standard V4L2 controls.
>>>   * For non-standard controls it will only fill in the given arguments
>>> - * and @name will be %NULL.
>>> + * and @name content will be filled with %NULL.  
>>
>> I'd say: 'set to %NULL'.
>>
>>>   *
>>>   * This function will overwrite the contents of @name, @type and @flags.
>>>   * The contents of @min, @max, @step and @def may be modified depending on
>>>   
>>
>> Regards,
>>
>> 	Hans
> 
> Thanks for reviewing. Version 2 follows.
> 
> ---
> 
> [PATCH v2] media: v4l2-ctrls.h: better document the arguments for
>  v4l2_ctrl_fill
> 
> The arguments for this function are pointers. Make it clear at
> its documentation.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 2d2aed56922f..044ea9bc83a8 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -340,17 +340,19 @@ struct v4l2_ctrl_config {
>   * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
>   *
>   * @id: ID of the control
> - * @name: name of the control
> - * @type: type of the control
> - * @min: minimum value for the control
> - * @max: maximum value for the control
> - * @step: control step
> - * @def: default value for the control
> - * @flags: flags to be used on the control
> + * @name: pointer to be filled with a string with the name of the control
> + * @type: pointer for storing the type of the control
> + * @min: pointer for storing the minimum value for the control
> + * @max: pointer for storing the maximum value for the control
> + * @step: pointer for storing the control step
> + * @def: pointer for storing the default value for the control
> + * @flags: pointer for storing the flags to be used on the control
>   *
>   * This works for all standard V4L2 controls.
>   * For non-standard controls it will only fill in the given arguments
> - * and @name will be %NULL.
> + * and @name content will be set to %NULL.
> + *
> + * if @name is NULL, only the @type will be filled.

Not quite correct, I'd say:

* if @name is NULL, then @type is set to V4L2_CTRL_TYPE_INTEGER and @flags to 0.

Regards,

	Hans

>   *
>   * This function will overwrite the contents of @name, @type and @flags.
>   * The contents of @min, @max, @step and @def may be modified depending on
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCH RESEND 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Laurent Pinchart]

Hi Hans,

On Friday 11 Aug 2017 08:05:03 Hans Verkuil wrote:
> On 11/08/17 02:16, Mauro Carvalho Chehab wrote:
> > In the past, only string controls were pointers. That changed when
> > compounded types got added, but the compat32 code was not updated.
> > 
> > We could just add those controls there, but maintaining it is flaw, as we
> > often forget about the compat code. So, instead, rely on the control type,
> > as this is always updated when new controls are added.
> > 
> > As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> > move the ctrl_is_pointer() helper function to v4l2-ctrl.c.
> 
> This series doesn't really solve anything:
> 
> - it introduces a circular dependency between two modules
> - it doesn't handle driver-custom controls (the old code didn't either). For
> example vivid has custom pointer controls.
> - it replaces a list of control IDs with a list of type IDs, which also has
> to be kept up to date.
> 
> I thought this over and I have a better and much simpler idea. I'll post a
> patch for that.

Wouldn't it be time to replace the large switch/case with an array of control 
information ? Maybe that was your better idea already :-)

> > ---
> > 
> > Re-sending this patch series, as it was c/c to the linux-doc ML by
> > mistake.
> > 
> > Mauro Carvalho Chehab (3):
> >   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
> >   media: v4l2-ctrls: prepare the function to be used by compat32 code
> >   media: compat32: reimplement ctrl_is_pointer()
> >  
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +---------
> >  drivers/media/v4l2-core/v4l2-ctrls.c          | 49 ++++++++++++++++++++--
> >  include/media/v4l2-ctrls.h                    | 28 ++++++++++-----
> >  3 files changed, 67 insertions(+), 28 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3] media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill

[Mauro Carvalho Chehab]

The arguments for this function are pointers. Make it clear at
its documentation.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---

Hans,

Feel free to pick this patch on your tree, if you're ok with it. Your approach for
using v4l_queryctl() sounds better,  as it covers private controls too.
So I'm not submitting the other patches that used to be in this series. Yet,
I think it doesn't hurt to make the documentation clearer about the
pointers.

 include/media/v4l2-ctrls.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 2d2aed56922f..dacfe54057f8 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -340,17 +340,17 @@ struct v4l2_ctrl_config {
  * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
  *
  * @id: ID of the control
- * @name: name of the control
- * @type: type of the control
- * @min: minimum value for the control
- * @max: maximum value for the control
- * @step: control step
- * @def: default value for the control
- * @flags: flags to be used on the control
+ * @name: pointer to be filled with a string with the name of the control
+ * @type: pointer for storing the type of the control
+ * @min: pointer for storing the minimum value for the control
+ * @max: pointer for storing the maximum value for the control
+ * @step: pointer for storing the control step
+ * @def: pointer for storing the default value for the control
+ * @flags: pointer for storing the flags to be used on the control
  *
  * This works for all standard V4L2 controls.
  * For non-standard controls it will only fill in the given arguments
- * and @name will be %NULL.
+ * and @name content will be set to %NULL.
  *
  * This function will overwrite the contents of @name, @type and @flags.
  * The contents of @min, @max, @step and @def may be modified depending on
-- 
2.13.3