au0828-video.c

REVIEW Hans Verkuil 4 participants 13 messages Dec 12, 2014 Media (V4L/DVB)

lore

Share & tools

lore

Hans Verkuil

author

2014-12-12 10:16 UTC #0 lore raw

Hi Shuah,

This is the video.c review with your patch applied.

Quoted 22 lines

/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/

Quoted 3 lines

/* Developer Notes:
*
* VBI support is not yet working

I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.

Quoted 16 lines

* The hardware scaler supported is unimplemented
* AC97 audio support is unimplemented (only i2s audio mode) *
*/
#include "au0828.h"
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/device.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-event.h>
#include <media/tuner.h>
#include "au0828-reg.h"

<snip>

Quoted 12 lines

static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,

		       unsigned int *nbuffers, unsigned int *nplanes,
		       unsigned int sizes[], void *alloc_ctxs[])
{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	unsigned long size;

	if (fmt)
		size = fmt->fmt.pix.sizeimage;
	else
		size =
		    (dev->width * dev->height * dev->bytesperline + 7) >> 3;

This is wrong. It should be 'size = dev->bytesperline * dev->height;'

If fmt is set, then you need to check if the fmt size is large enough for the current format.

Quoted 2 lines

	if (size == 0)

This should be 'size < img_size'.

Quoted 4 lines

		return -EINVAL;

	if (0 == *nbuffers)
		*nbuffers = 32;

Bogus check.

Quoted 6 lines

	*nplanes = 1;
	sizes[0] = size;

	return 0;
}

I would rewrite this function as:

{      
       struct au0828_dev *dev = vb2_get_drv_priv(vq);
       unsigned img_size = dev->bytesperline * dev->height;
       unsigned long size;

       size = fmt ? fmt->fmt.pix.sizeimage : img_size;
       if (size < img_size)
               return -EINVAL;
                   
       *nplanes = 1;
       sizes[0] = size;
       return 0;
}

Quoted 8 lines

static int
buffer_prepare(struct vb2_buffer *vb)

{
	struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
	struct au0828_dev    *dev = vb2_get_drv_priv(vb->vb2_queue);

	buf->length = (dev->width * dev->height * 16 + 7) >> 3;

Use buf->length = dev->bytesperline * dev->height;

Quoted 110 lines

	if (vb2_plane_size(vb, 0) < buf->length) {
		pr_err("%s data will not fit into plane (%lu < %lu)\n",
			__func__, vb2_plane_size(vb, 0), buf->length);
		return -EINVAL;
	}
	vb2_set_plane_payload(&buf->vb, 0, buf->length);
	return 0;
}

static void
buffer_queue(struct vb2_buffer *vb)

{
	struct au0828_buffer    *buf     = container_of(vb,
							struct au0828_buffer,
							vb);
	struct au0828_dev       *dev     = vb2_get_drv_priv(vb->vb2_queue);
	struct au0828_dmaqueue  *vidq    = &dev->vidq;
	unsigned long flags = 0;

	buf->mem = vb2_plane_vaddr(vb, 0);
	buf->length = vb2_plane_size(vb, 0);

	spin_lock_irqsave(&dev->slock, flags);
	list_add_tail(&buf->list, &vidq->active);
	spin_unlock_irqrestore(&dev->slock, flags);
}

static int au0828_i2s_init(struct au0828_dev *dev)

{
	/* Enable i2s mode */
	au0828_writereg(dev, AU0828_AUDIOCTRL_50C, 0x01);
	return 0;
}

/*
* Auvitek au0828 analog stream enable
*/
static int au0828_analog_stream_enable(struct au0828_dev *d)

{
	struct usb_interface *iface;
	int ret, h, w;

	dprintk(1, "au0828_analog_stream_enable called\n");

	iface = usb_ifnum_to_if(d->usbdev, 0);
	if (iface && iface->cur_altsetting->desc.bAlternateSetting != 5) {
		dprintk(1, "Changing intf#0 to alt 5\n");
		/* set au0828 interface0 to AS5 here again */
		ret = usb_set_interface(d->usbdev, 0, 5);
		if (ret < 0) {
			pr_info("Au0828 can't set alt setting to 5!\n");
			return -EBUSY;
		}
	}

	h = d->height / 2 + 2;
	w = d->width * 2;

	au0828_writereg(d, AU0828_SENSORCTRL_VBI_103, 0x00);
	au0828_writereg(d, 0x106, 0x00);
	/* set x position */
	au0828_writereg(d, 0x110, 0x00);
	au0828_writereg(d, 0x111, 0x00);
	au0828_writereg(d, 0x114, w & 0xff);
	au0828_writereg(d, 0x115, w >> 8);
	/* set y position */
	au0828_writereg(d, 0x112, 0x00);
	au0828_writereg(d, 0x113, 0x00);
	au0828_writereg(d, 0x116, h & 0xff);
	au0828_writereg(d, 0x117, h >> 8);
	au0828_writereg(d, AU0828_SENSORCTRL_100, 0xb3);

	return 0;
}

static int au0828_analog_stream_disable(struct au0828_dev *d)

{
	dprintk(1, "au0828_analog_stream_disable called\n");
	au0828_writereg(d, AU0828_SENSORCTRL_100, 0x0);
	return 0;
}

static void au0828_analog_stream_reset(struct au0828_dev *dev)

{
	dprintk(1, "au0828_analog_stream_reset called\n");
	au0828_writereg(dev, AU0828_SENSORCTRL_100, 0x0);
	mdelay(30);
	au0828_writereg(dev, AU0828_SENSORCTRL_100, 0xb3);
}

/*
* Some operations needs to stop current streaming */
static int au0828_stream_interrupt(struct au0828_dev *dev)

{
	int ret = 0;

	dev->stream_state = STREAM_INTERRUPT;
	if (dev->dev_state == DEV_DISCONNECTED)
		return -ENODEV;
	else if (ret) {
		dev->dev_state = DEV_MISCONFIGURED;
		dprintk(1, "%s device is misconfigured!\n", __func__);
		return ret;
	}
	return 0;
}

int au0828_start_analog_streaming(struct vb2_queue *vq, unsigned int count)

Quoted 11 lines

{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	int rc = 0;

	dprintk(1, "au0828_start_analog_streaming called %d\n",
		dev->streaming_users);

	/* Make sure streaming is not already in progress for this type
	   of filehandle (e.g. video, vbi) */
	if (vb2_is_streaming(vq))
		return -EBUSY;

Can never happen, so this test can be removed.

Quoted 1 line

You need to initialize the counters:

       if (vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
               dev->field_count = 0;
       else
               dev->vbi_field_count = 0;

I would also recommend renaming 'field' to 'frame', since you're counting frames, not fields.

Quoted 26 lines

	if (dev->streaming_users == 0) {
		/* If we were doing ac97 instead of i2s, it would go here...*/
		au0828_i2s_init(dev);
		rc = au0828_init_isoc(dev, AU0828_ISO_PACKETS_PER_URB,
				   AU0828_MAX_ISO_BUFS, dev->max_pkt_size,
				   au0828_isoc_copy);
		if (rc < 0) {
			pr_info("au0828_init_isoc failed\n");
			return rc;
		}

		if (vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
			v4l2_device_call_all(&dev->v4l2_dev, 0, video,
						s_stream, 1);
			dev->vid_timeout_running = 1;
			mod_timer(&dev->vid_timeout, jiffies + (HZ / 10));
		} else if (vq->type == V4L2_BUF_TYPE_VBI_CAPTURE) {
			dev->vbi_timeout_running = 1;
			mod_timer(&dev->vbi_timeout, jiffies + (HZ / 10));
		}
	}
	dev->streaming_users++;
	return rc;
}

static void au0828_stop_streaming(struct vb2_queue *vq)

Quoted 10 lines

{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	struct au0828_dmaqueue *vidq = &dev->vidq;
	unsigned long flags = 0;
	int i;

	dprintk(1, "au0828_stop_streaming called %d\n", dev->streaming_users);

	if (!vb2_is_streaming(vq))
		return;

Unnecessary test.

Quoted 21 lines

	if (dev->streaming_users-- == 1)
		au0828_uninit_isoc(dev);
	dprintk(1, "au0828_stop_streaming %d\n", dev->streaming_users);

	spin_lock_irqsave(&dev->slock, flags);
	if (dev->isoc_ctl.buf != NULL) {
		vb2_buffer_done(&dev->isoc_ctl.buf->vb, VB2_BUF_STATE_ERROR);
		dev->isoc_ctl.buf = NULL;
	}
	while (!list_empty(&vidq->active)) {
		struct au0828_buffer *buf;

		buf = list_entry(vidq->active.next, struct au0828_buffer, list);
		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
		list_del(&buf->list);
	}

	dev->vid_timeout_running = 0;
	del_timer_sync(&dev->vid_timeout);

Quoted 8 lines

	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);

	for (i = 0; i < AU0828_MAX_INPUT; i++) {
		if (AUVI_INPUT(i).audio_setup == NULL)
			continue;
		(AUVI_INPUT(i).audio_setup)(dev, 0);
	}
	spin_unlock_irqrestore(&dev->slock, flags);

Very bad. This unlock should go before the 'dev->vid_timeout_running = 0;' line. The way it is now the lines between the spinlock run without interrupts, so calls like del_timer_sync() will complain about scheduling in atomic context.

Hint: you should turn on at least the basic lock checks that the kernel offers when testing.

Quoted 27 lines

}

void au0828_stop_vbi_streaming(struct vb2_queue *vq)

{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	struct au0828_dmaqueue *vbiq = &dev->vbiq;
	unsigned long flags = 0;

	dprintk(1, "au0828_stop_vbi_streaming called %d\n",
		dev->streaming_users);
	if (!vb2_is_streaming(vq))
		return;

	if (dev->streaming_users-- == 1)
		au0828_uninit_isoc(dev);
	dprintk(1, "au0828_stop_vbi_streaming %d\n", dev->streaming_users);

	spin_lock_irqsave(&dev->slock, flags);
	if (dev->isoc_ctl.vbi_buf != NULL) {
		vb2_buffer_done(&dev->isoc_ctl.vbi_buf->vb,
				VB2_BUF_STATE_ERROR);
		dev->isoc_ctl.vbi_buf = NULL;
	}
	while (!list_empty(&vbiq->active)) {
		struct au0828_buffer *buf;

		buf = list_entry(vbiq->active.next, struct au0828_buffer, list);

Quoted 8 lines

		list_del(&buf->list);
		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
	}

	dev->vbi_timeout_running = 0;
	del_timer_sync(&dev->vbi_timeout);

	spin_unlock_irqrestore(&dev->slock, flags);

Same problem here, move up the spin_unlock to before the 'dev->vbi_timeout_running = 0;' line.

Quoted 32 lines

}
static struct vb2_ops au0828_video_qops = {
	.queue_setup     = queue_setup,
	.buf_prepare     = buffer_prepare,
	.buf_queue       = buffer_queue,
.start_streaming = au0828_start_analog_streaming, .stop_streaming = au0828_stop_streaming,
.wait_prepare = vb2_ops_wait_prepare,
.wait_finish = vb2_ops_wait_finish,
};
/* ------------------------------------------------------------------
V4L2 interface ------------------------------------------------------------------*/
/*
* au0828_analog_unregister
* unregister v4l2 devices
*/
void au0828_analog_unregister(struct au0828_dev *dev)
{
	dprintk(1, "au0828_analog_unregister called\n");
	mutex_lock(&au0828_sysfs_lock);

	if (dev->vdev)
		video_unregister_device(dev->vdev);
	if (dev->vbi_dev)
		video_unregister_device(dev->vbi_dev);

	mutex_unlock(&au0828_sysfs_lock);
}

Quoted 3 lines

/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */

Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).

It's not the driver's responsibility to do this and it only makes the code overly complex.

Quoted 74 lines

static void au0828_vid_buffer_timeout(unsigned long data)

