New control handling framework

include/media/v4l2-ctrls.h new file +0 -0

new file mode 100644
index 0000000..ea23f3d

include/media/v4l2-ctrls.h new file +8 -0

+/* Fill in the control fields based on the control ID. This works for all
+   standard V4L2 controls.
+   For non-standard controls it will only fill in the given arguments
+   and name will be 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
+   the type.
+   Do not use in drivers! It is used internally for backwards

compatibility + control handling only. Once all drivers are converted to
use the new + control framework this function will no longer be
exported. */ +void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type, + s32 *min, s32 *max, s32 *step, s32 *def, u32
*flags);

include/media/v4l2-dev.h +1 -0

index 2dee938..cc9ed09 100644
@@ -27,6 +27,7 @@
 struct v4l2_ioctl_callbacks;
 struct video_device;
 struct v4l2_device;
+struct v4l2_ctrl_handler;
 
 /* Flag to mark the video_device struct as registered.
    Drivers can clear this flag if they want to block all future

To make it easier to determine whether all controls are added in v4l2-ctrls.c the case statements inside the switch are re-ordered to match the header.

Signed-off-by Hans Verkuil <[email protected]>

Hi Hans,

Quoted 5 lines · On Monday 26 April 2010 09:33:33 Hans Verkuil wrote:

To make it easier to determine whether all controls are added in v4l2-ctrls.c the case statements inside the switch are re-ordered to match the header.

Signed-off-by Hans Verkuil <[email protected]>

This patch should be merged with the previous one.

Quoted 9 lines · On Sunday 02 May 2010 22:42:26 Laurent Pinchart wrote:

Hi Hans,

Quoted 5 lines · On Monday 26 April 2010 09:33:33 Hans Verkuil wrote:

To make it easier to determine whether all controls are added in v4l2-ctrls.c the case statements inside the switch are re-ordered to match the header.

Signed-off-by Hans Verkuil <[email protected]>

Quoted 1 line

This patch should be merged with the previous one.

I didn't do that in order to make the previous patch easier to review. That patch just moves this table unchanged from v4l2-common.c to v4l2-ctrl.c. Since the table doesn't change it is easy to verify that, well, no changes were made.

I will keep this as a separate patch if you don't mind.

Signed-off-by Hans Verkuil <[email protected]>

---
 Documentation/video4linux/v4l2-controls.txt |  543 ++++++++++++++++++++++++
 1 files changed, 543 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/video4linux/v4l2-controls.txt

Documentation/video4linux/v4l2-controls.txt new file +5 -0

new file mode 100644
index 0000000..29a92b4
@@ -0,0 +1,543 @@
+Introduction
+============
+
+The V4L2 control API seems simple enough, but quickly becomes very hard to
+implement correctly in drivers. But much of the code needed to handle

controls

+4) How do I validate the user's proposed control value? (i.e. try_ctrl)

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+All the rest is something that can be done centrally.
+
+The control framework was created in order to implement all the rules of

the
+V4L2 specification with respect to controls in a central place. And to make +life as easy as possible for the driver developer.

Documentation/video4linux/v4l2-controls.txt new file +8 -0

+
+
+Objects in the framework
+========================
+
+There are two main objects:
+
+The v4l2_ctrl object describes the control properties and keeps track of

the
+control's value (both the current value and the proposed new value).

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+v4l2_ctrl_handler is the object that keeps track of controls. It maintains

a

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+list of v4l2_ctrl objects that it owns and another list of references to
+controls, possibly to controls owned by other handlers.
+
+
+Basic usage

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+		...
+	};
+
+	struct foo_dev *foo;
+
+- Initialize handler:
+
+	v4l2_ctrl_handler_init(&foo->hdl, nr_of_controls);
+
+  The second argument is a hint telling the function how many controls

this
+ handled is expected to handle. It will allocate a hashtable based on this

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+  information. It is a hint only.
+
+- Hooking the control handler into a driver:
+
+  When a subdevice is being registered with a bridge driver and the
+  ctrl_handler fields of both v4l2_subdev and v4l2_device are set, then

the
+ controls of the subdev will become automatically available in the bridge
+ driver as well. If the subdev driver contains controls that already exist in
+ the bridge driver, then those will be skipped (so a bridge driver can always
+ override a subdev control).

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+  Finally, remove all control functions from your v4l2_ioctl_ops:
+  vidioc_queryctrl, vidioc_querymenu, vidioc_g_ctrl, vidioc_s_ctrl,
+  vidioc_g_ext_ctrls, vidioc_try_ext_ctrls and vidioc_s_ext_ctrls.
+  Those are now no longer needed.
+
+  How to hook the control handler into a subdev driver:
+
+	foo->sd.ctrl_handler = &foo->hdl;
+
+  And set all core control ops in your struct v4l2_subdev_core_ops to

these

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+  This is for backwards compatibility. Once all bridge drivers are

converted
+ these control ops can be removed just as they are already for bridge drivers.

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+- Clean up the handler at the end:
+
+	v4l2_ctrl_handler_free(&foo->hdl);
+

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+	...
+	if (foo->hdl.error) {
+		int err = foo->hdl.error;
+
+		v4l2_ctrl_handler_free(&foo->hdl);
+		return err;
+	}
+
+The v4l2_ctrl_new_std function returns the v4l2_ctrl pointer to the new
+control, but if you do not need to access the pointer outside the control

ops,

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+then there is no need to store it.
+
+Note that if something fails, the function will return NULL or an error

and

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+set hdl->error to the error code. If hdl->error was already set, then it
+will just return and do nothing. This is also true for

v4l2_ctrl_handler_init

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+if it cannot allocate the internal data structure.
+
+This makes it easy to init the handler and just add all controls and only

check
+the error code at the end. Saves a lot of repetitive error checking.

+4) Finally: implement the v4l2_ctrl_ops

Documentation/video4linux/v4l2-controls.txt new file +27 -0

+
+	static const struct v4l2_ctrl_ops foo_ctrl_ops = {
+		.s_ctrl = foo_s_ctrl,
+	};
+
+Usually all you need is s_ctrl:
+
+	static int foo_s_ctrl(struct v4l2_ctrl *ctrl)
+	{
+		struct foo *state = container_of(ctrl->handler, struct foo, hdl);
+
+		switch (ctrl->id) {
+		case V4L2_CID_BRIGHTNESS:
+			write_reg(0x123, ctrl->val);
+			break;
+		case V4L2_CID_CONTRAST:
+			write_reg(0x456, ctrl->val);
+			break;
+		}
+		return 0;
+	}
+
+The control ops are called with the v4l2_ctrl pointer as argument.
+The new control value has already been validated, so all you need to do is
+to actually update the hardware registers.
+
+You're done! And this is sufficient for most of the drivers we have. No

need
+to do any validation of control values, or implement QUERYCTRL/QUERYMENU. And

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+G/S_CTRL as well as G/TRY/S_EXT_CTRLS are automatically supported.
+
+
+==========================================================================

=====

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+The remainder of this document deals with more advanced topics and

scenarios.
+In practice the basic usage as described above is sufficient for most drivers.
+
+==========================================================================
=====

Documentation/video4linux/v4l2-controls.txt new file +22 -0

+
+
+Accessing Control Values
+========================
+
+The v4l2_ctrl struct contains these two unions:
+
+	/* The current control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	} cur;
+
+	/* The new control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	};
+
+Within the control ops you can freely use these. The val and val64 speak

for

+For try/s_ctrl the new values (i.e. as passed by the user) are filled in and

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+you can modify them in try_ctrl or set them in s_ctrl. The 'cur' union
+contains the current value, which you can use (but not change!) as well.
+
+If s_ctrl returns 0 (OK), then the control framework will copy the new

final

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+values to the 'cur' union.
+
+While in g/s/try_ctrl you can access the value of all controls owned by

the

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+same handler since the handler's lock is held. Do not attempt to access
+the value of controls owned by other handlers, though.
+
+Elsewhere in the driver you have to be more careful. You cannot just refer
+to the current control values without locking.
+
+There are two simple helper functions defined that will get or set a

single

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+Don't use these inside the control ops g/s/try_ctrl, though, that will

fail.

Documentation/video4linux/v4l2-controls.txt new file +21 -0

+
+You can also take the handler lock yourself:
+
+	mutex_lock(&state->hdl.lock);
+	printk(KERN_INFO "String value is '%s'\n", ctrl1->cur.string);
+	printk(KERN_INFO "Integer value is '%s'\n", ctrl2->cur.val);
+	mutex_unlock(&state->hdl.lock);
+
+
+Menu Controls
+=============
+
+Menu controls use the 'step' value differently compared to other control
+types. The v4l2_ctrl struct contains this union:
+
+	union {
+		u32 step;
+		u32 menu_skip_mask;
+	};
+
+For menu controls menu_skip_mask is used. What it does is that it allows

you

Documentation/video4linux/v4l2-controls.txt new file +9 -0

+to easily exclude certain menu items. This is used in the VIDIOC_QUERYMENU
+implementation where you can return -EINVAL if a certain menu item is not
+present. Note that VIDIOC_QUERYCTRL always returns a step value of 1 for
+menu controls.
+
+A good example is the MPEG Audio Layer II Bitrate menu control where the
+menu is a list of standardized possible bitrates. But in practice hardware
+implementations will only support a subset of those. By setting the skip
+mask you can tell the framework which menu items should be skipped.

Setting

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+it to 0 means that all menu items are supported.
+
+So when using v4l2_ctrl_new_std or v4l2_ctrl_new_custom you need to

remember
+that 'step' means 'skip mask' for menu controls. If you put in '1' by mistake,
+then the first menu item will be skipped.

+The v4l2_ctrl_new_std_menu can be used to add menu controls more easily:
it
+will calculate the min and max values automatically based on the size of the
+menu, and it has a proper 'mask' argument instead of 'step'.

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+
+
+Active and Grabbed Controls
+===========================
+
+If you get more complex relationships between controls, then you may have

to

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+The other flag is the grabbed flag. A grabbed control means that you

cannot

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+change it because it is in use by some resource. Typical examples are MPEG
+bitrate controls that cannot be changed while capturing is in progress.
+
+If a control is set to 'grabbed' using v4l2_ctrl_grab(), then the

framework

Documentation/video4linux/v4l2-controls.txt new file +15 -0

+will return -EBUSY if an attempt is made to set this control.
+
+Since this flag is used by the framework the v4l2_ctrl_grab function will
+take the control handler's lock. So it cannot be called from within the
+control ops. Instead this is typically called from the driver when it
+starts streaming.
+
+
+Control Clusters
+================
+
+By default all controls are independent from the others. But in more
+complex scenarios you can get dependencies from one control to another.
+In that case you need to 'cluster' them:
+

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+From now on whenever one or more of the controls belonging to the same
+cluster is set (or 'gotten', or 'tried'), only the control ops of the

first

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+control ('volume' in this example) is called. You effectively create a new
+composite control. Similar to how a 'struct' works in C.
+
+So when s_ctrl is called with V4L2_CID_AUDIO_VOLUME as argument, you should

set

Documentation/video4linux/v4l2-controls.txt new file +37 -0

+all two controls belonging to the 'volume' cluster:
+
+	static int foo_s_ctrl(struct v4l2_ctrl *ctrl)
+	{
+		struct foo *state = container_of(ctrl->handler, struct foo, hdl);
+
+		switch (ctrl->id) {
+		case V4L2_CID_AUDIO_VOLUME:
+			/* volume cluster */
+			write_reg(0x123, state->mute->val ? 0 : ctrl->val);
+			break;
+		case V4L2_CID_CONTRAST:
+			write_reg(0x456, ctrl->val);
+			break;
+		}
+		return 0;
+	}
+
+In the example above the following are equivalent for the VOLUME case:
+
+	ctrl == state->volume == ctrl->cluster[0]
+	state->mute == ctrl->cluster[1]
+
+Note that controls in a cluster may be NULL. For example, if for some
+reason mute was never added (because the hardware doesn't support that
+particular feature), then mute will be NULL. So in that case we have a
+cluster of 2 controls, of which only 1 is actually instantiated. The
+only restriction is that the first control of the cluster must already be
+present, since that is the 'master' control of the cluster. The master
+control is the one that identifies the cluster and that provides the
+pointer to the v4l2_ctrl_ops struct that is used for that cluster.
+
+
+VIDIOC_LOG_STATUS Support
+=========================
+
+This ioctl allow you to dump the current status of a driver to the kernel

