[v4,2/7] V4L: Events: Add new ioctls for events

  This patch adds a set of new ioctls to the V4L2 API. The ioctls conform to
V4L2 Events RFC version 2.3:

<URL:http://www.spinics.net/lists/linux-media/msg12033.html>

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
 drivers/media/video/v4l2-ioctl.c          |    3 +++
 include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+), 0 deletions(-)
  

On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
> This patch adds a set of new ioctls to the V4L2 API. The ioctls conform to
> V4L2 Events RFC version 2.3:

I've experimented with the events API to try and support it with ivtv and
I realized that it had some problems.

See comments below.

> 
> <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
>  drivers/media/video/v4l2-ioctl.c          |    3 +++
>  include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index 997975d..cba704c 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  	case VIDIOC_DBG_G_REGISTER:
>  	case VIDIOC_DBG_G_CHIP_IDENT:
>  	case VIDIOC_S_HW_FREQ_SEEK:
> +	case VIDIOC_DQEVENT:
> +	case VIDIOC_SUBSCRIBE_EVENT:
> +	case VIDIOC_UNSUBSCRIBE_EVENT:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 30cc334..bfc4696 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
>  
>  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
>  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
> +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
>  #endif
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 54af357..a19ae89 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
>  };
>  
>  /*
> + *	E V E N T S
> + */
> +
> +struct v4l2_event {
> +	__u32		count;

The name 'count' is confusing. Count of what? I think the name 'pending' might
be more understandable. A comment after the definition would also help.

> +	__u32		type;
> +	__u32		sequence;
> +	struct timespec	timestamp;
> +	__u32		reserved[9];
> +	__u8		data[64];
> +};

I also think we should reorder the fields and add a union. For ivtv I would
need this:

#define V4L2_EVENT_ALL                          0
#define V4L2_EVENT_VSYNC                        1
#define V4L2_EVENT_EOS                          2
#define V4L2_EVENT_PRIVATE_START                0x08000000

/* Payload for V4L2_EVENT_VSYNC */
struct v4l2_event_vsync {
        /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
        u8 field;
} __attribute__ ((packed));

struct v4l2_event {
        __u32           type;
        union {
                struct v4l2_event_vsync vsync;
                __u8    data[64];
        } u;
        __u32           sequence;
        struct timespec timestamp;
        __u32           pending;
        __u32           reserved[9];
};

The reason for rearranging the fields has to do with the fact that the first
two fields (type and the union) form the actual event data. The others are
more for administrative purposes. Separating those two makes sense to me.

So when I define an event for queuing it is nice if I can do just this:

static const struct v4l2_event ev_top = {
	.type = V4L2_EVENT_VSYNC,
	.u.vsync.field = V4L2_FIELD_TOP,
};

I would have preferred to have an anonymous union. Unfortunately gcc has
problems with initializers for fields inside an anonymous union. Hence the
need for a named union.

Regards,

	Hans

> +
> +struct v4l2_event_subscription {
> +	__u32		type;
> +	__u32		reserved[7];
> +};
> +
> +#define V4L2_EVENT_PRIVATE_START		0x08000000
> +
> +/*
>   *	A D V A N C E D   D E B U G G I N G
>   *
>   *	NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS!
> @@ -1651,6 +1671,9 @@ struct v4l2_dbg_chip_ident {
>  #endif
>  
>  #define VIDIOC_S_HW_FREQ_SEEK	 _IOW('V', 82, struct v4l2_hw_freq_seek)
> +#define VIDIOC_DQEVENT		 _IOR('V', 83, struct v4l2_event)
> +#define VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 84, struct v4l2_event_subscription)
> +#define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 85, struct v4l2_event_subscription)
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */
>  
>
  

Hi Hans,