{
	struct au0828_dev *dev = (struct au0828_dev *) data;
	struct au0828_dmaqueue *dma_q = &dev->vidq;
	struct au0828_buffer *buf;
	unsigned char *vid_data;
	unsigned long flags = 0;

	spin_lock_irqsave(&dev->slock, flags);

	buf = dev->isoc_ctl.buf;
	if (buf != NULL) {
		vid_data = vb2_plane_vaddr(&buf->vb, 0);
		memset(vid_data, 0x00, buf->length); /* Blank green frame */
		buffer_filled(dev, dma_q, buf);
	}
	get_next_buf(dma_q, &buf);

	if (dev->vid_timeout_running == 1)
		mod_timer(&dev->vid_timeout, jiffies + (HZ / 10));

	spin_unlock_irqrestore(&dev->slock, flags);
}

static void au0828_vbi_buffer_timeout(unsigned long data)

{
	struct au0828_dev *dev = (struct au0828_dev *) data;
	struct au0828_dmaqueue *dma_q = &dev->vbiq;
	struct au0828_buffer *buf;
	unsigned char *vbi_data;
	unsigned long flags = 0;

	spin_lock_irqsave(&dev->slock, flags);

	buf = dev->isoc_ctl.vbi_buf;
	if (buf != NULL) {
		vbi_data = vb2_plane_vaddr(&buf->vb, 0);
		memset(vbi_data, 0x00, buf->length);
		vbi_buffer_filled(dev, dma_q, buf);
	}
	vbi_get_next_buf(dma_q, &buf);

	if (dev->vbi_timeout_running == 1)
		mod_timer(&dev->vbi_timeout, jiffies + (HZ / 10));
	spin_unlock_irqrestore(&dev->slock, flags);
}

static int au0828_v4l2_open(struct file *filp)

{
	struct video_device *vdev = video_devdata(filp);
	struct au0828_dev *dev = video_drvdata(filp);
	int type;
	int ret = 0;

	switch (vdev->vfl_type) {
	case VFL_TYPE_GRABBER:
		type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
		break;
	case VFL_TYPE_VBI:
		type = V4L2_BUF_TYPE_VBI_CAPTURE;
		break;
	default:
		return -EINVAL;
	}

	dprintk(1,
		"%s called std_set %d dev_state %d stream users %d users %d\n",
		__func__, dev->std_set_in_tuner_core, dev->dev_state,
		dev->streaming_users, dev->users);

	if (mutex_lock_interruptible(&dev->lock))
		return -ERESTARTSYS;

	ret = v4l2_fh_open(filp);

Quoted 8 lines

	if (ret) {
		au0828_isocdbg("%s: v4l2_fh_open() returned error %d\n",
				__func__, ret);
		mutex_unlock(&dev->lock);
		return ret;
	}

	if (dev->users == 0) {

Replace with 'v4l2_fh_is_singular_file(filp)'. That way you can drop the user field. This can be a separate patch.

Quoted 11 lines

		au0828_analog_stream_enable(dev);
		au0828_analog_stream_reset(dev);
		dev->stream_state = STREAM_OFF;
		dev->dev_state |= DEV_INITIALIZED;
	}
	dev->users++;
	mutex_unlock(&dev->lock);
	return ret;
}

static int au0828_v4l2_close(struct file *filp)

Quoted 11 lines

{
	int ret;
	struct au0828_dev *dev = video_drvdata(filp);
	struct video_device *vdev = video_devdata(filp);

	dprintk(1,
		"%s called std_set %d dev_state %d stream users %d users %d\n",
		__func__, dev->std_set_in_tuner_core, dev->dev_state,
		dev->streaming_users, dev->users);

	vb2_fop_release(filp);

Drop this vb2_fop_release() call, will move to the end of the function.

Quoted 1 line

	mutex_lock(&dev->lock);

Change to:

	mutex_lock(&dev->lock);

Quoted 7 lines

	if (vdev->vfl_type == VFL_TYPE_GRABBER && dev->vid_timeout_running) {
		/* Cancel timeout thread in case they didn't call streamoff */
		dev->vid_timeout_running = 0;
		del_timer_sync(&dev->vid_timeout);
	} else if (vdev->vfl_type == VFL_TYPE_VBI &&
			dev->vbi_timeout_running) {
		/* Cancel timeout thread in case they didn't call streamoff */

Quoted 8 lines

		dev->vbi_timeout_running = 0;
		del_timer_sync(&dev->vbi_timeout);
	}

	if (dev->dev_state == DEV_DISCONNECTED)
		goto end;

	if (dev->users == 1) {

And:

	if (v4l2_fh_is_singular_file(filp))

Quoted 11 lines

		/* Save some power by putting tuner to sleep */
		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
		dev->std_set_in_tuner_core = 0;

		/* When close the device, set the usb intf0 into alt0 to free
		   USB bandwidth */
		ret = usb_set_interface(dev->usbdev, 0, 0);
		if (ret < 0)
			pr_info("Au0828 can't set alternate to 0!\n");
	}

end:

And the release goes here:

	_vb2_fop_release(filp, NULL);

Quoted 3 lines

	dev->users--;
	mutex_unlock(&dev->lock);
	wake_up_interruptible_nr(&dev->open, 1);

Huh? Seems unused and certainly looks very bogus.

Quoted 787 lines

	return 0;
}

/* Must be called with dev->lock held */ static void au0828_init_tuner(struct au0828_dev *dev)

{
	struct v4l2_frequency f = {
		.frequency = dev->ctrl_freq,
		.type = V4L2_TUNER_ANALOG_TV,
	};

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	if (dev->std_set_in_tuner_core)
		return;
	dev->std_set_in_tuner_core = 1;
	i2c_gate_ctrl(dev, 1);
	/* If we've never sent the standard in tuner core, do so now.
	   We don't do this at device probe because we don't want to
	   incur the cost of a firmware load */
	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_std, dev->std);
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, &f);
	i2c_gate_ctrl(dev, 0);
}

static int au0828_set_format(struct au0828_dev *dev, unsigned int cmd,
struct v4l2_format *format)

{
	int ret;
	int width = format->fmt.pix.width;
	int height = format->fmt.pix.height;

	/* If they are demanding a format other than the one we support,
	   bail out (tvtime asks for UYVY and then retries with YUYV) */
	if (format->fmt.pix.pixelformat != V4L2_PIX_FMT_UYVY)
		return -EINVAL;

	/* format->fmt.pix.width only support 720 and height 480 */
	if (width != 720)
		width = 720;
	if (height != 480)
		height = 480;

	format->fmt.pix.width = width;
	format->fmt.pix.height = height;
	format->fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
	format->fmt.pix.bytesperline = width * 2;
	format->fmt.pix.sizeimage = width * height * 2;
	format->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
	format->fmt.pix.field = V4L2_FIELD_INTERLACED;
	format->fmt.pix.priv = 0;

	if (cmd == VIDIOC_TRY_FMT)
		return 0;

	/* maybe set new image format, driver current only support 720*480 */
	dev->width = width;
	dev->height = height;
	dev->frame_size = width * height * 2;
	dev->field_size = width * height;
	dev->bytesperline = width * 2;

	if (dev->stream_state == STREAM_ON) {
		dprintk(1, "VIDIOC_SET_FMT: interrupting stream!\n");
		ret = au0828_stream_interrupt(dev);
		if (ret != 0) {
			dprintk(1, "error interrupting video stream!\n");
			return ret;
		}
	}

	au0828_analog_stream_enable(dev);

	return 0;
}

static int vidioc_querycap(struct file *file, void *priv,
struct v4l2_capability *cap)

{
	struct video_device *vdev = video_devdata(file);
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	strlcpy(cap->driver, "au0828", sizeof(cap->driver));
	strlcpy(cap->card, dev->board.name, sizeof(cap->card));
	usb_make_path(dev->usbdev, cap->bus_info, sizeof(cap->bus_info));

	/* set the device capabilities */
	cap->device_caps = V4L2_CAP_AUDIO |
		V4L2_CAP_READWRITE |
		V4L2_CAP_STREAMING |
		V4L2_CAP_TUNER;
	if (vdev->vfl_type == VFL_TYPE_GRABBER)
		cap->device_caps |= V4L2_CAP_VIDEO_CAPTURE;
	else
		cap->device_caps |= V4L2_CAP_VBI_CAPTURE;
	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS |
		V4L2_CAP_VBI_CAPTURE | V4L2_CAP_VIDEO_CAPTURE;
	return 0;
}

static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_fmtdesc *f)

{
	if (f->index)
		return -EINVAL;

	dprintk(1, "%s called\n", __func__);

	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
	strcpy(f->description, "Packed YUV2");

	f->flags = 0;
	f->pixelformat = V4L2_PIX_FMT_UYVY;

	return 0;
}

static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	f->fmt.pix.width = dev->width;
	f->fmt.pix.height = dev->height;
	f->fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
	f->fmt.pix.bytesperline = dev->bytesperline;
	f->fmt.pix.sizeimage = dev->frame_size;
	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; /* NTSC/PAL */
	f->fmt.pix.field = V4L2_FIELD_INTERLACED;
	f->fmt.pix.priv = 0;
	return 0;
}

static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	return au0828_set_format(dev, VIDIOC_TRY_FMT, f);
}

static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)

{
	struct au0828_dev *dev = video_drvdata(file);
	int rc;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	rc = check_dev(dev);
	if (rc < 0)
		return rc;

	if (vb2_is_busy(&dev->vb_vidq)) {
		pr_info("%s queue busy\n", __func__);
		rc = -EBUSY;
		goto out;
	}

	rc = au0828_set_format(dev, VIDIOC_S_FMT, f);
out:
	return rc;
}

static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	if (norm == dev->std)
		return 0;

	if (dev->streaming_users > 0)
		return -EBUSY;

	dev->std = norm;

	au0828_init_tuner(dev);

	i2c_gate_ctrl(dev, 1);

	/*
	 * FIXME: when we support something other than 60Hz standards,
	 * we are going to have to make the au0828 bridge adjust the size
	 * of its capture buffer, which is currently hardcoded at 720x480
	 */

	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_std, norm);

	i2c_gate_ctrl(dev, 0);

	return 0;
}

static int vidioc_g_std(struct file *file, void *priv, v4l2_std_id *norm)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	*norm = dev->std;
	return 0;
}

static int vidioc_enum_input(struct file *file, void *priv,
struct v4l2_input *input)

{
	struct au0828_dev *dev = video_drvdata(file);
	unsigned int tmp;

	static const char *inames[] = {
		[AU0828_VMUX_UNDEFINED] = "Undefined",
		[AU0828_VMUX_COMPOSITE] = "Composite",
		[AU0828_VMUX_SVIDEO] = "S-Video",
		[AU0828_VMUX_CABLE] = "Cable TV",
		[AU0828_VMUX_TELEVISION] = "Television",
		[AU0828_VMUX_DVB] = "DVB",
		[AU0828_VMUX_DEBUG] = "tv debug"
	};

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	tmp = input->index;

	if (tmp >= AU0828_MAX_INPUT)
		return -EINVAL;
	if (AUVI_INPUT(tmp).type == 0)
		return -EINVAL;

	input->index = tmp;
	strcpy(input->name, inames[AUVI_INPUT(tmp).type]);
	if ((AUVI_INPUT(tmp).type == AU0828_VMUX_TELEVISION) ||
	    (AUVI_INPUT(tmp).type == AU0828_VMUX_CABLE)) {
		input->type |= V4L2_INPUT_TYPE_TUNER;
		input->audioset = 1;
	} else {
		input->type |= V4L2_INPUT_TYPE_CAMERA;
		input->audioset = 2;
	}

	input->std = dev->vdev->tvnorms;

	return 0;
}

static int vidioc_g_input(struct file *file, void *priv, unsigned int *i)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	*i = dev->ctrl_input;
	return 0;
}

static void au0828_s_input(struct au0828_dev *dev, int index)

