[RFCv1 API PATCH 1/4] Two fixes and two v4l2-ctrl enhancements

[RFCv1 API PATCH 1/4] Two fixes and two v4l2-ctrl enhancements

[Hans Verkuil]

Hi all,

The first and last patches are bug fixes, and the second and third add
two new features to the control framework:

The first new feature adds a notifier to a control. When set the notifier
will be called whenever the control changes value. This feature is needed
to allow bridge drivers to detect changes in controls of a subdevice,
even if those controls are private to the subdevice. It does for kernel
drivers what the V4L2 event API does for userspace.

This functionality is initially required for the em28xx conversion to
the control framework, but it is also required for drivers that have to
deal with e.g. HDMI connectors with all the hotplug etc. events.

The second feature adds a filter function to the v4l2_ctrl_add_handler
function that allows you to select more precisely which controls you
want to add.

The primary purpose is to add only the audio controls to a control handler
for a radio device. Currently you will also see the video controls when
listing controls from the radio device of a combine tv/radio card and with
this filter function it is easy to fix that.

Comments are welcome!

Regards,

	Hans

[RFCv1 API PATCH 1/4] vb2: fix wrong owner check

[Hans Verkuil]

Check against q->fileio to see if the queue owner should be set or not.
The former check against the return value of read or write is wrong, since
read/write can return an error, even if the queue is in streaming mode.
For example, EAGAIN when in non-blocking mode.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 4da3df6..59ed522 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2278,7 +2278,7 @@ ssize_t vb2_fop_write(struct file *file, char __user *buf,
 		goto exit;
 	err = vb2_write(vdev->queue, buf, count, ppos,
 		       file->f_flags & O_NONBLOCK);
-	if (err >= 0)
+	if (vdev->queue->fileio)
 		vdev->queue->owner = file->private_data;
 exit:
 	if (lock)
@@ -2300,7 +2300,7 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf,
 		goto exit;
 	err = vb2_read(vdev->queue, buf, count, ppos,
 		       file->f_flags & O_NONBLOCK);
-	if (err >= 0)
+	if (vdev->queue->fileio)
 		vdev->queue->owner = file->private_data;
 exit:
 	if (lock)
-- 
1.7.10.4

[RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.

[Hans Verkuil]

Sometimes platform/bridge drivers need to be notified when a control from
a subdevice changes value. In order to support this a notify callback was
added.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/video4linux/v4l2-controls.txt |   22 ++++++++++++++--------
 drivers/media/v4l2-core/v4l2-ctrls.c        |   25 +++++++++++++++++++++++++
 include/media/v4l2-ctrls.h                  |   25 +++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 43da22b..cecaff8 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -687,14 +687,20 @@ a control of this type whenever the first control belonging to a new control
 class is added.
 
 
-Proposals for Extensions
-========================
+Adding Notify Callbacks
+=======================
+
+Sometimes the platform or bridge driver needs to be notified when a control
+from a sub-device driver changes. You can set a notify callback by calling
+this function:
 
-Some ideas for future extensions to the spec:
+void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl,
+	void (*notify)(struct v4l2_ctrl *ctrl, void *priv), void *priv);
 
-1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
-decimal. Useful for e.g. video_mute_yuv.
+Whenever the give control changes value the notify callback will be called
+with a pointer to the control and the priv pointer that was passed with
+v4l2_ctrl_notify. Note that the control's handler lock is held when the
+notify function is called.
 
-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
-control ID. Not sure if it is worth the effort, though.
+There can be only one notify function per control handler. Any attempt
+to set another notify function will cause a WARN_ON.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index f400035..43061e1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1160,6 +1160,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 		send_event(fh, ctrl,
 			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
 			(update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
+		if (ctrl->call_notify && changed && ctrl->handler->notify)
+			ctrl->handler->notify(ctrl, ctrl->handler->notify_priv);
 	}
 }
 
@@ -2628,6 +2630,29 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
 
+void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv)
+{
+	if (ctrl == NULL)
+		return;
+	if (notify == NULL) {
+		ctrl->call_notify = 0;
+		return;
+	}
+	/* Only one notifier is allowed. Should we ever need to support
+	   multiple notifiers, then some sort of linked list of notifiers
+	   should be implemented. But I don't see any real reason to implement
+	   that now. If you think you need multiple notifiers, then contact
+	   the linux-media mailinglist. */
+	if (WARN_ON(ctrl->handler->notify &&
+			(ctrl->handler->notify != notify ||
+			 ctrl->handler->notify_priv != priv)))
+		return;
+	ctrl->handler->notify = notify;
+	ctrl->handler->notify_priv = priv;
+	ctrl->call_notify = 1;
+}
+EXPORT_SYMBOL(v4l2_ctrl_notify);
+
 static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 6890f5e..4484fd3 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -53,6 +53,8 @@ struct v4l2_ctrl_ops {
 	int (*s_ctrl)(struct v4l2_ctrl *ctrl);
 };
 
+typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
+
 /** struct v4l2_ctrl - The control structure.
   * @node:	The list node.
   * @ev_subs:	The list of control event subscriptions.
@@ -72,6 +74,8 @@ struct v4l2_ctrl_ops {
   *		set this flag directly.
   * @has_volatiles: If set, then one or more members of the cluster are volatile.
   *		Drivers should never touch this flag.
+  * @call_notify: If set, then call the handler's notify function whenever the
+  *		control's value changes.
   * @manual_mode_value: If the is_auto flag is set, then this is the value
   *		of the auto control that determines if that control is in
   *		manual mode. So if the value of the auto control equals this
@@ -119,6 +123,7 @@ struct v4l2_ctrl {
 	unsigned int is_private:1;
 	unsigned int is_auto:1;
 	unsigned int has_volatiles:1;
+	unsigned int call_notify:1;
 	unsigned int manual_mode_value:8;
 
 	const struct v4l2_ctrl_ops *ops;
@@ -177,6 +182,10 @@ struct v4l2_ctrl_ref {
   *		control is needed multiple times, so this is a simple
   *		optimization.
   * @buckets:	Buckets for the hashing. Allows for quick control lookup.
+  * @notify:	A notify callback that is called whenever the control changes value.
+  *		Note that the handler's lock is held when the notify function
+  *		is called!
+  * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
   * @nr_of_buckets: Total number of buckets in the array.
   * @error:	The error code of the first failed control addition.
   */
@@ -187,6 +196,8 @@ struct v4l2_ctrl_handler {
 	struct list_head ctrl_refs;
 	struct v4l2_ctrl_ref *cached;
 	struct v4l2_ctrl_ref **buckets;
+	v4l2_ctrl_notify_fnc notify;
+	void *notify_priv;
 	u16 nr_of_buckets;
 	int error;
 };
@@ -488,6 +499,20 @@ static inline void v4l2_ctrl_unlock(struct v4l2_ctrl *ctrl)
 	mutex_unlock(ctrl->handler->lock);
 }
 