On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote:
> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
> > This patch adds a set of new ioctls to the V4L2 API. The ioctls conform
> > to V4L2 Events RFC version 2.3:
> I've experimented with the events API to try and support it with ivtv and
> I realized that it had some problems.
> 
> See comments below.
> 
> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> > ---
> > 
> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
> >  drivers/media/video/v4l2-ioctl.c          |    3 +++
> >  include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
> >  3 files changed, 29 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c
> > 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file,
> > unsigned int cmd, unsigned long arg)
> > 
> >  	case VIDIOC_DBG_G_REGISTER:
> >  	case VIDIOC_DBG_G_CHIP_IDENT:
> > 
> >  	case VIDIOC_S_HW_FREQ_SEEK:
> > +	case VIDIOC_DQEVENT:
> > +	case VIDIOC_SUBSCRIBE_EVENT:
> > 
> > +	case VIDIOC_UNSUBSCRIBE_EVENT:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> > 
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
> > 
> >  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> >  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
> > 
> > +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> > +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> > +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> > 
> >  #endif
> >  };
> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> > 
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 54af357..a19ae89 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
> > 
> >  };
> >  
> >  /*
> > 
> > + *	E V E N T S
> > + */
> > +
> > +struct v4l2_event {
> > +	__u32		count;
> 
> The name 'count' is confusing. Count of what? I think the name 'pending'
> might be more understandable. A comment after the definition would also
> help.
> 
> > +	__u32		type;
> > +	__u32		sequence;
> > +	struct timespec	timestamp;
> > +	__u32		reserved[9];
> > +	__u8		data[64];
> > +};
> 
> I also think we should reorder the fields and add a union. For ivtv I would
> need this:
> 
> #define V4L2_EVENT_ALL                          0
> #define V4L2_EVENT_VSYNC                        1
> #define V4L2_EVENT_EOS                          2
> #define V4L2_EVENT_PRIVATE_START                0x08000000
> 
> /* Payload for V4L2_EVENT_VSYNC */
> struct v4l2_event_vsync {
>         /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
>         u8 field;
> } __attribute__ ((packed));
> 
> struct v4l2_event {
>         __u32           type;
>         union {
>                 struct v4l2_event_vsync vsync;
>                 __u8    data[64];
>         } u;
>         __u32           sequence;
>         struct timespec timestamp;
>         __u32           pending;
>         __u32           reserved[9];
> };
> 
> The reason for rearranging the fields has to do with the fact that the
> first two fields (type and the union) form the actual event data. The
> others are more for administrative purposes. Separating those two makes
> sense to me.
> 
> So when I define an event for queuing it is nice if I can do just this:
> 
> static const struct v4l2_event ev_top = {
> 	.type = V4L2_EVENT_VSYNC,
> 	.u.vsync.field = V4L2_FIELD_TOP,
> };
> 
> I would have preferred to have an anonymous union. Unfortunately gcc has
> problems with initializers for fields inside an anonymous union. Hence the
> need for a named union.

Will all drivers add private events to the union ? This would then soon become 
a mess. Wouldn't it be better for drivers to define their own event structures 
(standard ones could be shared between drivers in videodev2.h) and cast the 
pointer to data to a pointer to the appropriate event structure ?
  

