[PATCH 1/5] [media] adv7604: Add support for control event notifications

[PATCH 1/5] [media] adv7604: Add support for control event notifications

[Lars-Peter Clausen]

Allow userspace applications to subscribe to control change events. This
can e.g. be used to monitor the 5V detect control to be notified when a
source is connected or disconnected.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7604.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 808360f..cf1cb5a 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -41,6 +41,7 @@
 #include <media/adv7604.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-dv-timings.h>
 #include <media/v4l2-of.h>
 
@@ -2356,6 +2357,8 @@ static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
 static const struct v4l2_subdev_core_ops adv76xx_core_ops = {
 	.log_status = adv76xx_log_status,
 	.interrupt_service_routine = adv76xx_isr,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = adv76xx_g_register,
 	.s_register = adv76xx_s_register,
@@ -2833,7 +2836,7 @@ static int adv76xx_probe(struct i2c_client *client,
 	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
 		id->name, i2c_adapter_id(client->adapter),
 		client->addr);
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 
 	/*
 	 * Verify that the chip is present. On ADV7604 the RD_INFO register only
-- 
2.1.4

[PATCH 2/5] [media] adv7842: Add support for control event notifications

[Lars-Peter Clausen]

Allow userspace applications to subscribe to control change events. This
can e.g. be used to monitor the 5V detect control to be notified when a
source is connected or disconnected.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7842.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 4cf79b2..ffc0655 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -40,6 +40,7 @@
 #include <linux/v4l2-dv-timings.h>
 #include <linux/hdmi.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-dv-timings.h>
 #include <media/adv7842.h>
@@ -3015,6 +3016,8 @@ static const struct v4l2_subdev_core_ops adv7842_core_ops = {
 	.log_status = adv7842_log_status,
 	.ioctl = adv7842_ioctl,
 	.interrupt_service_routine = adv7842_isr,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = adv7842_g_register,
 	.s_register = adv7842_s_register,
@@ -3210,7 +3213,7 @@ static int adv7842_probe(struct i2c_client *client,
 
 	sd = &state->sd;
 	v4l2_i2c_subdev_init(sd, client, &adv7842_ops);
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	state->mode = pdata->mode;
 
 	state->hdmi_port_a = pdata->input == ADV7842_SELECT_HDMI_PORT_A;
-- 
2.1.4

[PATCH 3/5] [media] Add helper function for subdev event notifications

[Lars-Peter Clausen]

Add a new helper function called v4l2_subdev_notify_event() which will
deliver the specified event to both the v4l2 subdev event queue as well as
to the notify callback. The former is typically used by userspace
applications to listen to notification events while the later is used by
bridge drivers. Combining both into the same function avoids boilerplate
code in subdev drivers.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
 include/media/v4l2-subdev.h           |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 6359606..83615b8 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -588,3 +588,21 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
 #endif
 }
 EXPORT_SYMBOL(v4l2_subdev_init);
+
+/**
+ * v4l2_subdev_notify_event() - Delivers event notification for subdevice
+ * @sd: The subdev for which to deliver the event
+ * @ev: The event to deliver
+ *
+ * Will deliver the specified event to all userspace event listeners which are
+ * subscribed to the v42l subdev event queue as well as to the bridge driver
+ * using the notify callback. The notification type for the notify callback
+ * will be V4L2_DEVICE_NOTIFY_EVENT.
+ */
+void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
+			      const struct v4l2_event *ev)
+{
+	v4l2_event_queue(sd->devnode, ev);
+	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index dc20102..65d4a5f 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -44,6 +44,7 @@
 
 struct v4l2_device;
 struct v4l2_ctrl_handler;
+struct v4l2_event;
 struct v4l2_event_subscription;
 struct v4l2_fh;
 struct v4l2_subdev;
@@ -693,4 +694,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
 #define v4l2_subdev_has_op(sd, o, f) \
 	((sd)->ops->o && (sd)->ops->o->f)
 
+void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
+			      const struct v4l2_event *ev);
+
 #endif