log.

+The v4l2_ctrl_handler_log_status(hdl, prefix) can be used to dump the value of

Documentation/video4linux/v4l2-controls.txt new file +16 -0

+the controls owned by the given handler to the log. You can supply a prefix
+as well. If the prefix didn't end with a space, then ': ' will be added for
+you.
+
+
+Different Handlers for Different Video Nodes
+============================================
+
+Usually the bridge driver has just one control handler that is global for
+all video nodes. But you can also specify different control handlers for
+different video nodes. It's no problem if there are no subdevs involved.
+But if there are, then you need to block the automatic merging of subdev
+controls to the global control handler. You do that by simply setting the
+ctrl_handler field in struct v4l2_device to NULL.
+
+After each subdev was added, you will then have to call

v4l2_ctrl_add_handler
+manually to add the subdev's control handler (sd->ctrl_handler) to the desired
+bridge control handler.

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+If you want to have one handler (e.g. for a radio device node) have a

subset

Documentation/video4linux/v4l2-controls.txt new file +32 -0

+of another handler (e.g. for a video device node), then you can first add
+the controls to the first handler, add the other controls to the second
+handler and finally add the first handler to the second. For example:
+
+	v4l2_ctrl_new_std(&radio_hdl, &radio_ops, V4L2_CID_AUDIO_VOLUME, ...);
+	v4l2_ctrl_new_std(&radio_hdl, &radio_ops, V4L2_CID_AUDIO_MUTE, ...);
+	v4l2_ctrl_new_std(&video_hdl, &video_ops, V4L2_CID_BRIGHTNESS, ...);
+	v4l2_ctrl_new_std(&video_hdl, &video_ops, V4L2_CID_CONTRAST, ...);
+	v4l2_ctrl_add_handler(&video_hdl, &radio_hdl);
+
+Or you can add specific controls to a handler:
+
+	volume = v4l2_ctrl_new_std(&video_hdl, &ops, V4L2_CID_AUDIO_VOLUME, ...);
+	v4l2_ctrl_new_std(&video_hdl, &ops, V4L2_CID_BRIGHTNESS, ...);
+	v4l2_ctrl_new_std(&video_hdl, &ops, V4L2_CID_CONTRAST, ...);
+	v4l2_ctrl_add_ctrl(&radio_hdl, volume);
+
+What you should not do is make two identical controls for two handlers.
+For example:
+
+	v4l2_ctrl_new_std(&radio_hdl, &radio_ops, V4L2_CID_AUDIO_MUTE, ...);
+	v4l2_ctrl_new_std(&video_hdl, &video_ops, V4L2_CID_AUDIO_MUTE, ...);
+
+This would be bad since muting the radio would not change the video mute
+control. The rule is to have one control for each hardware 'knob' that you
+can twiddle.
+
+
+Finding Controls
+================
+
+Normally you have created the controls yourself and you can store the

struct

Documentation/video4linux/v4l2-controls.txt new file +12 -0

+v4l2_ctrl pointer into your own struct.
+
+But sometimes you need to find a control from another handler that you do
+not own. For example, if you have to find a volume control from a subdev.
+
+You can do that by calling v4l2_ctrl_find:
+
+	struct v4l2_ctrl *volume;
+
+	volume = v4l2_ctrl_find(sd->ctrl_handler, V4L2_CID_AUDIO_VOLUME);
+
+Since v4l2_ctrl_find will lock the handler you have to be careful where

you

Documentation/video4linux/v4l2-controls.txt new file +23 -0

+use it. For example, this is not a good idea:
+
+	struct v4l2_ctrl_handler hdl;
+
+	v4l2_ctrl_new_std(&hdl, &video_ops, V4L2_CID_BRIGHTNESS, ...);
+	v4l2_ctrl_new_std(&hdl, &video_ops, V4L2_CID_CONTRAST, ...);
+
+...and in video_ops.s_ctrl:
+
+	case V4L2_CID_BRIGHTNESS:
+		contrast = v4l2_find_ctrl(&hdl, V4L2_CID_CONTRAST);
+		...
+
+When s_ctrl is called by the framework the hdl.lock is already taken, so
+attempting to find another control from the same handler will deadlock.
+
+It is recommended not to use this function from inside the control ops.
+
+
+Inheriting Controls
+===================
+
+When one control handler is added to another using v4l2_ctrl_add_handler,

then
+by default all controls from one are merged to the other. But a subdev might
+have low-level controls that make sense for some advanced embedded system, but
+not when it is used in consumer-level hardware. In that case you want to keep

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+These controls will now be skipped when v4l2_ctrl_add_handler is called.
+
+
+Strict Control Validation
+=========================
+
+By default when the application wants to change an integer control the

value
+passed to the framework will automatically be modified to map to the provided
+minimum, maximum and step values of the control. If instead you just want to
+validate the value and not modify it, then set the 'strict_validation' flag of

+This is currently specific to integer controls. The value for boolean controls

Documentation/video4linux/v4l2-controls.txt new file +8 -0

+is always mapped to 0 or 1, menu and string controls are already validated
+strictly, and integer64 controls are not validated at all.
+
+
+V4L2_CTRL_TYPE_CTRL_CLASS Controls
+==================================
+
+Controls of this type can be used by GUIs to get the name of the control

class.

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+A fully featured GUI can make a dialog with multiple tabs with each tab
+containing the controls belonging to a particular control class. The name

of
+each tab can be found by querying a special control with ID <control class | 1>.

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+Drivers do not have to care about this. The framework will automatically

add
+a control of this type whenever the first control belonging to a new control

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+class is added.
+
+
+Differences from the Spec
+=========================
+
+There are a few places where the framework acts slightly differently from

the
+V4L2 Specification. Those differences are described in this section. We will

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+have to see whether we need to adjust the spec or not.
+
+1) It is no longer required to have all controls contained in a
+v4l2_ext_control array be from the same control class. The framework will

be

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+able to handle any type of control in the array. You need to set ctrl_class
+to 0 in order to enable this. If ctrl_class is non-zero, then it will still
+check that all controls belong to that control class.
+
+If you set ctrl_class to 0 and count to 0, then it will only return an

error
+if there are no controls at all.

+4) Attempting to write to a read-only control will return -EACCES instead of
+-EINVAL as the spec says.

+5) The spec does not mention what should happen when you try to set/get a

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+control class controls. ivtv currently returns -EINVAL (indicating that the
+control ID does not exist) while the framework will return -EACCES, which
+makes more sense.
+
+
+Proposals for Extensions

Hi Hans,

I finally got some time to review your RFC. Let's see until what time it will keep me awake :-)

Quoted 18 lines · On Monday 26 April 2010 09:33:38 Hans Verkuil wrote:

Signed-off-by Hans Verkuil <[email protected]>

---
 Documentation/video4linux/v4l2-controls.txt |  543 ++++++++++++++++++++++++
 1 files changed, 543 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/video4linux/v4l2-controls.txt

Documentation/video4linux/v4l2-controls.txt new file +5 -0

new file mode 100644
index 0000000..29a92b4
@@ -0,0 +1,543 @@
+Introduction
+============
+
+The V4L2 control API seems simple enough, but quickly becomes very hard to
+implement correctly in drivers. But much of the code needed to handle

Documentation/video4linux/v4l2-controls.txt line 10

Quoted 6 lines

controls

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+is actually not driver specific and can be moved to the V4L core framework.
+
+After all, the only part that a driver developer is interested in is:
+
+1) How do I add a control?

Do you think we will need the ability to remove controls ?

Documentation/video4linux/v4l2-controls.txt line 15

Quoted 5 lines

+2) How do I set the control? (i.e. s_ctrl)

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+And occasionally:
+
+3) How do I update the control's value? (i.e. g_ctrl)

The wording of 2 and 3 got me a bit confused. "set"ting a control usually refers to the operation performed by userspace. Similarly, without the "i.e. g_ctrl" comment, I would have thought "update" meant writing the value to the hardware.

Quoted 15 lines

+4) How do I validate the user's proposed control value? (i.e. try_ctrl)

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+All the rest is something that can be done centrally.
+
+The control framework was created in order to implement all the rules of

the
+V4L2 specification with respect to controls in a central place. And to make +life as easy as possible for the driver developer.

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+
+
+Objects in the framework
+========================
+
+There are two main objects:

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+The v4l2_ctrl object describes the control properties and keeps track of

the
+control's value (both the current value and the proposed new value).

I'm not sure v4l2_ctrl is a good name. We already have a v4l2_control structure, and using the abbreviated name for the in-kernel version is going to be confusing.

It's obviously too late to change v4l2_control to something else. v4l2_control_info and v4l2_control_value are two names that come to my mind. Actually, it might be a good idea to split the v4l2_ctrl structure into a static driver-wide structure (v4l2_control_info) and an instance-specific structure (v4l2_control_value). There's no point in storing the same static data (function pointers, names, ...) for identical controls in different devices.

Quoted 15 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+v4l2_ctrl_handler is the object that keeps track of controls. It maintains

a

Documentation/video4linux/v4l2-controls.txt new file +11 -0

+list of v4l2_ctrl objects that it owns and another list of references to
+controls, possibly to controls owned by other handlers.
+
+
+Basic usage
+===========
+
+1) Prepare the driver:
+
+- Add the handler to your main bridge driver or sub-device driver top-level
+  struct:

Documentation/video4linux/v4l2-controls.txt line 48

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	struct foo_dev {
+		...
+		struct v4l2_ctrl_handler hdl;

Please use more descriptive names in the examples, such as "ctrl_handler". I expect some (many ?) developers to use the same names as the documentation does, making the driver code a bit difficult to read.

Quoted 11 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+		...
+	};
+
+	struct foo_dev *foo;
+
+- Initialize handler:
+
+	v4l2_ctrl_handler_init(&foo->hdl, nr_of_controls);
+
+  The second argument is a hint telling the function how many controls

Quoted 2 lines

this
+ handled is expected to handle. It will allocate a hashtable based on this

s/handled/handler/

Quoted 14 lines

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+  information. It is a hint only.
+
+- Hooking the control handler into a driver:
+
+  When a subdevice is being registered with a bridge driver and the
+  ctrl_handler fields of both v4l2_subdev and v4l2_device are set, then

the
+ controls of the subdev will become automatically available in the bridge
+ driver as well. If the subdev driver contains controls that already exist in
+ the bridge driver, then those will be skipped (so a bridge driver can always

Quoted 1 line

+ override a subdev control).

I think you should split the documentation differently. You're mixing video devices, bridges and subdevs. Developers of drivers that use no subdev will have trouble understanding the documentation. You should first describe how to use the framework in a simple driver (no host, bridge, subdev, ... just a foo_dev and a single video_device), and then add sections to describe bridge- specific and subdev-specific APIs.

Documentation/video4linux/v4l2-controls.txt line 73

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+  How to hook the handler into a bridge driver:
+
+	foo->v4l2_dev.ctrl_handler = &foo->hdl;

Is v4l2_dev an instance of video_device or v4l2_device ? It would be nice to add the field to the foo_dev structure in the example.

Documentation/video4linux/v4l2-controls.txt line 78

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+  And whenever you call video_register_device() you must set the
+  ctrl_handler field of struct video_device as well:
+
+	vdev->ctrl_handler = &foo->hdl;

I suppose that the above foo->v4l2_dev is an instance of v4l2_device then. Does the framework require v4l2_device ? If so, that should be explicit in the introduction.

Why can't the framework find the ctrl_handler instance from the video_device's v4l2_device parent ?

Quoted 14 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+  Finally, remove all control functions from your v4l2_ioctl_ops:
+  vidioc_queryctrl, vidioc_querymenu, vidioc_g_ctrl, vidioc_s_ctrl,
+  vidioc_g_ext_ctrls, vidioc_try_ext_ctrls and vidioc_s_ext_ctrls.
+  Those are now no longer needed.
+
+  How to hook the control handler into a subdev driver:
+
+	foo->sd.ctrl_handler = &foo->hdl;
+
+  And set all core control ops in your struct v4l2_subdev_core_ops to

these

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+  helpers:
+
+	.queryctrl = v4l2_sd_queryctrl,

Documentation/video4linux/v4l2-controls.txt line 98

Quoted 6 lines

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+	.querymenu = v4l2_sd_querymenu,
+	.g_ctrl = v4l2_sd_g_ctrl,
+	.s_ctrl = v4l2_sd_s_ctrl,
+	.g_ext_ctrls = v4l2_sd_g_ext_ctrls,
+	.try_ext_ctrls = v4l2_sd_try_ext_ctrls,
+	.s_ext_ctrls = v4l2_sd_s_ext_ctrls,

s/sd/subdev/ ?

Quoted 17 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+  This is for backwards compatibility. Once all bridge drivers are

converted
+ these control ops can be removed just as they are already for bridge drivers.

Documentation/video4linux/v4l2-controls.txt new file +11 -0

+
+- Clean up the handler at the end:
+
+	v4l2_ctrl_handler_free(&foo->hdl);
+
+
+2) Add controls:
+
+Typically done right after the handler_init:
+
+	v4l2_ctrl_handler_init(&foo->hdl, nr_of_controls);

Documentation/video4linux/v4l2-controls.txt line 116

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+	v4l2_ctrl_new_std(&foo->hdl, &foo_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
+	v4l2_ctrl_new_std(&foo->hdl, &foo_ctrl_ops,
+			V4L2_CID_CONTRAST, 0, 255, 1, 128);

Could you add the v4l2_ctrl_new_std function prototype to the documentation ? I expect the parameters to be min, max, step and default, but it would be better to make it explicit.

Should't v4l2_ctrl_new_std take a control type as well ?

What about hardware for which the boundaries are only known at runtime, or could depend on the values of other controls ? I'm thinking about UVC devices for instance, the boundaries, step and default values need to be retrieved from the hardware. I currently do that at runtime when the control is queried for the first time and cache the values, as doing it during initialization (probe function) crashes a few webcams. That doesn't seem to be possible with the control framework.

Quoted 19 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+	...
+	if (foo->hdl.error) {
+		int err = foo->hdl.error;
+
+		v4l2_ctrl_handler_free(&foo->hdl);
+		return err;
+	}
+
+The v4l2_ctrl_new_std function returns the v4l2_ctrl pointer to the new
+control, but if you do not need to access the pointer outside the control

ops,

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+then there is no need to store it.
+
+Note that if something fails, the function will return NULL or an error

and

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+set hdl->error to the error code. If hdl->error was already set, then it
+will just return and do nothing. This is also true for

v4l2_ctrl_handler_init

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+if it cannot allocate the internal data structure.
+
+This makes it easy to init the handler and just add all controls and only

check
+the error code at the end. Saves a lot of repetitive error checking.

I would still check the v4l2_ctrl_handler_init return value explicitly, but that may be just me.

Documentation/video4linux/v4l2-controls.txt line 138

Quoted 2 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+It is recommended to add controls in ascending control ID order: it will be
+a bit faster that way.

As long as it's not required that's OK. It would be difficult to do so on UVC hardware, as controls are discovered based on information reported by the device, and those are obviously not ordered based on the V4L2 CIDs :-)

Documentation/video4linux/v4l2-controls.txt line 146

Quoted 8 lines

+3) Optionally force initial control setup:

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+
+	v4l2_ctrl_handler_setup(&foo->hdl);
+
+This will call s_ctrl for all controls unconditionally. Effectively this
+initializes the hardware to the default control values. It is recommended
+that you do this.

What about the other way around, if I want to initialize the framework with the current values retrieved from the hardware ?

Quoted 70 lines

+4) Finally: implement the v4l2_ctrl_ops

Documentation/video4linux/v4l2-controls.txt new file +27 -0

+
+	static const struct v4l2_ctrl_ops foo_ctrl_ops = {
+		.s_ctrl = foo_s_ctrl,
+	};
+
+Usually all you need is s_ctrl:
+
+	static int foo_s_ctrl(struct v4l2_ctrl *ctrl)
+	{
+		struct foo *state = container_of(ctrl->handler, struct foo, hdl);
+
+		switch (ctrl->id) {
+		case V4L2_CID_BRIGHTNESS:
+			write_reg(0x123, ctrl->val);
+			break;
+		case V4L2_CID_CONTRAST:
+			write_reg(0x456, ctrl->val);
+			break;
+		}
+		return 0;
+	}
+
+The control ops are called with the v4l2_ctrl pointer as argument.
+The new control value has already been validated, so all you need to do is
+to actually update the hardware registers.
+
+You're done! And this is sufficient for most of the drivers we have. No

need
+to do any validation of control values, or implement QUERYCTRL/QUERYMENU. And

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+G/S_CTRL as well as G/TRY/S_EXT_CTRLS are automatically supported.
+
+
+==========================================================================

=====

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+The remainder of this document deals with more advanced topics and

scenarios.
+In practice the basic usage as described above is sufficient for most drivers.
+
+==========================================================================
=====

Documentation/video4linux/v4l2-controls.txt new file +22 -0

+
+
+Accessing Control Values
+========================
+
+The v4l2_ctrl struct contains these two unions:
+
+	/* The current control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	} cur;
+
+	/* The new control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	};
+
+Within the control ops you can freely use these. The val and val64 speak

for

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+themselves. The string pointers point to character buffers of length
+ctrl->maximum + 1, and are always 0-terminated.
+

Documentation/video4linux/v4l2-controls.txt line 215

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+For g_ctrl you have to update the current control values like this:
+
+	ctrl->cur.val = read_reg(0x123);
+
+The 'new value' union is not relevant in g_ctrl.

When is g_ctrl called ? When the userspace applications issues a VIDIOC_G_CTRL or VIDIOC_G_EXT_CTRLS ioctl ?

I suppose the operation only needs to be
implemented if the device needs to be queried because it modified the control value on its own (possibly as a result of setting another control). What if some controls need to be handled that way, while the others are not self- modifying ?

Quoted 19 lines

+For try/s_ctrl the new values (i.e. as passed by the user) are filled in and

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+you can modify them in try_ctrl or set them in s_ctrl. The 'cur' union
+contains the current value, which you can use (but not change!) as well.
+
+If s_ctrl returns 0 (OK), then the control framework will copy the new

final

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+values to the 'cur' union.
+
+While in g/s/try_ctrl you can access the value of all controls owned by

the

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+same handler since the handler's lock is held. Do not attempt to access
+the value of controls owned by other handlers, though.
+
+Elsewhere in the driver you have to be more careful. You cannot just refer
+to the current control values without locking.
+
+There are two simple helper functions defined that will get or set a

single

Documentation/video4linux/v4l2-controls.txt line 235

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+control value safely:
+
+	s32 v4l2_ctrl_g(struct v4l2_ctrl *ctrl);
+	int v4l2_ctrl_s(struct v4l2_ctrl *ctrl, s32 val);

I suppose that v4l2_ctrl_s can be used if the device notifies the driver that a control has changed. Is that correct ?

I don't really like the names of those two functions. For instance, v4l2_ctrl_s looks like it will call the driver to set a control.

Quoted 39 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+Don't use these inside the control ops g/s/try_ctrl, though, that will

fail.

Documentation/video4linux/v4l2-controls.txt new file +21 -0

+
+You can also take the handler lock yourself:
+
+	mutex_lock(&state->hdl.lock);
+	printk(KERN_INFO "String value is '%s'\n", ctrl1->cur.string);
+	printk(KERN_INFO "Integer value is '%s'\n", ctrl2->cur.val);
+	mutex_unlock(&state->hdl.lock);
+
+
+Menu Controls
+=============
+
+Menu controls use the 'step' value differently compared to other control
+types. The v4l2_ctrl struct contains this union:
+
+	union {
+		u32 step;
+		u32 menu_skip_mask;
+	};
+
+For menu controls menu_skip_mask is used. What it does is that it allows

you

Documentation/video4linux/v4l2-controls.txt new file +9 -0

+to easily exclude certain menu items. This is used in the VIDIOC_QUERYMENU
+implementation where you can return -EINVAL if a certain menu item is not
+present. Note that VIDIOC_QUERYCTRL always returns a step value of 1 for
+menu controls.
+
+A good example is the MPEG Audio Layer II Bitrate menu control where the
+menu is a list of standardized possible bitrates. But in practice hardware
+implementations will only support a subset of those. By setting the skip
+mask you can tell the framework which menu items should be skipped.

Setting

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+it to 0 means that all menu items are supported.
+
+So when using v4l2_ctrl_new_std or v4l2_ctrl_new_custom you need to

Quoted 4 lines

remember
+that 'step' means 'skip mask' for menu controls. If you put in '1' by mistake,
+then the first menu item will be skipped.

What about adding a v4l2_ctrl_new_menu then ? That will avoid such a mistake.

Quoted 5 lines

+The v4l2_ctrl_new_std_menu can be used to add menu controls more easily:
it
+will calculate the min and max values automatically based on the size of the
+menu, and it has a proper 'mask' argument instead of 'step'.

OK :-) Shouldn't v4l2_ctrl_new_custom be restricted to non-menu controls then ? Looking at the code, it should probably become an internal function, with another function added for custom controls without menu support.

Quoted 12 lines

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+
+
+Active and Grabbed Controls
+===========================
+
+If you get more complex relationships between controls, then you may have

to

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+activate and deactivate controls. For example, if the Chroma AGC control is
+on, then the Chroma Gain control is inactive. That is, you may set it, but
+the value will not be used by the hardware as long as the automatic gain
+control is on. Typically user interfaces can disable such input fields.

Documentation/video4linux/v4l2-controls.txt line 291

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+You can set the 'active' status using v4l2_ctrl_activate(). By default all
+controls are active. Note that the framework does not check for this flag.
+It is meant purely for GUIs. The function is typically called from within
+s_ctrl.

What you refer to as active/inactive is usually referred to as enabled/disabled in the GUI world. Might be worth using the same convention.

Quoted 28 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+The other flag is the grabbed flag. A grabbed control means that you

cannot

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+change it because it is in use by some resource. Typical examples are MPEG
+bitrate controls that cannot be changed while capturing is in progress.
+
+If a control is set to 'grabbed' using v4l2_ctrl_grab(), then the

framework

Documentation/video4linux/v4l2-controls.txt new file +20 -0

+will return -EBUSY if an attempt is made to set this control.
+
+Since this flag is used by the framework the v4l2_ctrl_grab function will
+take the control handler's lock. So it cannot be called from within the
+control ops. Instead this is typically called from the driver when it
+starts streaming.
+
+
+Control Clusters
+================
+
+By default all controls are independent from the others. But in more
+complex scenarios you can get dependencies from one control to another.
+In that case you need to 'cluster' them:
+
+	struct foo {
+		struct v4l2_ctrl_handler hdl;
+		struct v4l2_ctrl *volume;
+		struct v4l2_ctrl *mute;
+		...

Documentation/video4linux/v4l2-controls.txt line 322

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+	};
+
+	state->volume = v4l2_ctrl_new_std(&state->hdl, ...);
+	state->mute = v4l2_ctrl_new_std(&state->hdl, ...);
+	v4l2_ctrl_cluster(2, &state->volume);

What's the first argument to v4l2_ctrl_cluster ? The number of controls in the cluster ?

Does that imply that they need to be added in a sequence, right before v4l2_ctrl_cluster is called ? That seems a bit awkward. Wouldn't it be better to specify the relationships between controls explicitly, maybe by passing a pointer to the master control when creating the 'slave' controls in the cluster ?

Quoted 43 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+From now on whenever one or more of the controls belonging to the same
+cluster is set (or 'gotten', or 'tried'), only the control ops of the

first

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+control ('volume' in this example) is called. You effectively create a new
+composite control. Similar to how a 'struct' works in C.
+
+So when s_ctrl is called with V4L2_CID_AUDIO_VOLUME as argument, you should

set

Documentation/video4linux/v4l2-controls.txt new file +34 -0

+all two controls belonging to the 'volume' cluster:
+
+	static int foo_s_ctrl(struct v4l2_ctrl *ctrl)
+	{
+		struct foo *state = container_of(ctrl->handler, struct foo, hdl);
+
+		switch (ctrl->id) {
+		case V4L2_CID_AUDIO_VOLUME:
+			/* volume cluster */
+			write_reg(0x123, state->mute->val ? 0 : ctrl->val);
+			break;
+		case V4L2_CID_CONTRAST:
+			write_reg(0x456, ctrl->val);
+			break;
+		}
+		return 0;
+	}
+
+In the example above the following are equivalent for the VOLUME case:
+
+	ctrl == state->volume == ctrl->cluster[0]
+	state->mute == ctrl->cluster[1]
+
+Note that controls in a cluster may be NULL. For example, if for some
+reason mute was never added (because the hardware doesn't support that
+particular feature), then mute will be NULL. So in that case we have a
+cluster of 2 controls, of which only 1 is actually instantiated. The
+only restriction is that the first control of the cluster must already be
+present, since that is the 'master' control of the cluster. The master
+control is the one that identifies the cluster and that provides the
+pointer to the v4l2_ctrl_ops struct that is used for that cluster.
+
+
+VIDIOC_LOG_STATUS Support