+/** v4l2_ctrl_notify() - Function to set a notify callback for a control.
+  * @ctrl:	The control.
+  * @notify:	The callback function.
+  * @priv:	The callback private handle, passed as argument to the callback.
+  *
+  * This function sets a callback function for the control. If @ctrl is NULL,
+  * then it will do nothing. If @notify is NULL, then the notify callback will
+  * be removed.
+  *
+  * There can be only one notify. If another already exists, then a WARN_ON
+  * will be issued and the function will do nothing.
+  */
+void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv);
+
 /** v4l2_ctrl_g_ctrl() - Helper function to get the control's value from within a driver.
   * @ctrl:	The control.
   *
-- 
1.7.10.4

Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Friday 14 September 2012 13:15:34 Hans Verkuil wrote:
> Sometimes platform/bridge drivers need to be notified when a control from
> a subdevice changes value. In order to support this a notify callback was
> added.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  Documentation/video4linux/v4l2-controls.txt |   22 ++++++++++++++--------
>  drivers/media/v4l2-core/v4l2-ctrls.c        |   25 ++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h                  |   25 ++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/video4linux/v4l2-controls.txt
> b/Documentation/video4linux/v4l2-controls.txt index 43da22b..cecaff8 100644
> --- a/Documentation/video4linux/v4l2-controls.txt
> +++ b/Documentation/video4linux/v4l2-controls.txt
> @@ -687,14 +687,20 @@ a control of this type whenever the first control
> belonging to a new control class is added.
> 
> 
> -Proposals for Extensions
> -========================
> +Adding Notify Callbacks
> +=======================
> +
> +Sometimes the platform or bridge driver needs to be notified when a control
> +from a sub-device driver changes. You can set a notify callback by calling
> +this function:
> 
> -Some ideas for future extensions to the spec:
> +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl,
> +	void (*notify)(struct v4l2_ctrl *ctrl, void *priv), void *priv);
> 
> -1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
> -decimal. Useful for e.g. video_mute_yuv.
> +Whenever the give control changes value the notify callback will be called
> +with a pointer to the control and the priv pointer that was passed with
> +v4l2_ctrl_notify. Note that the control's handler lock is held when the
> +notify function is called.
> 
> -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
> -control ID. Not sure if it is worth the effort, though.
> +There can be only one notify function per control handler. Any attempt
> +to set another notify function will cause a WARN_ON.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index f400035..43061e1 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1160,6 +1160,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct
> v4l2_ctrl *ctrl, send_event(fh, ctrl,
>  			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
>  			(update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> +		if (ctrl->call_notify && changed && ctrl->handler->notify)
> +			ctrl->handler->notify(ctrl, ctrl->handler->notify_priv);
>  	}
>  }
> 
> @@ -2628,6 +2630,29 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl,
> s64 val) }
>  EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
> 
> +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
> void *priv)
> +{
> +	if (ctrl == NULL)
> +		return;

Isn't the caller supposed not to set ctrl to NULL ? A crash is easier to 
notice than a silent failure during development.

> +	if (notify == NULL) {
> +		ctrl->call_notify = 0;
> +		return;
> +	}
> +	/* Only one notifier is allowed. Should we ever need to support
> +	   multiple notifiers, then some sort of linked list of notifiers
> +	   should be implemented. But I don't see any real reason to implement
> +	   that now. If you think you need multiple notifiers, then contact
> +	   the linux-media mailinglist. */
> +	if (WARN_ON(ctrl->handler->notify &&
> +			(ctrl->handler->notify != notify ||
> +			 ctrl->handler->notify_priv != priv)))
> +		return;

I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be 
better to register the notifier with the handler explictly just once and then 
enable/disable notifications on a per-control basis ?