{
	int i;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	switch (AUVI_INPUT(index).type) {
	case AU0828_VMUX_SVIDEO:
		dev->input_type = AU0828_VMUX_SVIDEO;
		dev->ctrl_ainput = 1;
		break;
	case AU0828_VMUX_COMPOSITE:
		dev->input_type = AU0828_VMUX_COMPOSITE;
		dev->ctrl_ainput = 1;
		break;
	case AU0828_VMUX_TELEVISION:
		dev->input_type = AU0828_VMUX_TELEVISION;
		dev->ctrl_ainput = 0;
		break;
	default:
		dprintk(1, "unknown input type set [%d]\n",
			AUVI_INPUT(index).type);
		break;
	}

	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
			AUVI_INPUT(index).vmux, 0, 0);

	for (i = 0; i < AU0828_MAX_INPUT; i++) {
		int enable = 0;
		if (AUVI_INPUT(i).audio_setup == NULL)
			continue;

		if (i == index)
			enable = 1;
		else
			enable = 0;
		if (enable) {
			(AUVI_INPUT(i).audio_setup)(dev, enable);
		} else {
			/* Make sure we leave it turned on if some
			   other input is routed to this callback */
			if ((AUVI_INPUT(i).audio_setup) !=
			    ((AUVI_INPUT(index).audio_setup))) {
				(AUVI_INPUT(i).audio_setup)(dev, enable);
			}
		}
	}

	v4l2_device_call_all(&dev->v4l2_dev, 0, audio, s_routing,
			AUVI_INPUT(index).amux, 0, 0);
}

static int vidioc_s_input(struct file *file, void *priv, unsigned int index)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "VIDIOC_S_INPUT in function %s, input=%d\n", __func__,
		index);
	if (index >= AU0828_MAX_INPUT)
		return -EINVAL;
	if (AUVI_INPUT(index).type == 0)
		return -EINVAL;
	dev->ctrl_input = index;
	au0828_s_input(dev, index);
	return 0;
}

static int vidioc_enumaudio(struct file *file, void *priv, struct v4l2_audio *a)

{
	if (a->index > 1)
		return -EINVAL;

	dprintk(1, "%s called\n", __func__);

	if (a->index == 0)
		strcpy(a->name, "Television");
	else
		strcpy(a->name, "Line in");

	a->capability = V4L2_AUDCAP_STEREO;
	return 0;
}

static int vidioc_g_audio(struct file *file, void *priv, struct v4l2_audio *a)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	a->index = dev->ctrl_ainput;
	if (a->index == 0)
		strcpy(a->name, "Television");
	else
		strcpy(a->name, "Line in");

	a->capability = V4L2_AUDCAP_STEREO;
	return 0;
}

static int vidioc_s_audio(struct file *file, void *priv, const struct v4l2_audio *a)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (a->index != dev->ctrl_ainput)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);
	return 0;
}

static int vidioc_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (t->index != 0)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	strcpy(t->name, "Auvitek tuner");

	au0828_init_tuner(dev);
	i2c_gate_ctrl(dev, 1);
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, g_tuner, t);
	i2c_gate_ctrl(dev, 0);
	return 0;
}

static int vidioc_s_tuner(struct file *file, void *priv,
const struct v4l2_tuner *t)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (t->index != 0)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	au0828_init_tuner(dev);
	i2c_gate_ctrl(dev, 1);
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_tuner, t);
	i2c_gate_ctrl(dev, 0);

	dprintk(1, "VIDIOC_S_TUNER: signal = %x, afc = %x\n", t->signal,
		t->afc);

	return 0;

}

static int vidioc_g_frequency(struct file *file, void *priv,
struct v4l2_frequency *freq)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (freq->tuner != 0)
		return -EINVAL;
	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);
	freq->frequency = dev->ctrl_freq;
	return 0;
}

static int vidioc_s_frequency(struct file *file, void *priv,
const struct v4l2_frequency *freq)

{
	struct au0828_dev *dev = video_drvdata(file);
	struct v4l2_frequency new_freq = *freq;

	if (freq->tuner != 0)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	au0828_init_tuner(dev);
	i2c_gate_ctrl(dev, 1);

	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, freq);
	/* Get the actual set (and possibly clamped) frequency */
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, g_frequency, &new_freq);
	dev->ctrl_freq = new_freq.frequency;

	i2c_gate_ctrl(dev, 0);

	au0828_analog_stream_reset(dev);

	return 0;
}

/* RAW VBI ioctls */

static int vidioc_g_fmt_vbi_cap(struct file *file, void *priv,
struct v4l2_format *format)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	format->fmt.vbi.samples_per_line = dev->vbi_width;
	format->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
	format->fmt.vbi.offset = 0;
	format->fmt.vbi.flags = 0;
	format->fmt.vbi.sampling_rate = 6750000 * 4 / 2;

	format->fmt.vbi.count[0] = dev->vbi_height;
	format->fmt.vbi.count[1] = dev->vbi_height;
	format->fmt.vbi.start[0] = 21;
	format->fmt.vbi.start[1] = 284;
	memset(format->fmt.vbi.reserved, 0, sizeof(format->fmt.vbi.reserved));

	return 0;
}

static int vidioc_cropcap(struct file *file, void *priv,
struct v4l2_cropcap *cc)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (cc->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	cc->bounds.left = 0;
	cc->bounds.top = 0;
	cc->bounds.width = dev->width;
	cc->bounds.height = dev->height;

	cc->defrect = cc->bounds;

	cc->pixelaspect.numerator = 54;
	cc->pixelaspect.denominator = 59;

	return 0;
}

#ifdef CONFIG_VIDEO_ADV_DEBUG
static int vidioc_g_register(struct file *file, void *priv,
struct v4l2_dbg_register *reg)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	reg->val = au0828_read(dev, reg->reg);
	reg->size = 1;
	return 0;
}

static int vidioc_s_register(struct file *file, void *priv,
const struct v4l2_dbg_register *reg)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	return au0828_writereg(dev, reg->reg, reg->val);
}

#endif

static int vidioc_log_status(struct file *file, void *fh)

{
	struct video_device *vdev = video_devdata(file);

	dprintk(1, "%s called\n", __func__);

	v4l2_ctrl_log_status(file, fh);
	v4l2_device_call_all(vdev->v4l2_dev, 0, core, log_status);
	return 0;
}

void au0828_v4l2_suspend(struct au0828_dev *dev)

{
	struct urb *urb;
	int i;

	pr_info("stopping V4L2\n");

	if (dev->stream_state == STREAM_ON) {
		pr_info("stopping V4L2 active URBs\n");
		au0828_analog_stream_disable(dev);
		/* stop urbs */
		for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
			urb = dev->isoc_ctl.urb[i];
			if (urb) {
				if (!irqs_disabled())
					usb_kill_urb(urb);
				else
					usb_unlink_urb(urb);
			}
		}
	}

	if (dev->vid_timeout_running)
		del_timer_sync(&dev->vid_timeout);
	if (dev->vbi_timeout_running)
		del_timer_sync(&dev->vbi_timeout);
}

void au0828_v4l2_resume(struct au0828_dev *dev)

{
	int i, rc;

	pr_info("restarting V4L2\n");

	if (dev->stream_state == STREAM_ON) {
		au0828_stream_interrupt(dev);
		au0828_init_tuner(dev);
	}

	if (dev->vid_timeout_running)
		mod_timer(&dev->vid_timeout, jiffies + (HZ / 10));
	if (dev->vbi_timeout_running)
		mod_timer(&dev->vbi_timeout, jiffies + (HZ / 10));

	/* If we were doing ac97 instead of i2s, it would go here...*/
	au0828_i2s_init(dev);

	au0828_analog_stream_enable(dev);

	if (!(dev->stream_state == STREAM_ON)) {
		au0828_analog_stream_reset(dev);
		/* submit urbs */
		for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
			rc = usb_submit_urb(dev->isoc_ctl.urb[i], GFP_ATOMIC);
			if (rc) {
				au0828_isocdbg("submit of urb %i failed (error=%i)\n",
					       i, rc);
				au0828_uninit_isoc(dev);
			}
		}
	}
}

static struct v4l2_file_operations au0828_v4l_fops = {

	.owner      = THIS_MODULE,
	.open       = au0828_v4l2_open,
	.release    = au0828_v4l2_close,
	.read       = vb2_fop_read,
	.poll       = vb2_fop_poll,
	.mmap       = vb2_fop_mmap,

.unlocked_ioctl = video_ioctl2,
};

static const struct v4l2_ioctl_ops video_ioctl_ops = {

	.vidioc_querycap            = vidioc_querycap,
	.vidioc_enum_fmt_vid_cap    = vidioc_enum_fmt_vid_cap,
	.vidioc_g_fmt_vid_cap       = vidioc_g_fmt_vid_cap,
	.vidioc_try_fmt_vid_cap     = vidioc_try_fmt_vid_cap,
	.vidioc_s_fmt_vid_cap       = vidioc_s_fmt_vid_cap,
	.vidioc_g_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
	.vidioc_try_fmt_vbi_cap     = vidioc_g_fmt_vbi_cap,
	.vidioc_s_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
	.vidioc_enumaudio           = vidioc_enumaudio,
	.vidioc_g_audio             = vidioc_g_audio,
	.vidioc_s_audio             = vidioc_s_audio,
	.vidioc_cropcap             = vidioc_cropcap,

	.vidioc_reqbufs             = vb2_ioctl_reqbufs,
	.vidioc_create_bufs         = vb2_ioctl_create_bufs,
	.vidioc_prepare_buf         = vb2_ioctl_prepare_buf,
	.vidioc_querybuf            = vb2_ioctl_querybuf,
	.vidioc_qbuf                = vb2_ioctl_qbuf,
	.vidioc_dqbuf               = vb2_ioctl_dqbuf,

	.vidioc_s_std               = vidioc_s_std,
	.vidioc_g_std               = vidioc_g_std,
	.vidioc_enum_input          = vidioc_enum_input,
	.vidioc_g_input             = vidioc_g_input,
	.vidioc_s_input             = vidioc_s_input,

.vidioc_streamon = vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,

.vidioc_g_tuner = vidioc_g_tuner,
.vidioc_s_tuner = vidioc_s_tuner,
.vidioc_g_frequency = vidioc_g_frequency,
.vidioc_s_frequency = vidioc_s_frequency,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.vidioc_g_register = vidioc_g_register,
.vidioc_s_register = vidioc_s_register,
#endif
.vidioc_log_status = vidioc_log_status,
.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
};

static const struct video_device au0828_video_template = {
.fops = &au0828_v4l_fops,
.release = video_device_release,
.ioctl_ops = &video_ioctl_ops,
.tvnorms = V4L2_STD_NTSC_M | V4L2_STD_PAL_M,
};

static int au0828_vb2_setup(struct au0828_dev *dev)

{
	int rc;
	struct vb2_queue *q;

	/* Setup Videobuf2 for Video capture */
	q = &dev->vb_vidq;
	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
	q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
	q->drv_priv = dev;
	q->buf_struct_size = sizeof(struct au0828_buffer);
	q->ops = &au0828_video_qops;
	q->mem_ops = &vb2_vmalloc_memops;

	rc = vb2_queue_init(q);
	if (rc < 0)
		return rc;

	/* Setup Videobuf2 for VBI capture */
	q = &dev->vb_vbiq;
	q->type = V4L2_BUF_TYPE_VBI_CAPTURE;
	q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR;
	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
	q->drv_priv = dev;
	q->buf_struct_size = sizeof(struct au0828_buffer);
	q->ops = &au0828_vbi_qops;
	q->mem_ops = &vb2_vmalloc_memops;

	rc = vb2_queue_init(q);
	if (rc < 0)
		return rc;

	return 0;
}

/**************************************************************************/

int au0828_analog_register(struct au0828_dev *dev,
struct usb_interface *interface)

{
	int retval = -ENOMEM;
	struct usb_host_interface *iface_desc;
	struct usb_endpoint_descriptor *endpoint;
	int i, ret;

	dprintk(1, "au0828_analog_register called for intf#%d!\n",
		interface->cur_altsetting->desc.bInterfaceNumber);

	/* set au0828 usb interface0 to as5 */
	retval = usb_set_interface(dev->usbdev,
			interface->cur_altsetting->desc.bInterfaceNumber, 5);
	if (retval != 0) {
		pr_info("Failure setting usb interface0 to as5\n");
		return retval;
	}