-- 
2.1.4

[PATCH 4/5] [media] adv7604: Deliver resolution change events to userspace

[Lars-Peter Clausen]

Use the new v4l2_subdev_notify_event() helper function to deliver the
resolution change event to userspace via the v4l2 subdev event queue as
well as to the bridge driver using the callback notify mechanism.

This allows userspace applications to react to changes in resolution. This
is useful and often necessary for video pipelines where there is no direct
1-to-1 relationship between the subdevice converter and the video capture
device and hence it does not make sense to directly forward the event to
the video capture device node.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7604.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index cf1cb5a..b66f63e3 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1761,8 +1761,8 @@ static int adv76xx_s_routing(struct v4l2_subdev *sd,
 	select_input(sd);
 	enable_input(sd);
 
-	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
-			   (void *)&adv76xx_ev_fmt);
+	v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
+
 	return 0;
 }
 
@@ -1929,8 +1929,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 			"%s: fmt_change = 0x%x, fmt_change_digital = 0x%x\n",
 			__func__, fmt_change, fmt_change_digital);
 
-		v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
-				   (void *)&adv76xx_ev_fmt);
+		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
 
 		if (handled)
 			*handled = true;
@@ -2348,6 +2347,20 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static int adv76xx_subscribe_event(struct v4l2_subdev *sd,
+				   struct v4l2_fh *fh,
+				   struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_event_subdev_unsubscribe(sd, fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
@@ -2357,7 +2370,7 @@ static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
 static const struct v4l2_subdev_core_ops adv76xx_core_ops = {
 	.log_status = adv76xx_log_status,
 	.interrupt_service_routine = adv76xx_isr,
-	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.subscribe_event = adv76xx_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = adv76xx_g_register,
-- 
2.1.4

[PATCH 5/5] [media] adv7842: Deliver resolution change events to userspace

[Lars-Peter Clausen]

Use the new v4l2_subdev_notify_event() helper function to deliver the
resolution change event to userspace via the v4l2 subdev event queue as
well as to the bridge driver using the callback notify mechanism.

This allows userspace applications to react to changes in resolution. This
is useful and often necessary for video pipelines where there is no direct
1-to-1 relationship between the subdevice converter and the video capture
device and hence it does not make sense to directly forward the event to
the video capture device node.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7842.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index ffc0655..ed51aa7 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -1981,8 +1981,7 @@ static int adv7842_s_routing(struct v4l2_subdev *sd,
 	select_input(sd, state->vid_std_select);
 	enable_input(sd);
 
-	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
-			   (void *)&adv7842_ev_fmt);
+	v4l2_subdev_notify_event(sd, &adv7842_ev_fmt);
 
 	return 0;
 }
@@ -2215,8 +2214,7 @@ static int adv7842_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
 			 "%s: fmt_change_cp = 0x%x, fmt_change_digital = 0x%x, fmt_change_sdp = 0x%x\n",
 			 __func__, fmt_change_cp, fmt_change_digital,
 			 fmt_change_sdp);
-		v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
-				   (void *)&adv7842_ev_fmt);
+		v4l2_subdev_notify_event(sd, &adv7842_ev_fmt);
 		if (handled)
 			*handled = true;
 	}
@@ -3006,6 +3004,20 @@ static long adv7842_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
 	return -ENOTTY;
 }
 