Quoted 4 lines

+=========================

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+This ioctl allow you to dump the current status of a driver to the kernel

log.

This has nothing to do with the controls framework, but shouldn't that ioctl be restricted to root only ? It can potentially dump a lot of information to the kernel log, and thus system logs.

Quoted 16 lines

+The v4l2_ctrl_handler_log_status(hdl, prefix) can be used to dump the value of

Documentation/video4linux/v4l2-controls.txt new file +14 -0

+the controls owned by the given handler to the log. You can supply a prefix
+as well. If the prefix didn't end with a space, then ': ' will be added for
+you.
+
+
+Different Handlers for Different Video Nodes
+============================================
+
+Usually the bridge driver has just one control handler that is global for
+all video nodes. But you can also specify different control handlers for
+different video nodes. It's no problem if there are no subdevs involved.
+But if there are, then you need to block the automatic merging of subdev
+controls to the global control handler. You do that by simply setting the
+ctrl_handler field in struct v4l2_device to NULL.

Quoted 6 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+After each subdev was added, you will then have to call

v4l2_ctrl_add_handler
+manually to add the subdev's control handler (sd->ctrl_handler) to the desired
+bridge control handler.

Do you mean video device instead of bridge here ?

I wouldn't mention "bridge" in here. I assume that by bridge you mean v4l2_device. Please use that name directly. The term "bridge" is only applicable to a subset of the v4l2_device use cases.

Can controls for a specific subdev be reported through more than one video device node, but not all of them ?

Quoted 81 lines

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+If you want to have one handler (e.g. for a radio device node) have a

subset

Documentation/video4linux/v4l2-controls.txt new file +32 -0

+of another handler (e.g. for a video device node), then you can first add
+the controls to the first handler, add the other controls to the second
+handler and finally add the first handler to the second. For example:
+
+	v4l2_ctrl_new_std(&radio_hdl, &radio_ops, V4L2_CID_AUDIO_VOLUME, ...);
+	v4l2_ctrl_new_std(&radio_hdl, &radio_ops, V4L2_CID_AUDIO_MUTE, ...);
+	v4l2_ctrl_new_std(&video_hdl, &video_ops, V4L2_CID_BRIGHTNESS, ...);
+	v4l2_ctrl_new_std(&video_hdl, &video_ops, V4L2_CID_CONTRAST, ...);
+	v4l2_ctrl_add_handler(&video_hdl, &radio_hdl);
+
+Or you can add specific controls to a handler:
+
+	volume = v4l2_ctrl_new_std(&video_hdl, &ops, V4L2_CID_AUDIO_VOLUME, ...);
+	v4l2_ctrl_new_std(&video_hdl, &ops, V4L2_CID_BRIGHTNESS, ...);
+	v4l2_ctrl_new_std(&video_hdl, &ops, V4L2_CID_CONTRAST, ...);
+	v4l2_ctrl_add_ctrl(&radio_hdl, volume);
+
+What you should not do is make two identical controls for two handlers.
+For example:
+
+	v4l2_ctrl_new_std(&radio_hdl, &radio_ops, V4L2_CID_AUDIO_MUTE, ...);
+	v4l2_ctrl_new_std(&video_hdl, &video_ops, V4L2_CID_AUDIO_MUTE, ...);
+
+This would be bad since muting the radio would not change the video mute
+control. The rule is to have one control for each hardware 'knob' that you
+can twiddle.
+
+
+Finding Controls
+================
+
+Normally you have created the controls yourself and you can store the

struct

Documentation/video4linux/v4l2-controls.txt new file +12 -0

+v4l2_ctrl pointer into your own struct.
+
+But sometimes you need to find a control from another handler that you do
+not own. For example, if you have to find a volume control from a subdev.
+
+You can do that by calling v4l2_ctrl_find:
+
+	struct v4l2_ctrl *volume;
+
+	volume = v4l2_ctrl_find(sd->ctrl_handler, V4L2_CID_AUDIO_VOLUME);
+
+Since v4l2_ctrl_find will lock the handler you have to be careful where

you

Documentation/video4linux/v4l2-controls.txt new file +23 -0

+use it. For example, this is not a good idea:
+
+	struct v4l2_ctrl_handler hdl;
+
+	v4l2_ctrl_new_std(&hdl, &video_ops, V4L2_CID_BRIGHTNESS, ...);
+	v4l2_ctrl_new_std(&hdl, &video_ops, V4L2_CID_CONTRAST, ...);
+
+...and in video_ops.s_ctrl:
+
+	case V4L2_CID_BRIGHTNESS:
+		contrast = v4l2_find_ctrl(&hdl, V4L2_CID_CONTRAST);
+		...
+
+When s_ctrl is called by the framework the hdl.lock is already taken, so
+attempting to find another control from the same handler will deadlock.
+
+It is recommended not to use this function from inside the control ops.
+
+
+Inheriting Controls
+===================
+
+When one control handler is added to another using v4l2_ctrl_add_handler,

then
+by default all controls from one are merged to the other. But a subdev might
+have low-level controls that make sense for some advanced embedded system, but
+not when it is used in consumer-level hardware. In that case you want to keep
+those low-level controls local to the subdev. You can do this by simply

Documentation/video4linux/v4l2-controls.txt line 463

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+setting the 'is_private' flag of the control to 1:
+
+	ctrl = v4l2_ctrl_new_custom(&sd->hdl, &sd_ctrl_ops, ...);
+	if (ctrl)
+		ctrl->is_private = 1;

Wouldn't it make more sense to pass that as an argument to v4l2_ctrl_new_custom ?

I'm actually wondering if it wouldn't be better to pass a pointer to a structure with the required information (maybe the v4l2_control_info structure I mentioned earlier ?) to v4l2_ctrl_new_custom.

Quoted 15 lines

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+These controls will now be skipped when v4l2_ctrl_add_handler is called.
+
+
+Strict Control Validation
+=========================
+
+By default when the application wants to change an integer control the

value
+passed to the framework will automatically be modified to map to the provided
+minimum, maximum and step values of the control. If instead you just want to
+validate the value and not modify it, then set the 'strict_validation' flag of
+the control:

Documentation/video4linux/v4l2-controls.txt line 480

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+	ctrl->strict_validation = 1;
+
+Now -ERANGE will be returned if the new value does not match the control's
+requirements.

Why do we need the two behaviours ? Wouldn't it be better to standardize on one of them ?

Quoted 41 lines

+This is currently specific to integer controls. The value for boolean controls

Documentation/video4linux/v4l2-controls.txt new file +8 -0

+is always mapped to 0 or 1, menu and string controls are already validated
+strictly, and integer64 controls are not validated at all.
+
+
+V4L2_CTRL_TYPE_CTRL_CLASS Controls
+==================================
+
+Controls of this type can be used by GUIs to get the name of the control

class.

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+A fully featured GUI can make a dialog with multiple tabs with each tab
+containing the controls belonging to a particular control class. The name

of
+each tab can be found by querying a special control with ID <control class | 1>.

Documentation/video4linux/v4l2-controls.txt new file +2 -0

+
+Drivers do not have to care about this. The framework will automatically

add
+a control of this type whenever the first control belonging to a new control

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+class is added.
+
+
+Differences from the Spec
+=========================
+
+There are a few places where the framework acts slightly differently from

the
+V4L2 Specification. Those differences are described in this section. We will

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+have to see whether we need to adjust the spec or not.
+
+1) It is no longer required to have all controls contained in a
+v4l2_ext_control array be from the same control class. The framework will

be

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+able to handle any type of control in the array. You need to set ctrl_class
+to 0 in order to enable this. If ctrl_class is non-zero, then it will still
+check that all controls belong to that control class.
+
+If you set ctrl_class to 0 and count to 0, then it will only return an

Quoted 2 lines

error
+if there are no controls at all.

I don't know why we had such a limitation in the first place. I would indeed remove it completely.

Documentation/video4linux/v4l2-controls.txt line 522

Quoted 10 lines

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+
+2) Clarified the way error_idx works. For get and set it will be equal to
+count if nothing was done yet. If it is less than count then only the

controls

Documentation/video4linux/v4l2-controls.txt new file +5 -1

+up to error_idx-1 were successfully applied.
+
+3) When attempting to read a button control the framework will return
-EACCES
+instead of -EINVAL as stated in the spec. It seems to make more sense since
+button controls are write-only controls.

Agreed. We should change the spec.

Quoted 3 lines

+4) Attempting to write to a read-only control will return -EACCES instead of
+-EINVAL as the spec says.

This makes sense as well.

Quoted 16 lines

+5) The spec does not mention what should happen when you try to set/get a

Documentation/video4linux/v4l2-controls.txt new file +15 -0

+control class controls. ivtv currently returns -EINVAL (indicating that the
+control ID does not exist) while the framework will return -EACCES, which
+makes more sense.
+
+
+Proposals for Extensions
+========================
+
+Some ideas for future extensions to the spec:
+
+1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
+decimal. Useful for e.g. video_mute_yuv.
+
+2) It is possible to mark in the controls array which controls have been
+successfully written and which failed by for example adding a bit to the

Quoted 1 line

+control ID. Not sure if it is worth the effort, though.

I still feel a bit awkward about the interface. One particular point that might require attention is the split of the v4l2_ctrl structure into v4l2_control_info and v4l2_control_value structures (the names are not set into stone). I would also like to see if we can't pass pointers to v4l2_control_info (or similar) to the control creation functions instead of a plethora of arguments.