	/* Figure out which endpoint has the isoc interface */
	iface_desc = interface->cur_altsetting;
	for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
		endpoint = &iface_desc->endpoint[i].desc;
		if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
		     == USB_DIR_IN) &&
		    ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
		     == USB_ENDPOINT_XFER_ISOC)) {

			/* we find our isoc in endpoint */
			u16 tmp = le16_to_cpu(endpoint->wMaxPacketSize);
			dev->max_pkt_size = (tmp & 0x07ff) *
				(((tmp & 0x1800) >> 11) + 1);
			dev->isoc_in_endpointaddr = endpoint->bEndpointAddress;
			dprintk(1,
				"Found isoc endpoint 0x%02x, max size = %d\n",
				dev->isoc_in_endpointaddr, dev->max_pkt_size);
		}
	}
	if (!(dev->isoc_in_endpointaddr)) {
		pr_info("Could not locate isoc endpoint\n");
		kfree(dev);
		return -ENODEV;
	}

	init_waitqueue_head(&dev->open);
	spin_lock_init(&dev->slock);

	/* init video dma queues */
	INIT_LIST_HEAD(&dev->vidq.active);
	INIT_LIST_HEAD(&dev->vbiq.active);

	dev->vid_timeout.function = au0828_vid_buffer_timeout;
	dev->vid_timeout.data = (unsigned long) dev;
	init_timer(&dev->vid_timeout);

	dev->vbi_timeout.function = au0828_vbi_buffer_timeout;
	dev->vbi_timeout.data = (unsigned long) dev;
	init_timer(&dev->vbi_timeout);

	dev->width = NTSC_STD_W;
	dev->height = NTSC_STD_H;
	dev->field_size = dev->width * dev->height;
	dev->frame_size = dev->field_size << 1;
	dev->bytesperline = dev->width << 1;
	dev->vbi_width = 720;
	dev->vbi_height = 1;
	dev->ctrl_ainput = 0;
	dev->ctrl_freq = 960;
	dev->std = V4L2_STD_NTSC_M;
	au0828_s_input(dev, 0);

	/* allocate and fill v4l2 video struct */

Quoted 8 lines

	dev->vdev = video_device_alloc();
	if (NULL == dev->vdev) {
		dprintk(1, "Can't allocate video_device.\n");
		return -ENOMEM;
	}

	/* allocate the VBI struct */
	dev->vbi_dev = video_device_alloc();

I recommend that these video_device structs are embedded in au0828_dev. That way you don't need to check for errors here.

Quoted 39 lines

	if (NULL == dev->vbi_dev) {
		dprintk(1, "Can't allocate vbi_device.\n");
		ret = -ENOMEM;
		goto err_vdev;
	}

	mutex_init(&dev->vb_queue_lock);
	mutex_init(&dev->vb_vbi_queue_lock);

	/* Fill the video capture device struct */
	*dev->vdev = au0828_video_template;
	dev->vdev->v4l2_dev = &dev->v4l2_dev;
	dev->vdev->lock = &dev->lock;
	dev->vdev->queue = &dev->vb_vidq;
	dev->vdev->queue->lock = &dev->vb_queue_lock;
	strcpy(dev->vdev->name, "au0828a video");

	/* Setup the VBI device */
	*dev->vbi_dev = au0828_video_template;
	dev->vbi_dev->v4l2_dev = &dev->v4l2_dev;
	dev->vbi_dev->lock = &dev->lock;
	dev->vbi_dev->queue = &dev->vb_vbiq;
	dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
	strcpy(dev->vbi_dev->name, "au0828a vbi");

	/* Register the v4l2 device */
	video_set_drvdata(dev->vdev, dev);
	retval = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);
	if (retval != 0) {
		dprintk(1, "unable to register video device (error = %d).\n",
			retval);
		ret = -ENODEV;
		goto err_vbi_dev;
	}

	/* Register the vbi device */
	video_set_drvdata(dev->vbi_dev, dev);
	retval = video_register_device(dev->vbi_dev, VFL_TYPE_VBI, -1);
	if (retval != 0) {

Quoted 8 lines

		dprintk(1, "unable to register vbi device (error = %d).\n",
			retval);
		ret = -ENODEV;
		goto err_vbi_dev;
	}

	/* initialize videobuf2 stuff */
	retval = au0828_vb2_setup(dev);

This should be done *before* calling video_register_device.

Quoted 11 lines

	if (retval != 0) {
		dprintk(1, "unable to setup videobuf2 queues (error = %d).\n",
			retval);
		ret = -ENODEV;
		goto err_vbi_dev;
	}

	dprintk(1, "%s completed!\n", __func__);

	return 0;

Quoted 7 lines

err_vbi_dev:

	video_device_release(dev->vbi_dev);
err_vdev:
	video_device_release(dev->vdev);
	return ret;
}

Regards,

Hans

Mauro Carvalho Chehab

2014-12-12 12:49 UTC #1 lore raw

Em Fri, 12 Dec 2014 11:16:01 +0100
Hans Verkuil <[email protected]> escreveu:

Quoted 26 lines

Hi Shuah,
This is the video.c review with your patch applied.
Quoted 22 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/

Quoted 7 lines