+static int adv7842_subscribe_event(struct v4l2_subdev *sd,
+				   struct v4l2_fh *fh,
+				   struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_event_subdev_unsubscribe(sd, fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops adv7842_ctrl_ops = {
@@ -3016,7 +3028,7 @@ static const struct v4l2_subdev_core_ops adv7842_core_ops = {
 	.log_status = adv7842_log_status,
 	.ioctl = adv7842_ioctl,
 	.interrupt_service_routine = adv7842_isr,
-	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.subscribe_event = adv7842_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = adv7842_g_register,
-- 
2.1.4

Re: [PATCH 3/5] [media] Add helper function for subdev event notifications

[Sakari Ailus]

Hi Lars-Peter,

On Wed, Jun 24, 2015 at 06:50:29PM +0200, Lars-Peter Clausen wrote:
> Add a new helper function called v4l2_subdev_notify_event() which will
> deliver the specified event to both the v4l2 subdev event queue as well as
> to the notify callback. The former is typically used by userspace
> applications to listen to notification events while the later is used by
> bridge drivers. Combining both into the same function avoids boilerplate
> code in subdev drivers.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
>  include/media/v4l2-subdev.h           |  4 ++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 6359606..83615b8 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -588,3 +588,21 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>  #endif
>  }
>  EXPORT_SYMBOL(v4l2_subdev_init);
> +
> +/**
> + * v4l2_subdev_notify_event() - Delivers event notification for subdevice
> + * @sd: The subdev for which to deliver the event
> + * @ev: The event to deliver
> + *
> + * Will deliver the specified event to all userspace event listeners which are
> + * subscribed to the v42l subdev event queue as well as to the bridge driver
> + * using the notify callback. The notification type for the notify callback
> + * will be V4L2_DEVICE_NOTIFY_EVENT.
> + */
> +void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> +			      const struct v4l2_event *ev)
> +{
> +	v4l2_event_queue(sd->devnode, ev);
> +	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);

This is ugly. The casting avoids a warning for passing a const variable as
non-const.

Could v4l2_subdev_notify() be changed to take the third argument as const?
Alternatively I'd just leave it out from v4l2_subdev_notify_event().

> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index dc20102..65d4a5f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -44,6 +44,7 @@
>  
>  struct v4l2_device;
>  struct v4l2_ctrl_handler;
> +struct v4l2_event;
>  struct v4l2_event_subscription;
>  struct v4l2_fh;
>  struct v4l2_subdev;
> @@ -693,4 +694,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
>  #define v4l2_subdev_has_op(sd, o, f) \
>  	((sd)->ops->o && (sd)->ops->o->f)
>  
> +void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> +			      const struct v4l2_event *ev);
> +
>  #endif

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] [media] adv7604: Deliver resolution change events to userspace

[Sakari Ailus]

Hi Lars-Peter,

On Wed, Jun 24, 2015 at 06:50:30PM +0200, Lars-Peter Clausen wrote:
> Use the new v4l2_subdev_notify_event() helper function to deliver the
> resolution change event to userspace via the v4l2 subdev event queue as
> well as to the bridge driver using the callback notify mechanism.
> 
> This allows userspace applications to react to changes in resolution. This
> is useful and often necessary for video pipelines where there is no direct
> 1-to-1 relationship between the subdevice converter and the video capture
> device and hence it does not make sense to directly forward the event to
> the video capture device node.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/media/i2c/adv7604.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index cf1cb5a..b66f63e3 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1761,8 +1761,8 @@ static int adv76xx_s_routing(struct v4l2_subdev *sd,
>  	select_input(sd);
>  	enable_input(sd);
>  
> -	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
> -			   (void *)&adv76xx_ev_fmt);
> +	v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
> +
>  	return 0;
>  }
>  
> @@ -1929,8 +1929,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>  			"%s: fmt_change = 0x%x, fmt_change_digital = 0x%x\n",
>  			__func__, fmt_change, fmt_change_digital);
>  
> -		v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
> -				   (void *)&adv76xx_ev_fmt);
> +		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
>  
>  		if (handled)
>  			*handled = true;
> @@ -2348,6 +2347,20 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
>  	return 0;
>  }
>  
> +static int adv76xx_subscribe_event(struct v4l2_subdev *sd,
> +				   struct v4l2_fh *fh,
> +				   struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_SOURCE_CHANGE:
> +		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_event_subdev_unsubscribe(sd, fh, sub);

This should be ..._subscribe(), shouldn't it?

You could simply use v4l2_event_subscribe(fh, sub),
v4l2_event_subdev_unsubscribe() is there so you can use it directly as the
subscribe_event() op.

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /* ----------------------------------------------------------------------- */
>  
>  static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
> @@ -2357,7 +2370,7 @@ static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
>  static const struct v4l2_subdev_core_ops adv76xx_core_ops = {
>  	.log_status = adv76xx_log_status,
>  	.interrupt_service_routine = adv76xx_isr,
> -	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.subscribe_event = adv76xx_subscribe_event,
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	.g_register = adv76xx_g_register,

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 5/5] [media] adv7842: Deliver resolution change events to userspace

[Sakari Ailus]

Hi Lars-peter,

On Wed, Jun 24, 2015 at 06:50:31PM +0200, Lars-Peter Clausen wrote:
> Use the new v4l2_subdev_notify_event() helper function to deliver the
> resolution change event to userspace via the v4l2 subdev event queue as
> well as to the bridge driver using the callback notify mechanism.
> 
> This allows userspace applications to react to changes in resolution. This
> is useful and often necessary for video pipelines where there is no direct
> 1-to-1 relationship between the subdevice converter and the video capture
> device and hence it does not make sense to directly forward the event to
> the video capture device node.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/media/i2c/adv7842.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index ffc0655..ed51aa7 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -1981,8 +1981,7 @@ static int adv7842_s_routing(struct v4l2_subdev *sd,
>  	select_input(sd, state->vid_std_select);
>  	enable_input(sd);
>  
> -	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
> -			   (void *)&adv7842_ev_fmt);
> +	v4l2_subdev_notify_event(sd, &adv7842_ev_fmt);
>  
>  	return 0;
>  }
> @@ -2215,8 +2214,7 @@ static int adv7842_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>  			 "%s: fmt_change_cp = 0x%x, fmt_change_digital = 0x%x, fmt_change_sdp = 0x%x\n",
>  			 __func__, fmt_change_cp, fmt_change_digital,
>  			 fmt_change_sdp);
> -		v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
> -				   (void *)&adv7842_ev_fmt);
> +		v4l2_subdev_notify_event(sd, &adv7842_ev_fmt);
>  		if (handled)
>  			*handled = true;
>  	}
> @@ -3006,6 +3004,20 @@ static long adv7842_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
>  	return -ENOTTY;
>  }
>  
> +static int adv7842_subscribe_event(struct v4l2_subdev *sd,
> +				   struct v4l2_fh *fh,
> +				   struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_SOURCE_CHANGE:
> +		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_event_subdev_unsubscribe(sd, fh, sub);

Correcting my comment to the previous patch --- shouldn't this be
v4l2_event_subscribe(fh, sub, ..., NULL)?

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /* ----------------------------------------------------------------------- */
>  
>  static const struct v4l2_ctrl_ops adv7842_ctrl_ops = {
> @@ -3016,7 +3028,7 @@ static const struct v4l2_subdev_core_ops adv7842_core_ops = {
>  	.log_status = adv7842_log_status,
>  	.ioctl = adv7842_ioctl,
>  	.interrupt_service_routine = adv7842_isr,
> -	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.subscribe_event = adv7842_subscribe_event,
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	.g_register = adv7842_g_register,

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 3/5] [media] Add helper function for subdev event notifications

[Lars-Peter Clausen]

On 06/25/2015 11:47 AM, Sakari Ailus wrote:
> Hi Lars-Peter,
>
> On Wed, Jun 24, 2015 at 06:50:29PM +0200, Lars-Peter Clausen wrote:
>> Add a new helper function called v4l2_subdev_notify_event() which will
>> deliver the specified event to both the v4l2 subdev event queue as well as
>> to the notify callback. The former is typically used by userspace
>> applications to listen to notification events while the later is used by
>> bridge drivers. Combining both into the same function avoids boilerplate
>> code in subdev drivers.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
>>   include/media/v4l2-subdev.h           |  4 ++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 6359606..83615b8 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -588,3 +588,21 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>>   #endif
>>   }
>>   EXPORT_SYMBOL(v4l2_subdev_init);
>> +
>> +/**
>> + * v4l2_subdev_notify_event() - Delivers event notification for subdevice
>> + * @sd: The subdev for which to deliver the event
>> + * @ev: The event to deliver
>> + *
>> + * Will deliver the specified event to all userspace event listeners which are
>> + * subscribed to the v42l subdev event queue as well as to the bridge driver
>> + * using the notify callback. The notification type for the notify callback
>> + * will be V4L2_DEVICE_NOTIFY_EVENT.
>> + */
>> +void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>> +			      const struct v4l2_event *ev)
>> +{
>> +	v4l2_event_queue(sd->devnode, ev);
>> +	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>
> This is ugly. The casting avoids a warning for passing a const variable as
> non-const.

v4l2_subdev_notify() is a ioctl() style interface, which means you don't get 
type safety for the arg parameter, this includes any modifiers like const. 
Any subscriber to the notify callback needs to be aware that the type for 
the arg parameter for events of type V4L2_DEVICE_NOTIFY_EVENT is const 
struct v4l2_event *.

Having the cast here confined to a single place hides the ugliness from the 
users and you don't have to put the case, like it is right now, into each 
individual driver.

>
> Could v4l2_subdev_notify() be changed to take the third argument as const?

You could, I guess, but you don't really gain anything. The subscriber still 
needs to be aware what the correct type of the arg parameter is for a 
specific event type.

> Alternatively I'd just leave it out from v4l2_subdev_notify_event().

The whole point of the helper function is to have something that does both, 
add the event to the asynchronous event queue as well as call the 
synchronous event callback.

Re: [PATCH 4/5] [media] adv7604: Deliver resolution change events to userspace

[Lars-Peter Clausen]

On 06/25/2015 12:21 PM, Sakari Ailus wrote:
> Hi Lars-Peter,
>
> On Wed, Jun 24, 2015 at 06:50:30PM +0200, Lars-Peter Clausen wrote:
>> Use the new v4l2_subdev_notify_event() helper function to deliver the
>> resolution change event to userspace via the v4l2 subdev event queue as
>> well as to the bridge driver using the callback notify mechanism.
>>
>> This allows userspace applications to react to changes in resolution. This
>> is useful and often necessary for video pipelines where there is no direct
>> 1-to-1 relationship between the subdevice converter and the video capture
>> device and hence it does not make sense to directly forward the event to
>> the video capture device node.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   drivers/media/i2c/adv7604.c | 23 ++++++++++++++++++-----
>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>> index cf1cb5a..b66f63e3 100644
>> --- a/drivers/media/i2c/adv7604.c
>> +++ b/drivers/media/i2c/adv7604.c
>> @@ -1761,8 +1761,8 @@ static int adv76xx_s_routing(struct v4l2_subdev *sd,
>>   	select_input(sd);
>>   	enable_input(sd);
>>
>> -	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
>> -			   (void *)&adv76xx_ev_fmt);
>> +	v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
>> +
>>   	return 0;
>>   }
>>
>> @@ -1929,8 +1929,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>>   			"%s: fmt_change = 0x%x, fmt_change_digital = 0x%x\n",
>>   			__func__, fmt_change, fmt_change_digital);
>>
>> -		v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
>> -				   (void *)&adv76xx_ev_fmt);
>> +		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
>>
>>   		if (handled)
>>   			*handled = true;
>> @@ -2348,6 +2347,20 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
>>   	return 0;
>>   }
>>
>> +static int adv76xx_subscribe_event(struct v4l2_subdev *sd,
>> +				   struct v4l2_fh *fh,
>> +				   struct v4l2_event_subscription *sub)
>> +{
>> +	switch (sub->type) {
>> +	case V4L2_EVENT_SOURCE_CHANGE:
>> +		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
>> +	case V4L2_EVENT_CTRL:
>> +		return v4l2_event_subdev_unsubscribe(sd, fh, sub);
>
> This should be ..._subscribe(), shouldn't it?

Right, not sure how that happened.

>
> You could simply use v4l2_event_subscribe(fh, sub),
> v4l2_event_subdev_unsubscribe() is there so you can use it directly as the
> subscribe_event() op.

It's just to be on the safe side in case v4l2_event_subdev_subscribe() 
starts to do something in addition to just being a wrapper around 
v4l2_event_subscribe().

Thanks,
- Lars

Re: [PATCH 4/5] [media] adv7604: Deliver resolution change events to userspace

[Hans Verkuil]

On 06/24/2015 06:50 PM, Lars-Peter Clausen wrote:
> Use the new v4l2_subdev_notify_event() helper function to deliver the
> resolution change event to userspace via the v4l2 subdev event queue as
> well as to the bridge driver using the callback notify mechanism.
> 
> This allows userspace applications to react to changes in resolution. This
> is useful and often necessary for video pipelines where there is no direct
> 1-to-1 relationship between the subdevice converter and the video capture
> device and hence it does not make sense to directly forward the event to
> the video capture device node.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/media/i2c/adv7604.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index cf1cb5a..b66f63e3 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1761,8 +1761,8 @@ static int adv76xx_s_routing(struct v4l2_subdev *sd,
>  	select_input(sd);
>  	enable_input(sd);
>  
> -	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
> -			   (void *)&adv76xx_ev_fmt);
> +	v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
> +
>  	return 0;
>  }
>  
> @@ -1929,8 +1929,7 @@ static int adv76xx_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>  			"%s: fmt_change = 0x%x, fmt_change_digital = 0x%x\n",
>  			__func__, fmt_change, fmt_change_digital);
>  
> -		v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT,
> -				   (void *)&adv76xx_ev_fmt);
> +		v4l2_subdev_notify_event(sd, &adv76xx_ev_fmt);
>  
>  		if (handled)
>  			*handled = true;
> @@ -2348,6 +2347,20 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
>  	return 0;
>  }
>  
> +static int adv76xx_subscribe_event(struct v4l2_subdev *sd,
> +				   struct v4l2_fh *fh,
> +				   struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_SOURCE_CHANGE:
> +		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_event_subdev_unsubscribe(sd, fh, sub);

This should be v4l2_ctrl_subdev_subscribe_event() of course. I'll fix this in
the patch before sending the pull request. Ditto for the adv7842 patch.

Regards,

	Hans

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /* ----------------------------------------------------------------------- */
>  
>  static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
> @@ -2357,7 +2370,7 @@ static const struct v4l2_ctrl_ops adv76xx_ctrl_ops = {
>  static const struct v4l2_subdev_core_ops adv76xx_core_ops = {
>  	.log_status = adv76xx_log_status,
>  	.interrupt_service_routine = adv76xx_isr,
> -	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.subscribe_event = adv76xx_subscribe_event,
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	.g_register = adv76xx_g_register,
> 

Re: [PATCH 4/5] [media] adv7604: Deliver resolution change events to userspace

[Lars-Peter Clausen]

On 07/13/2015 11:02 AM, Hans Verkuil wrote:
>> +static int adv76xx_subscribe_event(struct v4l2_subdev *sd,
>> +				   struct v4l2_fh *fh,
>> +				   struct v4l2_event_subscription *sub)
>> +{
>> +	switch (sub->type) {
>> +	case V4L2_EVENT_SOURCE_CHANGE:
>> +		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
>> +	case V4L2_EVENT_CTRL:
>> +		return v4l2_event_subdev_unsubscribe(sd, fh, sub);
>
> This should be v4l2_ctrl_subdev_subscribe_event() of course. I'll fix this in
> the patch before sending the pull request. Ditto for the adv7842 patch.

Thanks.