Message-Id: <201306101305.05038.hverkuil@xs4all.nl>
Date:	Mon, 10 Jun 2013 13:05:04 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Lubomir Rintel <lkundrak@...sk>
Cc:	linux-media@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [media] usbtv: Add driver for Fushicai USBTV007 video frame grabber

Hi Lubomir!

Thanks for working on this.

I've got some review comments below...

On Mon June 10 2013 11:52:11 Lubomir Rintel wrote:
> Reverse-engineered driver for cheapo video digitizer, made from observations of
> Windows XP driver. The protocol is not yet completely understood, so far we
> don't provide any controls, only support a single format out of three and don't
> support the audio device.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@...sk>
> Cc: Mauro Carvalho Chehab <mchehab@...hat.com>
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-media@...r.kernel.org
> ---
> Hi everyone,
> 
> this is my first experience with v4l2, videobuf2, usb and a video video
> hardware. Therefore, please be careful reviewing this as I could have made
> mistakes that are not likely to happen for more experienced people.
> 
> I've not figured out the controls for the hardware yet, thus the huge set of
> unidentified register settings. I've been very unsuccessfully struggling with
> my Windows installation (the Windows driver for hardware was not usable enough
> with any other player than the cranky one shipped with it, crashing and
> deadlocking quite often). It seems quite possible to create a simpler
> directshow app that would just set the controls; and I plan to do that as time
> permits.
> 
> Also, I the hardware uses V4L2_FIELD_ALTERNATE interlacing, but I couldn't make
> it work,

What didn't work exactly?

> so I'm doing deinterlacing in the driver, which is obviously not the
> right thing to do. Could anyone educate me on proper way of dealing with data
> interlaced this way? I could not find a decent example, and I'm not even sure
> what the sizes in format specification are (like, is the height after or before
> deinterlacing?).

FIELD_ALTERNATE means that each buffer contains one field, so the format height
should be 240/288 (NTSC/PAL).

Drivers using FIELD_ALTERNATE are very rare.