Quoted 3 lines
/* Developer Notes:
*
* VBI support is not yet working
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.

This is a left-over. VBI support works on this driver. I tested.

Probably, the patches that added VBI support forgot to remove the above notice.

Quoted 11 lines

Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).
It's not the driver's responsibility to do this and it only makes the code overly complex.

Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it.

Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime.

Regards,
Mauro

Hans Verkuil

author

2014-12-12 12:55 UTC #2 lore raw

Quoted 33 lines · Mauro Carvalho Chehab

Em Fri, 12 Dec 2014 11:16:01 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 30 lines
Hi Shuah,
This is the video.c review with your patch applied.
Quoted 25 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
/* Developer Notes:
*
* VBI support is not yet working

Quoted 5 lines

Quoted 3 lines
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.
This is a left-over. VBI support works on this driver. I tested.

Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed.

Quoted 13 lines

Probably, the patches that added VBI support forgot to remove the above notice.
Quoted 9 lines
Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).

Quoted 5 lines

Quoted 2 lines
It's not the driver's responsibility to do this and it only makes the code overly complex.
Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it.

Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts.

Quoted 3 lines

Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime.

Most likely.

Regards,

Hans

Mauro Carvalho Chehab

2014-12-12 13:14 UTC #3 lore raw

Em Fri, 12 Dec 2014 13:55:14 +0100
Hans Verkuil <[email protected]> escreveu:

Quoted 60 lines · Mauro Carvalho Chehab

Quoted 33 lines
Em Fri, 12 Dec 2014 11:16:01 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 30 lines
Hi Shuah,
This is the video.c review with your patch applied.
Quoted 25 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
/* Developer Notes:
*
* VBI support is not yet working
Quoted 5 lines
Quoted 3 lines
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.
This is a left-over. VBI support works on this driver. I tested.
Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed.
Quoted 16 lines
Probably, the patches that added VBI support forgot to remove the above notice.
Quoted 11 lines
Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).
It's not the driver's responsibility to do this and it only makes the code overly complex.

Quoted 6 lines

Quoted 2 lines
Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it.
Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts.

If I remember the bug well, what used to happen is that tvtime would wait for a certain amount of time for a frame. If nothing arrives, it stops capturing.

The net effect is that tvtime shows no picture. This used to be so bad that tvtime didn't work with vivi at all.

The bug used also to manifest there if lots of frames got dropped when, for example, changing from one channel to another.

Btw, on a quick look, I'm not seeing any patch at tvtime since we took it over that would be fixing it. So, it was either a VB bug or the bug is still there.

Quoted 11 lines

Quoted 4 lines
Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime.
Most likely.
Regards,
Hans

Shuah Khan

2014-12-12 15:26 UTC #4 lore raw

Quoted 46 lines · Mauro Carvalho Chehab

Em Fri, 12 Dec 2014 13:55:14 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 43 lines · Mauro Carvalho Chehab
Quoted 33 lines
Em Fri, 12 Dec 2014 11:16:01 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 30 lines
Hi Shuah,
This is the video.c review with your patch applied.
Quoted 25 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
/* Developer Notes:
*
* VBI support is not yet working
Quoted 5 lines
Quoted 3 lines
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.
This is a left-over. VBI support works on this driver. I tested.
Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed.

Want me to remove the comment with this work or as a separate patch??

Quoted 31 lines

Quoted 17 lines
Quoted 17 lines
Probably, the patches that added VBI support forgot to remove the above notice.
Quoted 11 lines
Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).
It's not the driver's responsibility to do this and it only makes the code overly complex.
Quoted 6 lines
Quoted 2 lines
Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it.
Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts.
If I remember the bug well, what used to happen is that tvtime would wait for a certain amount of time for a frame. If nothing arrives, it stops capturing.
The net effect is that tvtime shows no picture. This used to be so bad that tvtime didn't work with vivi at all.

Quoted 12 lines

The bug used also to manifest there if lots of frames got dropped when, for example, changing from one channel to another.
Btw, on a quick look, I'm not seeing any patch at tvtime since we took it over that would be fixing it. So, it was either a VB bug or the bug is still there.
Quoted 5 lines
Quoted 4 lines
Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime.

I take it that we decided to keep the timeout handling for now.

thanks,
-- Shuah

Signature · 5 lines

Shuah Khan
Sr. Linux Kernel Developer
Samsung Open Source Group
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978

Hans Verkuil

author

2014-12-12 15:28 UTC #5 lore raw

Quoted 42 lines · Shuah Khan

Quoted 41 lines · Mauro Carvalho Chehab
Em Fri, 12 Dec 2014 13:55:14 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 38 lines · Mauro Carvalho Chehab
Quoted 37 lines
Em Fri, 12 Dec 2014 11:16:01 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 33 lines
Hi Shuah,
This is the video.c review with your patch applied.
Quoted 22 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
Quoted 3 lines
/* Developer Notes:
*
* VBI support is not yet working
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
Quoted 1 line
This is a left-over. VBI support works on this driver. I tested.
Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed.
Want me to remove the comment with this work or as a separate patch??

Separate, I think.

Quoted 40 lines

Quoted 40 lines
Quoted 18 lines
Quoted 18 lines
Probably, the patches that added VBI support forgot to remove the above notice.
Quoted 11 lines
Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).
It's not the driver's responsibility to do this and it only makes the code overly complex.
Quoted 6 lines
Quoted 2 lines
Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it.
Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts.
If I remember the bug well, what used to happen is that tvtime would wait for a certain amount of time for a frame. If nothing arrives, it stops capturing.
The net effect is that tvtime shows no picture. This used to be so bad that tvtime didn't work with vivi at all.
The bug used also to manifest there if lots of frames got dropped when, for example, changing from one channel to another.
Btw, on a quick look, I'm not seeing any patch at tvtime since we took it over that would be fixing it. So, it was either a VB bug or the bug is still there.
Quoted 1 line

Quoted 6 lines

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime.
I take it that we decided to keep the timeout handling for now.

No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch.

Regards,

Hans

Shuah Khan

2014-12-12 15:30 UTC #6 lore raw

Quoted 98 lines · Hans Verkuil

Quoted 42 lines · Shuah Khan
Quoted 41 lines · Mauro Carvalho Chehab
Em Fri, 12 Dec 2014 13:55:14 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 38 lines · Mauro Carvalho Chehab
Quoted 37 lines
Em Fri, 12 Dec 2014 11:16:01 +0100
Hans Verkuil <[email protected]> escreveu:
Quoted 33 lines
Hi Shuah,
This is the video.c review with your patch applied.
Quoted 22 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
Quoted 3 lines
/* Developer Notes:
*
* VBI support is not yet working
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
This is a left-over. VBI support works on this driver. I tested.
Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed.
Want me to remove the comment with this work or as a separate patch??
Separate, I think.
Quoted 45 lines
Quoted 44 lines
Quoted 18 lines
Quoted 18 lines
Probably, the patches that added VBI support forgot to remove the above notice.
Quoted 11 lines
Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).
It's not the driver's responsibility to do this and it only makes the code overly complex.
Quoted 6 lines
Quoted 2 lines
Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it.
Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts.
If I remember the bug well, what used to happen is that tvtime would wait for a certain amount of time for a frame. If nothing arrives, it stops capturing.
The net effect is that tvtime shows no picture. This used to be so bad that tvtime didn't work with vivi at all.
The bug used also to manifest there if lots of frames got dropped when, for example, changing from one channel to another.
Btw, on a quick look, I'm not seeing any patch at tvtime since we took it over that would be fixing it. So, it was either a VB bug or the bug is still there.
Quoted 5 lines
Quoted 4 lines
Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime.

Quoted 6 lines

Quoted 1 line
I take it that we decided to keep the timeout handling for now.
No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch.

Will do. This will reduce the complexity other drives don't have.

-- Shuah

Signature · 5 lines

Shuah Khan
Sr. Linux Kernel Developer
Samsung Open Source Group
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978

Devin Heitmueller

2014-12-12 15:52 UTC #7 lore raw

In reply to Hans Verkuil

Quoted 3 lines

No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch.

Did you actually try it? Do you have some patches to tvtime which aren't upstream?

I wrote the comment in question (and added the associated code). The issue is that tvtime does *everything* in a single thread (except the recent ALSA audio work), that includes servicing the video/vbi devices as well as the user interface. That thread blocks on a DQBUF ioctl until data arrives, and thus if frames are not being delivered it will hang the entire tvtime user interface.

Now you can certainly argue that is a bad design decision, but it's been that way for 15+ years, so we can't break it now. Hence why I generate dummy frames on a timeout if the decoder isn't delivering video. Unfortunately the au8522 doesn't have a free running mode (i.e. blue screen if no video), which is why most of the other devices work fine (decoders by Conexant, NXP, Trident, etc all have such functionality).

Don't get me wrong - I *hate* that I had to put that timer crap in the driver, but it was necessary to be compatible with one of the most popular applications out there.

In short, that code cannot be removed.

Devin

Signature · 2 lines

Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Hans Verkuil

author

2014-12-12 16:32 UTC #8 lore raw

Quoted 5 lines · Devin Heitmueller

Quoted 3 lines
No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch.
Did you actually try it?

Mauro tried it, not me. I'm not sure if he looked at whether the user interface is blocked when waiting for a frame.

Quoted 18 lines

Do you have some patches to tvtime which
aren't upstream?
I wrote the comment in question (and added the associated code). The issue is that tvtime does *everything* in a single thread (except the recent ALSA audio work), that includes servicing the video/vbi devices as well as the user interface. That thread blocks on a DQBUF ioctl until data arrives, and thus if frames are not being delivered it will hang the entire tvtime user interface.
Now you can certainly argue that is a bad design decision, but it's been that way for 15+ years, so we can't break it now. Hence why I generate dummy frames on a timeout if the decoder isn't delivering video. Unfortunately the au8522 doesn't have a free running mode (i.e. blue screen if no video), which is why most of the other devices work fine (decoders by Conexant, NXP, Trident, etc all have such functionality).

Quoted 5 lines

Don't get me wrong - I *hate* that I had to put that timer crap in the driver, but it was necessary to be compatible with one of the most popular applications out there.
In short, that code cannot be removed.

Sure it can. I just tried tvtime and you are right, it blocks the GUI. But the fix is very easy as well. So now I've updated tvtime so that it timeouts and gives the GUI time to update itself.

No more need for such an ugly hack in au0828. The au0828 isn't the only driver that can block, others do as well. Admittedly, they aren't very common, but they do exist. So it is much better to fix the application than adding application workarounds in the kernel.

Regards,

Hans

Devin Heitmueller

2014-12-12 16:46 UTC #9 lore raw

Quoted 5 lines

Quoted 1 line
In short, that code cannot be removed.
Sure it can. I just tried tvtime and you are right, it blocks the GUI. But the fix is very easy as well. So now I've updated tvtime so that it timeouts and gives the GUI time to update itself.

That's a nice change to tvtime and I'm sure it will make it more robust.

Quoted 4 lines

No more need for such an ugly hack in au0828. The au0828 isn't the only driver that can block, others do as well. Admittedly, they aren't very common, but they do exist. So it is much better to fix the application than adding application workarounds in the kernel.

You're breaking the ABI. You're making a change to the kernel that causes existing applications to stop working. Sure you can make the argument that applications probably never should have expected such behavior (even if it's relied on that behavior for 15+ years). And sure, you can make a change to the application in some random git repository that avoids the issue, and that change might get sucked in to the major distributions over the next couple of years. That doesn't change the fact that you're breaking the ABI and everybody who has the existing application that updates their kernel will stop working.

Please don't do this.

Devin

Signature · 2 lines

Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Mauro Carvalho Chehab

2014-12-12 17:36 UTC #10 lore raw

Em Fri, 12 Dec 2014 11:46:13 -0500
Devin Heitmueller <[email protected]> escreveu:

Quoted 24 lines

Quoted 5 lines
Quoted 1 line
In short, that code cannot be removed.
Sure it can. I just tried tvtime and you are right, it blocks the GUI. But the fix is very easy as well. So now I've updated tvtime so that it timeouts and gives the GUI time to update itself.
That's a nice change to tvtime and I'm sure it will make it more robust.
Quoted 4 lines
No more need for such an ugly hack in au0828. The au0828 isn't the only driver that can block, others do as well. Admittedly, they aren't very common, but they do exist. So it is much better to fix the application than adding application workarounds in the kernel.
You're breaking the ABI. You're making a change to the kernel that causes existing applications to stop working. Sure you can make the argument that applications probably never should have expected such behavior (even if it's relied on that behavior for 15+ years). And sure, you can make a change to the application in some random git repository that avoids the issue, and that change might get sucked in to the major distributions over the next couple of years. That doesn't change the fact that you're breaking the ABI and everybody who has the existing application that updates their kernel will stop working.

Quoted 1 line

Please don't do this.

I agree. We should not break the ABI, except if we're 100% sure that all apps that rely on the old behavior got fixed at the distros.

That means that we'll need to keep holding such timeout code for years, until all distros update to a new tvtime, of course assuming that this is the only one application with such issue.

With regards to tvtime, I think we need to bump version there and update it at the distros.

I added myself a few patches today, in order to fix it to work with vivid driver on webcam mode.

My proposal is to wait for one extra week for people to review it.

As we've discussed on IRC channel, it would be good to add support for format enumeration on it, but the changes don't seem to be trivial. I'm not willing to do it, due to my lack of time, but, if someone steps up for doing that, then we can wait for those patches before bumping the version.

In anycase, if everything is ok after ~1 week of waiting for tests, we can bump to version 1.0.5 and I can port the latest version to Fedora. I dunno who maintains it on other distros.

Regards,
Mauro

Devin Heitmueller

2014-12-12 17:46 UTC #11 lore raw

Quoted 5 lines

As we've discussed on IRC channel, it would be good to add support for format enumeration on it, but the changes don't seem to be trivial. I'm not willing to do it, due to my lack of time, but, if someone steps up for doing that, then we can wait for those patches before bumping the version.

I agree that format enumeration will be a PITA - I looked at doing it myself a couple of years ago. Much of the problems are related to limitations in the home-grown widget toolkit that tvtime uses. I've also got patches to support HD resolutions (i.e. HDMI capture) which we use internally, but haven't felt they were worthwhile to upstream since they don't depend on the ENUM_FRAMESIZES/FRAMEINTERVALS. If somebody feels the urge to put some time into it, I can make available the patches.

Devin

Signature · 2 lines

Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Shuah Khan

2014-12-12 15:23 UTC #12 lore raw

In reply to Hans Verkuil

Hi Hans,

Thanks for a quick review.

Quoted 60 lines · Hans Verkuil

Hi Shuah,
This is the video.c review with your patch applied.
Quoted 22 lines
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller <[email protected]>
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version. *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
Quoted 3 lines
/* Developer Notes:
*
* VBI support is not yet working
I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support.
Quoted 16 lines
* The hardware scaler supported is unimplemented
* AC97 audio support is unimplemented (only i2s audio mode) *
*/
#include "au0828.h"
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/device.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-event.h>
#include <media/tuner.h>
#include "au0828-reg.h"
<snip>
Quoted 7 lines
static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
		       unsigned int *nbuffers, unsigned int *nplanes,
		       unsigned int sizes[], void *alloc_ctxs[])
{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	unsigned long size;

Quoted 7 lines

Quoted 5 lines
	if (fmt)
		size = fmt->fmt.pix.sizeimage;
	else
		size =
		    (dev->width * dev->height * dev->bytesperline + 7) >> 3;
This is wrong. It should be 'size = dev->bytesperline * dev->height;'

Will fix it.

Quoted 8 lines

If fmt is set, then you need to check if the fmt size is large enough for the current format.
Quoted 2 lines
	if (size == 0)

Quoted 8 lines

This should be 'size < img_size'.
Quoted 4 lines
		return -EINVAL;

	if (0 == *nbuffers)
		*nbuffers = 32;
Bogus check.

Will fix it.

Quoted 10 lines

Quoted 7 lines
	*nplanes = 1;
	sizes[0] = size;

	return 0;
}
I would rewrite this function as:

Quoted 13 lines

{      
       struct au0828_dev *dev = vb2_get_drv_priv(vq);
       unsigned img_size = dev->bytesperline * dev->height;
       unsigned long size;

       size = fmt ? fmt->fmt.pix.sizeimage : img_size;
       if (size < img_size)
               return -EINVAL;
                   
       *nplanes = 1;
       sizes[0] = size;
       return 0;
}

That simplifies it a lot.

Quoted 11 lines

Quoted 9 lines
static int
buffer_prepare(struct vb2_buffer *vb)
{
	struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
	struct au0828_dev    *dev = vb2_get_drv_priv(vb->vb2_queue);

	buf->length = (dev->width * dev->height * 16 + 7) >> 3;
Use buf->length = dev->bytesperline * dev->height;

Yes. Will do.

Quoted 118 lines


	if (vb2_plane_size(vb, 0) < buf->length) {
		pr_err("%s data will not fit into plane (%lu < %lu)\n",
			__func__, vb2_plane_size(vb, 0), buf->length);
		return -EINVAL;
	}
	vb2_set_plane_payload(&buf->vb, 0, buf->length);
	return 0;
}

static void
buffer_queue(struct vb2_buffer *vb)

{
	struct au0828_buffer    *buf     = container_of(vb,
							struct au0828_buffer,
							vb);
	struct au0828_dev       *dev     = vb2_get_drv_priv(vb->vb2_queue);
	struct au0828_dmaqueue  *vidq    = &dev->vidq;
	unsigned long flags = 0;

	buf->mem = vb2_plane_vaddr(vb, 0);
	buf->length = vb2_plane_size(vb, 0);

	spin_lock_irqsave(&dev->slock, flags);
	list_add_tail(&buf->list, &vidq->active);
	spin_unlock_irqrestore(&dev->slock, flags);
}

static int au0828_i2s_init(struct au0828_dev *dev)

{
	/* Enable i2s mode */
	au0828_writereg(dev, AU0828_AUDIOCTRL_50C, 0x01);
	return 0;
}

/*
* Auvitek au0828 analog stream enable
*/
static int au0828_analog_stream_enable(struct au0828_dev *d)

{
	struct usb_interface *iface;
	int ret, h, w;

	dprintk(1, "au0828_analog_stream_enable called\n");

	iface = usb_ifnum_to_if(d->usbdev, 0);
	if (iface && iface->cur_altsetting->desc.bAlternateSetting != 5) {
		dprintk(1, "Changing intf#0 to alt 5\n");
		/* set au0828 interface0 to AS5 here again */
		ret = usb_set_interface(d->usbdev, 0, 5);
		if (ret < 0) {
			pr_info("Au0828 can't set alt setting to 5!\n");
			return -EBUSY;
		}
	}

	h = d->height / 2 + 2;
	w = d->width * 2;

	au0828_writereg(d, AU0828_SENSORCTRL_VBI_103, 0x00);
	au0828_writereg(d, 0x106, 0x00);
	/* set x position */
	au0828_writereg(d, 0x110, 0x00);
	au0828_writereg(d, 0x111, 0x00);
	au0828_writereg(d, 0x114, w & 0xff);
	au0828_writereg(d, 0x115, w >> 8);
	/* set y position */
	au0828_writereg(d, 0x112, 0x00);
	au0828_writereg(d, 0x113, 0x00);
	au0828_writereg(d, 0x116, h & 0xff);
	au0828_writereg(d, 0x117, h >> 8);
	au0828_writereg(d, AU0828_SENSORCTRL_100, 0xb3);

	return 0;
}

static int au0828_analog_stream_disable(struct au0828_dev *d)

{
	dprintk(1, "au0828_analog_stream_disable called\n");
	au0828_writereg(d, AU0828_SENSORCTRL_100, 0x0);
	return 0;
}