> Hi Hans,
>
> On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote:
>> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
>> > This patch adds a set of new ioctls to the V4L2 API. The ioctls
>> conform
>> > to V4L2 Events RFC version 2.3:
>> I've experimented with the events API to try and support it with ivtv
>> and
>> I realized that it had some problems.
>>
>> See comments below.
>>
>> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
>> >
>> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> > ---
>> >
>> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
>> >  drivers/media/video/v4l2-ioctl.c          |    3 +++
>> >  include/linux/videodev2.h                 |   23
>> +++++++++++++++++++++++
>> >  3 files changed, 29 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
>> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c
>> > 100644
>> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file,
>> > unsigned int cmd, unsigned long arg)
>> >
>> >  	case VIDIOC_DBG_G_REGISTER:
>> >  	case VIDIOC_DBG_G_CHIP_IDENT:
>> >
>> >  	case VIDIOC_S_HW_FREQ_SEEK:
>> > +	case VIDIOC_DQEVENT:
>> > +	case VIDIOC_SUBSCRIBE_EVENT:
>> >
>> > +	case VIDIOC_UNSUBSCRIBE_EVENT:
>> >  		ret = do_video_ioctl(file, cmd, arg);
>> >  		break;
>> >
>> > diff --git a/drivers/media/video/v4l2-ioctl.c
>> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644
>> > --- a/drivers/media/video/v4l2-ioctl.c
>> > +++ b/drivers/media/video/v4l2-ioctl.c
>> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
>> >
>> >  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
>> >  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
>> >
>> > +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>> > +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>> > +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
>> >
>> >  #endif
>> >  };
>> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>> >
>> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> > index 54af357..a19ae89 100644
>> > --- a/include/linux/videodev2.h
>> > +++ b/include/linux/videodev2.h
>> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
>> >
>> >  };
>> >
>> >  /*
>> >
>> > + *	E V E N T S
>> > + */
>> > +
>> > +struct v4l2_event {
>> > +	__u32		count;
>>
>> The name 'count' is confusing. Count of what? I think the name 'pending'
>> might be more understandable. A comment after the definition would also
>> help.
>>
>> > +	__u32		type;
>> > +	__u32		sequence;
>> > +	struct timespec	timestamp;
>> > +	__u32		reserved[9];
>> > +	__u8		data[64];
>> > +};
>>
>> I also think we should reorder the fields and add a union. For ivtv I
>> would
>> need this:
>>
>> #define V4L2_EVENT_ALL                          0
>> #define V4L2_EVENT_VSYNC                        1
>> #define V4L2_EVENT_EOS                          2
>> #define V4L2_EVENT_PRIVATE_START                0x08000000
>>
>> /* Payload for V4L2_EVENT_VSYNC */
>> struct v4l2_event_vsync {
>>         /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
>>         u8 field;
>> } __attribute__ ((packed));
>>
>> struct v4l2_event {
>>         __u32           type;
>>         union {
>>                 struct v4l2_event_vsync vsync;
>>                 __u8    data[64];
>>         } u;
>>         __u32           sequence;
>>         struct timespec timestamp;
>>         __u32           pending;
>>         __u32           reserved[9];
>> };
>>
>> The reason for rearranging the fields has to do with the fact that the
>> first two fields (type and the union) form the actual event data. The
>> others are more for administrative purposes. Separating those two makes
>> sense to me.
>>
>> So when I define an event for queuing it is nice if I can do just this:
>>
>> static const struct v4l2_event ev_top = {
>> 	.type = V4L2_EVENT_VSYNC,
>> 	.u.vsync.field = V4L2_FIELD_TOP,
>> };
>>
>> I would have preferred to have an anonymous union. Unfortunately gcc has
>> problems with initializers for fields inside an anonymous union. Hence
>> the
>> need for a named union.
>
> Will all drivers add private events to the union ? This would then soon
> become
> a mess. Wouldn't it be better for drivers to define their own event
> structures
> (standard ones could be shared between drivers in videodev2.h) and cast
> the
> pointer to data to a pointer to the appropriate event structure ?

I would prefer to have the actual event type defines in videodev2.h (just
as we do for private control IDs). The actual payload structure can be
defined elsewhere as far as I am concerned.

An alternative in the long run might be to split off the event structs
into a separate public header. I have been thinking along those lines as
well for the controls. videodev2.h is getting pretty huge and it might be
more managable if it is split up in multiple parts with videodev2.h
including those parts.

Regards,

        Hans

>
> --
> Regards,
>
> Laurent Pinchart
>
  

Hi Hans,