> 
> Thanks a lot!
> Lubo
> 
>  drivers/media/usb/Kconfig        |    1 +
>  drivers/media/usb/Makefile       |    1 +
>  drivers/media/usb/usbtv/Kconfig  |   10 +
>  drivers/media/usb/usbtv/Makefile |    1 +
>  drivers/media/usb/usbtv/usbtv.c  |  723 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 736 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/usb/usbtv/Kconfig
>  create mode 100644 drivers/media/usb/usbtv/Makefile
>  create mode 100644 drivers/media/usb/usbtv/usbtv.c
> 
> diff --git a/drivers/media/usb/Kconfig b/drivers/media/usb/Kconfig
> index 0a7d520..8e10267 100644
> --- a/drivers/media/usb/Kconfig
> +++ b/drivers/media/usb/Kconfig
> @@ -17,6 +17,7 @@ source "drivers/media/usb/zr364xx/Kconfig"
>  source "drivers/media/usb/stkwebcam/Kconfig"
>  source "drivers/media/usb/s2255/Kconfig"
>  source "drivers/media/usb/sn9c102/Kconfig"
> +source "drivers/media/usb/usbtv/Kconfig"
>  endif
>  
>  if MEDIA_ANALOG_TV_SUPPORT
> diff --git a/drivers/media/usb/Makefile b/drivers/media/usb/Makefile
> index 7f51d7e..0935f47 100644
> --- a/drivers/media/usb/Makefile
> +++ b/drivers/media/usb/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_VIDEO_STK1160) += stk1160/
>  obj-$(CONFIG_VIDEO_CX231XX) += cx231xx/
>  obj-$(CONFIG_VIDEO_TM6000) += tm6000/
>  obj-$(CONFIG_VIDEO_EM28XX) += em28xx/
> +obj-$(CONFIG_VIDEO_USBTV) += usbtv/
> diff --git a/drivers/media/usb/usbtv/Kconfig b/drivers/media/usb/usbtv/Kconfig
> new file mode 100644
> index 0000000..8864436
> --- /dev/null
> +++ b/drivers/media/usb/usbtv/Kconfig
> @@ -0,0 +1,10 @@
> +config VIDEO_USBTV
> +        tristate "USBTV007 video capture support"
> +        depends on VIDEO_DEV
> +        select VIDEOBUF2_VMALLOC
> +
> +        ---help---
> +          This is a video4linux2 driver for USBTV007 based video capture devices.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called usbtv
> diff --git a/drivers/media/usb/usbtv/Makefile b/drivers/media/usb/usbtv/Makefile
> new file mode 100644
> index 0000000..28b872f
> --- /dev/null
> +++ b/drivers/media/usb/usbtv/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_USBTV) += usbtv.o
> diff --git a/drivers/media/usb/usbtv/usbtv.c b/drivers/media/usb/usbtv/usbtv.c
> new file mode 100644
> index 0000000..d165cb1
> --- /dev/null
> +++ b/drivers/media/usb/usbtv/usbtv.c
> @@ -0,0 +1,723 @@
> +/*
> + * Fushicai USBTV007 Video Grabber Driver
> + *
> + * Product web site:
> + * http://www.fushicai.com/products_detail/&productId=d05449ee-b690-42f9-a661-aa7353894bed.html
> + *
> + * Following LWN articles were very useful in construction of this driver:
> + * Video4Linux2 API series: http://lwn.net/Articles/203924/
> + * videobuf2 API explanation: http://lwn.net/Articles/447435/
> + * Thanks go to Jonathan Corbet for providing this quality documentation.
> + * He is awesome.
> + *
> + * Copyright (c) 2013 Lubomir Rintel
> + * All rights reserved.
> + * No physical hadrware was harmed running Windows during the

typo: hardware

> + * reverse-engineering activity
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions, and the following disclaimer,
> + *    without modification.
> + * 2. The name of the author may not be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL").
> + */
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/version.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-vmalloc.h>
> +
> +/* Hardware. */
> +#define USBTV_VIDEO_ENDP	0x81
> +#define USBTV_BASE		0xc000
> +#define USBTV_REQUEST_REG	12
> +
> +/* Number of concurrent isochronous urbs submitted.
> + * Higher numbers was seen to overly saturate the USB bus. */
> +#define USBTV_ISOC_TRANSFERS	16
> +#define USBTV_ISOC_PACKETS	8
> +
> +#define USBTV_WIDTH		720
> +#define USBTV_HEIGHT		480
> +
> +#define USBTV_CHUNK_SIZE	256
> +#define USBTV_CHUNK		240
> +#define USBTV_CHUNKS		(USBTV_WIDTH * USBTV_HEIGHT \
> +					/ 2 / USBTV_CHUNK)
> +
> +/* Chunk header. */
> +#define USBTV_MAGIC_OK(chunk)	((be32_to_cpu(chunk[0]) & 0xff000000) \
> +							== 0x88000000)
> +#define USBTV_FRAME_ID(chunk)	((be32_to_cpu(chunk[0]) & 0x00ff0000) >> 16)
> +#define USBTV_ODD(chunk)	((be32_to_cpu(chunk[0]) & 0x0000f000) >> 15)
> +#define USBTV_CHUNK_NO(chunk)	(be32_to_cpu(chunk[0]) & 0x00000fff)
> +
> +/* A single videobuf2 frame buffer. */
> +struct usbtv_buf {
> +	struct vb2_buffer vb;
> +	struct list_head list;
> +};
> +
> +/* Per-device structure. */
> +struct usbtv {
> +	struct device *dev;
> +	struct usb_device *udev;
> +	struct v4l2_device v4l2_dev;
> +	struct video_device vdev;
> +	struct vb2_queue vb2q;
> +	struct mutex v4l2_lock;
> +	struct mutex vb2q_lock;
> +
> +	/* List of videobuf2 buffers protected by a lock. */
> +	spinlock_t buflock;
> +	struct list_head bufs;
> +
> +	/* Number of currently processed frame, useful find
> +	 * out when a new one begins. */
> +	u32 frame_id;
> +
> +	int iso_size;
> +	unsigned int sequence;
> +	struct urb *isoc_urbs[USBTV_ISOC_TRANSFERS];
> +};
> +
> +static int usbtv_setup_capture(struct usbtv *usbtv)
> +{
> +	int ret;
> +	int pipe = usb_rcvctrlpipe(usbtv->udev, 0);
> +	int i;
> +	u16 protoregs[][2] = {

Add 'static const' since this table is static and const.

> +		/* These seem to enable the device. */
> +		{ USBTV_BASE + 0x0008, 0x0001 },
> +		{ USBTV_BASE + 0x01d0, 0x00ff },
> +		{ USBTV_BASE + 0x01d9, 0x0002 },
> +
> +		/* These seem to influence color parameters, such as
> +		 * brightness, etc. */
> +		{ USBTV_BASE + 0x0239, 0x0040 },
> +		{ USBTV_BASE + 0x0240, 0x0000 },
> +		{ USBTV_BASE + 0x0241, 0x0000 },
> +		{ USBTV_BASE + 0x0242, 0x0002 },
> +		{ USBTV_BASE + 0x0243, 0x0080 },
> +		{ USBTV_BASE + 0x0244, 0x0012 },
> +		{ USBTV_BASE + 0x0245, 0x0090 },
> +		{ USBTV_BASE + 0x0246, 0x0000 },
> +
> +		{ USBTV_BASE + 0x0278, 0x002d },
> +		{ USBTV_BASE + 0x0279, 0x000a },
> +		{ USBTV_BASE + 0x027a, 0x0032 },
> +		{ 0xf890, 0x000c },
> +		{ 0xf894, 0x0086 },
> +
> +		{ USBTV_BASE + 0x00ac, 0x00c0 },
> +		{ USBTV_BASE + 0x00ad, 0x0000 },
> +		{ USBTV_BASE + 0x00a2, 0x0012 },
> +		{ USBTV_BASE + 0x00a3, 0x00e0 },
> +		{ USBTV_BASE + 0x00a4, 0x0028 },
> +		{ USBTV_BASE + 0x00a5, 0x0082 },
> +		{ USBTV_BASE + 0x00a7, 0x0080 },
> +		{ USBTV_BASE + 0x0000, 0x0014 },
> +		{ USBTV_BASE + 0x0006, 0x0003 },
> +		{ USBTV_BASE + 0x0090, 0x0099 },
> +		{ USBTV_BASE + 0x0091, 0x0090 },
> +		{ USBTV_BASE + 0x0094, 0x0068 },
> +		{ USBTV_BASE + 0x0095, 0x0070 },
> +		{ USBTV_BASE + 0x009c, 0x0030 },
> +		{ USBTV_BASE + 0x009d, 0x00c0 },
> +		{ USBTV_BASE + 0x009e, 0x00e0 },
> +		{ USBTV_BASE + 0x0019, 0x0006 },
> +		{ USBTV_BASE + 0x008c, 0x00ba },
> +		{ USBTV_BASE + 0x0101, 0x00ff },
> +		{ USBTV_BASE + 0x010c, 0x00b3 },
> +		{ USBTV_BASE + 0x01b2, 0x0080 },
> +		{ USBTV_BASE + 0x01b4, 0x00a0 },
> +		{ USBTV_BASE + 0x014c, 0x00ff },
> +		{ USBTV_BASE + 0x014d, 0x00ca },
> +		{ USBTV_BASE + 0x0113, 0x0053 },
> +		{ USBTV_BASE + 0x0119, 0x008a },
> +		{ USBTV_BASE + 0x013c, 0x0003 },
> +		{ USBTV_BASE + 0x0150, 0x009c },
> +		{ USBTV_BASE + 0x0151, 0x0071 },
> +		{ USBTV_BASE + 0x0152, 0x00c6 },
> +		{ USBTV_BASE + 0x0153, 0x0084 },
> +		{ USBTV_BASE + 0x0154, 0x00bc },
> +		{ USBTV_BASE + 0x0155, 0x00a0 },
> +		{ USBTV_BASE + 0x0156, 0x00a0 },
> +		{ USBTV_BASE + 0x0157, 0x009c },
> +		{ USBTV_BASE + 0x0158, 0x001f },
> +		{ USBTV_BASE + 0x0159, 0x0006 },
> +		{ USBTV_BASE + 0x015d, 0x0000 },
> +
> +		{ USBTV_BASE + 0x0284, 0x0088 },
> +		{ USBTV_BASE + 0x0003, 0x0004 },
> +		{ USBTV_BASE + 0x001a, 0x0079 },
> +		{ USBTV_BASE + 0x0100, 0x00d3 },
> +		{ USBTV_BASE + 0x010e, 0x0068 },
> +		{ USBTV_BASE + 0x010f, 0x009c },
> +		{ USBTV_BASE + 0x0112, 0x00f0 },
> +		{ USBTV_BASE + 0x0115, 0x0015 },
> +		{ USBTV_BASE + 0x0117, 0x0000 },
> +		{ USBTV_BASE + 0x0118, 0x00fc },
> +		{ USBTV_BASE + 0x012d, 0x0004 },
> +		{ USBTV_BASE + 0x012f, 0x0008 },
> +		{ USBTV_BASE + 0x0220, 0x002e },
> +		{ USBTV_BASE + 0x0225, 0x0008 },
> +		{ USBTV_BASE + 0x024e, 0x0002 },
> +		{ USBTV_BASE + 0x024f, 0x0001 },
> +		{ USBTV_BASE + 0x0254, 0x005f },
> +		{ USBTV_BASE + 0x025a, 0x0012 },
> +		{ USBTV_BASE + 0x025b, 0x0001 },
> +		{ USBTV_BASE + 0x0263, 0x001c },
> +		{ USBTV_BASE + 0x0266, 0x0011 },
> +		{ USBTV_BASE + 0x0267, 0x0005 },
> +		{ USBTV_BASE + 0x024e, 0x0002 },
> +		{ USBTV_BASE + 0x024f, 0x0002 },
> +	};
> +
> +	for (i = 0; i < sizeof(protoregs)/sizeof(protoregs[0]); i++) {
> +		u16 index = protoregs[i][0];
> +		u16 value = protoregs[i][1];
> +
> +		ret = usb_control_msg(usbtv->udev, pipe, USBTV_REQUEST_REG,
> +			USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +			value, index, NULL, 0, 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Called for each 256-byte image chunk.
> + * First word identifies the chunk, followed by 240 words of image
> + * data and padding. */
> +static void usbtv_image_chunk(struct usbtv *usbtv, u32 *chunk)
> +{
> +	int frame_id, odd, chunk_no;
> +	u32 *frame;
> +	struct usbtv_buf *buf;
> +	unsigned long flags;
> +
> +	/* Ignore corrupted lines. */
> +	if (!USBTV_MAGIC_OK(chunk))
> +		return;
> +	frame_id = USBTV_FRAME_ID(chunk);
> +	odd = USBTV_ODD(chunk);
> +	chunk_no = USBTV_CHUNK_NO(chunk);
> +
> +	/* Deinterlace. TODO: Use interlaced frame format. */
> +	chunk_no = (chunk_no - chunk_no % 3) * 2 + chunk_no % 3;
> +	chunk_no += !odd % 2 * 3;

Huh? '!odd' is 0 or 1, so the '% 2' part does nothing and is quite confusing.

> +
> +	if (chunk_no >= USBTV_CHUNKS)
> +		return;
> +
> +	/* Beginning of a frame. */
> +	if (chunk_no == 0)
> +		usbtv->frame_id = frame_id;
> +
> +	spin_lock_irqsave(&usbtv->buflock, flags);
> +	if (list_empty(&usbtv->bufs)) {
> +		/* No free buffers. Userspace likely too slow. */
> +		spin_unlock_irqrestore(&usbtv->buflock, flags);
> +		return;
> +	}
> +
> +	/* First available buffer. */
> +	buf = list_first_entry(&usbtv->bufs, struct usbtv_buf, list);
> +	frame = vb2_plane_vaddr(&buf->vb, 0);
> +
> +	/* Copy the chunk. */
> +	memcpy(&frame[chunk_no * USBTV_CHUNK], &chunk[1],
> +			USBTV_CHUNK * sizeof(chunk[1]));
> +
> +	/* Last chunk in a frame, signalling an end */
> +	if (usbtv->frame_id && chunk_no == USBTV_CHUNKS-1) {
> +		int size = vb2_plane_size(&buf->vb, 0);
> +
> +		buf->vb.v4l2_buf.field = V4L2_FIELD_NONE;
> +		buf->vb.v4l2_buf.sequence = usbtv->sequence++;
> +		vb2_set_plane_payload(&buf->vb, 0, size);
> +		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_DONE);
> +		list_del(&buf->list);
> +	}
> +
> +	spin_unlock_irqrestore(&usbtv->buflock, flags);
> +}
> +
> +/* Got image data. Each packet contains a number of 256-word chunks we
> + * compose the image from. */
> +static void usbtv_iso_cb(struct urb *ip)
> +{
> +	int ret;
> +	int i;
> +	struct usbtv *usbtv = (struct usbtv *)ip->context;
> +
> +	switch (ip->status) {
> +	/* All fine. */
> +	case 0:
> +		break;
> +	/* Device disconnected or capture stopped? */
> +	case -ENODEV:
> +	case -ENOENT:
> +	case -ECONNRESET:
> +	case -ESHUTDOWN:
> +		return;
> +	/* Unknown error. Retry. */
> +	default:
> +		dev_warn(usbtv->dev, "Bad response for ISO request.\n");
> +		goto resubmit;
> +	}
> +
> +	for (i = 0; i < ip->number_of_packets; i++) {
> +		int size = ip->iso_frame_desc[i].actual_length;
> +		unsigned char *data = ip->transfer_buffer +
> +				ip->iso_frame_desc[i].offset;
> +		int offset;
> +
> +		for (offset = 0; USBTV_CHUNK_SIZE * offset < size; offset++)
> +			usbtv_image_chunk(usbtv,
> +				(u32 *)&data[USBTV_CHUNK_SIZE * offset]);
> +	}
> +
> +resubmit:
> +	ret = usb_submit_urb(ip, GFP_ATOMIC);
> +	if (ret < 0)
> +		dev_warn(usbtv->dev, "Could not resubmit ISO URB\n");
> +}
> +
> +static struct urb *usbtv_setup_iso_transfer(struct usbtv *usbtv)
> +{
> +	struct urb *ip;
> +	int size = usbtv->iso_size;
> +	int i;
> +
> +	ip = usb_alloc_urb(USBTV_ISOC_PACKETS, GFP_KERNEL);
> +	if (ip == NULL)
> +		return NULL;
> +
> +	ip->dev = usbtv->udev;
> +	ip->context = usbtv;
> +	ip->pipe = usb_rcvisocpipe(usbtv->udev, USBTV_VIDEO_ENDP);
> +	ip->interval = 1;
> +	ip->transfer_flags = URB_ISO_ASAP;
> +	ip->transfer_buffer = kzalloc(size * USBTV_ISOC_PACKETS,
> +						GFP_KERNEL);
> +	ip->complete = usbtv_iso_cb;
> +	ip->number_of_packets = USBTV_ISOC_PACKETS;
> +	ip->transfer_buffer_length = size * USBTV_ISOC_PACKETS;
> +	for (i = 0; i < USBTV_ISOC_PACKETS; i++) {
> +		ip->iso_frame_desc[i].offset = size * i;
> +		ip->iso_frame_desc[i].length = size;
> +	}
> +
> +	return ip;
> +}
> +
> +static void usbtv_stop(struct usbtv *usbtv)
> +{
> +	int i;
> +	unsigned long flags;
> +
> +	/* Cancel running transfers. */
> +	for (i = 0; i < USBTV_ISOC_TRANSFERS; i++) {
> +		struct urb *ip = usbtv->isoc_urbs[i];
> +		if (ip == NULL)
> +			continue;
> +		usb_kill_urb(ip);
> +		kfree(ip->transfer_buffer);
> +		usb_free_urb(ip);
> +		usbtv->isoc_urbs[i] = NULL;
> +	}
> +
> +	/* Return buffers to userspace. */
> +	spin_lock_irqsave(&usbtv->buflock, flags);
> +	while (!list_empty(&usbtv->bufs)) {
> +		struct usbtv_buf *buf = list_first_entry(&usbtv->bufs,
> +						struct usbtv_buf, list);
> +		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> +		list_del(&buf->list);
> +	}
> +	spin_unlock_irqrestore(&usbtv->buflock, flags);
> +}
> +
> +static int usbtv_start(struct usbtv *usbtv)
> +{
> +	int i;
> +	int ret;
> +
> +	ret = usb_set_interface(usbtv->udev, 0, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usbtv_setup_capture(usbtv);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = usb_set_interface(usbtv->udev, 0, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < USBTV_ISOC_TRANSFERS; i++) {
> +		struct urb *ip;
> +
> +		ip = usbtv_setup_iso_transfer(usbtv);
> +		if (ip == NULL) {
> +			ret = -ENOMEM;
> +			goto start_fail;
> +		}
> +		usbtv->isoc_urbs[i] = ip;
> +
> +		ret = usb_submit_urb(ip, GFP_KERNEL);
> +		if (ret < 0)
> +			goto start_fail;
> +	}
> +
> +	return 0;
> +
> +start_fail:
> +	usbtv_stop(usbtv);
> +	return ret;
> +}
> +
> +struct usb_device_id usbtv_id_table[] = {
> +	{ USB_DEVICE(0x1b71, 0x3002) },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(usb, usbtv_id_table);
> +
> +static int usbtv_querycap(struct file *file, void *priv,
> +				struct v4l2_capability *cap)
> +{
> +	struct usbtv *dev = video_drvdata(file);
> +
> +	strncpy(cap->driver, "usbtv", sizeof(cap->driver));
> +	strncpy(cap->card, "usbtv", sizeof(cap->card));
> +	usb_make_path(dev->udev, cap->bus_info, sizeof(cap->bus_info));
> +	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE;
> +	cap->device_caps |= V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
> +	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +	return 0;
> +}
> +
> +static int usbtv_enum_input(struct file *file, void *priv,
> +					struct v4l2_input *i)
> +{
> +	if (i->index > 0)
> +		return -EINVAL;
> +
> +	strncpy(i->name, "Composite", sizeof(i->name));
> +	i->type = V4L2_INPUT_TYPE_CAMERA;
> +	i->std = V4L2_STD_525_60;
> +	return 0;
> +}
> +
> +static int usbtv_enum_fmt_vid_cap(struct file *file, void  *priv,
> +					struct v4l2_fmtdesc *f)
> +{
> +	if (f->index > 0)
> +		return -EINVAL;
> +
> +	strncpy(f->description, "16 bpp YUY2, 4:2:2, packed",
> +					sizeof(f->description));
> +	f->pixelformat = V4L2_PIX_FMT_YUYV;
> +	return 0;
> +}
> +
> +static int usbtv_g_fmt_vid_cap(struct file *file, void *priv,
> +					struct v4l2_format *f)

Rename to usbtv_fmt_vid_cap and use this for all g/try/s ops.

> +{
> +	f->fmt.pix.width = USBTV_WIDTH;
> +	f->fmt.pix.height = USBTV_HEIGHT;
> +	f->fmt.pix.pixelformat = V4L2_PIX_FMT_YUYV;
> +	f->fmt.pix.field = V4L2_FIELD_NONE;

This should be V4L2_FIELD_ALTERNATE I guess.

> +	f->fmt.pix.bytesperline = USBTV_WIDTH * 2;
> +	f->fmt.pix.sizeimage = (f->fmt.pix.bytesperline * f->fmt.pix.height);
> +	f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	return 0;
> +}
> +
> +static int usbtv_try_fmt_vid_cap(struct file *file, void *priv,
> +					struct v4l2_format *f)
> +{
> +	return usbtv_g_fmt_vid_cap(file, priv, f);
> +}
> +
> +static int usbtv_s_fmt_vid_cap(struct file *file, void *priv,
> +					struct v4l2_format *f)
> +{
> +	return usbtv_try_fmt_vid_cap(file, priv, f);
> +}

These two can be dropped.

> +
> +static int usbtv_g_std(struct file *file, void *priv, v4l2_std_id *norm)
> +{
> +	*norm = V4L2_STD_525_60;
> +	return 0;
> +}
> +
> +static int usbtv_g_input(struct file *file, void *priv, unsigned int *i)
> +{
> +	*i = 0;
> +	return 0;
> +}
> +
> +static int usbtv_s_input(struct file *file, void *priv, unsigned int i)
> +{
> +	if (i > 0)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int usbtv_s_std(struct file *file, void *priv, v4l2_std_id norm)
> +{
> +	if (norm & V4L2_STD_525_60)
> +		return 0;
> +	return -EINVAL;
> +}
> +
> +static int usbtv_queryctrl(struct file *file, void *priv,
> +				struct v4l2_queryctrl *ctrl)
> +{
> +	return -EINVAL;
> +}

Drop this ioctl. If it doesn't do anything, then don't specify it.

> +
> +struct v4l2_ioctl_ops usbtv_ioctl_ops = {
> +	.vidioc_querycap = usbtv_querycap,
> +	.vidioc_enum_input = usbtv_enum_input,
> +	.vidioc_enum_fmt_vid_cap = usbtv_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap = usbtv_g_fmt_vid_cap,
> +	.vidioc_try_fmt_vid_cap = usbtv_try_fmt_vid_cap,
> +	.vidioc_s_fmt_vid_cap = usbtv_s_fmt_vid_cap,
> +	.vidioc_g_std = usbtv_g_std,
> +	.vidioc_s_std = usbtv_s_std,
> +	.vidioc_g_input = usbtv_g_input,
> +	.vidioc_s_input = usbtv_s_input,
> +	.vidioc_queryctrl = usbtv_queryctrl,
> +
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
> +};
> +
> +struct v4l2_file_operations usbtv_fops = {
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = video_ioctl2,
> +	.mmap = vb2_fop_mmap,
> +	.open = v4l2_fh_open,
> +	.release = vb2_fop_release,
> +	.read = vb2_fop_read,
> +	.poll = vb2_fop_poll,
> +};
> +
> +static int usbtv_queue_setup(struct vb2_queue *vq,
> +	const struct v4l2_format *v4l_fmt, unsigned int *nbuffers,
> +	unsigned int *nplanes, unsigned int sizes[], void *alloc_ctxs[])
> +{
> +	if (*nbuffers == 0)
> +		*nbuffers = 8;
> +	*nplanes = 1;
> +	sizes[0] = USBTV_CHUNK * USBTV_CHUNKS * sizeof(u32);
> +
> +	return 0;
> +}
> +
> +static void usbtv_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct usbtv *usbtv = vb2_get_drv_priv(vb->vb2_queue);
> +	struct usbtv_buf *buf = container_of(vb, struct usbtv_buf, vb);
> +	unsigned long flags;
> +
> +	if (usbtv->udev == NULL) {
> +		vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> +		return;
> +	}
> +
> +	spin_lock_irqsave(&usbtv->buflock, flags);
> +	list_add_tail(&buf->list, &usbtv->bufs);
> +	spin_unlock_irqrestore(&usbtv->buflock, flags);
> +}
> +
> +static int usbtv_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> +	struct usbtv *usbtv = vb2_get_drv_priv(vq);
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&usbtv->v4l2_lock))
> +		return -ERESTARTSYS;
> +	if (usbtv->udev == NULL) {
> +		mutex_unlock(&usbtv->v4l2_lock);
> +		return -ENODEV;
> +	}
> +
> +	ret = usbtv_start(usbtv);
> +	mutex_unlock(&usbtv->v4l2_lock);
> +	return ret;
> +}
> +
> +static int usbtv_stop_streaming(struct vb2_queue *vq)
> +{
> +	struct usbtv *usbtv = vb2_get_drv_priv(vq);
> +
> +	if (mutex_lock_interruptible(&usbtv->v4l2_lock))
> +		return -ERESTARTSYS;
> +	if (usbtv->udev == NULL) {
> +		mutex_unlock(&usbtv->v4l2_lock);
> +		return -ENODEV;
> +	}
> +
> +	usbtv_stop(usbtv);
> +	mutex_unlock(&usbtv->v4l2_lock);
> +	return 0;
> +}
> +
> +struct vb2_ops usbtv_vb2_ops = {
> +	.queue_setup = usbtv_queue_setup,
> +	.buf_queue = usbtv_buf_queue,
> +	.start_streaming = usbtv_start_streaming,
> +	.stop_streaming = usbtv_stop_streaming,
> +};
> +
> +static void usbtv_release(struct v4l2_device *v4l2_dev)
> +{
> +	struct usbtv *usbtv = container_of(v4l2_dev, struct usbtv, v4l2_dev);
> +
> +	v4l2_device_unregister(&usbtv->v4l2_dev);
> +	vb2_queue_release(&usbtv->vb2q);
> +	kfree(usbtv);
> +}
> +
> +static int usbtv_probe(struct usb_interface *intf,
> +	const struct usb_device_id *id)
> +{
> +	int ret;
> +	int size;
> +	struct device *dev = &intf->dev;
> +	struct usbtv *usbtv;
> +
> +	/* Checks that the device is what we think it is. */
> +	if (intf->num_altsetting != 2)
> +		return -ENODEV;
> +	if (intf->altsetting[1].desc.bNumEndpoints != 4)
> +		return -ENODEV;
> +
> +	/* Packet size is split into 11 bits of base size and count of
> +	 * extra multiplies of it.*/
> +	size = usb_endpoint_maxp(&intf->altsetting[1].endpoint[0].desc);
> +	size = (size & 0x07ff) * (((size & 0x1800) >> 11) + 1);
> +
> +	/* Device structure */
> +	usbtv = kzalloc(sizeof(struct usbtv), GFP_KERNEL);
> +	if (usbtv == NULL)
> +		return -ENOMEM;
> +	usbtv->dev = dev;
> +	usbtv->udev = usb_get_dev(interface_to_usbdev(intf));
> +	usbtv->iso_size = size;
> +	spin_lock_init(&usbtv->buflock);
> +	mutex_init(&usbtv->v4l2_lock);
> +	mutex_init(&usbtv->vb2q_lock);
> +	INIT_LIST_HEAD(&usbtv->bufs);
> +
> +	/* videobuf2 structure */
> +	usbtv->vb2q.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	usbtv->vb2q.io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;
> +	usbtv->vb2q.drv_priv = usbtv;
> +	usbtv->vb2q.buf_struct_size = sizeof(struct usbtv_buf);
> +	usbtv->vb2q.ops = &usbtv_vb2_ops;
> +	usbtv->vb2q.mem_ops = &vb2_vmalloc_memops;
> +	usbtv->vb2q.timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +	usbtv->vb2q.lock = &usbtv->vb2q_lock;
> +	ret = vb2_queue_init(&usbtv->vb2q);
> +	if (ret < 0) {
> +		dev_warn(dev, "Could not initialize videobuf2 queue\n");
> +		goto usbtv_fail;
> +	}
> +
> +	/* v4l2 structure */
> +	usbtv->v4l2_dev.release = usbtv_release;
> +	ret = v4l2_device_register(dev, &usbtv->v4l2_dev);
> +	if (ret < 0) {
> +		dev_warn(dev, "Could not register v4l2 device\n");
> +		goto v4l2_fail;
> +	}
> +
> +	usb_set_intfdata(intf, usbtv);
> +
> +	/* Video structure */
> +	strncpy(usbtv->vdev.name, "usbtv", sizeof(usbtv->vdev.name));
> +	usbtv->vdev.v4l2_dev = &usbtv->v4l2_dev;
> +	usbtv->vdev.release = video_device_release_empty;
> +	usbtv->vdev.fops = &usbtv_fops;
> +	usbtv->vdev.ioctl_ops = &usbtv_ioctl_ops;
> +	usbtv->vdev.tvnorms = V4L2_STD_525_60;
> +	usbtv->vdev.queue = &usbtv->vb2q;
> +	usbtv->vdev.lock = &usbtv->v4l2_lock;
> +	ret = video_register_device(&usbtv->vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret < 0) {
> +		dev_warn(dev, "Could not register video device\n");
> +		goto vdev_fail;
> +	}
> +	video_set_drvdata(&usbtv->vdev, usbtv);
> +
> +	dev_info(dev, "Fushicai USBTV007 Video Grabber Driver\n");
> +	return 0;
> +
> +vdev_fail:
> +	v4l2_device_unregister(&usbtv->v4l2_dev);
> +v4l2_fail:
> +	vb2_queue_release(&usbtv->vb2q);
> +usbtv_fail:
> +	kfree(usbtv);
> +
> +	return ret;
> +}
> +
> +static void usbtv_disconnect(struct usb_interface *intf)
> +{
> +	struct usbtv *usbtv = usb_get_intfdata(intf);
> +
> +	mutex_lock(&usbtv->vb2q_lock);
> +	mutex_lock(&usbtv->v4l2_lock);
> +
> +	usbtv_stop(usbtv);
> +	usb_set_intfdata(intf, NULL);
> +	video_unregister_device(&usbtv->vdev);
> +	v4l2_device_disconnect(&usbtv->v4l2_dev);
> +	usb_put_dev(usbtv->udev);
> +	usbtv->udev = NULL;
> +
> +	mutex_unlock(&usbtv->v4l2_lock);
> +	mutex_unlock(&usbtv->vb2q_lock);
> +
> +	v4l2_device_put(&usbtv->v4l2_dev);
> +}
> +
> +MODULE_AUTHOR("Lubomir Rintel");
> +MODULE_DESCRIPTION("Fushicai USBTV007 Video Grabber Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +struct usb_driver usbtv_usb_driver = {
> +	.name = "usbtv",
> +	.id_table = usbtv_id_table,
> +	.probe = usbtv_probe,
> +	.disconnect = usbtv_disconnect,
> +};
> +
> +module_usb_driver(usbtv_usb_driver);
> 

It doesn't look too bad :-) You're using all the latest frameworks which is
excellent.

Can you run the v4l2-compliance tool and post the output of that tool?

It's part of the v4l-utils.git repo (http://git.linuxtv.org/v4l-utils.git).
Please compile from the repo, don't rely on your distro. The v4l2-compliance
tool as installed by a distro is often too old.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists