From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34621 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753799Ab0GJN7m (ORCPT ); Sat, 10 Jul 2010 09:59:42 -0400 Message-ID: <4C387CDB.2030308@redhat.com> Date: Sat, 10 Jul 2010 10:59:55 -0300 From: Mauro Carvalho Chehab MIME-Version: 1.0 To: Laurent Pinchart CC: linux-media@vger.kernel.org, sakari.ailus@maxwell.research.nokia.com Subject: Re: [RFC/PATCH v2 6/7] v4l: subdev: Events support References: <1278689512-30849-1-git-send-email-laurent.pinchart@ideasonboard.com> <1278689512-30849-7-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: <1278689512-30849-7-git-send-email-laurent.pinchart@ideasonboard.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em 09-07-2010 12:31, Laurent Pinchart escreveu: > From: Sakari Ailus > > Provide v4l2_subdevs with v4l2_event support. Subdev drivers only need very > little to support events. > > Signed-off-by: Sakari Ailus > Signed-off-by: David Cohen > Signed-off-by: Laurent Pinchart > --- > Documentation/video4linux/v4l2-framework.txt | 18 +++++++ > drivers/media/video/v4l2-subdev.c | 71 +++++++++++++++++++++++++- > include/media/v4l2-subdev.h | 10 ++++ > 3 files changed, 98 insertions(+), 1 deletions(-) > > diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt > index 9c3f33c..89bd881 100644 > --- a/Documentation/video4linux/v4l2-framework.txt > +++ b/Documentation/video4linux/v4l2-framework.txt > @@ -347,6 +347,24 @@ VIDIOC_TRY_EXT_CTRLS > controls can be also be accessed through one (or several) V4L2 device > nodes. > > +VIDIOC_DQEVENT > +VIDIOC_SUBSCRIBE_EVENT > +VIDIOC_UNSUBSCRIBE_EVENT > + > + The events ioctls are identical to the ones defined in V4L2. They > + behave identically, with the only exception that they deal only with > + events generated by the sub-device. Depending on the driver, those > + events can also be reported by one (or several) V4L2 device nodes. > + > + Sub-device drivers that want to use events need to set the > + V4L2_SUBDEV_USES_EVENTS v4l2_subdev::flags and initialize > + v4l2_subdev::nevents to events queue depth before registering the > + sub-device. After registration events can be queued as usual on the > + v4l2_subdev::devnode device node. > + > + To properly support events, the poll() file operation is also > + implemented. > + > > I2C sub-device drivers > ---------------------- > diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c > index 0ebd760..31bec67 100644 > --- a/drivers/media/video/v4l2-subdev.c > +++ b/drivers/media/video/v4l2-subdev.c > @@ -18,26 +18,64 @@ > * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > */ > > -#include > #include > +#include > +#include > #include > > #include > #include > +#include > +#include > > static int subdev_open(struct file *file) > { > struct video_device *vdev = video_devdata(file); > struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev); > + struct v4l2_fh *vfh; > + int ret; > > if (!sd->initialized) > return -EAGAIN; > > + vfh = kzalloc(sizeof(*vfh), GFP_KERNEL); > + if (vfh == NULL) > + return -ENOMEM; > + > + ret = v4l2_fh_init(vfh, vdev); > + if (ret) > + goto err; Why to allocate/init the file handler for devices that don't need it? I would just move the above logic to happen only if V4L2_SUBDEV_FL_HAS_EVENTS. > + > + if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) { > + ret = v4l2_event_init(vfh); > + if (ret) > + goto err; > + > + ret = v4l2_event_alloc(vfh, sd->nevents); > + if (ret) > + goto err; > + } > + > + v4l2_fh_add(vfh); > + file->private_data = vfh; > + > return 0; > + > +err: > + v4l2_fh_exit(vfh); > + kfree(vfh); > + > + return ret; > } > > static int subdev_close(struct file *file) > { > + struct v4l2_fh *vfh = file->private_data; > + > + v4l2_fh_del(vfh); > + v4l2_fh_exit(vfh); > + kfree(vfh); > + > return 0; > } > > @@ -45,6 +83,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > { > struct video_device *vdev = video_devdata(file); > struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev); > + struct v4l2_fh *fh = file->private_data; > > switch (cmd) { > case VIDIOC_QUERYCTRL: > @@ -68,6 +107,18 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > case VIDIOC_TRY_EXT_CTRLS: > return v4l2_subdev_call(sd, core, try_ext_ctrls, arg); > > + case VIDIOC_DQEVENT: > + if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)) > + return -ENOIOCTLCMD; > + > + return v4l2_event_dequeue(fh, arg, file->f_flags & O_NONBLOCK); > + > + case VIDIOC_SUBSCRIBE_EVENT: > + return v4l2_subdev_call(sd, core, subscribe_event, fh, arg); > + > + case VIDIOC_UNSUBSCRIBE_EVENT: > + return v4l2_subdev_call(sd, core, unsubscribe_event, fh, arg); Hmm... shouldn't it test also for V4L2_SUBDEV_FL_HAS_EVENTS? I would do, instead: if (fh) { switch(cmd) { /* FH events logic */ } } > + > default: > return -ENOIOCTLCMD; > } > @@ -81,11 +132,29 @@ static long subdev_ioctl(struct file *file, unsigned int cmd, > return video_usercopy(file, cmd, arg, subdev_do_ioctl); > } > > +static unsigned int subdev_poll(struct file *file, poll_table *wait) > +{ > + struct video_device *vdev = video_devdata(file); > + struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev); > + struct v4l2_fh *fh = file->private_data; > + > + if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)) > + return POLLERR; > + > + poll_wait(file, &fh->events->wait, wait); > + > + if (v4l2_event_pending(fh)) > + return POLLPRI; > + > + return 0; > +} > + > const struct v4l2_file_operations v4l2_subdev_fops = { > .owner = THIS_MODULE, > .open = subdev_open, > .unlocked_ioctl = subdev_ioctl, > .release = subdev_close, > + .poll = subdev_poll, > }; > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 9ee45c8..55a8c93 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -36,6 +36,8 @@ > #define V4L2_SUBDEV_IR_TX_FIFO_SERVICE_REQ 0x00000001 > > struct v4l2_device; > +struct v4l2_event_subscription; > +struct v4l2_fh; > struct v4l2_subdev; > struct tuner_setup; > > @@ -134,6 +136,10 @@ struct v4l2_subdev_core_ops { > int (*s_register)(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg); > #endif > int (*s_power)(struct v4l2_subdev *sd, int on); > + int (*subscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh, > + struct v4l2_event_subscription *sub); > + int (*unsubscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh, > + struct v4l2_event_subscription *sub); > }; > > /* s_mode: switch the tuner to a specific tuner mode. Replacement of s_radio. > @@ -408,6 +414,8 @@ struct v4l2_subdev_ops { > #define V4L2_SUBDEV_FL_IS_SPI (1U << 1) > /* Set this flag if this subdev needs a device node. */ > #define V4L2_SUBDEV_FL_HAS_DEVNODE (1U << 2) > +/* Set this flag if this subdev generates events. */ > +#define V4L2_SUBDEV_FL_HAS_EVENTS (1U << 3) > > /* Each instance of a subdev driver should create this struct, either > stand-alone or embedded in a larger struct. > @@ -427,6 +435,8 @@ struct v4l2_subdev { > /* subdev device node */ > struct video_device devnode; > unsigned int initialized; > + /* number of events to be allocated on open */ > + unsigned int nevents; > }; > > #define vdev_to_v4l2_subdev(vdev) \