static void au0828_analog_stream_reset(struct au0828_dev *dev)

{
	dprintk(1, "au0828_analog_stream_reset called\n");
	au0828_writereg(dev, AU0828_SENSORCTRL_100, 0x0);
	mdelay(30);
	au0828_writereg(dev, AU0828_SENSORCTRL_100, 0xb3);
}

/*
* Some operations needs to stop current streaming */
static int au0828_stream_interrupt(struct au0828_dev *dev)

{
	int ret = 0;

	dev->stream_state = STREAM_INTERRUPT;
	if (dev->dev_state == DEV_DISCONNECTED)
		return -ENODEV;
	else if (ret) {
		dev->dev_state = DEV_MISCONFIGURED;
		dprintk(1, "%s device is misconfigured!\n", __func__);
		return ret;
	}
	return 0;
}

int au0828_start_analog_streaming(struct vb2_queue *vq, unsigned int count)

{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	int rc = 0;

	dprintk(1, "au0828_start_analog_streaming called %d\n",
		dev->streaming_users);

Quoted 6 lines

Quoted 4 lines

	/* Make sure streaming is not already in progress for this type
	   of filehandle (e.g. video, vbi) */
	if (vb2_is_streaming(vq))
		return -EBUSY;

Can never happen, so this test can be removed.

Quoted 4 lines

Quoted 2 lines
You need to initialize the counters:

Yes. Thanks for the catch.

Quoted 8 lines

       if (vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
               dev->field_count = 0;
       else
               dev->vbi_field_count = 0;
I would also recommend renaming 'field' to 'frame', since you're counting frames, not fields.

That is a better naming. Will change that.

Quoted 34 lines


	if (dev->streaming_users == 0) {
		/* If we were doing ac97 instead of i2s, it would go here...*/
		au0828_i2s_init(dev);
		rc = au0828_init_isoc(dev, AU0828_ISO_PACKETS_PER_URB,
				   AU0828_MAX_ISO_BUFS, dev->max_pkt_size,
				   au0828_isoc_copy);
		if (rc < 0) {
			pr_info("au0828_init_isoc failed\n");
			return rc;
		}

		if (vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
			v4l2_device_call_all(&dev->v4l2_dev, 0, video,
						s_stream, 1);
			dev->vid_timeout_running = 1;
			mod_timer(&dev->vid_timeout, jiffies + (HZ / 10));
		} else if (vq->type == V4L2_BUF_TYPE_VBI_CAPTURE) {
			dev->vbi_timeout_running = 1;
			mod_timer(&dev->vbi_timeout, jiffies + (HZ / 10));
		}
	}
	dev->streaming_users++;
	return rc;
}

static void au0828_stop_streaming(struct vb2_queue *vq)

{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	struct au0828_dmaqueue *vidq = &dev->vidq;
	unsigned long flags = 0;
	int i;

Quoted 6 lines

Quoted 4 lines

	dprintk(1, "au0828_stop_streaming called %d\n", dev->streaming_users);

	if (!vb2_is_streaming(vq))
		return;

Unnecessary test.

Will be removed.

Quoted 31 lines

Quoted 22 lines


	if (dev->streaming_users-- == 1)
		au0828_uninit_isoc(dev);
	dprintk(1, "au0828_stop_streaming %d\n", dev->streaming_users);

	spin_lock_irqsave(&dev->slock, flags);
	if (dev->isoc_ctl.buf != NULL) {
		vb2_buffer_done(&dev->isoc_ctl.buf->vb, VB2_BUF_STATE_ERROR);
		dev->isoc_ctl.buf = NULL;
	}
	while (!list_empty(&vidq->active)) {
		struct au0828_buffer *buf;

		buf = list_entry(vidq->active.next, struct au0828_buffer, list);
		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
		list_del(&buf->list);
	}

	dev->vid_timeout_running = 0;
	del_timer_sync(&dev->vid_timeout);

Quoted 8 lines

	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);

	for (i = 0; i < AU0828_MAX_INPUT; i++) {
		if (AUVI_INPUT(i).audio_setup == NULL)
			continue;
		(AUVI_INPUT(i).audio_setup)(dev, 0);
	}
	spin_unlock_irqrestore(&dev->slock, flags);

Quoted 3 lines

Very bad. This unlock should go before the 'dev->vid_timeout_running = 0;' line. The way it is now the lines between the spinlock run without interrupts, so calls like del_timer_sync() will complain about scheduling in atomic context.

Thanks for catching this. Should have been checking for this, since I ran into this once at least. :)

Quoted 3 lines

Hint: you should turn on at least the basic lock checks that the kernel offers when testing.

I thought I had them enabled. Will do.

Quoted 32 lines

}

void au0828_stop_vbi_streaming(struct vb2_queue *vq)

{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	struct au0828_dmaqueue *vbiq = &dev->vbiq;
	unsigned long flags = 0;

	dprintk(1, "au0828_stop_vbi_streaming called %d\n",
		dev->streaming_users);
	if (!vb2_is_streaming(vq))
		return;

	if (dev->streaming_users-- == 1)
		au0828_uninit_isoc(dev);
	dprintk(1, "au0828_stop_vbi_streaming %d\n", dev->streaming_users);

	spin_lock_irqsave(&dev->slock, flags);
	if (dev->isoc_ctl.vbi_buf != NULL) {
		vb2_buffer_done(&dev->isoc_ctl.vbi_buf->vb,
				VB2_BUF_STATE_ERROR);
		dev->isoc_ctl.vbi_buf = NULL;
	}
	while (!list_empty(&vbiq->active)) {
		struct au0828_buffer *buf;

		buf = list_entry(vbiq->active.next, struct au0828_buffer, list);
		list_del(&buf->list);
		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
	}

Quoted 7 lines

Quoted 4 lines
	dev->vbi_timeout_running = 0;
	del_timer_sync(&dev->vbi_timeout);

	spin_unlock_irqrestore(&dev->slock, flags);
Same problem here, move up the spin_unlock to before the 'dev->vbi_timeout_running = 0;' line.

Right.

Quoted 126 lines

Quoted 33 lines
}
static struct vb2_ops au0828_video_qops = {
	.queue_setup     = queue_setup,
	.buf_prepare     = buffer_prepare,
	.buf_queue       = buffer_queue,
.start_streaming = au0828_start_analog_streaming, .stop_streaming = au0828_stop_streaming,
.wait_prepare = vb2_ops_wait_prepare,
.wait_finish = vb2_ops_wait_finish,
};
/* ------------------------------------------------------------------
V4L2 interface ------------------------------------------------------------------*/
/*
* au0828_analog_unregister
* unregister v4l2 devices
*/
void au0828_analog_unregister(struct au0828_dev *dev)
{
	dprintk(1, "au0828_analog_unregister called\n");
	mutex_lock(&au0828_sysfs_lock);

	if (dev->vdev)
		video_unregister_device(dev->vdev);
	if (dev->vbi_dev)
		video_unregister_device(dev->vbi_dev);

	mutex_unlock(&au0828_sysfs_lock);
}
Quoted 3 lines
/* This function ensures that video frames continue to be delivered even if
the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */
Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves).
It's not the driver's responsibility to do this and it only makes the code overly complex.
Quoted 81 lines
static void au0828_vid_buffer_timeout(unsigned long data)
{
	struct au0828_dev *dev = (struct au0828_dev *) data;
	struct au0828_dmaqueue *dma_q = &dev->vidq;
	struct au0828_buffer *buf;
	unsigned char *vid_data;
	unsigned long flags = 0;

	spin_lock_irqsave(&dev->slock, flags);

	buf = dev->isoc_ctl.buf;
	if (buf != NULL) {
		vid_data = vb2_plane_vaddr(&buf->vb, 0);
		memset(vid_data, 0x00, buf->length); /* Blank green frame */
		buffer_filled(dev, dma_q, buf);
	}
	get_next_buf(dma_q, &buf);

	if (dev->vid_timeout_running == 1)
		mod_timer(&dev->vid_timeout, jiffies + (HZ / 10));

	spin_unlock_irqrestore(&dev->slock, flags);
}
static void au0828_vbi_buffer_timeout(unsigned long data)
{
	struct au0828_dev *dev = (struct au0828_dev *) data;
	struct au0828_dmaqueue *dma_q = &dev->vbiq;
	struct au0828_buffer *buf;
	unsigned char *vbi_data;
	unsigned long flags = 0;

	spin_lock_irqsave(&dev->slock, flags);

	buf = dev->isoc_ctl.vbi_buf;
	if (buf != NULL) {
		vbi_data = vb2_plane_vaddr(&buf->vb, 0);
		memset(vbi_data, 0x00, buf->length);
		vbi_buffer_filled(dev, dma_q, buf);
	}
	vbi_get_next_buf(dma_q, &buf);

	if (dev->vbi_timeout_running == 1)
		mod_timer(&dev->vbi_timeout, jiffies + (HZ / 10));
	spin_unlock_irqrestore(&dev->slock, flags);
}
static int au0828_v4l2_open(struct file *filp)
{
	struct video_device *vdev = video_devdata(filp);
	struct au0828_dev *dev = video_drvdata(filp);
	int type;
	int ret = 0;

	switch (vdev->vfl_type) {
	case VFL_TYPE_GRABBER:
		type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
		break;
	case VFL_TYPE_VBI:
		type = V4L2_BUF_TYPE_VBI_CAPTURE;
		break;
	default:
		return -EINVAL;
	}

	dprintk(1,
		"%s called std_set %d dev_state %d stream users %d users %d\n",
		__func__, dev->std_set_in_tuner_core, dev->dev_state,
		dev->streaming_users, dev->users);

	if (mutex_lock_interruptible(&dev->lock))
		return -ERESTARTSYS;

	ret = v4l2_fh_open(filp);
	if (ret) {
		au0828_isocdbg("%s: v4l2_fh_open() returned error %d\n",
				__func__, ret);
		mutex_unlock(&dev->lock);
		return ret;
	}

Quoted 4 lines

Quoted 1 line
	if (dev->users == 0) {
Replace with 'v4l2_fh_is_singular_file(filp)'. That way you can drop the user field. This can be a separate patch.

Will do.

Quoted 17 lines

		au0828_analog_stream_enable(dev);
		au0828_analog_stream_reset(dev);
		dev->stream_state = STREAM_OFF;
		dev->dev_state |= DEV_INITIALIZED;
	}
	dev->users++;
	mutex_unlock(&dev->lock);
	return ret;
}

static int au0828_v4l2_close(struct file *filp)

{
	int ret;
	struct au0828_dev *dev = video_drvdata(filp);
	struct video_device *vdev = video_devdata(filp);

Quoted 8 lines

Quoted 6 lines

	dprintk(1,
		"%s called std_set %d dev_state %d stream users %d users %d\n",
		__func__, dev->std_set_in_tuner_core, dev->dev_state,
		dev->streaming_users, dev->users);

	vb2_fop_release(filp);

Drop this vb2_fop_release() call, will move to the end of the function.

Yes I had this at the end and changed it when I was seeing a problem because of some other bug and never went back to it. Will fix it.

Quoted 41 lines

Quoted 2 lines

	mutex_lock(&dev->lock);

Change to:

	mutex_lock(&dev->lock);

Quoted 7 lines

	if (vdev->vfl_type == VFL_TYPE_GRABBER && dev->vid_timeout_running) {
		/* Cancel timeout thread in case they didn't call streamoff */
		dev->vid_timeout_running = 0;
		del_timer_sync(&dev->vid_timeout);
	} else if (vdev->vfl_type == VFL_TYPE_VBI &&
			dev->vbi_timeout_running) {
		/* Cancel timeout thread in case they didn't call streamoff */

Quoted 8 lines

		dev->vbi_timeout_running = 0;
		del_timer_sync(&dev->vbi_timeout);
	}

	if (dev->dev_state == DEV_DISCONNECTED)
		goto end;

	if (dev->users == 1) {

And:

	if (v4l2_fh_is_singular_file(filp))

Quoted 11 lines

		/* Save some power by putting tuner to sleep */
		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
		dev->std_set_in_tuner_core = 0;

		/* When close the device, set the usb intf0 into alt0 to free
		   USB bandwidth */
		ret = usb_set_interface(dev->usbdev, 0, 0);
		if (ret < 0)
			pr_info("Au0828 can't set alternate to 0!\n");
	}

end:

And the release goes here:

Quoted 7 lines

	_vb2_fop_release(filp, NULL);
Quoted 3 lines
	dev->users--;
	mutex_unlock(&dev->lock);
	wake_up_interruptible_nr(&dev->open, 1);
Huh? Seems unused and certainly looks very bogus.

Right. I am not sure about this either. I did see some problems early on when I removed it. I will remove it and try again.

Quoted 794 lines

	return 0;
}