On Monday 15 February 2010 11:28:52 Hans Verkuil wrote:
> > Hi Hans,
> > 
> > On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote:
> >> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
> >> > This patch adds a set of new ioctls to the V4L2 API. The ioctls
> >> 
> >> conform
> >> 
> >> > to V4L2 Events RFC version 2.3:
> >> I've experimented with the events API to try and support it with ivtv
> >> and
> >> I realized that it had some problems.
> >> 
> >> See comments below.
> >> 
> >> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
> >> > 
> >> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> >> > ---
> >> > 
> >> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
> >> >  drivers/media/video/v4l2-ioctl.c          |    3 +++
> >> >  include/linux/videodev2.h                 |   23
> >> 
> >> +++++++++++++++++++++++
> >> 
> >> >  3 files changed, 29 insertions(+), 0 deletions(-)
> >> > 
> >> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> >> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c
> >> > 100644
> >> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> >> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> >> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file,
> >> > unsigned int cmd, unsigned long arg)
> >> > 
> >> >  	case VIDIOC_DBG_G_REGISTER:
> >> >  	case VIDIOC_DBG_G_CHIP_IDENT:
> >> > 
> >> >  	case VIDIOC_S_HW_FREQ_SEEK:
> >> > +	case VIDIOC_DQEVENT:
> >> > +	case VIDIOC_SUBSCRIBE_EVENT:
> >> > 
> >> > +	case VIDIOC_UNSUBSCRIBE_EVENT:
> >> >  		ret = do_video_ioctl(file, cmd, arg);
> >> >  		break;
> >> > 
> >> > diff --git a/drivers/media/video/v4l2-ioctl.c
> >> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644
> >> > --- a/drivers/media/video/v4l2-ioctl.c
> >> > +++ b/drivers/media/video/v4l2-ioctl.c
> >> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
> >> > 
> >> >  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> >> >  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
> >> > 
> >> > +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >> > +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >> > +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> >> > 
> >> >  #endif
> >> >  };
> >> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >> > 
> >> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> >> > index 54af357..a19ae89 100644
> >> > --- a/include/linux/videodev2.h
> >> > +++ b/include/linux/videodev2.h
> >> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
> >> > 
> >> >  };
> >> >  
> >> >  /*
> >> > 
> >> > + *	E V E N T S
> >> > + */
> >> > +
> >> > +struct v4l2_event {
> >> > +	__u32		count;
> >> 
> >> The name 'count' is confusing. Count of what? I think the name 'pending'
> >> might be more understandable. A comment after the definition would also
> >> help.
> >> 
> >> > +	__u32		type;
> >> > +	__u32		sequence;
> >> > +	struct timespec	timestamp;
> >> > +	__u32		reserved[9];
> >> > +	__u8		data[64];
> >> > +};
> >> 
> >> I also think we should reorder the fields and add a union. For ivtv I
> >> would
> >> need this:
> >> 
> >> #define V4L2_EVENT_ALL                          0
> >> #define V4L2_EVENT_VSYNC                        1
> >> #define V4L2_EVENT_EOS                          2
> >> #define V4L2_EVENT_PRIVATE_START                0x08000000
> >> 
> >> /* Payload for V4L2_EVENT_VSYNC */
> >> struct v4l2_event_vsync {
> >> 
> >>         /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
> >>         u8 field;
> >> 
> >> } __attribute__ ((packed));
> >> 
> >> struct v4l2_event {
> >> 
> >>         __u32           type;
> >>         union {
> >>         
> >>                 struct v4l2_event_vsync vsync;
> >>                 __u8    data[64];
> >>         
> >>         } u;
> >>         __u32           sequence;
> >>         struct timespec timestamp;
> >>         __u32           pending;
> >>         __u32           reserved[9];
> >> 
> >> };
> >> 
> >> The reason for rearranging the fields has to do with the fact that the
> >> first two fields (type and the union) form the actual event data. The
> >> others are more for administrative purposes. Separating those two makes
> >> sense to me.
> >> 
> >> So when I define an event for queuing it is nice if I can do just this:
> >> 
> >> static const struct v4l2_event ev_top = {
> >> 
> >> 	.type = V4L2_EVENT_VSYNC,
> >> 	.u.vsync.field = V4L2_FIELD_TOP,
> >> 
> >> };
> >> 
> >> I would have preferred to have an anonymous union. Unfortunately gcc has
> >> problems with initializers for fields inside an anonymous union. Hence
> >> the need for a named union.
> > 
> > Will all drivers add private events to the union ? This would then soon
> > become a mess. Wouldn't it be better for drivers to define their own event
> > structures (standard ones could be shared between drivers in videodev2.h)
> > and cast the pointer to data to a pointer to the appropriate event
> > structure ?
> 
> I would prefer to have the actual event type defines in videodev2.h (just
> as we do for private control IDs). The actual payload structure can be
> defined elsewhere as far as I am concerned.

I'm OK with defining the types in videodev2.h, but using a big union would 
require every event type to add a field to the union. That's the part that 
would become messy.

> An alternative in the long run might be to split off the event structs
> into a separate public header. I have been thinking along those lines as
> well for the controls. videodev2.h is getting pretty huge and it might be
> more managable if it is split up in multiple parts with videodev2.h
> including those parts.

Agreed.