> +	ctrl->handler->notify = notify;
> +	ctrl->handler->notify_priv = priv;
> +	ctrl->call_notify = 1;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_notify);
> +
>  static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned
> elems) {
>  	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 6890f5e..4484fd3 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -53,6 +53,8 @@ struct v4l2_ctrl_ops {
>  	int (*s_ctrl)(struct v4l2_ctrl *ctrl);
>  };
> 
> +typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> +
>  /** struct v4l2_ctrl - The control structure.
>    * @node:	The list node.
>    * @ev_subs:	The list of control event subscriptions.
> @@ -72,6 +74,8 @@ struct v4l2_ctrl_ops {
>    *		set this flag directly.
>    * @has_volatiles: If set, then one or more members of the cluster are
> volatile. *		Drivers should never touch this flag.
> +  * @call_notify: If set, then call the handler's notify function whenever
> the +  *		control's value changes.
>    * @manual_mode_value: If the is_auto flag is set, then this is the value
>    *		of the auto control that determines if that control is in
>    *		manual mode. So if the value of the auto control equals this
> @@ -119,6 +123,7 @@ struct v4l2_ctrl {
>  	unsigned int is_private:1;
>  	unsigned int is_auto:1;
>  	unsigned int has_volatiles:1;
> +	unsigned int call_notify:1;
>  	unsigned int manual_mode_value:8;
> 
>  	const struct v4l2_ctrl_ops *ops;
> @@ -177,6 +182,10 @@ struct v4l2_ctrl_ref {
>    *		control is needed multiple times, so this is a simple
>    *		optimization.
>    * @buckets:	Buckets for the hashing. Allows for quick control lookup.
> +  * @notify:	A notify callback that is called whenever the control 
changes
> value. +  *		Note that the handler's lock is held when the notify 
function
> +  *		is called!
> +  * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
>    * @nr_of_buckets: Total number of buckets in the array.
>    * @error:	The error code of the first failed control addition.
>    */
> @@ -187,6 +196,8 @@ struct v4l2_ctrl_handler {
>  	struct list_head ctrl_refs;
>  	struct v4l2_ctrl_ref *cached;
>  	struct v4l2_ctrl_ref **buckets;
> +	v4l2_ctrl_notify_fnc notify;
> +	void *notify_priv;
>  	u16 nr_of_buckets;
>  	int error;
>  };
> @@ -488,6 +499,20 @@ static inline void v4l2_ctrl_unlock(struct v4l2_ctrl
> *ctrl) mutex_unlock(ctrl->handler->lock);
>  }
> 
> +/** v4l2_ctrl_notify() - Function to set a notify callback for a control.
> +  * @ctrl:	The control.
> +  * @notify:	The callback function.
> +  * @priv:	The callback private handle, passed as argument to the callback.
> +  *
> +  * This function sets a callback function for the control. If @ctrl is
> NULL, +  * then it will do nothing. If @notify is NULL, then the notify
> callback will +  * be removed.
> +  *
> +  * There can be only one notify. If another already exists, then a WARN_ON
> +  * will be issued and the function will do nothing.
> +  */
> +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
> void *priv); +
>  /** v4l2_ctrl_g_ctrl() - Helper function to get the control's value from
> within a driver. * @ctrl:	The control.
>    *
-- 
Regards,

Laurent Pinchart

Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.

[Hans Verkuil]

On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Friday 14 September 2012 13:15:34 Hans Verkuil wrote:
> > Sometimes platform/bridge drivers need to be notified when a control from
> > a subdevice changes value. In order to support this a notify callback was
> > added.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  Documentation/video4linux/v4l2-controls.txt |   22 ++++++++++++++--------
> >  drivers/media/v4l2-core/v4l2-ctrls.c        |   25 ++++++++++++++++++++++++
> >  include/media/v4l2-ctrls.h                  |   25 ++++++++++++++++++++++++
> >  3 files changed, 64 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/video4linux/v4l2-controls.txt
> > b/Documentation/video4linux/v4l2-controls.txt index 43da22b..cecaff8 100644
> > --- a/Documentation/video4linux/v4l2-controls.txt
> > +++ b/Documentation/video4linux/v4l2-controls.txt
> > @@ -687,14 +687,20 @@ a control of this type whenever the first control
> > belonging to a new control class is added.
> > 
> > 
> > -Proposals for Extensions
> > -========================
> > +Adding Notify Callbacks
> > +=======================
> > +
> > +Sometimes the platform or bridge driver needs to be notified when a control
> > +from a sub-device driver changes. You can set a notify callback by calling
> > +this function:
> > 
> > -Some ideas for future extensions to the spec:
> > +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl,
> > +	void (*notify)(struct v4l2_ctrl *ctrl, void *priv), void *priv);
> > 
> > -1) Add a V4L2_CTRL_FLAG_HEX to have values shown as hexadecimal instead of
> > -decimal. Useful for e.g. video_mute_yuv.
> > +Whenever the give control changes value the notify callback will be called
> > +with a pointer to the control and the priv pointer that was passed with
> > +v4l2_ctrl_notify. Note that the control's handler lock is held when the
> > +notify function is called.
> > 
> > -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
> > -control ID. Not sure if it is worth the effort, though.
> > +There can be only one notify function per control handler. Any attempt
> > +to set another notify function will cause a WARN_ON.
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> > b/drivers/media/v4l2-core/v4l2-ctrls.c index f400035..43061e1 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1160,6 +1160,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct
> > v4l2_ctrl *ctrl, send_event(fh, ctrl,
> >  			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
> >  			(update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> > +		if (ctrl->call_notify && changed && ctrl->handler->notify)
> > +			ctrl->handler->notify(ctrl, ctrl->handler->notify_priv);
> >  	}
> >  }
> > 
> > @@ -2628,6 +2630,29 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl,
> > s64 val) }
> >  EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
> > 
> > +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
> > void *priv)
> > +{
> > +	if (ctrl == NULL)
> > +		return;
> 
> Isn't the caller supposed not to set ctrl to NULL ? A crash is easier to 
> notice than a silent failure during development.

The reason why I do this (not only here, but in other places in the control
framework as well), is that it simplifies driver development if you have a
driver that adds controls based on the chip version. In cases like that you
only have to look at the chip version when adding the controls, but after
that you can use almost all functions without having to check the control
pointer every time. I.e., if the control wasn't added, then the control
pointer would be NULL and all these functions would be nops.

> > +	if (notify == NULL) {
> > +		ctrl->call_notify = 0;
> > +		return;
> > +	}
> > +	/* Only one notifier is allowed. Should we ever need to support
> > +	   multiple notifiers, then some sort of linked list of notifiers
> > +	   should be implemented. But I don't see any real reason to implement
> > +	   that now. If you think you need multiple notifiers, then contact
> > +	   the linux-media mailinglist. */
> > +	if (WARN_ON(ctrl->handler->notify &&
> > +			(ctrl->handler->notify != notify ||
> > +			 ctrl->handler->notify_priv != priv)))
> > +		return;
> 
> I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be 
> better to register the notifier with the handler explictly just once and then 
> enable/disable notifications on a per-control basis ?

I thought about that, but I prefer this method because it allows me to switch
to per-control notifiers in the future. In addition, different controls can have
different handlers. If you have to set the notifier for handlers, then the
driver needs to figure out which handlers are involved for the controls it wants
to be notified on. It's much easier to do it like this.

Regards,

	Hans

> > +	ctrl->handler->notify = notify;
> > +	ctrl->handler->notify_priv = priv;
> > +	ctrl->call_notify = 1;
> > +}
> > +EXPORT_SYMBOL(v4l2_ctrl_notify);
> > +
> >  static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned
> > elems) {
> >  	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 6890f5e..4484fd3 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -53,6 +53,8 @@ struct v4l2_ctrl_ops {
> >  	int (*s_ctrl)(struct v4l2_ctrl *ctrl);
> >  };
> > 
> > +typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> > +
> >  /** struct v4l2_ctrl - The control structure.
> >    * @node:	The list node.
> >    * @ev_subs:	The list of control event subscriptions.
> > @@ -72,6 +74,8 @@ struct v4l2_ctrl_ops {
> >    *		set this flag directly.
> >    * @has_volatiles: If set, then one or more members of the cluster are
> > volatile. *		Drivers should never touch this flag.
> > +  * @call_notify: If set, then call the handler's notify function whenever
> > the +  *		control's value changes.
> >    * @manual_mode_value: If the is_auto flag is set, then this is the value
> >    *		of the auto control that determines if that control is in
> >    *		manual mode. So if the value of the auto control equals this
> > @@ -119,6 +123,7 @@ struct v4l2_ctrl {
> >  	unsigned int is_private:1;
> >  	unsigned int is_auto:1;
> >  	unsigned int has_volatiles:1;
> > +	unsigned int call_notify:1;
> >  	unsigned int manual_mode_value:8;
> > 
> >  	const struct v4l2_ctrl_ops *ops;
> > @@ -177,6 +182,10 @@ struct v4l2_ctrl_ref {
> >    *		control is needed multiple times, so this is a simple
> >    *		optimization.
> >    * @buckets:	Buckets for the hashing. Allows for quick control lookup.
> > +  * @notify:	A notify callback that is called whenever the control 
> changes
> > value. +  *		Note that the handler's lock is held when the notify 
> function
> > +  *		is called!
> > +  * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
> >    * @nr_of_buckets: Total number of buckets in the array.
> >    * @error:	The error code of the first failed control addition.
> >    */
> > @@ -187,6 +196,8 @@ struct v4l2_ctrl_handler {
> >  	struct list_head ctrl_refs;
> >  	struct v4l2_ctrl_ref *cached;
> >  	struct v4l2_ctrl_ref **buckets;
> > +	v4l2_ctrl_notify_fnc notify;
> > +	void *notify_priv;
> >  	u16 nr_of_buckets;
> >  	int error;
> >  };
> > @@ -488,6 +499,20 @@ static inline void v4l2_ctrl_unlock(struct v4l2_ctrl
> > *ctrl) mutex_unlock(ctrl->handler->lock);
> >  }
> > 
> > +/** v4l2_ctrl_notify() - Function to set a notify callback for a control.
> > +  * @ctrl:	The control.
> > +  * @notify:	The callback function.
> > +  * @priv:	The callback private handle, passed as argument to the callback.
> > +  *
> > +  * This function sets a callback function for the control. If @ctrl is
> > NULL, +  * then it will do nothing. If @notify is NULL, then the notify
> > callback will +  * be removed.
> > +  *
> > +  * There can be only one notify. If another already exists, then a WARN_ON
> > +  * will be issued and the function will do nothing.
> > +  */
> > +void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify,
> > void *priv); +
> >  /** v4l2_ctrl_g_ctrl() - Helper function to get the control's value from
> > within a driver. * @ctrl:	The control.
> >    *
> 

Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.

[Mauro Carvalho Chehab]

Em Thu, 27 Sep 2012 08:44:25 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:

> > > +	if (notify == NULL) {
> > > +		ctrl->call_notify = 0;
> > > +		return;
> > > +	}
> > > +	/* Only one notifier is allowed. Should we ever need to support
> > > +	   multiple notifiers, then some sort of linked list of notifiers
> > > +	   should be implemented. But I don't see any real reason to implement
> > > +	   that now. If you think you need multiple notifiers, then contact
> > > +	   the linux-media mailinglist. */