/* Must be called with dev->lock held */ static void au0828_init_tuner(struct au0828_dev *dev)

{
	struct v4l2_frequency f = {
		.frequency = dev->ctrl_freq,
		.type = V4L2_TUNER_ANALOG_TV,
	};

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	if (dev->std_set_in_tuner_core)
		return;
	dev->std_set_in_tuner_core = 1;
	i2c_gate_ctrl(dev, 1);
	/* If we've never sent the standard in tuner core, do so now.
	   We don't do this at device probe because we don't want to
	   incur the cost of a firmware load */
	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_std, dev->std);
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, &f);
	i2c_gate_ctrl(dev, 0);
}

static int au0828_set_format(struct au0828_dev *dev, unsigned int cmd,
struct v4l2_format *format)

{
	int ret;
	int width = format->fmt.pix.width;
	int height = format->fmt.pix.height;

	/* If they are demanding a format other than the one we support,
	   bail out (tvtime asks for UYVY and then retries with YUYV) */
	if (format->fmt.pix.pixelformat != V4L2_PIX_FMT_UYVY)
		return -EINVAL;

	/* format->fmt.pix.width only support 720 and height 480 */
	if (width != 720)
		width = 720;
	if (height != 480)
		height = 480;

	format->fmt.pix.width = width;
	format->fmt.pix.height = height;
	format->fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
	format->fmt.pix.bytesperline = width * 2;
	format->fmt.pix.sizeimage = width * height * 2;
	format->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
	format->fmt.pix.field = V4L2_FIELD_INTERLACED;
	format->fmt.pix.priv = 0;

	if (cmd == VIDIOC_TRY_FMT)
		return 0;

	/* maybe set new image format, driver current only support 720*480 */
	dev->width = width;
	dev->height = height;
	dev->frame_size = width * height * 2;
	dev->field_size = width * height;
	dev->bytesperline = width * 2;

	if (dev->stream_state == STREAM_ON) {
		dprintk(1, "VIDIOC_SET_FMT: interrupting stream!\n");
		ret = au0828_stream_interrupt(dev);
		if (ret != 0) {
			dprintk(1, "error interrupting video stream!\n");
			return ret;
		}
	}

	au0828_analog_stream_enable(dev);

	return 0;
}

static int vidioc_querycap(struct file *file, void *priv,
struct v4l2_capability *cap)

{
	struct video_device *vdev = video_devdata(file);
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	strlcpy(cap->driver, "au0828", sizeof(cap->driver));
	strlcpy(cap->card, dev->board.name, sizeof(cap->card));
	usb_make_path(dev->usbdev, cap->bus_info, sizeof(cap->bus_info));

	/* set the device capabilities */
	cap->device_caps = V4L2_CAP_AUDIO |
		V4L2_CAP_READWRITE |
		V4L2_CAP_STREAMING |
		V4L2_CAP_TUNER;
	if (vdev->vfl_type == VFL_TYPE_GRABBER)
		cap->device_caps |= V4L2_CAP_VIDEO_CAPTURE;
	else
		cap->device_caps |= V4L2_CAP_VBI_CAPTURE;
	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS |
		V4L2_CAP_VBI_CAPTURE | V4L2_CAP_VIDEO_CAPTURE;
	return 0;
}

static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_fmtdesc *f)

{
	if (f->index)
		return -EINVAL;

	dprintk(1, "%s called\n", __func__);

	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
	strcpy(f->description, "Packed YUV2");

	f->flags = 0;
	f->pixelformat = V4L2_PIX_FMT_UYVY;

	return 0;
}

static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	f->fmt.pix.width = dev->width;
	f->fmt.pix.height = dev->height;
	f->fmt.pix.pixelformat = V4L2_PIX_FMT_UYVY;
	f->fmt.pix.bytesperline = dev->bytesperline;
	f->fmt.pix.sizeimage = dev->frame_size;
	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; /* NTSC/PAL */
	f->fmt.pix.field = V4L2_FIELD_INTERLACED;
	f->fmt.pix.priv = 0;
	return 0;
}

static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	return au0828_set_format(dev, VIDIOC_TRY_FMT, f);
}

static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)

{
	struct au0828_dev *dev = video_drvdata(file);
	int rc;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	rc = check_dev(dev);
	if (rc < 0)
		return rc;

	if (vb2_is_busy(&dev->vb_vidq)) {
		pr_info("%s queue busy\n", __func__);
		rc = -EBUSY;
		goto out;
	}

	rc = au0828_set_format(dev, VIDIOC_S_FMT, f);
out:
	return rc;
}

static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	if (norm == dev->std)
		return 0;

	if (dev->streaming_users > 0)
		return -EBUSY;

	dev->std = norm;

	au0828_init_tuner(dev);

	i2c_gate_ctrl(dev, 1);

	/*
	 * FIXME: when we support something other than 60Hz standards,
	 * we are going to have to make the au0828 bridge adjust the size
	 * of its capture buffer, which is currently hardcoded at 720x480
	 */

	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_std, norm);

	i2c_gate_ctrl(dev, 0);

	return 0;
}

static int vidioc_g_std(struct file *file, void *priv, v4l2_std_id *norm)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	*norm = dev->std;
	return 0;
}

static int vidioc_enum_input(struct file *file, void *priv,
struct v4l2_input *input)

{
	struct au0828_dev *dev = video_drvdata(file);
	unsigned int tmp;

	static const char *inames[] = {
		[AU0828_VMUX_UNDEFINED] = "Undefined",
		[AU0828_VMUX_COMPOSITE] = "Composite",
		[AU0828_VMUX_SVIDEO] = "S-Video",
		[AU0828_VMUX_CABLE] = "Cable TV",
		[AU0828_VMUX_TELEVISION] = "Television",
		[AU0828_VMUX_DVB] = "DVB",
		[AU0828_VMUX_DEBUG] = "tv debug"
	};

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	tmp = input->index;

	if (tmp >= AU0828_MAX_INPUT)
		return -EINVAL;
	if (AUVI_INPUT(tmp).type == 0)
		return -EINVAL;

	input->index = tmp;
	strcpy(input->name, inames[AUVI_INPUT(tmp).type]);
	if ((AUVI_INPUT(tmp).type == AU0828_VMUX_TELEVISION) ||
	    (AUVI_INPUT(tmp).type == AU0828_VMUX_CABLE)) {
		input->type |= V4L2_INPUT_TYPE_TUNER;
		input->audioset = 1;
	} else {
		input->type |= V4L2_INPUT_TYPE_CAMERA;
		input->audioset = 2;
	}

	input->std = dev->vdev->tvnorms;

	return 0;
}

static int vidioc_g_input(struct file *file, void *priv, unsigned int *i)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	*i = dev->ctrl_input;
	return 0;
}

static void au0828_s_input(struct au0828_dev *dev, int index)

{
	int i;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	switch (AUVI_INPUT(index).type) {
	case AU0828_VMUX_SVIDEO:
		dev->input_type = AU0828_VMUX_SVIDEO;
		dev->ctrl_ainput = 1;
		break;
	case AU0828_VMUX_COMPOSITE:
		dev->input_type = AU0828_VMUX_COMPOSITE;
		dev->ctrl_ainput = 1;
		break;
	case AU0828_VMUX_TELEVISION:
		dev->input_type = AU0828_VMUX_TELEVISION;
		dev->ctrl_ainput = 0;
		break;
	default:
		dprintk(1, "unknown input type set [%d]\n",
			AUVI_INPUT(index).type);
		break;
	}

	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
			AUVI_INPUT(index).vmux, 0, 0);

	for (i = 0; i < AU0828_MAX_INPUT; i++) {
		int enable = 0;
		if (AUVI_INPUT(i).audio_setup == NULL)
			continue;

		if (i == index)
			enable = 1;
		else
			enable = 0;
		if (enable) {
			(AUVI_INPUT(i).audio_setup)(dev, enable);
		} else {
			/* Make sure we leave it turned on if some
			   other input is routed to this callback */
			if ((AUVI_INPUT(i).audio_setup) !=
			    ((AUVI_INPUT(index).audio_setup))) {
				(AUVI_INPUT(i).audio_setup)(dev, enable);
			}
		}
	}

	v4l2_device_call_all(&dev->v4l2_dev, 0, audio, s_routing,
			AUVI_INPUT(index).amux, 0, 0);
}

static int vidioc_s_input(struct file *file, void *priv, unsigned int index)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "VIDIOC_S_INPUT in function %s, input=%d\n", __func__,
		index);
	if (index >= AU0828_MAX_INPUT)
		return -EINVAL;
	if (AUVI_INPUT(index).type == 0)
		return -EINVAL;
	dev->ctrl_input = index;
	au0828_s_input(dev, index);
	return 0;
}

static int vidioc_enumaudio(struct file *file, void *priv, struct v4l2_audio *a)

{
	if (a->index > 1)
		return -EINVAL;

	dprintk(1, "%s called\n", __func__);

	if (a->index == 0)
		strcpy(a->name, "Television");
	else
		strcpy(a->name, "Line in");

	a->capability = V4L2_AUDCAP_STEREO;
	return 0;
}

static int vidioc_g_audio(struct file *file, void *priv, struct v4l2_audio *a)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	a->index = dev->ctrl_ainput;
	if (a->index == 0)
		strcpy(a->name, "Television");
	else
		strcpy(a->name, "Line in");

	a->capability = V4L2_AUDCAP_STEREO;
	return 0;
}

static int vidioc_s_audio(struct file *file, void *priv, const struct v4l2_audio *a)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (a->index != dev->ctrl_ainput)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);
	return 0;
}

static int vidioc_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (t->index != 0)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	strcpy(t->name, "Auvitek tuner");

	au0828_init_tuner(dev);
	i2c_gate_ctrl(dev, 1);
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, g_tuner, t);
	i2c_gate_ctrl(dev, 0);
	return 0;
}

static int vidioc_s_tuner(struct file *file, void *priv,
const struct v4l2_tuner *t)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (t->index != 0)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	au0828_init_tuner(dev);
	i2c_gate_ctrl(dev, 1);
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_tuner, t);
	i2c_gate_ctrl(dev, 0);

	dprintk(1, "VIDIOC_S_TUNER: signal = %x, afc = %x\n", t->signal,
		t->afc);

	return 0;

}

static int vidioc_g_frequency(struct file *file, void *priv,
struct v4l2_frequency *freq)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (freq->tuner != 0)
		return -EINVAL;
	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);
	freq->frequency = dev->ctrl_freq;
	return 0;
}

static int vidioc_s_frequency(struct file *file, void *priv,
const struct v4l2_frequency *freq)

{
	struct au0828_dev *dev = video_drvdata(file);
	struct v4l2_frequency new_freq = *freq;

	if (freq->tuner != 0)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	au0828_init_tuner(dev);
	i2c_gate_ctrl(dev, 1);

	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, freq);
	/* Get the actual set (and possibly clamped) frequency */
	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, g_frequency, &new_freq);
	dev->ctrl_freq = new_freq.frequency;

	i2c_gate_ctrl(dev, 0);

	au0828_analog_stream_reset(dev);

	return 0;
}

/* RAW VBI ioctls */

static int vidioc_g_fmt_vbi_cap(struct file *file, void *priv,
struct v4l2_format *format)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	format->fmt.vbi.samples_per_line = dev->vbi_width;
	format->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
	format->fmt.vbi.offset = 0;
	format->fmt.vbi.flags = 0;
	format->fmt.vbi.sampling_rate = 6750000 * 4 / 2;

	format->fmt.vbi.count[0] = dev->vbi_height;
	format->fmt.vbi.count[1] = dev->vbi_height;
	format->fmt.vbi.start[0] = 21;
	format->fmt.vbi.start[1] = 284;
	memset(format->fmt.vbi.reserved, 0, sizeof(format->fmt.vbi.reserved));

	return 0;
}