I'll now have a look at the code. As there are already quite a few comments on the documentation I won't perform a very in-depth code review though, I'll save that for later after we agree on the spec :-)

Quoted 1 line · On Sunday 02 May 2010 22:39:11 Laurent Pinchart wrote:

On Monday 26 April 2010 09:33:38 Hans Verkuil wrote:

Quoted 4 lines

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Objects in the framework
+========================
+
+There are two main objects:

Quoted 8 lines

Quoted 4 lines

(quoted diff) +2 -0

+
+The v4l2_ctrl object describes the control properties and keeps track

of the
+control's value (both the current value and the proposed new value).

I'm not sure v4l2_ctrl is a good name. We already have a v4l2_control structure, and using the abbreviated name for the in-kernel version is going to be confusing.

Originally it was called v4l2_ctrl_info but that became very cumbersome. It's not really an info object anyway, it really describes a control. When using this framework the driver no longer sees the v4l2_control anymore, so from the point of view of the driver there is only v4l2_ctrl.

Quoted 7 lines

It's obviously too late to change v4l2_control to something else. v4l2_control_info and v4l2_control_value are two names that come to my mind. Actually, it might be a good idea to split the v4l2_ctrl structure into a static driver-wide structure (v4l2_control_info) and an instance-specific structure (v4l2_control_value). There's no point in storing the same static data (function pointers, names, ...) for identical controls in different devices.

Other than the name there isn't anything that is static. And the name is already static.

Quoted 4 lines

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+  How to hook the handler into a bridge driver:
+
+	foo->v4l2_dev.ctrl_handler = &foo->hdl;

Quoted 5 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+  And whenever you call video_register_device() you must set the
+  ctrl_handler field of struct video_device as well:
+
+	vdev->ctrl_handler = &foo->hdl;

Quoted 2 lines

Why can't the framework find the ctrl_handler instance from the video_device's v4l2_device parent ?

Sometimes you want to provide different control handlers to different video devices. E.g. /dev/radio0 may have only a small subset of the controls of /dev/video0 (bttv does that, for example).

One option is to automatically let video_register_device copy ctrl_handler from the v4l2_dev struct, but I think it is clearer if it is explicitly set.

Quoted 14 lines

Quoted 14 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+  Finally, remove all control functions from your v4l2_ioctl_ops:
+  vidioc_queryctrl, vidioc_querymenu, vidioc_g_ctrl, vidioc_s_ctrl,
+  vidioc_g_ext_ctrls, vidioc_try_ext_ctrls and vidioc_s_ext_ctrls.
+  Those are now no longer needed.
+
+  How to hook the control handler into a subdev driver:
+
+	foo->sd.ctrl_handler = &foo->hdl;
+
+  And set all core control ops in your struct v4l2_subdev_core_ops to

these

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+  helpers:
+
+	.queryctrl = v4l2_sd_queryctrl,

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt line 98

Quoted 6 lines

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+	.querymenu = v4l2_sd_querymenu,
+	.g_ctrl = v4l2_sd_g_ctrl,
+	.s_ctrl = v4l2_sd_s_ctrl,
+	.g_ext_ctrls = v4l2_sd_g_ext_ctrls,
+	.try_ext_ctrls = v4l2_sd_try_ext_ctrls,
+	.s_ext_ctrls = v4l2_sd_s_ext_ctrls,

s/sd/subdev/ ?

I have to sleep on that... :-)

Quoted 7 lines

What about hardware for which the boundaries are only known at runtime, or could depend on the values of other controls ? I'm thinking about UVC devices for instance, the boundaries, step and default values need to be retrieved from the hardware. I currently do that at runtime when the control is queried for the first time and cache the values, as doing it during initialization (probe function) crashes a few webcams. That doesn't seem to be possible with the control framework.

It is possible to add controls to an existing control handler at runtime. It is also possible to change boundaries at runtime: you just change the relevant values in v4l2_ctrl. There is no function for that, it's enough to call v4l2_ctrl_lock(), change the values and call unlock().

I could make a function that does this, but UVC is the only driver that I know of that might need this.

Quoted 19 lines

Quoted 19 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+	...
+	if (foo->hdl.error) {
+		int err = foo->hdl.error;
+
+		v4l2_ctrl_handler_free(&foo->hdl);
+		return err;
+	}
+
+The v4l2_ctrl_new_std function returns the v4l2_ctrl pointer to the new
+control, but if you do not need to access the pointer outside the

control ops,

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+then there is no need to store it.
+
+Note that if something fails, the function will return NULL or an error

and
+set hdl->error to the error code. If hdl->error was already set, then it
+will just return and do nothing. This is also true for v4l2_ctrl_handler_init

Quoted 8 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+if it cannot allocate the internal data structure.
+
+This makes it easy to init the handler and just add all controls and

only check
+the error code at the end. Saves a lot of repetitive error checking.

I would still check the v4l2_ctrl_handler_init return value explicitly, but that may be just me.

Feel free to do that :-)

Quoted 11 lines

Quoted 8 lines

+3) Optionally force initial control setup:

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	v4l2_ctrl_handler_setup(&foo->hdl);
+
+This will call s_ctrl for all controls unconditionally. Effectively

this
+initializes the hardware to the default control values. It is recommended +that you do this.

What about the other way around, if I want to initialize the framework with the current values retrieved from the hardware ?

In that case you would set the default value of the control based on the value reported by the hardware.

Quoted 24 lines

Quoted 24 lines

Documentation/video4linux/v4l2-controls.txt new file +20 -0

+Accessing Control Values
+========================
+
+The v4l2_ctrl struct contains these two unions:
+
+	/* The current control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	} cur;
+
+	/* The new control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	};
+
+Within the control ops you can freely use these. The val and val64

speak for

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+themselves. The string pointers point to character buffers of length
+ctrl->maximum + 1, and are always 0-terminated.
+

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt line 215

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+For g_ctrl you have to update the current control values like this:
+
+	ctrl->cur.val = read_reg(0x123);
+
+The 'new value' union is not relevant in g_ctrl.

When is g_ctrl called ? When the userspace applications issues a VIDIOC_G_CTRL or VIDIOC_G_EXT_CTRLS ioctl ?

Yes.

Quoted 4 lines

I suppose the operation only needs to be implemented if the device needs to be queried because it modified the control value on its own (possibly as a result of setting another control). What if some controls need to be handled that way, while the others are not self-modifying ?

g_ctrl only has to take care of self-modifying controls and can ignore the others. saa7115 has a use case like that.

Quoted 27 lines

Quoted 19 lines

+For try/s_ctrl the new values (i.e. as passed by the user) are filled in and

(quoted diff) +2 -0

+you can modify them in try_ctrl or set them in s_ctrl. The 'cur' union
+contains the current value, which you can use (but not change!) as

well. +
+If s_ctrl returns 0 (OK), then the control framework will copy the new final

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+values to the 'cur' union.
+
+While in g/s/try_ctrl you can access the value of all controls owned

by the
+same handler since the handler's lock is held. Do not attempt to access +the value of controls owned by other handlers, though.

(quoted diff) +2 -0

+
+Elsewhere in the driver you have to be more careful. You cannot just

refer +to the current control values without locking.

(quoted diff) +2 -0

+
+There are two simple helper functions defined that will get or set a

single

Documentation/video4linux/v4l2-controls.txt line 235

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+control value safely:
+
+	s32 v4l2_ctrl_g(struct v4l2_ctrl *ctrl);
+	int v4l2_ctrl_s(struct v4l2_ctrl *ctrl, s32 val);

I suppose that v4l2_ctrl_s can be used if the device notifies the driver that a control has changed. Is that correct ?

Quoted 2 lines

I don't really like the names of those two functions. For instance, v4l2_ctrl_s looks like it will call the driver to set a control.

I think there is a misunderstanding here: v4l2_ctrl_s() will indeed change the control's value.

Quoted 34 lines

Quoted 26 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Menu Controls
+=============
+
+Menu controls use the 'step' value differently compared to other

control +types. The v4l2_ctrl struct contains this union:

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+
+	union {
+		u32 step;
+		u32 menu_skip_mask;
+	};
+
+For menu controls menu_skip_mask is used. What it does is that it

allows you
+to easily exclude certain menu items. This is used in the VIDIOC_QUERYMENU +implementation where you can return -EINVAL if a certain menu item is not +present. Note that VIDIOC_QUERYCTRL always returns a step value of 1 for +menu controls.

(quoted diff) +2 -0

+
+A good example is the MPEG Audio Layer II Bitrate menu control where

the +menu is a list of standardized possible bitrates. But in practice hardware +implementations will only support a subset of those. By setting the skip +mask you can tell the framework which menu items should be skipped. Setting

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+it to 0 means that all menu items are supported.
+
+So when using v4l2_ctrl_new_std or v4l2_ctrl_new_custom you need to

Quoted 4 lines

remember
+that 'step' means 'skip mask' for menu controls. If you put in '1' by mistake,
+then the first menu item will be skipped.

What about adding a v4l2_ctrl_new_menu then ? That will avoid such a mistake.

Quoted 10 lines

Quoted 5 lines

+The v4l2_ctrl_new_std_menu can be used to add menu controls more
easily: it
+will calculate the min and max values automatically based on the size of the
+menu, and it has a proper 'mask' argument instead of 'step'.

OK :-) Shouldn't v4l2_ctrl_new_custom be restricted to non-menu controls then ? Looking at the code, it should probably become an internal function, with another function added for custom controls without menu support.

I might add that in the future. I want to avoid creating a long series of different functions, all alike :-)

Quoted 10 lines

Quoted 10 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Active and Grabbed Controls
+===========================
+
+If you get more complex relationships between controls, then you may

have to
+activate and deactivate controls. For example, if the Chroma AGC control is +on, then the Chroma Gain control is inactive. That is, you may set it, but +the value will not be used by the hardware as long as the automatic gain +control is on. Typically user interfaces can disable such input fields. +

Quoted 8 lines

Quoted 4 lines

+You can set the 'active' status using v4l2_ctrl_activate(). By default all +controls are active. Note that the framework does not check for this flag. +It is meant purely for GUIs. The function is typically called from within +s_ctrl.

What you refer to as active/inactive is usually referred to as enabled/disabled in the GUI world. Might be worth using the same convention.

Strictly speaking it is not quite the same. An inactive control can still be written, the new value will simply not be used until the control becomes active again. In a GUI that would be confusing, so the control is disabled instead. It's a subtle difference that may or may not be important.

Quoted 12 lines

Quoted 12 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+Control Clusters
+================
+
+By default all controls are independent from the others. But in more
+complex scenarios you can get dependencies from one control to

another. +In that case you need to 'cluster' them:

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+
+	struct foo {
+		struct v4l2_ctrl_handler hdl;
+		struct v4l2_ctrl *volume;
+		struct v4l2_ctrl *mute;
+		...

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt line 322

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+	};
+
+	state->volume = v4l2_ctrl_new_std(&state->hdl, ...);
+	state->mute = v4l2_ctrl_new_std(&state->hdl, ...);
+	v4l2_ctrl_cluster(2, &state->volume);

What's the first argument to v4l2_ctrl_cluster ? The number of controls in the cluster ?

Yes.

Quoted 5 lines

Does that imply that they need to be added in a sequence, right before v4l2_ctrl_cluster is called ? That seems a bit awkward. Wouldn't it be better to specify the relationships between controls explicitly, maybe by passing a pointer to the master control when creating the 'slave' controls in the cluster ?

OK, I clearly need to explain this better.

What you pass is an array of v4l2_ctrl pointers, e.g.:

	struct foo {
		struct v4l2_ctrl *cluster[2];
		...
	};

	foo->cluster[CTRL_VOLUME] = v4l2_ctrl_new_std();
	foo->cluster[CTRL_MUTE] = v4l2_ctrl_new_std();
	v4l2_ctrl_cluster(2, foo->cluster);

cluster[0] is always the master.

Now, using arrays here becomes quickly very annoying, so instead you can just say:

	struct foo {
		/* volume/mute cluster */
		struct v4l2_ctrl *volume;
		struct v4l2_ctrl *mute;
		...
	};

	foo->volume = v4l2_ctrl_new_std();
	foo->mute = v4l2_ctrl_new_std();
	v4l2_ctrl_cluster(2, &foo->volume);

