From: "Frank Schäfer" <fschaefer.oss@googlemail.com>
To: Hans Verkuil <hverkuil@xs4all.nl>, 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: Sun, 11 May 2014 22:50:07 +0200 [thread overview]
Message-ID: <536FE27F.1080504@googlemail.com> (raw)
In-Reply-To: <536C9D85.10504@xs4all.nl>
Am 09.05.2014 11:19, schrieb Hans Verkuil:
> 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.
Yes, but consider that only em286x and em288x devices provide VBI
support and we have even less devices with radio support (~ 3 of 100).
So with most devices, we would waste memory.
Regards,
Frank
>
> 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-11 20:49 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
2014-05-11 20:50 ` Frank Schäfer [this message]
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=536FE27F.1080504@googlemail.com \
--to=fschaefer.oss@googlemail.com \
--cc=hverkuil@xs4all.nl \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox