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 <lkund...@v3.sk>
> Cc: Mauro Carvalho Chehab <mche...@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-me...@vger.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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to