Same thing, but without the cumbersome array indices.

Quoted 9 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+VIDIOC_LOG_STATUS Support
+=========================
+
+This ioctl allow you to dump the current status of a driver to the

kernel log.

This has nothing to do with the controls framework, but shouldn't that ioctl be restricted to root only ? It can potentially dump a lot of information to the kernel log, and thus system logs.

Personally I see no reason to change this.

Quoted 10 lines

Quoted 10 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Different Handlers for Different Video Nodes
+============================================
+
+Usually the bridge driver has just one control handler that is global

for +all video nodes. But you can also specify different control handlers for +different video nodes. It's no problem if there are no subdevs involved. +But if there are, then you need to block the automatic merging of subdev +controls to the global control handler. You do that by simply setting the +ctrl_handler field in struct v4l2_device to NULL.

Quoted 8 lines

Quoted 6 lines

(quoted diff) +2 -0

+
+After each subdev was added, you will then have to call

v4l2_ctrl_add_handler
+manually to add the subdev's control handler (sd->ctrl_handler) to the desired
+bridge control handler.

Do you mean video device instead of bridge here ?

struct v4l2_device.

Quoted 3 lines

I wouldn't mention "bridge" in here. I assume that by bridge you mean v4l2_device. Please use that name directly. The term "bridge" is only applicable to a subset of the v4l2_device use cases.

Good point.

Quoted 2 lines

Can controls for a specific subdev be reported through more than one video device node, but not all of them ?

Yes. It will be a bit fiddly to set up, but that's to be expected if you want to do things like that.

Quoted 12 lines

Quoted 12 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Inheriting Controls
+===================
+
+When one control handler is added to another using

v4l2_ctrl_add_handler, then
+by default all controls from one are merged to the other. But a subdev might
+have low-level controls that make sense for some advanced embedded system, but
+not when it is used in consumer-level hardware. In that case you want to keep
+those low-level controls local to the subdev. You can do this by

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt line 463

Quoted 5 lines

simply +setting the 'is_private' flag of the control to 1:

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	ctrl = v4l2_ctrl_new_custom(&sd->hdl, &sd_ctrl_ops, ...);
+	if (ctrl)
+		ctrl->is_private = 1;

Wouldn't it make more sense to pass that as an argument to v4l2_ctrl_new_custom ?

As I said earlier, I tried to avoid creating zillions of very similar functions, or functions with very long arg lists. For now at least private controls are not used at all.

It is usually easier for the driver to write a few special static inlines that do exactly what the driver needs. Of course, if many drivers need to do the same thing, then such inlines should be moved to the core header.

Quoted 3 lines

I'm actually wondering if it wouldn't be better to pass a pointer to a structure with the required information (maybe the v4l2_control_info structure I mentioned earlier ?) to v4l2_ctrl_new_custom.

I thought about that, but 1) that means yet another structure and 2) I dislike code like that because the definition is usually quite far from the place were it is actually used.

Quoted 15 lines

Quoted 15 lines

+These controls will now be skipped when v4l2_ctrl_add_handler is called. +

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+Strict Control Validation
+=========================
+
+By default when the application wants to change an integer control the

value
+passed to the framework will automatically be modified to map to the provided
+minimum, maximum and step values of the control. If instead you just want to
+validate the value and not modify it, then set the 'strict_validation' flag of
+the control:

Quoted 8 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	ctrl->strict_validation = 1;
+
+Now -ERANGE will be returned if the new value does not match the

control's +requirements.

Why do we need the two behaviours ? Wouldn't it be better to standardize on one of them ?

I may have fallen in the pitfall of featurism here. Just because it is easy to implement, that doesn't mean that you should actually do it.

I think I'll remove strict_validation.

Quoted 10 lines

Quoted 10 lines

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+Proposals for Extensions
+========================
+
+Some ideas for future extensions to the spec:
+
+1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal

instead of +decimal. Useful for e.g. video_mute_yuv.
+
+2) It is possible to mark in the controls array which controls have been +successfully written and which failed by for example adding a

Quoted 8 lines

Quoted 1 line

bit to the +control ID. Not sure if it is worth the effort, though.

I still feel a bit awkward about the interface. One particular point that might require attention is the split of the v4l2_ctrl structure into v4l2_control_info and v4l2_control_value structures (the names are not set into stone). I would also like to see if we can't pass pointers to v4l2_control_info (or similar) to the control creation functions instead of a plethora of arguments.

One of my original attempts was indeed to have two structs. But that made the code only more complex. Having two structs only makes sense if the 'static' part can be, well, static.

But then you have UVC where that isn't static. So the code then has to differentiate between these two cases and it quickly becomes a big mess.

Regarding the plethora of arguments: remember that 99% of all controls will just use v4l2_ctrl_new_std(). And that ain't too bad. new_custom is indeed a big one, but currently only used by cx2341x where it is wrapped inside a much easier to digest function. I've tried to make the 90% of easy cases easy, and for the remaining 10% you will indeed have to do more work.

Quoted 3 lines

I'll now have a look at the code. As there are already quite a few comments on the documentation I won't perform a very in-depth code review though, I'll save that for later after we agree on the spec :-)

Thanks for the review!

This was very useful.

Hi Hans,

Quoted 2 lines · On Monday 03 May 2010 00:21:46 Hans Verkuil wrote:

Quoted 1 line · On Sunday 02 May 2010 22:39:11 Laurent Pinchart wrote:

On Monday 26 April 2010 09:33:38 Hans Verkuil wrote:

[snip]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Objects in the framework
+========================
+
+There are two main objects:

Quoted 8 lines

Quoted 4 lines

(quoted diff) +2 -0

+
+The v4l2_ctrl object describes the control properties and keeps track

of the
+control's value (both the current value and the proposed new value).

I'm not sure v4l2_ctrl is a good name. We already have a v4l2_control structure, and using the abbreviated name for the in-kernel version is going to be confusing.

Quoted 4 lines

Originally it was called v4l2_ctrl_info but that became very cumbersome. It's not really an info object anyway, it really describes a control. When using this framework the driver no longer sees the v4l2_control anymore, so from the point of view of the driver there is only v4l2_ctrl.

Sure, but it's still misleading. Using an both the abbreviated and non- abbreviated names to refer to two different things has been done too often in V4L2, and it's starting to bother me. It makes the API look inconsistent. We need to define a naming policy and stick to it for the future. That can be a topic for the V4L2 summit in June.

Quoted 10 lines

Quoted 7 lines

It's obviously too late to change v4l2_control to something else. v4l2_control_info and v4l2_control_value are two names that come to my mind. Actually, it might be a good idea to split the v4l2_ctrl structure into a static driver-wide structure (v4l2_control_info) and an instance-specific structure (v4l2_control_value). There's no point in storing the same static data (function pointers, names, ...) for identical controls in different devices.

Other than the name there isn't anything that is static. And the name is already static.

The following fields are static (they're identical across all v4l2_ctrl instances for the same control):

- ops
- id
- name
- type
- menu (not too sure about this one)

[snip]

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+  How to hook the handler into a bridge driver:
+
+	foo->v4l2_dev.ctrl_handler = &foo->hdl;

[snip]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+  And whenever you call video_register_device() you must set the
+  ctrl_handler field of struct video_device as well:
+
+	vdev->ctrl_handler = &foo->hdl;

[snip]

Quoted 10 lines

Quoted 2 lines

Why can't the framework find the ctrl_handler instance from the video_device's v4l2_device parent ?

Sometimes you want to provide different control handlers to different video devices. E.g. /dev/radio0 may have only a small subset of the controls of /dev/video0 (bttv does that, for example).

One option is to automatically let video_register_device copy ctrl_handler from the v4l2_dev struct, but I think it is clearer if it is explicitly set.

I was thinking about copying it implicitly if it's not set, yes. That way only the v4l2_device instance would need an explicit handler, video devices would inherit it from their v4l2_device parent. The driver would still have the ability to set it if it wants to override the default behaviour.

It might be clearer to always set it explicitly, I have no strong opinion on this one.

Quoted 23 lines

Quoted 22 lines

Quoted 11 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+  Finally, remove all control functions from your v4l2_ioctl_ops:
+  vidioc_queryctrl, vidioc_querymenu, vidioc_g_ctrl, vidioc_s_ctrl,
+  vidioc_g_ext_ctrls, vidioc_try_ext_ctrls and vidioc_s_ext_ctrls.
+  Those are now no longer needed.
+
+  How to hook the control handler into a subdev driver:
+
+	foo->sd.ctrl_handler = &foo->hdl;
+
+  And set all core control ops in your struct v4l2_subdev_core_ops to

these

Documentation/video4linux/v4l2-controls.txt line 98

Quoted 9 lines

Documentation/video4linux/v4l2-controls.txt new file +9 -0

+  helpers:
+
+	.queryctrl = v4l2_sd_queryctrl,
+	.querymenu = v4l2_sd_querymenu,
+	.g_ctrl = v4l2_sd_g_ctrl,
+	.s_ctrl = v4l2_sd_s_ctrl,
+	.g_ext_ctrls = v4l2_sd_g_ext_ctrls,
+	.try_ext_ctrls = v4l2_sd_try_ext_ctrls,
+	.s_ext_ctrls = v4l2_sd_s_ext_ctrls,

s/sd/subdev/ ?

Quoted 1 line

I have to sleep on that... :-)

You've slept on it for a week now :-)

We really have too many abbreviations in V4L2, or at least no consistent usage of them.

[snip]

Quoted 13 lines

Quoted 7 lines

What about hardware for which the boundaries are only known at runtime, or could depend on the values of other controls ? I'm thinking about UVC devices for instance, the boundaries, step and default values need to be retrieved from the hardware. I currently do that at runtime when the control is queried for the first time and cache the values, as doing it during initialization (probe function) crashes a few webcams. That doesn't seem to be possible with the control framework.

It is possible to add controls to an existing control handler at runtime. It is also possible to change boundaries at runtime: you just change the relevant values in v4l2_ctrl. There is no function for that, it's enough to call v4l2_ctrl_lock(), change the values and call unlock().

Quoted 2 lines

I could make a function that does this, but UVC is the only driver that I know of that might need this.

My issue is that the UVC driver would need to set the boundaries the first time the control is queried. There's no callback for that at the moment.

Quoted 28 lines

Quoted 27 lines

Quoted 22 lines

Documentation/video4linux/v4l2-controls.txt new file +10 -0

+	...
+	if (foo->hdl.error) {
+		int err = foo->hdl.error;
+
+		v4l2_ctrl_handler_free(&foo->hdl);
+		return err;
+	}
+
+The v4l2_ctrl_new_std function returns the v4l2_ctrl pointer to the new
+control, but if you do not need to access the pointer outside the

control ops,

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+then there is no need to store it.
+
+Note that if something fails, the function will return NULL or an error

and
+set hdl->error to the error code. If hdl->error was already set, then it
+will just return and do nothing. This is also true for v4l2_ctrl_handler_init

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+if it cannot allocate the internal data structure.
+
+This makes it easy to init the handler and just add all controls and

Quoted 2 lines

only check
+the error code at the end. Saves a lot of repetitive error checking.

I would still check the v4l2_ctrl_handler_init return value explicitly, but that may be just me.

Quoted 1 line

Feel free to do that :-)