static int vidioc_cropcap(struct file *file, void *priv,
struct v4l2_cropcap *cc)

{
	struct au0828_dev *dev = video_drvdata(file);

	if (cc->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
		return -EINVAL;

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	cc->bounds.left = 0;
	cc->bounds.top = 0;
	cc->bounds.width = dev->width;
	cc->bounds.height = dev->height;

	cc->defrect = cc->bounds;

	cc->pixelaspect.numerator = 54;
	cc->pixelaspect.denominator = 59;

	return 0;
}

#ifdef CONFIG_VIDEO_ADV_DEBUG
static int vidioc_g_register(struct file *file, void *priv,
struct v4l2_dbg_register *reg)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	reg->val = au0828_read(dev, reg->reg);
	reg->size = 1;
	return 0;
}

static int vidioc_s_register(struct file *file, void *priv,
const struct v4l2_dbg_register *reg)

{
	struct au0828_dev *dev = video_drvdata(file);

	dprintk(1, "%s called std_set %d dev_state %d\n", __func__,
		dev->std_set_in_tuner_core, dev->dev_state);

	return au0828_writereg(dev, reg->reg, reg->val);
}

#endif

static int vidioc_log_status(struct file *file, void *fh)

{
	struct video_device *vdev = video_devdata(file);

	dprintk(1, "%s called\n", __func__);

	v4l2_ctrl_log_status(file, fh);
	v4l2_device_call_all(vdev->v4l2_dev, 0, core, log_status);
	return 0;
}

void au0828_v4l2_suspend(struct au0828_dev *dev)

{
	struct urb *urb;
	int i;

	pr_info("stopping V4L2\n");

	if (dev->stream_state == STREAM_ON) {
		pr_info("stopping V4L2 active URBs\n");
		au0828_analog_stream_disable(dev);
		/* stop urbs */
		for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
			urb = dev->isoc_ctl.urb[i];
			if (urb) {
				if (!irqs_disabled())
					usb_kill_urb(urb);
				else
					usb_unlink_urb(urb);
			}
		}
	}

	if (dev->vid_timeout_running)
		del_timer_sync(&dev->vid_timeout);
	if (dev->vbi_timeout_running)
		del_timer_sync(&dev->vbi_timeout);
}

void au0828_v4l2_resume(struct au0828_dev *dev)

{
	int i, rc;

	pr_info("restarting V4L2\n");

	if (dev->stream_state == STREAM_ON) {
		au0828_stream_interrupt(dev);
		au0828_init_tuner(dev);
	}

	if (dev->vid_timeout_running)
		mod_timer(&dev->vid_timeout, jiffies + (HZ / 10));
	if (dev->vbi_timeout_running)
		mod_timer(&dev->vbi_timeout, jiffies + (HZ / 10));

	/* If we were doing ac97 instead of i2s, it would go here...*/
	au0828_i2s_init(dev);

	au0828_analog_stream_enable(dev);

	if (!(dev->stream_state == STREAM_ON)) {
		au0828_analog_stream_reset(dev);
		/* submit urbs */
		for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
			rc = usb_submit_urb(dev->isoc_ctl.urb[i], GFP_ATOMIC);
			if (rc) {
				au0828_isocdbg("submit of urb %i failed (error=%i)\n",
					       i, rc);
				au0828_uninit_isoc(dev);
			}
		}
	}
}

static struct v4l2_file_operations au0828_v4l_fops = {

	.owner      = THIS_MODULE,
	.open       = au0828_v4l2_open,
	.release    = au0828_v4l2_close,
	.read       = vb2_fop_read,
	.poll       = vb2_fop_poll,
	.mmap       = vb2_fop_mmap,

.unlocked_ioctl = video_ioctl2,
};

static const struct v4l2_ioctl_ops video_ioctl_ops = {

	.vidioc_querycap            = vidioc_querycap,
	.vidioc_enum_fmt_vid_cap    = vidioc_enum_fmt_vid_cap,
	.vidioc_g_fmt_vid_cap       = vidioc_g_fmt_vid_cap,
	.vidioc_try_fmt_vid_cap     = vidioc_try_fmt_vid_cap,
	.vidioc_s_fmt_vid_cap       = vidioc_s_fmt_vid_cap,
	.vidioc_g_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
	.vidioc_try_fmt_vbi_cap     = vidioc_g_fmt_vbi_cap,
	.vidioc_s_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
	.vidioc_enumaudio           = vidioc_enumaudio,
	.vidioc_g_audio             = vidioc_g_audio,
	.vidioc_s_audio             = vidioc_s_audio,
	.vidioc_cropcap             = vidioc_cropcap,

	.vidioc_reqbufs             = vb2_ioctl_reqbufs,
	.vidioc_create_bufs         = vb2_ioctl_create_bufs,
	.vidioc_prepare_buf         = vb2_ioctl_prepare_buf,
	.vidioc_querybuf            = vb2_ioctl_querybuf,
	.vidioc_qbuf                = vb2_ioctl_qbuf,
	.vidioc_dqbuf               = vb2_ioctl_dqbuf,

	.vidioc_s_std               = vidioc_s_std,
	.vidioc_g_std               = vidioc_g_std,
	.vidioc_enum_input          = vidioc_enum_input,
	.vidioc_g_input             = vidioc_g_input,
	.vidioc_s_input             = vidioc_s_input,

.vidioc_streamon = vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,

static int au0828_vb2_setup(struct au0828_dev *dev)

{
	int rc;
	struct vb2_queue *q;

	/* Setup Videobuf2 for Video capture */
	q = &dev->vb_vidq;
	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
	q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
	q->drv_priv = dev;
	q->buf_struct_size = sizeof(struct au0828_buffer);
	q->ops = &au0828_video_qops;
	q->mem_ops = &vb2_vmalloc_memops;

	rc = vb2_queue_init(q);
	if (rc < 0)
		return rc;

	/* Setup Videobuf2 for VBI capture */
	q = &dev->vb_vbiq;
	q->type = V4L2_BUF_TYPE_VBI_CAPTURE;
	q->io_modes = VB2_READ | VB2_MMAP | VB2_USERPTR;
	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
	q->drv_priv = dev;
	q->buf_struct_size = sizeof(struct au0828_buffer);
	q->ops = &au0828_vbi_qops;
	q->mem_ops = &vb2_vmalloc_memops;

	rc = vb2_queue_init(q);
	if (rc < 0)
		return rc;

	return 0;
}

/**************************************************************************/

int au0828_analog_register(struct au0828_dev *dev,
struct usb_interface *interface)

{
	int retval = -ENOMEM;
	struct usb_host_interface *iface_desc;
	struct usb_endpoint_descriptor *endpoint;
	int i, ret;

	dprintk(1, "au0828_analog_register called for intf#%d!\n",
		interface->cur_altsetting->desc.bInterfaceNumber);

	/* set au0828 usb interface0 to as5 */
	retval = usb_set_interface(dev->usbdev,
			interface->cur_altsetting->desc.bInterfaceNumber, 5);
	if (retval != 0) {
		pr_info("Failure setting usb interface0 to as5\n");
		return retval;
	}

	/* Figure out which endpoint has the isoc interface */
	iface_desc = interface->cur_altsetting;
	for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
		endpoint = &iface_desc->endpoint[i].desc;
		if (((endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
		     == USB_DIR_IN) &&
		    ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
		     == USB_ENDPOINT_XFER_ISOC)) {

			/* we find our isoc in endpoint */
			u16 tmp = le16_to_cpu(endpoint->wMaxPacketSize);
			dev->max_pkt_size = (tmp & 0x07ff) *
				(((tmp & 0x1800) >> 11) + 1);
			dev->isoc_in_endpointaddr = endpoint->bEndpointAddress;
			dprintk(1,
				"Found isoc endpoint 0x%02x, max size = %d\n",
				dev->isoc_in_endpointaddr, dev->max_pkt_size);
		}
	}
	if (!(dev->isoc_in_endpointaddr)) {
		pr_info("Could not locate isoc endpoint\n");
		kfree(dev);
		return -ENODEV;
	}

	init_waitqueue_head(&dev->open);
	spin_lock_init(&dev->slock);

	/* init video dma queues */
	INIT_LIST_HEAD(&dev->vidq.active);
	INIT_LIST_HEAD(&dev->vbiq.active);

	dev->vid_timeout.function = au0828_vid_buffer_timeout;
	dev->vid_timeout.data = (unsigned long) dev;
	init_timer(&dev->vid_timeout);

	dev->vbi_timeout.function = au0828_vbi_buffer_timeout;
	dev->vbi_timeout.data = (unsigned long) dev;
	init_timer(&dev->vbi_timeout);

	dev->width = NTSC_STD_W;
	dev->height = NTSC_STD_H;
	dev->field_size = dev->width * dev->height;
	dev->frame_size = dev->field_size << 1;
	dev->bytesperline = dev->width << 1;
	dev->vbi_width = 720;
	dev->vbi_height = 1;
	dev->ctrl_ainput = 0;
	dev->ctrl_freq = 960;
	dev->std = V4L2_STD_NTSC_M;
	au0828_s_input(dev, 0);

	/* allocate and fill v4l2 video struct */
	dev->vdev = video_device_alloc();
	if (NULL == dev->vdev) {
		dprintk(1, "Can't allocate video_device.\n");
		return -ENOMEM;
	}

Quoted 5 lines

Quoted 2 lines
	/* allocate the VBI struct */
	dev->vbi_dev = video_device_alloc();
I recommend that these video_device structs are embedded in au0828_dev. That way you don't need to check for errors here.

Do you mean make them sttaic alloc as opposed allocating memory here at register time?? usbtv does that looks like. I am fine with that.

Quoted 46 lines

	if (NULL == dev->vbi_dev) {
		dprintk(1, "Can't allocate vbi_device.\n");
		ret = -ENOMEM;
		goto err_vdev;
	}

	mutex_init(&dev->vb_queue_lock);
	mutex_init(&dev->vb_vbi_queue_lock);

	/* Fill the video capture device struct */
	*dev->vdev = au0828_video_template;
	dev->vdev->v4l2_dev = &dev->v4l2_dev;
	dev->vdev->lock = &dev->lock;
	dev->vdev->queue = &dev->vb_vidq;
	dev->vdev->queue->lock = &dev->vb_queue_lock;
	strcpy(dev->vdev->name, "au0828a video");

	/* Setup the VBI device */
	*dev->vbi_dev = au0828_video_template;
	dev->vbi_dev->v4l2_dev = &dev->v4l2_dev;
	dev->vbi_dev->lock = &dev->lock;
	dev->vbi_dev->queue = &dev->vb_vbiq;
	dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
	strcpy(dev->vbi_dev->name, "au0828a vbi");

	/* Register the v4l2 device */
	video_set_drvdata(dev->vdev, dev);
	retval = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);
	if (retval != 0) {
		dprintk(1, "unable to register video device (error = %d).\n",
			retval);
		ret = -ENODEV;
		goto err_vbi_dev;
	}

	/* Register the vbi device */
	video_set_drvdata(dev->vbi_dev, dev);
	retval = video_register_device(dev->vbi_dev, VFL_TYPE_VBI, -1);
	if (retval != 0) {
		dprintk(1, "unable to register vbi device (error = %d).\n",
			retval);
		ret = -ENODEV;
		goto err_vbi_dev;
	}

Quoted 4 lines

Quoted 2 lines
	/* initialize videobuf2 stuff */
	retval = au0828_vb2_setup(dev);
This should be done *before* calling video_register_device.

Will fix it.

Quoted 12 lines

	if (retval != 0) {
		dprintk(1, "unable to setup videobuf2 queues (error = %d).\n",
			retval);
		ret = -ENODEV;
		goto err_vbi_dev;
	}

	dprintk(1, "%s completed!\n", __func__);

	return 0;

Quoted 8 lines

Quoted 7 lines

err_vbi_dev:

	video_device_release(dev->vbi_dev);
err_vdev:
	video_device_release(dev->vdev);
	return ret;
}

-- Shuah

Signature · 5 lines

Shuah Khan
Sr. Linux Kernel Developer
Samsung Open Source Group
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978