If only one notifier is allowed, then you should clearly state that at the
API documentation.

> > > +	if (WARN_ON(ctrl->handler->notify &&
> > > +			(ctrl->handler->notify != notify ||
> > > +			 ctrl->handler->notify_priv != priv)))
> > > +		return;
> > 
> > I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be 
> > better to register the notifier with the handler explictly just once and then 
> > enable/disable notifications on a per-control basis ?
> 
> I thought about that, but I prefer this method because it allows me to switch
> to per-control notifiers in the future. In addition, different controls can have
> different handlers. If you have to set the notifier for handlers, then the
> driver needs to figure out which handlers are involved for the controls it wants
> to be notified on. It's much easier to do it like this.

That also sounded hackish on my eyes. If just one notifier is allowed, the
function should simply refuse any other call to it, as any other call to it
is a driver's bug. So:

	if (WARN_ON(ctrl->handler->notify))
		return;

seems to be enough.

Regards,
Mauro

Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.

[Hans Verkuil]

On Mon October 1 2012 22:01:38 Mauro Carvalho Chehab wrote:
> Em Thu, 27 Sep 2012 08:44:25 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:
> 
> > > > +	if (notify == NULL) {
> > > > +		ctrl->call_notify = 0;
> > > > +		return;
> > > > +	}
> > > > +	/* Only one notifier is allowed. Should we ever need to support
> > > > +	   multiple notifiers, then some sort of linked list of notifiers
> > > > +	   should be implemented. But I don't see any real reason to implement
> > > > +	   that now. If you think you need multiple notifiers, then contact
> > > > +	   the linux-media mailinglist. */
> 
> If only one notifier is allowed, then you should clearly state that at the
> API documentation.

Well, v4l2-controls.txt says:

"There can be only one notify function per control handler. Any attempt
to set another notify function will cause a WARN_ON."

And it is documented as well in the header, so what more do you want?

> > > > +	if (WARN_ON(ctrl->handler->notify &&
> > > > +			(ctrl->handler->notify != notify ||
> > > > +			 ctrl->handler->notify_priv != priv)))
> > > > +		return;
> > > 
> > > I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be 
> > > better to register the notifier with the handler explictly just once and then 
> > > enable/disable notifications on a per-control basis ?
> > 
> > I thought about that, but I prefer this method because it allows me to switch
> > to per-control notifiers in the future. In addition, different controls can have
> > different handlers. If you have to set the notifier for handlers, then the
> > driver needs to figure out which handlers are involved for the controls it wants
> > to be notified on. It's much easier to do it like this.
> 
> That also sounded hackish on my eyes. If just one notifier is allowed, the
> function should simply refuse any other call to it, as any other call to it
> is a driver's bug. So:
> 
> 	if (WARN_ON(ctrl->handler->notify))
> 		return;
> 
> seems to be enough.

No, it's not. Multiple controls share the same handler, so this would only work
for the first control and block any attempts to set the notifier for other
controls of the same handler.

The point is that in today's implementation the notifier is stored in the handler
because that was the easiest implementation and because it is all we need today.
But in the future we may have to support different notifiers and also more than
one notifier per control, and in that case the notifier would move to the control
data structure.

I want to be able to make such a change without having to change all drivers that
use today's implementation.

The API for setting a notifier should act on a control, not on a control handler.
The 'hack' above is just a check that ensures that you don't violate the constraints
of the current implementation. If anyone hits that warning and contacts the ml, then
I can see if there are good enough reasons to go the extra mile and remove this
constraint.

So I really don't want to change this patch.

Regards,

	Hans

[RFCv1 API PATCH 3/4] v4l2-ctrls: add a filter function to v4l2_ctrl_add_handler.

[Hans Verkuil]

With a filter function you can control more precisely which controls
are added. This is useful in particular for radio device nodes for
combined TV/Radio cards where you want to show just the radio-specific
controls and not controls like brightness.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/video4linux/v4l2-controls.txt    |    6 +++++-
 drivers/media/pci/cx88/cx88-blackbird.c        |    2 +-
 drivers/media/pci/cx88/cx88-video.c            |    2 +-
 drivers/media/platform/s5p-fimc/fimc-capture.c |    2 +-
 drivers/media/platform/soc_camera/soc_camera.c |    2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c           |   25 +++++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-device.c          |    2 +-
 include/media/v4l2-ctrls.h                     |   18 +++++++++++++++--
 8 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index cecaff8..cd4a26c 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -594,7 +594,11 @@ handler and finally add the first handler to the second. For example:
 	v4l2_ctrl_new_std(&radio_ctrl_handler, &radio_ops, V4L2_CID_AUDIO_MUTE, ...);
 	v4l2_ctrl_new_std(&video_ctrl_handler, &video_ops, V4L2_CID_BRIGHTNESS, ...);
 	v4l2_ctrl_new_std(&video_ctrl_handler, &video_ops, V4L2_CID_CONTRAST, ...);
-	v4l2_ctrl_add_handler(&video_ctrl_handler, &radio_ctrl_handler);
+	v4l2_ctrl_add_handler(&video_ctrl_handler, &radio_ctrl_handler, NULL);
+
+The last argument to v4l2_ctrl_add_handler() is a filter function that allows
+you to filter which controls will be added. Set it to NULL if you want to add
+all controls.
 
 Or you can add specific controls to a handler:
 