You know what I meant :-) I agree this is a good way to handle errors for v4l2_ctrl_new_*, but v4l2_ctrl_handler_init feels... different.

[snip]

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

+3) Optionally force initial control setup:

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	v4l2_ctrl_handler_setup(&foo->hdl);
+
+This will call s_ctrl for all controls unconditionally. Effectively

this
+initializes the hardware to the default control values. It is recommended +that you do this.

What about the other way around, if I want to initialize the framework with the current values retrieved from the hardware ?

Quoted 2 lines

In that case you would set the default value of the control based on the value reported by the hardware.

The default value can still be different than the current value. Don't you mean I should initialize the current value, not the default one ?

What should be recommended, calling v4l2_ctrl_handler_setup, or initializing the framework with the current value ?

Thinking some more about this, it can be a problem for UVC devices, much like querying the min/max/step/default values is. Some devices just crash if you send too many requests too fast. I would like to get a g_ctrl call the first time the current value needs to be read, and then have the framework cache that value.

By the way (still more thinking :-)), does the framework handle a value cache that can be configured per control ? Some controls are volatile and need to be read from the driver every time, some others can be cached. How is that handled ?

Last thought: does the framework query the driver for the current control values when setting a control in a cluster ? If two clustered controls are stored in a single hardware "register", and one of them needs to be modified, a read-update-write operation needs to be performed. The update part will be done by the framework, the write part by the driver, but what about the read part ?

[snip]

Quoted 33 lines

Quoted 32 lines

Quoted 21 lines

Documentation/video4linux/v4l2-controls.txt new file +20 -0

+Accessing Control Values
+========================
+
+The v4l2_ctrl struct contains these two unions:
+
+	/* The current control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	} cur;
+
+	/* The new control value. */
+	union {
+		s32 val;
+		s64 val64;
+		char *string;
+	};
+
+Within the control ops you can freely use these. The val and val64

speak for

Documentation/video4linux/v4l2-controls.txt line 215

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt new file +8 -0

+themselves. The string pointers point to character buffers of length
+ctrl->maximum + 1, and are always 0-terminated.
+
+For g_ctrl you have to update the current control values like this:
+
+	ctrl->cur.val = read_reg(0x123);
+
+The 'new value' union is not relevant in g_ctrl.

When is g_ctrl called ? When the userspace applications issues a VIDIOC_G_CTRL or VIDIOC_G_EXT_CTRLS ioctl ?

Quoted 1 line

Yes.

OK. Can you make that explicit ?

Quoted 5 lines

Quoted 4 lines

I suppose the operation only needs to be implemented if the device needs to be queried because it modified the control value on its own (possibly as a result of setting another control). What if some controls need to be handled that way, while the others are not self-modifying ?

Quoted 2 lines

g_ctrl only has to take care of self-modifying controls and can ignore the others. saa7115 has a use case like that.

Instead of calling g_ctrl for all drivers, have it go through a big switch and do nothing, wouldn't it be better to add a volatile flag to the v4l2_ctrl structure ? g_ctrl calls would be skipped for non-volatile controls (except on the first read of course).

Quoted 30 lines

Quoted 29 lines

Quoted 19 lines

+For try/s_ctrl the new values (i.e. as passed by the user) are filled in and

(quoted diff) +2 -0

+you can modify them in try_ctrl or set them in s_ctrl. The 'cur' union
+contains the current value, which you can use (but not change!) as

well. +
+If s_ctrl returns 0 (OK), then the control framework will copy the new final

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+values to the 'cur' union.
+
+While in g/s/try_ctrl you can access the value of all controls owned

by the
+same handler since the handler's lock is held. Do not attempt to access +the value of controls owned by other handlers, though.

(quoted diff) +2 -0

+
+Elsewhere in the driver you have to be more careful. You cannot just

refer +to the current control values without locking.

(quoted diff) +2 -0

+
+There are two simple helper functions defined that will get or set a

single

Documentation/video4linux/v4l2-controls.txt line 235

Quoted 4 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+control value safely:
+
+	s32 v4l2_ctrl_g(struct v4l2_ctrl *ctrl);
+	int v4l2_ctrl_s(struct v4l2_ctrl *ctrl, s32 val);

I suppose that v4l2_ctrl_s can be used if the device notifies the driver that a control has changed. Is that correct ?

I don't really like the names of those two functions. For instance, v4l2_ctrl_s looks like it will call the driver to set a control.

Quoted 2 lines

I think there is a misunderstanding here: v4l2_ctrl_s() will indeed change the control's value.

Do you mean that it will call the driver's s_ctrl operation or that it will modify the current value in the v4l2_ctrl structure ? The misunderstanding probably calls for a function name change :-)

[snip]

Quoted 45 lines

Quoted 44 lines

Quoted 26 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Menu Controls
+=============
+
+Menu controls use the 'step' value differently compared to other

control +types. The v4l2_ctrl struct contains this union:

Documentation/video4linux/v4l2-controls.txt new file +7 -0

+
+	union {
+		u32 step;
+		u32 menu_skip_mask;
+	};
+
+For menu controls menu_skip_mask is used. What it does is that it

allows you
+to easily exclude certain menu items. This is used in the VIDIOC_QUERYMENU +implementation where you can return -EINVAL if a certain menu item is not +present. Note that VIDIOC_QUERYCTRL always returns a step value of 1 for +menu controls.

(quoted diff) +2 -0

+
+A good example is the MPEG Audio Layer II Bitrate menu control where

the +menu is a list of standardized possible bitrates. But in practice hardware +implementations will only support a subset of those. By setting the skip +mask you can tell the framework which menu items should be skipped. Setting

Documentation/video4linux/v4l2-controls.txt new file +3 -0

+it to 0 means that all menu items are supported.
+
+So when using v4l2_ctrl_new_std or v4l2_ctrl_new_custom you need to

Quoted 4 lines

remember
+that 'step' means 'skip mask' for menu controls. If you put in '1' by mistake,
+then the first menu item will be skipped.

What about adding a v4l2_ctrl_new_menu then ? That will avoid such a mistake.

Quoted 5 lines

+The v4l2_ctrl_new_std_menu can be used to add menu controls more
easily: it
+will calculate the min and max values automatically based on the size of the
+menu, and it has a proper 'mask' argument instead of 'step'.

OK :-) Shouldn't v4l2_ctrl_new_custom be restricted to non-menu controls then ? Looking at the code, it should probably become an internal function, with another function added for custom controls without menu support.

Quoted 2 lines

I might add that in the future. I want to avoid creating a long series of different functions, all alike :-)

Then drivers will use the function that will later become internal. You'll have to change a bunch of drivers later, and you will tell me that it would be too much work and wouldn't be worth it ;-)

Quoted 19 lines

Quoted 18 lines

Quoted 6 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Active and Grabbed Controls
+===========================
+
+If you get more complex relationships between controls, then you may

have to
+activate and deactivate controls. For example, if the Chroma AGC

Quoted 8 lines

control is +on, then the Chroma Gain control is inactive. That is, you may set it, but +the value will not be used by the hardware as long as the automatic gain +control is on. Typically user interfaces can disable such input fields. +
+You can set the 'active' status using v4l2_ctrl_activate(). By default all +controls are active. Note that the framework does not check for this flag. +It is meant purely for GUIs. The function is typically called from within +s_ctrl.

What you refer to as active/inactive is usually referred to as enabled/disabled in the GUI world. Might be worth using the same convention.

Quoted 5 lines

Strictly speaking it is not quite the same. An inactive control can still be written, the new value will simply not be used until the control becomes active again. In a GUI that would be confusing, so the control is disabled instead. It's a subtle difference that may or may not be important.

What does the framework do when the userspace application attempts to write to a disabled control ?

[snip]

Quoted 57 lines

Quoted 12 lines

Quoted 12 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+Control Clusters
+================
+
+By default all controls are independent from the others. But in more
+complex scenarios you can get dependencies from one control to

another. +In that case you need to 'cluster' them:

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+
+	struct foo {
+		struct v4l2_ctrl_handler hdl;
+		struct v4l2_ctrl *volume;
+		struct v4l2_ctrl *mute;
+		...

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt line 322

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+	};
+
+	state->volume = v4l2_ctrl_new_std(&state->hdl, ...);
+	state->mute = v4l2_ctrl_new_std(&state->hdl, ...);
+	v4l2_ctrl_cluster(2, &state->volume);

What's the first argument to v4l2_ctrl_cluster ? The number of controls in the cluster ?

Yes.

Quoted 5 lines

Does that imply that they need to be added in a sequence, right before v4l2_ctrl_cluster is called ? That seems a bit awkward. Wouldn't it be better to specify the relationships between controls explicitly, maybe by passing a pointer to the master control when creating the 'slave' controls in the cluster ?

OK, I clearly need to explain this better.

What you pass is an array of v4l2_ctrl pointers, e.g.:

	struct foo {
		struct v4l2_ctrl *cluster[2];
		...
	};

	foo->cluster[CTRL_VOLUME] = v4l2_ctrl_new_std();
	foo->cluster[CTRL_MUTE] = v4l2_ctrl_new_std();
	v4l2_ctrl_cluster(2, foo->cluster);

cluster[0] is always the master.

Now, using arrays here becomes quickly very annoying, so instead you can just say:

	struct foo {
		/* volume/mute cluster */
		struct v4l2_ctrl *volume;
		struct v4l2_ctrl *mute;
		...
	};

	foo->volume = v4l2_ctrl_new_std();
	foo->mute = v4l2_ctrl_new_std();
	v4l2_ctrl_cluster(2, &foo->volume);

Quoted 1 line

Same thing, but without the cumbersome array indices.

That looks *very* dangerous. People will not get it (I haven't) and all kind of bad things will happen, like adding a field between two struct v4l2_ctrl pointers. This part of the API needs to be reworked, if you require an array then an array must be passed to the function.

[snip]

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+VIDIOC_LOG_STATUS Support
+=========================
+
+This ioctl allow you to dump the current status of a driver to the

kernel log.

This has nothing to do with the controls framework, but shouldn't that ioctl be restricted to root only ? It can potentially dump a lot of information to the kernel log, and thus system logs.

Quoted 1 line

Personally I see no reason to change this.

You don't think it opens a door for unprivileged applications to flood the kernel and system logs ?

[snip]

Quoted 19 lines

Quoted 18 lines

Quoted 12 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Different Handlers for Different Video Nodes
+============================================
+
+Usually the bridge driver has just one control handler that is global

for +all video nodes. But you can also specify different control handlers for +different video nodes. It's no problem if there are no subdevs involved. +But if there are, then you need to block the automatic merging of subdev +controls to the global control handler. You do that by simply setting the +ctrl_handler field in struct v4l2_device to NULL.

(quoted diff) +2 -0

+
+After each subdev was added, you will then have to call

Quoted 4 lines

v4l2_ctrl_add_handler
+manually to add the subdev's control handler (sd->ctrl_handler) to the desired
+bridge control handler.

Do you mean video device instead of bridge here ?

Quoted 1 line

struct v4l2_device.

I don't get it then. If you want to use different handlers for different video nodes, you need to add the subdev handler to the video node (video_device) handler, not the subdev handler to the v4l2_device handle.

Quoted 11 lines

Quoted 3 lines

I wouldn't mention "bridge" in here. I assume that by bridge you mean v4l2_device. Please use that name directly. The term "bridge" is only applicable to a subset of the v4l2_device use cases.

Good point.

Quoted 2 lines

Can controls for a specific subdev be reported through more than one video device node, but not all of them ?

Yes. It will be a bit fiddly to set up, but that's to be expected if you want to do things like that.

I was just wondering if it was possible, I have no use case in mind. I wonder if we should even allow that...

[snip]

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+Inheriting Controls
+===================
+
+When one control handler is added to another using

v4l2_ctrl_add_handler, then
+by default all controls from one are merged to the other. But a subdev might
+have low-level controls that make sense for some advanced embedded system, but
+not when it is used in consumer-level hardware. In that case you want to keep
+those low-level controls local to the subdev. You can do this by

Quoted 8 lines

Documentation/video4linux/v4l2-controls.txt line 463

Quoted 5 lines

simply +setting the 'is_private' flag of the control to 1:

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	ctrl = v4l2_ctrl_new_custom(&sd->hdl, &sd_ctrl_ops, ...);
+	if (ctrl)
+		ctrl->is_private = 1;

Wouldn't it make more sense to pass that as an argument to v4l2_ctrl_new_custom ?

Quoted 7 lines

As I said earlier, I tried to avoid creating zillions of very similar functions, or functions with very long arg lists. For now at least private controls are not used at all.

It is usually easier for the driver to write a few special static inlines that do exactly what the driver needs. Of course, if many drivers need to do the same thing, then such inlines should be moved to the core header.

"Maybe this feature won't be used" sounds more like a reason to remove it than to implement it badly and leave it to be fixed later :-)

