From: Hans Verkuil <hverkuil@xs4all.nl>
To: "Frank Schäfer" <fschaefer.oss@googlemail.com>, m.chehab@samsung.com
Cc: linux-media@vger.kernel.org
Subject: Re: [PATCH 06/19] em28xx: move video_device structs from struct em28xx to struct v4l2
Date: Fri, 09 May 2014 11:19:01 +0200 [thread overview]
Message-ID: <536C9D85.10504@xs4all.nl> (raw)
In-Reply-To: <1395689605-2705-7-git-send-email-fschaefer.oss@googlemail.com>
On 03/24/2014 08:33 PM, Frank Schäfer wrote:
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
> drivers/media/usb/em28xx/em28xx-video.c | 120 ++++++++++++++------------------
> drivers/media/usb/em28xx/em28xx.h | 7 +-
> 2 files changed, 56 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 4fb0053..7252eef 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -1447,7 +1447,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
> (EM28XX_VMUX_CABLE == INPUT(n)->type))
> i->type = V4L2_INPUT_TYPE_TUNER;
>
> - i->std = dev->vdev->tvnorms;
> + i->std = dev->v4l2->vdev->tvnorms;
> /* webcams do not have the STD API */
> if (dev->board.is_webcam)
> i->capabilities = 0;
> @@ -1691,9 +1691,10 @@ static int vidioc_s_register(struct file *file, void *priv,
> static int vidioc_querycap(struct file *file, void *priv,
> struct v4l2_capability *cap)
> {
> - struct video_device *vdev = video_devdata(file);
> - struct em28xx_fh *fh = priv;
> - struct em28xx *dev = fh->dev;
> + struct video_device *vdev = video_devdata(file);
> + struct em28xx_fh *fh = priv;
> + struct em28xx *dev = fh->dev;
> + struct em28xx_v4l2 *v4l2 = dev->v4l2;
>
> strlcpy(cap->driver, "em28xx", sizeof(cap->driver));
> strlcpy(cap->card, em28xx_boards[dev->model].name, sizeof(cap->card));
> @@ -1715,9 +1716,9 @@ static int vidioc_querycap(struct file *file, void *priv,
>
> cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS |
> V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> - if (dev->vbi_dev)
> + if (v4l2->vbi_dev)
> cap->capabilities |= V4L2_CAP_VBI_CAPTURE;
> - if (dev->radio_dev)
> + if (v4l2->radio_dev)
> cap->capabilities |= V4L2_CAP_RADIO;
> return 0;
> }
> @@ -1955,20 +1956,20 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>
> - if (dev->radio_dev) {
> + if (v4l2->radio_dev) {
> em28xx_info("V4L2 device %s deregistered\n",
> - video_device_node_name(dev->radio_dev));
> - video_unregister_device(dev->radio_dev);
> + video_device_node_name(v4l2->radio_dev));
> + video_unregister_device(v4l2->radio_dev);
> }
> - if (dev->vbi_dev) {
> + if (v4l2->vbi_dev) {
> em28xx_info("V4L2 device %s deregistered\n",
> - video_device_node_name(dev->vbi_dev));
> - video_unregister_device(dev->vbi_dev);
> + video_device_node_name(v4l2->vbi_dev));
> + video_unregister_device(v4l2->vbi_dev);
> }
> - if (dev->vdev) {
> + if (v4l2->vdev) {
> em28xx_info("V4L2 device %s deregistered\n",
> - video_device_node_name(dev->vdev));
> - video_unregister_device(dev->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> @@ -2061,23 +2062,6 @@ exit:
> return 0;
> }
>
> -/*
> - * em28xx_videodevice_release()
> - * called when the last user of the video device exits and frees the memeory
> - */
> -static void em28xx_videodevice_release(struct video_device *vdev)
> -{
> - struct em28xx *dev = video_get_drvdata(vdev);
> -
> - video_device_release(vdev);
> - if (vdev == dev->vdev)
> - dev->vdev = NULL;
> - else if (vdev == dev->vbi_dev)
> - dev->vbi_dev = NULL;
> - else if (vdev == dev->radio_dev)
> - dev->radio_dev = NULL;
> -}
> -
> static const struct v4l2_file_operations em28xx_v4l_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2134,7 +2118,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> static const struct video_device em28xx_video_template = {
> .fops = &em28xx_v4l_fops,
> .ioctl_ops = &video_ioctl_ops,
> - .release = em28xx_videodevice_release,
> + .release = video_device_release,
> .tvnorms = V4L2_STD_ALL,
> };
>
> @@ -2163,7 +2147,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
> static struct video_device em28xx_radio_template = {
> .fops = &radio_fops,
> .ioctl_ops = &radio_ioctl_ops,
> - .release = em28xx_videodevice_release,
> + .release = video_device_release,
> };
>
> /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
> @@ -2493,36 +2477,36 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> goto unregister_dev;
>
> /* allocate and fill video video_device struct */
> - dev->vdev = em28xx_vdev_init(dev, &em28xx_video_template, "video");
> - if (!dev->vdev) {
> + v4l2->vdev = em28xx_vdev_init(dev, &em28xx_video_template, "video");
> + if (!v4l2->vdev) {
> em28xx_errdev("cannot allocate video_device.\n");
> ret = -ENODEV;
> goto unregister_dev;
> }
> - dev->vdev->queue = &dev->vb_vidq;
> - dev->vdev->queue->lock = &dev->vb_queue_lock;
> + v4l2->vdev->queue = &dev->vb_vidq;
> + v4l2->vdev->queue->lock = &dev->vb_queue_lock;
>
> /* disable inapplicable ioctls */
> if (dev->board.is_webcam) {
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
> } else {
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_S_PARM);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
> }
> if (dev->tuner_type == TUNER_ABSENT) {
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_G_TUNER);
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_S_TUNER);
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_G_FREQUENCY);
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_S_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
> }
> if (!dev->audio_mode.has_audio) {
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_G_AUDIO);
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_S_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
> }
>
> /* register v4l2 video video_device */
> - ret = video_register_device(dev->vdev, VFL_TYPE_GRABBER,
> + ret = video_register_device(v4l2->vdev, VFL_TYPE_GRABBER,
> video_nr[dev->devno]);
> if (ret) {
> em28xx_errdev("unable to register video device (error=%i).\n",
> @@ -2532,27 +2516,27 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> /* Allocate and fill vbi video_device struct */
> if (em28xx_vbi_supported(dev) == 1) {
> - dev->vbi_dev = em28xx_vdev_init(dev, &em28xx_video_template,
> + v4l2->vbi_dev = em28xx_vdev_init(dev, &em28xx_video_template,
> "vbi");
>
> - dev->vbi_dev->queue = &dev->vb_vbiq;
> - dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
> + v4l2->vbi_dev->queue = &dev->vb_vbiq;
> + v4l2->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
>
> /* disable inapplicable ioctls */
> - v4l2_disable_ioctl(dev->vdev, VIDIOC_S_PARM);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
> if (dev->tuner_type == TUNER_ABSENT) {
> - v4l2_disable_ioctl(dev->vbi_dev, VIDIOC_G_TUNER);
> - v4l2_disable_ioctl(dev->vbi_dev, VIDIOC_S_TUNER);
> - v4l2_disable_ioctl(dev->vbi_dev, VIDIOC_G_FREQUENCY);
> - v4l2_disable_ioctl(dev->vbi_dev, VIDIOC_S_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_G_TUNER);
> + v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_S_TUNER);
> + v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_G_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_S_FREQUENCY);
> }
> if (!dev->audio_mode.has_audio) {
> - v4l2_disable_ioctl(dev->vbi_dev, VIDIOC_G_AUDIO);
> - v4l2_disable_ioctl(dev->vbi_dev, VIDIOC_S_AUDIO);
> + v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_G_AUDIO);
> + v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_S_AUDIO);
> }
>
> /* register v4l2 vbi video_device */
> - ret = video_register_device(dev->vbi_dev, VFL_TYPE_VBI,
> + ret = video_register_device(v4l2->vbi_dev, VFL_TYPE_VBI,
> vbi_nr[dev->devno]);
> if (ret < 0) {
> em28xx_errdev("unable to register vbi device\n");
> @@ -2561,29 +2545,29 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> }
>
> if (em28xx_boards[dev->model].radio.type == EM28XX_RADIO) {
> - dev->radio_dev = em28xx_vdev_init(dev, &em28xx_radio_template,
> - "radio");
> - if (!dev->radio_dev) {
> + v4l2->radio_dev = em28xx_vdev_init(dev, &em28xx_radio_template,
> + "radio");
> + if (!v4l2->radio_dev) {
> em28xx_errdev("cannot allocate video_device.\n");
> ret = -ENODEV;
> goto unregister_dev;
> }
> - ret = video_register_device(dev->radio_dev, VFL_TYPE_RADIO,
> + ret = video_register_device(v4l2->radio_dev, VFL_TYPE_RADIO,
> radio_nr[dev->devno]);
> if (ret < 0) {
> em28xx_errdev("can't register radio device\n");
> goto unregister_dev;
> }
> em28xx_info("Registered radio device as %s\n",
> - video_device_node_name(dev->radio_dev));
> + video_device_node_name(v4l2->radio_dev));
> }
>
> em28xx_info("V4L2 video device registered as %s\n",
> - video_device_node_name(dev->vdev));
> + video_device_node_name(v4l2->vdev));
>
> - if (dev->vbi_dev)
> + if (v4l2->vbi_dev)
> em28xx_info("V4L2 VBI device registered as %s\n",
> - video_device_node_name(dev->vbi_dev));
> + video_device_node_name(v4l2->vbi_dev));
>
> /* Save some power by putting tuner to sleep */
> v4l2_device_call_all(&v4l2->v4l2_dev, 0, core, s_power, 0);
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index a4d26bf..88d0589 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -504,6 +504,10 @@ struct em28xx_v4l2 {
> struct v4l2_device v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
> struct v4l2_clk *clk;
> +
> + struct video_device *vdev;
> + struct video_device *vbi_dev;
> + struct video_device *radio_dev;
Think about embedding these structs. That way you don't have to allocate them which
removes the complexity of checking for ENOMEM errors.
Regards,
Hans
> };
>
> struct em28xx_audio {
> @@ -614,7 +618,6 @@ struct em28xx {
> /* video for linux */
> int users; /* user count for exclusive use */
> int streaming_users; /* Number of actively streaming users */
> - struct video_device *vdev; /* video for linux device struct */
> v4l2_std_id norm; /* selected tv norm */
> int ctl_freq; /* selected frequency */
> unsigned int ctl_input; /* selected input */
> @@ -645,8 +648,6 @@ struct em28xx {
> /* locks */
> struct mutex lock;
> struct mutex ctrl_urb_lock; /* protects urb_buf */
> - struct video_device *vbi_dev;
> - struct video_device *radio_dev;
>
> /* Videobuf2 */
> struct vb2_queue vb_vidq;
>
next prev parent reply other threads:[~2014-05-09 9:19 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-24 19:33 [PATCH 00/19] em28xx: clean up the main device struct and move sub-module specific data to its own data structs Frank Schäfer
2014-03-24 19:33 ` [PATCH 01/19] em28xx: move sub-module data structs to a common place in the main struct Frank Schäfer
2014-03-24 19:33 ` [PATCH 02/19] em28xx-video: simplify usage of the pointer to struct v4l2_ctrl_handler in em28xx_v4l2_init() Frank Schäfer
2014-03-24 19:33 ` [PATCH 03/19] em28xx: start moving em28xx-v4l specific data to its own struct Frank Schäfer
2014-05-09 9:17 ` Hans Verkuil
2014-05-11 20:46 ` Frank Schäfer
2014-05-12 8:20 ` Hans Verkuil
2014-03-24 19:33 ` [PATCH 04/19] em28xx: move struct v4l2_ctrl_handler ctrl_handler from struct em28xx to struct v4l2 Frank Schäfer
2014-03-24 19:33 ` [PATCH 05/19] em28xx: move struct v4l2_clk *clk " Frank Schäfer
2014-03-24 19:33 ` [PATCH 06/19] em28xx: move video_device structs " Frank Schäfer
2014-05-09 9:19 ` Hans Verkuil [this message]
2014-05-11 20:50 ` Frank Schäfer
2014-05-12 8:09 ` Hans Verkuil
2014-03-24 19:33 ` [PATCH 07/19] em28xx: move videobuf2 related data " Frank Schäfer
2014-03-24 19:33 ` [PATCH 08/19] em28xx: move v4l2 frame resolutions and scale " Frank Schäfer
2014-03-24 19:33 ` [PATCH 09/19] em28xx: move vinmode and vinctrl " Frank Schäfer
2014-03-24 19:33 ` [PATCH 10/19] em28xx: move TV norm " Frank Schäfer
2014-03-24 19:33 ` [PATCH 11/19] em28xx: move struct em28xx_fmt *format " Frank Schäfer
2014-03-24 19:33 ` [PATCH 12/19] em28xx: move progressive/interlaced fields " Frank Schäfer
2014-03-24 19:33 ` [PATCH 13/19] em28xx: move sensor parameter " Frank Schäfer
2014-03-24 19:33 ` [PATCH 14/19] em28xx: move capture state tracking " Frank Schäfer
2014-03-24 19:33 ` [PATCH 15/19] em28xx: move v4l2 user counting " Frank Schäfer
2014-03-24 19:33 ` [PATCH 16/19] em28xx: move tuner frequency field " Frank Schäfer
2014-03-24 19:33 ` [PATCH 17/19] em28xx: remove field tda9887_conf from struct em28xx Frank Schäfer
2014-03-24 19:33 ` [PATCH 18/19] em28xx: remove field tuner_addr " Frank Schäfer
2014-03-24 19:33 ` [PATCH 19/19] em28xx: move fields wq_trigger and streaming_started from struct em28xx to struct em28xx_audio Frank Schäfer
2014-05-09 9:04 ` [PATCH 00/19] em28xx: clean up the main device struct and move sub-module specific data to its own data structs Hans Verkuil
2014-05-11 21:01 ` Frank Schäfer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=536C9D85.10504@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=fschaefer.oss@googlemail.com \
--cc=linux-media@vger.kernel.org \
--cc=m.chehab@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.