diff --git a/drivers/media/pci/cx88/cx88-blackbird.c b/drivers/media/pci/cx88/cx88-blackbird.c
index 843ffd9..def363f 100644
--- a/drivers/media/pci/cx88/cx88-blackbird.c
+++ b/drivers/media/pci/cx88/cx88-blackbird.c
@@ -1236,7 +1236,7 @@ static int cx8802_blackbird_probe(struct cx8802_driver *drv)
 	err = cx2341x_handler_init(&dev->cxhdl, 36);
 	if (err)
 		goto fail_core;
-	v4l2_ctrl_add_handler(&dev->cxhdl.hdl, &core->video_hdl);
+	v4l2_ctrl_add_handler(&dev->cxhdl.hdl, &core->video_hdl, NULL);
 
 	/* blackbird stuff */
 	printk("%s/2: cx23416 based mpeg encoder (blackbird reference design)\n",
diff --git a/drivers/media/pci/cx88/cx88-video.c b/drivers/media/pci/cx88/cx88-video.c
index f6fcc7e..a146d50 100644
--- a/drivers/media/pci/cx88/cx88-video.c
+++ b/drivers/media/pci/cx88/cx88-video.c
@@ -1795,7 +1795,7 @@ static int __devinit cx8800_initdev(struct pci_dev *pci_dev,
 		if (vc->id == V4L2_CID_CHROMA_AGC)
 			core->chroma_agc = vc;
 	}
-	v4l2_ctrl_add_handler(&core->video_hdl, &core->audio_hdl);
+	v4l2_ctrl_add_handler(&core->video_hdl, &core->audio_hdl, NULL);
 
 	/* load and configure helper modules */
 
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index 8e413dd..dde0901 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -472,7 +472,7 @@ int fimc_capture_ctrls_create(struct fimc_dev *fimc)
 		return ret;
 
 	return v4l2_ctrl_add_handler(&vid_cap->ctx->ctrls.handler,
-		    fimc->pipeline.subdevs[IDX_SENSOR]->ctrl_handler);
+		    fimc->pipeline.subdevs[IDX_SENSOR]->ctrl_handler, NULL);
 }
 
 static int fimc_capture_set_default_format(struct fimc_dev *fimc);
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f6b1c1f..3be9294 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1184,7 +1184,7 @@ static int soc_camera_probe(struct soc_camera_device *icd)
 	sd->grp_id = soc_camera_grp_id(icd);
 	v4l2_set_subdev_hostdata(sd, icd);
 
-	if (v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler))
+	if (v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler, NULL))
 		goto ectrl;
 
 	/* At this point client .probe() should have run already */
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 43061e1..b1b4660 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1689,7 +1689,8 @@ EXPORT_SYMBOL(v4l2_ctrl_add_ctrl);
 
 /* Add the controls from another handler to our own. */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