The control framework is a big change to the V4L2 framework, we should take great care to get it as good as possible on the first try.

Quoted 8 lines

Quoted 4 lines

I'm actually wondering if it wouldn't be better to pass a pointer to a structure with the required information (maybe the v4l2_control_info structure I mentioned earlier ?) to v4l2_ctrl_new_custom.

I thought about that, but 1) that means yet another structure and 2) I dislike code like that because the definition is usually quite far from the place were it is actually used.

Yes, it means yet another structure. On the other hand all those structures can be nicely grouped in an array, and controls can then be added using a loop instead of a bunch of function calls. I think it would be much cleaner, code would be more readable and error handling would be easier.

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

+These controls will now be skipped when v4l2_ctrl_add_handler is called. +

Documentation/video4linux/v4l2-controls.txt new file +5 -0

+
+Strict Control Validation
+=========================
+
+By default when the application wants to change an integer control the

value
+passed to the framework will automatically be modified to map to the provided
+minimum, maximum and step values of the control. If instead you just want to
+validate the value and not modify it, then set the 'strict_validation' flag of
+the control:

Quoted 8 lines

Quoted 5 lines

Documentation/video4linux/v4l2-controls.txt new file +4 -0

+
+	ctrl->strict_validation = 1;
+
+Now -ERANGE will be returned if the new value does not match the

control's +requirements.

Why do we need the two behaviours ? Wouldn't it be better to standardize on one of them ?

Quoted 4 lines

I may have fallen in the pitfall of featurism here. Just because it is easy to implement, that doesn't mean that you should actually do it.

I think I'll remove strict_validation.

Thanks.

[snip]

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

Documentation/video4linux/v4l2-controls.txt new file +6 -0

+Proposals for Extensions
+========================
+
+Some ideas for future extensions to the spec:
+
+1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal

instead of +decimal. Useful for e.g. video_mute_yuv.
+
+2) It is possible to mark in the controls array which controls have been +successfully written and which failed by for example adding a

Quoted 8 lines

Quoted 1 line

bit to the +control ID. Not sure if it is worth the effort, though.

I still feel a bit awkward about the interface. One particular point that might require attention is the split of the v4l2_ctrl structure into v4l2_control_info and v4l2_control_value structures (the names are not set into stone). I would also like to see if we can't pass pointers to v4l2_control_info (or similar) to the control creation functions instead of a plethora of arguments.

Quoted 3 lines

One of my original attempts was indeed to have two structs. But that made the code only more complex. Having two structs only makes sense if the 'static' part can be, well, static.

Indeed :-) Like I pointed out above there's some static information, but maybe not enough of them to split them into another structure. On the other hand, like I also pointed above, I think passing a structure, even if it's not big, to the control creation functions would make driver code cleaner.

Quoted 2 lines

But then you have UVC where that isn't static. So the code then has to differentiate between these two cases and it quickly becomes a big mess.

There's static information in UVC. Look at uvc_ctrl.c, it starts with two big arrays.

Quoted 5 lines

Regarding the plethora of arguments: remember that 99% of all controls will just use v4l2_ctrl_new_std(). And that ain't too bad. new_custom is indeed a big one, but currently only used by cx2341x where it is wrapped inside a much easier to digest function. I've tried to make the 90% of easy cases easy, and for the remaining 10% you will indeed have to do more work.

If we pass structures to the functions, drivers could only fill the fields they need and skip the other ones. For instance, a name field would be left untouched by drivers that call v4l2_ctrl_new_std(). The ones that call v4l2_ctrl_new_custom() would just need to fill the field in the v4l2_ctrl_whatever array. Clean and easy :-)

This (whether we should use a structure to create new controls) is something on which I'd like to hear other people's opinions (Sakari, I asked you to review this RFC, so please answer :-)).

Quoted 8 lines

Quoted 4 lines

I'll now have a look at the code. As there are already quite a few comments on the documentation I won't perform a very in-depth code review though, I'll save that for later after we agree on the spec :-)

Thanks for the review!

This was very useful.

You're welcome. I might bother you with other issues later if I catch new one :-)

Quoted 1 line

Should't v4l2_ctrl_new_std take a control type as well ?

The type is set automatically as that is determined by the control ID.

Quoted 7 lines

What about hardware for which the boundaries are only known at runtime, or could depend on the values of other controls ? I'm thinking about UVC devices for instance, the boundaries, step and default values need to be retrieved from the hardware. I currently do that at runtime when the control is queried for the first time and cache the values, as doing it during initialization (probe function) crashes a few webcams. That doesn't seem to be possible with the control framework.

It is possible to add controls to an existing control handler at runtime. It is also possible to change boundaries at runtime: you just change the relevant values in v4l2_ctrl. There is no function for that, it's enough to call v4l2_ctrl_lock(), change the values and call unlock().

I could make a function that does this, but UVC is the only driver that I know of that might need this.

Signed-off-by Hans Verkuil <[email protected]>

---
 drivers/media/video/saa7115.c |  180 ++++++++++++++++++-----------------------

 1 files changed, 80 insertions(+), 100 deletions(-)

drivers/media/video/saa7115.c +14 -46

index 72eaa66..8db056f 100644
@@ -45,6 +45,7 @@
  #include <linux/i2c.h>
  #include <linux/videodev2.h>
  #include <media/v4l2-device.h>
+#include <media/v4l2-ctrls.h>
  #include <media/v4l2-chip-ident.h>
  #include <media/v4l2-i2c-drv.h>
  #include <media/saa7115.h>

@@ -65,16 +66,17 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 
  struct saa711x_state {
        struct v4l2_subdev sd;
+       struct v4l2_ctrl_handler hdl;
+
+       /* chroma gain control cluster */
+       struct v4l2_ctrl *agc;
+       struct v4l2_ctrl *gain;
+
        v4l2_std_id std;
        int input;
        int output;
        int enable;
        int radio;
-       int bright;
-       int contrast;
-       int hue;
-       int sat;
-       int chroma_agc;
        int width;
        int height;
        u32 ident;

@@ -90,6 +92,11 @@ static inline struct saa711x_state *to_state(struct v4l2_subdev *sd)
        return container_of(sd, struct saa711x_state, sd);
  }
 
+static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
+{
+       return &container_of(ctrl->handler, struct saa711x_state, hdl)->sd;
+}
+
  /* ----------------------------------------------------------------------- */
 
  static inline int saa711x_write(struct v4l2_subdev *sd, u8 reg, u8 value)

@@ -741,96 +748,53 @@ static int saa711x_s_clock_freq(struct v4l2_subdev *sd, u32 freq)
        return 0;
  }
 
-static int saa711x_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int saa711x_g_ctrl(struct v4l2_ctrl *ctrl)
  {
+       struct v4l2_subdev *sd = to_sd(ctrl);
        struct saa711x_state *state = to_state(sd);
-       u8 val;
 
        switch (ctrl->id) {
-       case V4L2_CID_BRIGHTNESS:
-               if (ctrl->value < 0 || ctrl->value > 255) {
-                       v4l2_err(sd, "invalid brightness setting %d\n", ctrl->value);
-                       return -ERANGE;
-               }
-
-               state->bright = ctrl->value;
-               saa711x_write(sd, R_0A_LUMA_BRIGHT_CNTL, state->bright);
-               break;
-
-       case V4L2_CID_CONTRAST:
-               if (ctrl->value < 0 || ctrl->value > 127) {
-                       v4l2_err(sd, "invalid contrast setting %d\n", ctrl->value);
-                       return -ERANGE;
-               }
-
-               state->contrast = ctrl->value;
-               saa711x_write(sd, R_0B_LUMA_CONTRAST_CNTL, state->contrast);
-               break;
-
-       case V4L2_CID_SATURATION:
-               if (ctrl->value < 0 || ctrl->value > 127) {
-                       v4l2_err(sd, "invalid saturation setting %d\n", ctrl->value);
-                       return -ERANGE;
-               }
-
-               state->sat = ctrl->value;
-               saa711x_write(sd, R_0C_CHROMA_SAT_CNTL, state->sat);
-               break;
-
-       case V4L2_CID_HUE:
-               if (ctrl->value < -128 || ctrl->value > 127) {
-                       v4l2_err(sd, "invalid hue setting %d\n", ctrl->value);
-                       return -ERANGE;
-               }
-
-               state->hue = ctrl->value;
-               saa711x_write(sd, R_0D_CHROMA_HUE_CNTL, state->hue);
-               break;

       case V4L2_CID_CHROMA_AGC:
-               val = saa711x_read(sd, R_0F_CHROMA_GAIN_CNTL);
-               state->chroma_agc = ctrl->value;
-               if (ctrl->value)
-                       val &= 0x7f;
-               else
-                       val |= 0x80;
-               saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL, val);
+               /* chroma gain cluster */
+               if (state->agc->cur.val)
+                       state->gain->cur.val =
+                               saa711x_read(sd, R_0F_CHROMA_GAIN_CNTL) & 0x7f; break;
-       case V4L2_CID_CHROMA_GAIN:
-               /* Chroma gain cannot be set when AGC is enabled */
-               if (state->chroma_agc == 1)
-                       return -EINVAL;
-               saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL, ctrl->value | 0x80);
-               break;
-       default:
-               return -EINVAL;
       }
-
       return 0;
 }

drivers/media/video/saa7115.c +1 -1

-static int saa711x_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int saa711x_s_ctrl(struct v4l2_ctrl *ctrl)

 {
+       struct v4l2_subdev *sd = to_sd(ctrl); struct saa711x_state *state = to_state(sd);

       switch (ctrl->id) {
       case V4L2_CID_BRIGHTNESS:
-               ctrl->value = state->bright;
+               saa711x_write(sd, R_0A_LUMA_BRIGHT_CNTL, ctrl->val); break;
+
       case V4L2_CID_CONTRAST:
-               ctrl->value = state->contrast;
+               saa711x_write(sd, R_0B_LUMA_CONTRAST_CNTL, ctrl->val); break;
+
       case V4L2_CID_SATURATION:
-               ctrl->value = state->sat;
+               saa711x_write(sd, R_0C_CHROMA_SAT_CNTL, ctrl->val); break;
+
       case V4L2_CID_HUE:
-               ctrl->value = state->hue;
+               saa711x_write(sd, R_0D_CHROMA_HUE_CNTL, ctrl->val); break;
+
       case V4L2_CID_CHROMA_AGC:
-               ctrl->value = state->chroma_agc;
-               break;
-       case V4L2_CID_CHROMA_GAIN:
-               ctrl->value = saa711x_read(sd, R_0F_CHROMA_GAIN_CNTL) & 0x7f;
+               /* chroma gain cluster */
+               if (state->agc->val)
+                       saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL, state->gain->val);