-			  struct v4l2_ctrl_handler *add)
+			  struct v4l2_ctrl_handler *add,
+			  bool (*filter)(const struct v4l2_ctrl *ctrl))
 {
 	struct v4l2_ctrl_ref *ref;
 	int ret = 0;
@@ -1709,6 +1710,9 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 		/* And control classes */
 		if (ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
 			continue;
+		/* Filter any unwanted controls */
+		if (filter && !filter(ctrl))
+			continue;
 		ret = handler_new_ref(hdl, ctrl);
 		if (ret)
 			break;
@@ -1718,6 +1722,25 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_add_handler);
 
+bool v4l2_ctrl_radio_filter(const struct v4l2_ctrl *ctrl)
+{
+	if (V4L2_CTRL_ID2CLASS(ctrl->id) == V4L2_CTRL_CLASS_FM_TX)
+		return true;
+	switch (ctrl->id) {
+	case V4L2_CID_AUDIO_MUTE:
+	case V4L2_CID_AUDIO_VOLUME:
+	case V4L2_CID_AUDIO_BALANCE:
+	case V4L2_CID_AUDIO_BASS:
+	case V4L2_CID_AUDIO_TREBLE:
+	case V4L2_CID_AUDIO_LOUDNESS:
+		return true;
+	default:
+		break;
+	}
+	return false;
+}
+EXPORT_SYMBOL(v4l2_ctrl_radio_filter);
+
 /* Cluster controls */
 void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
 {
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 1f203b8..513969f 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -166,7 +166,7 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 	}
 
 	/* This just returns 0 if either of the two args is NULL */
-	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler);
+	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL);
 	if (err) {
 		if (sd->internal_ops && sd->internal_ops->unregistered)
 			sd->internal_ops->unregistered(sd);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 4484fd3..2c486be 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -395,14 +395,28 @@ struct v4l2_ctrl *v4l2_ctrl_add_ctrl(struct v4l2_ctrl_handler *hdl,
   * @hdl:	The control handler.
   * @add:	The control handler whose controls you want to add to
   *		the @hdl control handler.
+  * @filter:	This function will filter which controls should be added.
   *
-  * Does nothing if either of the two is a NULL pointer.
+  * Does nothing if either of the two handlers is a NULL pointer.
+  * If @filter is NULL, then all controls are added. Otherwise only those
+  * controls for which @filter returns true will be added.
   * In case of an error @hdl->error will be set to the error code (if it
   * wasn't set already).
   */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
-			  struct v4l2_ctrl_handler *add);
+			  struct v4l2_ctrl_handler *add,
+			  bool (*filter)(const struct v4l2_ctrl *ctrl));
 
+/** v4l2_ctrl_radio_filter() - Standard filter for radio controls.
+  * @ctrl:	The control that is filtered.
+  *
+  * This will return true for any controls that are valid for radio device
+  * nodes. Those are all of the V4L2_CID_AUDIO_* user controls and all FM
+  * transmitter class controls.
+  *
+  * This function is to be used with v4l2_ctrl_add_handler().
+  */
+bool v4l2_ctrl_radio_filter(const struct v4l2_ctrl *ctrl);
 
 /** v4l2_ctrl_cluster() - Mark all controls in the cluster as belonging to that cluster.
   * @ncontrols:	The number of controls in this cluster.
-- 
1.7.10.4

[RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Hans Verkuil]

Fixes a v4l2-compliance error: setting audmode to a value other than mono
or stereo for a radio device should map to MODE_STEREO.

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

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index b5a819a..ea71371 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	if (set_mode(t, vt->type))
 		return 0;
 
-	if (t->mode == V4L2_TUNER_RADIO)
+	if (t->mode == V4L2_TUNER_RADIO) {
 		t->audmode = vt->audmode;
+		if (t->audmode > V4L2_TUNER_MODE_STEREO)
+			t->audmode = V4L2_TUNER_MODE_STEREO;
+	}
 	set_freq(t, 0);
 
 	return 0;
-- 
1.7.10.4

Re: [RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Mauro Carvalho Chehab]

Em Fri, 14 Sep 2012 13:15:36 +0200
Hans Verkuil <hans.verkuil@cisco.com> escreveu:

> Fixes a v4l2-compliance error: setting audmode to a value other than mono
> or stereo for a radio device should map to MODE_STEREO.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/tuner-core.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> index b5a819a..ea71371 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  	if (set_mode(t, vt->type))
>  		return 0;
>  
> -	if (t->mode == V4L2_TUNER_RADIO)
> +	if (t->mode == V4L2_TUNER_RADIO) {
>  		t->audmode = vt->audmode;
> +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> +			t->audmode = V4L2_TUNER_MODE_STEREO;

NACK. It is not a core's task to fix driver's bugs. It would be ok to have here a
WARN_ON(), but, if a driver is reporting a wrong radio audmode, the fix should be
there at the drivers, and not here at the core.

> +	}
>  	set_freq(t, 0);
>  
>  	return 0;


-- 
Regards,
Mauro

Re: [RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Hans Verkuil]

On Tue 25 September 2012 15:33:40 Mauro Carvalho Chehab wrote:
> Em Fri, 14 Sep 2012 13:15:36 +0200
> Hans Verkuil <hans.verkuil@cisco.com> escreveu:
> 
> > Fixes a v4l2-compliance error: setting audmode to a value other than mono
> > or stereo for a radio device should map to MODE_STEREO.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/v4l2-core/tuner-core.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> > index b5a819a..ea71371 100644
> > --- a/drivers/media/v4l2-core/tuner-core.c
> > +++ b/drivers/media/v4l2-core/tuner-core.c
> > @@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> >  	if (set_mode(t, vt->type))
> >  		return 0;
> >  
> > -	if (t->mode == V4L2_TUNER_RADIO)
> > +	if (t->mode == V4L2_TUNER_RADIO) {
> >  		t->audmode = vt->audmode;
> > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > +			t->audmode = V4L2_TUNER_MODE_STEREO;
> 
> NACK. It is not a core's task to fix driver's bugs. It would be ok to have here a
> WARN_ON(), but, if a driver is reporting a wrong radio audmode, the fix should be
> there at the drivers, and not here at the core.

tuner-core *is* the driver. A bridge driver just passes v4l2_tuner on to the
subdev driver(s), and it is the subdev driver such as tuner-core that needs to
process the audmode as specified by the user. Which in this case means mapping
audmodes that are invalid when in radio mode to something that is valid as per
the spec.

So this is a real tuner-core bug, not a bridge driver bug since they don't
generally touch the audmode field, they just pass it along. And the vt->audmode
value comes straight from userspace, not from the bridge driver.

Regards,

	Hans

Re: [RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Mauro Carvalho Chehab]

Em Tue, 25 Sep 2012 15:45:00 +0200
Hans Verkuil <hansverk@cisco.com> escreveu:

> On Tue 25 September 2012 15:33:40 Mauro Carvalho Chehab wrote:
> > Em Fri, 14 Sep 2012 13:15:36 +0200
> > Hans Verkuil <hans.verkuil@cisco.com> escreveu:
> > 
> > > Fixes a v4l2-compliance error: setting audmode to a value other than mono
> > > or stereo for a radio device should map to MODE_STEREO.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > >  drivers/media/v4l2-core/tuner-core.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> > > index b5a819a..ea71371 100644
> > > --- a/drivers/media/v4l2-core/tuner-core.c
> > > +++ b/drivers/media/v4l2-core/tuner-core.c
> > > @@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> > >  	if (set_mode(t, vt->type))
> > >  		return 0;
> > >  
> > > -	if (t->mode == V4L2_TUNER_RADIO)
> > > +	if (t->mode == V4L2_TUNER_RADIO) {
> > >  		t->audmode = vt->audmode;
> > > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > > +			t->audmode = V4L2_TUNER_MODE_STEREO;
> > 
> > NACK. It is not a core's task to fix driver's bugs. It would be ok to have here a
> > WARN_ON(), but, if a driver is reporting a wrong radio audmode, the fix should be
> > there at the drivers, and not here at the core.
> 
> tuner-core *is* the driver.

Not really... it is a driver's glue between the real I2C driver and the bridge
driver.

> A bridge driver just passes v4l2_tuner on to the
> subdev driver(s), and it is the subdev driver such as tuner-core that needs to
> process the audmode as specified by the user. Which in this case means mapping
> audmodes that are invalid when in radio mode to something that is valid as per
> the spec.

Well, when the user is requesting an invalid mode, it should just return -EINVAL.
It makes sense to add a check there at tuner-core to reject audmode if userspace
is requesting, for example, a second language[1]. 

[1] Yet, I think that digital audio standards allow more than one audio channels.
So, this may require to be pushed down into the drivers in some future.

What is invalid actually depends on the device. For example, AM ISA drivers
don't support stereo. Ok, all tuners supported by tuner-core are FM. Even so,
some of them may not support stereo[2].

[2] afaikt, some designs with tuner xc2028 don't support stereo. The driver currently
doesn't handle such border cases, but the point is that such checks should happen
at driver's level.

> So this is a real tuner-core bug, not a bridge driver bug since they don't
> generally touch the audmode field, they just pass it along. And the vt->audmode
> value comes straight from userspace, not from the bridge driver.

Well, tuner-core is just an unified way to talk to the real tuner device.
The is elsewhere (tuner-simple, tea5***, ...).

IMHO, the better is to put such fix at the real device, instead of hacking the
code with something that doesn't really belong there.

Regards,
Mauro

Re: [RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Hans Verkuil]

On Wed September 26 2012 04:29:33 Mauro Carvalho Chehab wrote:
> Em Tue, 25 Sep 2012 15:45:00 +0200
> Hans Verkuil <hansverk@cisco.com> escreveu:
> 
> > On Tue 25 September 2012 15:33:40 Mauro Carvalho Chehab wrote:
> > > Em Fri, 14 Sep 2012 13:15:36 +0200
> > > Hans Verkuil <hans.verkuil@cisco.com> escreveu:
> > > 
> > > > Fixes a v4l2-compliance error: setting audmode to a value other than mono
> > > > or stereo for a radio device should map to MODE_STEREO.
> > > > 
> > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > ---
> > > >  drivers/media/v4l2-core/tuner-core.c |    5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> > > > index b5a819a..ea71371 100644
> > > > --- a/drivers/media/v4l2-core/tuner-core.c
> > > > +++ b/drivers/media/v4l2-core/tuner-core.c
> > > > @@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> > > >  	if (set_mode(t, vt->type))
> > > >  		return 0;
> > > >  
> > > > -	if (t->mode == V4L2_TUNER_RADIO)
> > > > +	if (t->mode == V4L2_TUNER_RADIO) {
> > > >  		t->audmode = vt->audmode;
> > > > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > > > +			t->audmode = V4L2_TUNER_MODE_STEREO;
> > > 
> > > NACK. It is not a core's task to fix driver's bugs. It would be ok to have here a
> > > WARN_ON(), but, if a driver is reporting a wrong radio audmode, the fix should be
> > > there at the drivers, and not here at the core.
> > 
> > tuner-core *is* the driver.
> 
> Not really... it is a driver's glue between the real I2C driver and the bridge
> driver.
> 
> > A bridge driver just passes v4l2_tuner on to the
> > subdev driver(s), and it is the subdev driver such as tuner-core that needs to
> > process the audmode as specified by the user. Which in this case means mapping
> > audmodes that are invalid when in radio mode to something that is valid as per
> > the spec.
> 
> Well, when the user is requesting an invalid mode, it should just return -EINVAL.
> It makes sense to add a check there at tuner-core to reject audmode if userspace
> is requesting, for example, a second language[1]. 

My interpretation of the spec is that it will map invalid audmodes to valid audmodes.
>From the VIDIOC_S_TUNER documentation:

"The selected audio mode, see Table A.89, “Tuner Audio Modes” for valid values. The
audio mode does not affect audio subprogram detection, and like a control it does not
automatically change unless the requested mode is invalid or unsupported. See Table
A.90, “Tuner Audio Matrix” for possible results when the selected and received audio
programs do not match."

So my interpretation is that if an audmode is provided that is not valid for the
given device, then the device maps it to something valid rather than returning an
error. The error code list only states that -EINVAL is returned if the index field
is out-of-bounds, not for invalid audmodes.

I think this makes sense as well, otherwise apps would have to laboriously check
which audmodes are supported before they can call S_TUNER. It's much easier to
just give the 'best' audmode and let the driver downgrade if it isn't supported.
This is what happens today anyway, so we can't change that behavior. But the one
thing that should work is that the actual audmode is returned when calling G_TUNER,
which is why the current tuner-core fails with v4l2-compliance.

> [1] Yet, I think that digital audio standards allow more than one audio channels.
> So, this may require to be pushed down into the drivers in some future.
> 
> What is invalid actually depends on the device. For example, AM ISA drivers
> don't support stereo. Ok, all tuners supported by tuner-core are FM. Even so,
> some of them may not support stereo[2].
> 
> [2] afaikt, some designs with tuner xc2028 don't support stereo. The driver currently
> doesn't handle such border cases, but the point is that such checks should happen
> at driver's level.

99% of all those tuner drivers do support stereo, so let's do this simple check
in tuner-core so we don't have to fix all of them. The spec is also clear that
radio devices only support mono or stereo audmodes. Those tuner drivers that
only support mono can easily enforce that explicitly. Or they could, if tuner-core
would copy back the audmode value after calling analog_ops->set_params().

Just as we do basic checks in v4l2-ioctl.c, so we can do basic checks in tuner-core
as well.

Regards,

	Hans

Re: [RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Mauro Carvalho Chehab]

Em Wed, 26 Sep 2012 09:03:13 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed September 26 2012 04:29:33 Mauro Carvalho Chehab wrote:
> > Em Tue, 25 Sep 2012 15:45:00 +0200
> > Hans Verkuil <hansverk@cisco.com> escreveu:
> > 
> > > On Tue 25 September 2012 15:33:40 Mauro Carvalho Chehab wrote:
> > > > Em Fri, 14 Sep 2012 13:15:36 +0200
> > > > Hans Verkuil <hans.verkuil@cisco.com> escreveu:
> > > > 
> > > > > Fixes a v4l2-compliance error: setting audmode to a value other than mono
> > > > > or stereo for a radio device should map to MODE_STEREO.
> > > > > 
> > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > ---
> > > > >  drivers/media/v4l2-core/tuner-core.c |    5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> > > > > index b5a819a..ea71371 100644
> > > > > --- a/drivers/media/v4l2-core/tuner-core.c
> > > > > +++ b/drivers/media/v4l2-core/tuner-core.c
> > > > > @@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> > > > >  	if (set_mode(t, vt->type))
> > > > >  		return 0;
> > > > >  
> > > > > -	if (t->mode == V4L2_TUNER_RADIO)
> > > > > +	if (t->mode == V4L2_TUNER_RADIO) {
> > > > >  		t->audmode = vt->audmode;
> > > > > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > > > > +			t->audmode = V4L2_TUNER_MODE_STEREO;
> > > > 
> > > > NACK. It is not a core's task to fix driver's bugs. It would be ok to have here a
> > > > WARN_ON(), but, if a driver is reporting a wrong radio audmode, the fix should be
> > > > there at the drivers, and not here at the core.
> > > 
> > > tuner-core *is* the driver.
> > 
> > Not really... it is a driver's glue between the real I2C driver and the bridge
> > driver.
> > 
> > > A bridge driver just passes v4l2_tuner on to the
> > > subdev driver(s), and it is the subdev driver such as tuner-core that needs to
> > > process the audmode as specified by the user. Which in this case means mapping
> > > audmodes that are invalid when in radio mode to something that is valid as per
> > > the spec.
> > 
> > Well, when the user is requesting an invalid mode, it should just return -EINVAL.
> > It makes sense to add a check there at tuner-core to reject audmode if userspace
> > is requesting, for example, a second language[1]. 
> 
> My interpretation of the spec is that it will map invalid audmodes to valid audmodes.
> From the VIDIOC_S_TUNER documentation:
> 
> "The selected audio mode, see Table A.89, “Tuner Audio Modes” for valid values. The
> audio mode does not affect audio subprogram detection, and like a control it does not
> automatically change unless the requested mode is invalid or unsupported. See Table
> A.90, “Tuner Audio Matrix” for possible results when the selected and received audio
> programs do not match."
> 
> So my interpretation is that if an audmode is provided that is not valid for the
> given device, then the device maps it to something valid rather than returning an
> error. The error code list only states that -EINVAL is returned if the index field
> is out-of-bounds, not for invalid audmodes.
> 
> I think this makes sense as well, otherwise apps would have to laboriously check
> which audmodes are supported before they can call S_TUNER. It's much easier to
> just give the 'best' audmode and let the driver downgrade if it isn't supported.
> This is what happens today anyway, so we can't change that behavior. But the one
> thing that should work is that the actual audmode is returned when calling G_TUNER,
> which is why the current tuner-core fails with v4l2-compliance.

Ok, you convinced me on that. Please be more verbose at the patch description,
describing why it is falling back to a different mode.

Also, please change that:

> > > > > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > > > > +			t->audmode = V4L2_TUNER_MODE_STEREO;

to something like:

	if (t->audmode != V4L2_TUNER_MODE_STEREO &&
	    t->audmode != V4L2_TUNER_MODE_MONO)
			t->audmode = V4L2_TUNER_MODE_STEREO;

We use those enums/defines to not having to remember the actual numbers associated
with them. By using operators like greater/lower than, people will actually need to
dig into the videodev2.h, in order to know what's covered there.

Besides that, the compiler will likely optimize it to greater than anyway, as audmode
is unsigned.

> > [1] Yet, I think that digital audio standards allow more than one audio channels.
> > So, this may require to be pushed down into the drivers in some future.
> > 
> > What is invalid actually depends on the device. For example, AM ISA drivers
> > don't support stereo. Ok, all tuners supported by tuner-core are FM. Even so,
> > some of them may not support stereo[2].
> > 
> > [2] afaikt, some designs with tuner xc2028 don't support stereo. The driver currently
> > doesn't handle such border cases, but the point is that such checks should happen
> > at driver's level.
> 
> 99% of all those tuner drivers do support stereo, so let's do this simple check
> in tuner-core so we don't have to fix all of them. The spec is also clear that
> radio devices only support mono or stereo audmodes. Those tuner drivers that
> only support mono can easily enforce that explicitly. Or they could, if tuner-core
> would copy back the audmode value after calling analog_ops->set_params().

It makes sense to do such change, allowing drivers to override it.

> 
> Just as we do basic checks in v4l2-ioctl.c, so we can do basic checks in tuner-core
> as well.
> 
> Regards,
> 
> 	Hans


-- 
Regards,
Mauro

Re: [RFCv1 API PATCH 4/4] tuner-core: map audmode to STEREO for radio devices.

[Hans Verkuil]

On Wed 26 September 2012 11:35:27 Mauro Carvalho Chehab wrote:
> Em Wed, 26 Sep 2012 09:03:13 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Wed September 26 2012 04:29:33 Mauro Carvalho Chehab wrote:
> > > Em Tue, 25 Sep 2012 15:45:00 +0200
> > > Hans Verkuil <hansverk@cisco.com> escreveu:
> > > 
> > > > On Tue 25 September 2012 15:33:40 Mauro Carvalho Chehab wrote:
> > > > > Em Fri, 14 Sep 2012 13:15:36 +0200
> > > > > Hans Verkuil <hans.verkuil@cisco.com> escreveu:
> > > > > 
> > > > > > Fixes a v4l2-compliance error: setting audmode to a value other than mono
> > > > > > or stereo for a radio device should map to MODE_STEREO.
> > > > > > 
> > > > > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > > ---
> > > > > >  drivers/media/v4l2-core/tuner-core.c |    5 ++++-
> > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> > > > > > index b5a819a..ea71371 100644
> > > > > > --- a/drivers/media/v4l2-core/tuner-core.c
> > > > > > +++ b/drivers/media/v4l2-core/tuner-core.c
> > > > > > @@ -1235,8 +1235,11 @@ static int tuner_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> > > > > >  	if (set_mode(t, vt->type))
> > > > > >  		return 0;
> > > > > >  
> > > > > > -	if (t->mode == V4L2_TUNER_RADIO)
> > > > > > +	if (t->mode == V4L2_TUNER_RADIO) {
> > > > > >  		t->audmode = vt->audmode;
> > > > > > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > > > > > +			t->audmode = V4L2_TUNER_MODE_STEREO;
> > > > > 
> > > > > NACK. It is not a core's task to fix driver's bugs. It would be ok to have here a
> > > > > WARN_ON(), but, if a driver is reporting a wrong radio audmode, the fix should be
> > > > > there at the drivers, and not here at the core.
> > > > 
> > > > tuner-core *is* the driver.
> > > 
> > > Not really... it is a driver's glue between the real I2C driver and the bridge
> > > driver.
> > > 
> > > > A bridge driver just passes v4l2_tuner on to the
> > > > subdev driver(s), and it is the subdev driver such as tuner-core that needs to
> > > > process the audmode as specified by the user. Which in this case means mapping
> > > > audmodes that are invalid when in radio mode to something that is valid as per
> > > > the spec.
> > > 
> > > Well, when the user is requesting an invalid mode, it should just return -EINVAL.
> > > It makes sense to add a check there at tuner-core to reject audmode if userspace
> > > is requesting, for example, a second language[1]. 
> > 
> > My interpretation of the spec is that it will map invalid audmodes to valid audmodes.
> > From the VIDIOC_S_TUNER documentation:
> > 
> > "The selected audio mode, see Table A.89, “Tuner Audio Modes” for valid values. The
> > audio mode does not affect audio subprogram detection, and like a control it does not
> > automatically change unless the requested mode is invalid or unsupported. See Table
> > A.90, “Tuner Audio Matrix” for possible results when the selected and received audio
> > programs do not match."
> > 
> > So my interpretation is that if an audmode is provided that is not valid for the
> > given device, then the device maps it to something valid rather than returning an
> > error. The error code list only states that -EINVAL is returned if the index field
> > is out-of-bounds, not for invalid audmodes.
> > 
> > I think this makes sense as well, otherwise apps would have to laboriously check
> > which audmodes are supported before they can call S_TUNER. It's much easier to
> > just give the 'best' audmode and let the driver downgrade if it isn't supported.
> > This is what happens today anyway, so we can't change that behavior. But the one
> > thing that should work is that the actual audmode is returned when calling G_TUNER,
> > which is why the current tuner-core fails with v4l2-compliance.
> 
> Ok, you convinced me on that. Please be more verbose at the patch description,
> describing why it is falling back to a different mode.
> 
> Also, please change that:
> 
> > > > > > +		if (t->audmode > V4L2_TUNER_MODE_STEREO)
> > > > > > +			t->audmode = V4L2_TUNER_MODE_STEREO;
> 
> to something like:
> 
> 	if (t->audmode != V4L2_TUNER_MODE_STEREO &&
> 	    t->audmode != V4L2_TUNER_MODE_MONO)
> 			t->audmode = V4L2_TUNER_MODE_STEREO;
> 
> We use those enums/defines to not having to remember the actual numbers associated
> with them. By using operators like greater/lower than, people will actually need to
> dig into the videodev2.h, in order to know what's covered there.
> 
> Besides that, the compiler will likely optimize it to greater than anyway, as audmode
> is unsigned.
> 
> > > [1] Yet, I think that digital audio standards allow more than one audio channels.
> > > So, this may require to be pushed down into the drivers in some future.
> > > 
> > > What is invalid actually depends on the device. For example, AM ISA drivers
> > > don't support stereo. Ok, all tuners supported by tuner-core are FM. Even so,
> > > some of them may not support stereo[2].
> > > 
> > > [2] afaikt, some designs with tuner xc2028 don't support stereo. The driver currently
> > > doesn't handle such border cases, but the point is that such checks should happen
> > > at driver's level.
> > 
> > 99% of all those tuner drivers do support stereo, so let's do this simple check
> > in tuner-core so we don't have to fix all of them. The spec is also clear that
> > radio devices only support mono or stereo audmodes. Those tuner drivers that
> > only support mono can easily enforce that explicitly. Or they could, if tuner-core
> > would copy back the audmode value after calling analog_ops->set_params().
> 
> It makes sense to do such change, allowing drivers to override it.

I'll make the requested changes and I'll post a pull request.

Regards,

	Hans