All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-14 20:59 Greg KH
  2008-05-15  1:17   ` Markus Rechberger
                   ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Greg KH @ 2008-05-14 20:59 UTC (permalink / raw)
  To: mchehab, v4l-dvb-maintainer; +Cc: linux-usb, linux-kernel, video4linux-list

From: Dean Anderson <dean@sensoray.com>

This driver adds support for the Sensoray 2255 devices.

It was primarily developed by Dean Anderson with only a little bit of
guidance and cleanup by Greg.

From: Dean Anderson <dean@sensoray.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/media/video/Kconfig    |    9 
 drivers/media/video/Makefile   |    1 
 drivers/media/video/s2255drv.c | 2719 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 2729 insertions(+)

--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -893,6 +893,15 @@ config USB_STKWEBCAM
 	  To compile this driver as a module, choose M here: the
 	  module will be called stkwebcam.
 
+config USB_S2255
+	tristate "USB Sensoray 2255 video capture device"
+	depends on VIDEO_V4L2
+	select VIDEOBUF_VMALLOC
+	default n
+	help
+	  Say Y here if you want support for the Sensoray 2255 USB device.
+	  This driver can be compiled as a module, called s2255drv.
+
 endif # V4L_USB_DRIVERS
 
 config SOC_CAMERA
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -122,6 +122,7 @@ obj-$(CONFIG_USB_IBMCAM)        += usbvi
 obj-$(CONFIG_USB_KONICAWC)      += usbvideo/
 obj-$(CONFIG_USB_VICAM)         += usbvideo/
 obj-$(CONFIG_USB_QUICKCAM_MESSENGER)	+= usbvideo/
+obj-$(CONFIG_USB_S2255)		+= s2255drv.o
 
 obj-$(CONFIG_VIDEO_IVTV) += ivtv/
 obj-$(CONFIG_VIDEO_CX18) += cx18/
--- /dev/null
+++ b/drivers/media/video/s2255drv.c
@@ -0,0 +1,2719 @@
+/*
+ * Sensoray 2255 USB Video for Linux driver
+ *
+ * Copyright (C) 2007-2008 by Sensoray Company Inc.
+ *                            Dean Anderson
+ *
+ *      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, version 2.
+ *
+ * Some video buffer code based on vivi driver:
+ *
+ * TODO: Incorporate videodev2 frame rate(FR) enumeration
+ *       (currently experimental.)
+ *
+ *       2255 device supports 4 simultaneous channels.
+ *       The channels are not "crossbar" inputs, they are physically
+ *       attached to separate video decoders.
+ *       Because of USB2.0 bandwidth limitations. There is only a
+ *       certain amount of data which may be transferred at one time
+ *       Because FR control is not in V4L yet, we may want to
+ *       limit the cases:
+ *       1) full size, color mode YUYV or YUV422P:
+ *          2 video_devices allowed at full size.
+ *       2) full or half size Grey scale:
+ *          4 video_devices
+ *       3) half size, color mode YUYV or YUV422P
+ *          4 video_devices
+ */
+#include <linux/module.h>
+#include <linux/firmware.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/mm.h>
+#include <linux/ioport.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/pci.h>
+#include <linux/random.h>
+#include <linux/mutex.h>
+#include <linux/videodev2.h>
+#include <linux/version.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <media/videobuf-vmalloc.h>
+#include <media/v4l2-common.h>
+#include <linux/kthread.h>
+#include <linux/highmem.h>
+#include <linux/freezer.h>
+#include <linux/vmalloc.h>
+#include <linux/usb.h>
+
+#define FIRMWARE_FILE_NAME "f2255usb.bin"
+
+#define DIR_IN			0
+#define DIR_OUT			1
+
+/* firmware query */
+#define VX_FW 			0x30
+
+#define MAX_CHANNELS		4
+#define FRAME_MARKER		0x2255DA4AL
+#define MAX_PIPE_USBBLOCK	(40*1024)
+#define DEFAULT_PIPE_USBBLOCK	(16*1024)
+#define MAX_CHANNELS		4
+#define MAX_PIPE_BUFFERS	1
+#define SYS_FRAMES		4
+/* maximum size is PAL full size plus room for the marker header(s) */
+#define SYS_FRAMES_MAXSIZE	(720*288*2*2 + 4096)
+#define DEF_USB_BLOCK		(4096)
+#define LINE_SZ_4CIFS_NTSC	640
+#define LINE_SZ_2CIFS_NTSC	640
+#define LINE_SZ_1CIFS_NTSC	320
+#define LINE_SZ_4CIFS_PAL	704
+#define LINE_SZ_2CIFS_PAL	704
+#define LINE_SZ_1CIFS_PAL	352
+#define NUM_LINES_4CIFS_NTSC	240
+#define NUM_LINES_2CIFS_NTSC	240
+#define NUM_LINES_1CIFS_NTSC	240
+#define NUM_LINES_4CIFS_PAL	288
+#define NUM_LINES_2CIFS_PAL	288
+#define NUM_LINES_1CIFS_PAL	288
+#define LINE_SZ_DEF		640
+#define NUM_LINES_DEF		240
+
+
+/* predefined settings */
+#define FORMAT_NTSC	1
+#define FORMAT_PAL	2
+
+#define SCALE_4CIFS	1	/* 640x480(NTSC) or 704x576(PAL) */
+#define SCALE_2CIFS	2	/* 640x240(NTSC) or 704x288(PAL) */
+#define SCALE_1CIFS	3	/* 320x240(NTSC) or 352x288(PAL) */
+
+#define COLOR_YUVPL	1	/* YUV planar */
+#define COLOR_YUVPK	2	/* YUV packed */
+#define COLOR_RGB	3	/* RGB */
+#define COLOR_Y8	4	/* monochrome */
+
+/* frame decimation. Not implemented by V4L yet(experimental in V4L) */
+#define FDEC_1		1	/* capture every frame. default */
+#define FDEC_2		2	/* capture every 2nd frame */
+#define FDEC_3		3	/* capture every 3rd frame */
+#define FDEC_5		5	/* capture every 5th frame */
+
+/*-------------------------------------------------------
+ * Default mode parameters.
+ *-------------------------------------------------------*/
+#define DEF_SCALE	SCALE_4CIFS
+#define DEF_COLOR	COLOR_YUVPL
+#define DEF_FDEC	FDEC_1
+#define DEF_BRIGHT	0
+#define DEF_CONTRAST	0x5c
+#define DEF_SATURATION	0x80
+#define DEF_HUE		0
+
+/* usb config commands */
+#define IN_DATA_TOKEN	0x2255c0de
+#define CMD_2255	0xc2255000
+#define CMD_SET_MODE	(CMD_2255 | 0x10)
+#define CMD_START	(CMD_2255 | 0x20)
+#define CMD_STOP	(CMD_2255 | 0x30)
+#define CMD_STATUS	(CMD_2255 | 0x40)
+
+struct mode2255i {
+	u32 format;	/* input video format (NTSC, PAL) */
+	u32 scale;	/* output video scale */
+	u32 color;	/* output video color format */
+	u32 fdec;	/* frame decimation */
+	u32 bright;	/* brightness */
+	u32 contrast;	/* contrast */
+	u32 saturation;	/* saturation */
+	u32 hue;	/* hue (NTSC only)*/
+	u32 single;	/* capture 1 frame at a time (!=0), continuously (==0)*/
+	u32 usb_block;	/* block size. should be 4096 of DEF_USB_BLOCK */
+	u32 restart;	/* if DSP requires restart */
+};
+
+/* frame structure */
+#define FRAME_STATE_UNUSED	0
+#define FRAME_STATE_FILLING	1
+#define FRAME_STATE_FULL	2
+
+
+struct framei {
+	unsigned long size;
+
+	unsigned long ulState;	/* ulState ==0 unused, 1 being filled, 2 full */
+	void *lpvbits;		/* image data */
+	unsigned long cur_size;	/* current data copied to it */
+};
+
+/* image buffer structure */
+struct bufferi {
+	unsigned long dwFrames;			/* number of frames in buffer */
+	struct framei frame[SYS_FRAMES];	/* array of FRAME structures */
+};
+
+#define DEF_MODEI_NTSC_CONT	FORMAT_NTSC, DEF_SCALE, DEF_COLOR, \
+		DEF_FDEC, DEF_BRIGHT, DEF_CONTRAST, DEF_SATURATION, \
+		DEF_HUE, 0, DEF_USB_BLOCK, 0
+
+#define DEF_MODEI_PAL_CONT	FORMAT_PAL, DEF_SCALE, DEF_COLOR, DEF_FDEC,\
+		DEF_BRIGHT, DEF_CONTRAST, DEF_SATURATION, DEF_HUE, 0, \
+		DEF_USB_BLOCK, 0
+
+#define DEF_MODEI_NTSC_SING	FORMAT_NTSC, DEF_SCALE, DEF_COLOR, DEF_FDEC,\
+		DEF_BRIGHT, DEF_CONTRAST, DEF_SATURATION, DEF_HUE, 1,\
+		DEF_USB_BLOCK, 0
+
+#define DEF_MODEI_PAL_SING	FORMAT_PAL, DEF_SCALE, DEF_COLOR, DEF_FDEC, \
+		DEF_BRIGHT, DEF_CONTRAST, DEF_SATURATION, DEF_HUE, 1,\
+		DEF_USB_BLOCK, 0
+
+struct s2255_dmaqueue {
+	struct list_head	active;
+	struct list_head	queued;
+	struct timer_list	timeout;
+	/* thread for acquisition */
+	struct task_struct	*kthread;
+	int			frame;
+	struct s2255_dev	*dev;
+	int			channel;
+};
+
+/* for firmware loading */
+#define FWSTATE_NOTLOADED	0
+#define FWSTATE_SUCCESS		1
+#define FWSTATE_FAILED		2
+
+struct complete_data {
+	int		fw_loaded;
+	int		fw_size;
+	struct urb	*fw_urb;
+	int		fw_state;
+	void		*pfw_data;
+	const struct firmware *fw;
+};
+
+struct s2255_pipeinfo {
+	u32 max_transfer_size;
+	u32 cur_transfer_size;
+	u8 *transfer_buffer;
+	u32 transfer_flags;;
+	u32 state;
+	u32 prev_state;
+	u32 urb_size;
+	void *stream_urb;
+	void *dev;	/* back pointer to s2255_dev struct*/
+	u32 err_count;
+	u32 buf_index;
+	u32 idx;
+	u32 priority_set;
+};
+
+struct s2255_fmt; /*forward declaration */
+
+struct s2255_dev {
+	int			frames;
+	int			users[MAX_CHANNELS];
+	struct mutex		lock;
+	int			resources[MAX_CHANNELS];
+	struct usb_device	*udev;
+	struct usb_interface	*interface;
+	u8			read_endpoint;
+
+	struct s2255_dmaqueue	vidq[MAX_CHANNELS];
+	struct video_device	*vdev[MAX_CHANNELS];
+	struct list_head	s2255_devlist;
+	struct timer_list	timer;
+	struct complete_data	*fw_data;
+	int			board_num;
+	int			is_open;
+	struct s2255_pipeinfo	pipes[MAX_PIPE_BUFFERS];
+	struct bufferi		buffer[MAX_CHANNELS];
+	struct mode2255i	mode[MAX_CHANNELS];
+	const struct s2255_fmt	*cur_fmt[MAX_CHANNELS];
+	int			cur_frame[MAX_CHANNELS];
+	int			last_frame[MAX_CHANNELS];
+	u32			cc;	/* current channel */
+	int			b_acquire[MAX_CHANNELS];
+	unsigned long		req_image_size[MAX_CHANNELS];
+	int			bad_payload[MAX_CHANNELS];
+	unsigned long		frame_count[MAX_CHANNELS];
+	int			frame_ready;
+	struct kref		kref;
+	spinlock_t              slock;
+};
+#define to_s2255_dev(d) container_of(d, struct s2255_dev, kref)
+
+struct s2255_fmt {
+	char *name;
+	u32 fourcc;
+	int depth;
+};
+
+/* buffer for one video frame */
+struct s2255_buffer {
+	/* common v4l buffer stuff -- must be first */
+	struct videobuf_buffer vb;
+	const struct s2255_fmt *fmt;
+	/* future use */
+	int reserved[32];
+};
+
+struct s2255_fh {
+	struct s2255_dev	*dev;
+	unsigned int		resources;
+	const struct s2255_fmt	*fmt;
+	unsigned int		width;
+	unsigned int		height;
+	struct videobuf_queue	vb_vidq;
+	enum v4l2_buf_type	type;
+	int			channel;
+	/* mode below is the desired mode.
+	   mode in s2255_dev is the current mode that was last set */
+	struct mode2255i	mode;
+};
+
+
+#define CUR_USB_FWVER	774	/* current cypress EEPROM firmware version */
+#define S2255_MAJOR_VERSION	1
+#define S2255_MINOR_VERSION	13
+#define S2255_RELEASE		0
+#define S2255_VERSION		KERNEL_VERSION(S2255_MAJOR_VERSION, \
+					       S2255_MINOR_VERSION, \
+					       S2255_RELEASE)
+
+/* vendor ids */
+#define USB_S2255_VENDOR_ID	0x1943
+#define USB_S2255_PRODUCT_ID	0x2255
+#define S2255_NORMS		(V4L2_STD_PAL_B | V4L2_STD_NTSC_M)
+/* frame prefix size (sent once every frame) */
+#define PREFIX_SIZE		512
+
+/* Because the channels were physically printed on the box in
+   reverse order than originally planned */
+static unsigned long G_chnmap[MAX_CHANNELS] = { 3, 2, 1, 0 };
+
+static LIST_HEAD(s2255_devlist);
+
+static int debug;
+static int *s2255_debug = &debug;
+
+static int s2255_start_readpipe(struct s2255_dev *dev);
+static void s2255_stop_readpipe(struct s2255_dev *dev);
+static int s2255_start_acquire(struct s2255_dev *dev, unsigned long chn);
+static int s2255_stop_acquire(struct s2255_dev *dev, unsigned long chn);
+static void s2255_fillbuff(struct s2255_dev *dev, struct s2255_buffer *buf,
+			   int chn);
+static int s2255_set_mode(struct s2255_dev *dev, unsigned long chn,
+			  struct mode2255i *mode);
+
+#define dprintk(level, fmt, arg...)					\
+	do {								\
+		if (*s2255_debug >= (level)) {				\
+			printk(KERN_DEBUG "s2255: " fmt, ##arg);	\
+		}							\
+	} while (0)
+
+
+static DEFINE_MUTEX(usb_s2255_open_mutex);
+static struct usb_driver s2255_driver;
+
+/* Declare static vars that will be used as parameters */
+static unsigned int vid_limit = 16;	/* Video memory limit, in Mb */
+
+/* start video number */
+static int video_nr = -1;	/* /dev/videoN, -1 for autodetect */
+
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level(0-100) default 0");
+module_param(vid_limit, int, 0);
+MODULE_PARM_DESC(vid_limit, "video memory limit(Mb)");
+module_param(video_nr, int, 0);
+MODULE_PARM_DESC(video_nr, "start video minor(-1 default autodetect)");
+
+/* USB device table */
+static struct usb_device_id s2255_table[] = {
+	{USB_DEVICE(USB_S2255_VENDOR_ID, USB_S2255_PRODUCT_ID)},
+	{ }			/* Terminating entry */
+};
+MODULE_DEVICE_TABLE(usb, s2255_table);
+
+
+#define BUFFER_TIMEOUT msecs_to_jiffies(400)
+
+/* supported controls */
+static struct v4l2_queryctrl s2255_qctrl[] = {
+	{
+	.id = V4L2_CID_BRIGHTNESS,
+	.type = V4L2_CTRL_TYPE_INTEGER,
+	.name = "Brightness",
+	.minimum = -127,
+	.maximum = 128,
+	.step = 1,
+	.default_value = 0,
+	.flags = 0,
+	},
+	{
+	.id = V4L2_CID_CONTRAST,
+	.type = V4L2_CTRL_TYPE_INTEGER,
+	.name = "Contrast",
+	.minimum = 0,
+	.maximum = 255,
+	.step = 0x1,
+	.default_value = DEF_CONTRAST,
+	.flags = 0,
+	},
+	{
+	.id = V4L2_CID_SATURATION,
+	.type = V4L2_CTRL_TYPE_INTEGER,
+	.name = "Saturation",
+	.minimum = 0,
+	.maximum = 255,
+	.step = 0x1,
+	.default_value = DEF_SATURATION,
+	.flags = 0,
+	},
+	{
+	.id = V4L2_CID_HUE,
+	.type = V4L2_CTRL_TYPE_INTEGER,
+	.name = "Hue",
+	.minimum = 0,
+	.maximum = 255,
+	.step = 0x1,
+	.default_value = DEF_HUE,
+	.flags = 0,
+	}
+};
+
+static int qctl_regs[ARRAY_SIZE(s2255_qctrl)];
+
+/* image formats.  Note RGB formats are software converted.
+ *  because the 2255 transfers in YUV for maximum USB efficiency
+ * in order to allow 2 full size color channels at full frame rate
+ */
+static const struct s2255_fmt formats[] = {
+	{
+	.name = "4:2:2, planar, YUV422P",
+	.fourcc = V4L2_PIX_FMT_YUV422P,
+	.depth = 16
+	},
+	{
+	.name = "4:2:2, packed, YUYV",
+	.fourcc = V4L2_PIX_FMT_YUYV,
+	.depth = 16
+	},
+	{
+	.name = "BGR24",
+	.fourcc = V4L2_PIX_FMT_BGR24,
+	.depth = 24
+	},
+	{
+	.name = "RGB24",
+	.fourcc = V4L2_PIX_FMT_RGB24,
+	.depth = 24
+	},
+	{
+	.name = "BGR32",
+	.fourcc = V4L2_PIX_FMT_BGR32,
+	.depth = 32
+	},
+	{
+	.name = "RGB32",
+	.fourcc = V4L2_PIX_FMT_RGB32,
+	.depth = 32
+	},
+	{
+	.name = "RGB565",
+	.fourcc = V4L2_PIX_FMT_RGB565,
+	.depth = 16
+	},
+	{
+	.name = "RGB565 big endian",
+	.fourcc = V4L2_PIX_FMT_RGB565X,
+	.depth = 16
+	},
+	{
+	.name = "8bpp GREY",
+	.fourcc = V4L2_PIX_FMT_GREY,
+	.depth = 8
+	},
+};
+
+static int norm_maxw(struct video_device *vdev)
+{
+	return (vdev->current_norm != V4L2_STD_PAL_B) ?
+	    LINE_SZ_4CIFS_NTSC : LINE_SZ_4CIFS_PAL;
+}
+
+static int norm_maxh(struct video_device *vdev)
+{
+	return (vdev->current_norm != V4L2_STD_PAL_B) ?
+	    (NUM_LINES_1CIFS_NTSC * 2) : (NUM_LINES_1CIFS_PAL * 2);
+}
+
+static int norm_minw(struct video_device *vdev)
+{
+	return (vdev->current_norm != V4L2_STD_PAL_B) ?
+	    LINE_SZ_1CIFS_NTSC : LINE_SZ_1CIFS_PAL;
+}
+
+static int norm_minh(struct video_device *vdev)
+{
+	return (vdev->current_norm != V4L2_STD_PAL_B) ?
+	    (NUM_LINES_1CIFS_NTSC) : (NUM_LINES_1CIFS_PAL);
+}
+
+/*
+ * convert from YUV(YCrCb) to RGB
+ * 65536 R = 76533(Y-16) + 104936 * (Cr-128)
+ * 65536 G = 76533(Y-16) - 53451(Cr-128) - 25703(Cb -128)
+ * 65536 B = 76533(Y-16) + 132677(Cb-128)
+ */
+static void YCrCb2RGB(int Y, int Cr, int Cb, unsigned char *pR,
+		      unsigned char *pG, unsigned char *pB)
+{
+	int R, G, B;
+
+	Y = Y - 16;
+	Cr = Cr - 128;
+	Cb = Cb - 128;
+
+	R = (76533 * Y + 104936 * Cr) >> 16;
+	G = ((76533 * Y) - (53451 * Cr) - (25703 * Cb)) >> 16;
+	B = ((76533 * Y) + (132677 * Cb)) >> 16;
+	/* even with proper conversion, some values still need clipping. */
+	if (R > 255)
+		R = 255;
+	if (G > 255)
+		G = 255;
+	if (B > 255)
+		B = 255;
+	if (R < 0)
+		R = 0;
+	if (G < 0)
+		G = 0;
+	if (B < 0)
+		B = 0;
+	*pR = R;
+	*pG = G;
+	*pB = B;
+	return;
+}
+
+/* converts 2255 planar format to yuyv */
+static void planar422p_to_yuy2(const unsigned char *in, unsigned char *out,
+			       int width, int height)
+{
+	unsigned char *pY;
+	unsigned char *pCb;
+	unsigned char *pCr;
+	unsigned long size = height * width;
+	unsigned int i;
+	pY = (unsigned char *)in;
+	pCr = (unsigned char *)in + height * width;
+	pCb = (unsigned char *)in + height * width + (height * width / 2);
+	for (i = 0; i < size * 2; i += 4) {
+		out[i] = *pY++;
+		out[i + 1] = *pCr++;
+		out[i + 2] = *pY++;
+		out[i + 3] = *pCb++;
+	}
+	return;
+}
+
+/*
+ * basic 422 planar to RGB24 or BGR24 software conversion.
+ * This is best done with MMX. Update to kernel function
+ * when image conversion functions added to kernel.
+ */
+static void planar422p_to_rgb24(const unsigned char *in,
+				unsigned char *out, int width,
+				int height, int rev_order)
+{
+	unsigned char *pY;
+	unsigned char *pYEND;
+	unsigned char *pCb;
+	unsigned char *pCr;
+	unsigned char Cr, Cb, Y, r, g, b;
+	unsigned long k = 0;
+	pY = (unsigned char *)in;
+	pCb = (unsigned char *)in + (height * width);
+	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
+	pYEND = pCb;
+	while (pY < pYEND) {
+		Y = *pY++;
+		Cr = *pCr;
+		Cb = *pCb;
+		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
+		out[k++] = !rev_order ? b : r;
+		out[k++] = g;
+		out[k++] = !rev_order ? r : b;
+		if (pY >= pYEND)
+			break;
+		Y = *pY++;
+		Cr = *pCr++;
+		Cb = *pCb++;
+		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
+		out[k++] = !rev_order ? b : r;
+		out[k++] = g;
+		out[k++] = !rev_order ? r : b;
+	}
+	return;
+}
+
+static void planar422p_to_rgb32(const unsigned char *in, unsigned char *out,
+				int width, int height, int rev_order)
+{
+	unsigned char *pY;
+	unsigned char *pYEND;
+	unsigned char *pCb;
+	unsigned char *pCr;
+	unsigned char Cr, Cb, Y, r, g, b;
+	unsigned long k = 0;
+	pY = (unsigned char *)in;
+	pCb = (unsigned char *)in + (height * width);
+	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
+	pYEND = pCb;
+	while (pY < pYEND) {
+		Y = *pY++;
+		Cr = *pCr;
+		Cb = *pCb;
+		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
+		out[k++] = rev_order ? b : r;
+		out[k++] = g;
+		out[k++] = rev_order ? r : b;
+		out[k++] = 0;
+		if (pY >= pYEND)
+			break;
+		Y = *pY++;
+		Cr = *pCr++;
+		Cb = *pCb++;
+		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
+		out[k++] = rev_order ? b : r;
+		out[k++] = g;
+		out[k++] = rev_order ? r : b;
+		out[k++] = 0;
+	}
+
+	return;
+}
+
+static void planar422p_to_rgb565(unsigned char const *in, unsigned char *out,
+				 int width, int height, int rev_order)
+{
+	unsigned char *pY;
+	unsigned char *pYEND;
+	unsigned char *pCb;
+	unsigned char *pCr;
+	unsigned char Cr, Cb, Y, r, g, b;
+	unsigned long k = 0;
+	unsigned short rgbbytes;
+	pY = (unsigned char *)in;
+	pCb = (unsigned char *)in + (height * width);
+	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
+	pYEND = pCb;
+	while (pY < pYEND) {
+		Y = *pY++;
+		Cr = *pCr;
+		Cb = *pCb;
+		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
+		r = r >> 3;
+		g = g >> 2;
+		b = b >> 3;
+		if (rev_order)
+			rgbbytes = b + (g << 5) + (r << (5 + 6));
+		else
+			rgbbytes = r + (g << 5) + (b << (5 + 6));
+		out[k++] = rgbbytes & 0xff;
+		out[k++] = (rgbbytes >> 8) & 0xff;
+		Y = *pY++;
+		Cr = *pCr++;
+		Cb = *pCb++;
+		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
+		r = r >> 3;
+		g = g >> 2;
+		b = b >> 3;
+		if (rev_order)
+			rgbbytes = b + (g << 5) + (r << (5 + 6));
+		else
+			rgbbytes = r + (g << 5) + (b << (5 + 6));
+		out[k++] = rgbbytes & 0xff;
+		out[k++] = (rgbbytes >> 8) & 0xff;
+	}
+	return;
+}
+
+/* kickstarts the firmware loading. from probe
+ */
+static void s2255_timer(unsigned long user_data)
+{
+	struct complete_data *data = (struct complete_data *)user_data;
+	dprintk(100, "s2255 timer\n");
+	if (usb_submit_urb(data->fw_urb, GFP_ATOMIC) < 0) {
+		printk(KERN_ERR "s2255: can't submit urb\n");
+		if (data->fw) {
+			release_firmware(data->fw);
+			data->fw = NULL;
+		}
+		return;
+	}
+}
+
+/* this loads the firmware asynchronously.
+   Originally this was done synchroously in probe.
+   But it is better to load it asynchronously here than block
+   inside the probe function. Blocking inside probe affects boot time.
+   FW loading is triggered by the timer in the probe function
+*/
+static void s2255_fwchunk_complete(struct urb *urb)
+{
+	struct complete_data *data = urb->context;
+	struct usb_device *udev = urb->dev;
+	int len;
+	dprintk(100, "udev %p urb %p", udev, urb);
+
+	if (urb->status) {
+		dev_err(&udev->dev, "URB failed with status %d", urb->status);
+		return;
+	}
+	if (data->fw_urb == NULL) {
+		dev_err(&udev->dev, "early disconncect\n");
+		return;
+	}
+#define CHUNK_SIZE 512
+	/* all USB transfers must be done with continuous kernel memory.
+	   can't allocate more than 128k in current linux kernel, so
+	   upload the firmware in chunks
+	 */
+	if (data->fw_loaded < data->fw_size) {
+		len = (data->fw_loaded + CHUNK_SIZE) > data->fw_size ?
+		    data->fw_size % CHUNK_SIZE : CHUNK_SIZE;
+
+		if (len < CHUNK_SIZE)
+			memset(data->pfw_data, 0, CHUNK_SIZE);
+
+		dprintk(100, "completed len %d, loaded %d \n", len,
+			data->fw_loaded);
+
+		memcpy(data->pfw_data,
+		       (char *) data->fw->data + data->fw_loaded, len);
+		usb_fill_bulk_urb(data->fw_urb, udev, usb_sndbulkpipe(udev, 2),
+				  data->pfw_data, CHUNK_SIZE,
+				  s2255_fwchunk_complete, data);
+		if (usb_submit_urb(data->fw_urb, GFP_ATOMIC) < 0) {
+			dev_err(&udev->dev, "failed submit URB\n");
+			data->fw_state = FWSTATE_FAILED;
+			return;
+		}
+		data->fw_loaded += len;
+	} else {
+		data->fw_state = FWSTATE_SUCCESS;
+		dev_info(&udev->dev, "firmware loaded successfully\n");
+	}
+
+	dprintk(100, "2255 complete done\n");
+	return;
+
+}
+
+static int s2255_got_frame(struct s2255_dev *dev, int chn)
+{
+	struct s2255_dmaqueue *dma_q = &dev->vidq[chn];
+	struct s2255_buffer *buf;
+
+	dprintk(2, "wakeup: %p channel: %d\n", &dma_q, chn);
+	if (list_empty(&dma_q->active)) {
+		dprintk(1, "No active queue to serve\n");
+		return -1;
+	}
+	buf = list_entry(dma_q->active.next,
+			 struct s2255_buffer, vb.queue);
+
+	if (!waitqueue_active(&buf->vb.done)) {
+		/* no one active,
+		   add timer here as process may be suspended(ctrl-z).
+		   once it becomes active again, we want to recover.
+		   resetting the timer allows this.
+		*/
+		mod_timer(&dma_q->timeout, jiffies + BUFFER_TIMEOUT);
+		return -1;
+	}
+
+	do_gettimeofday(&buf->vb.ts);
+	dprintk(100, "[%p/%d] wakeup\n", buf, buf->vb.i);
+	mod_timer(&dma_q->timeout, jiffies + BUFFER_TIMEOUT);
+	s2255_fillbuff(dev, buf, dma_q->channel);
+	return 0;
+}
+
+
+static const struct s2255_fmt *format_by_fourcc(int fourcc)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(formats); i++) {
+		if (-1 == formats[i].fourcc)
+			continue;
+		if (formats[i].fourcc == fourcc)
+			return formats + i;
+	}
+	return NULL;
+}
+
+
+
+
+/* video buffer thread and vmalloc based partly on VIVI driver which is
+ *          Copyright (c) 2006 by
+ *                  Mauro Carvalho Chehab <mchehab--a.t--infradead.org>
+ *                  Ted Walther <ted--a.t--enumera.com>
+ *                  John Sokol <sokol--a.t--videotechnology.com>
+ *                  http://v4l.videotechnology.com/
+ *
+ */
+static void s2255_fillbuff(struct s2255_dev *dev, struct s2255_buffer *buf,
+			   int chn)
+{
+	int pos = 0;
+	struct timeval ts;
+	const char *tmpbuf;
+	char *vbuf = videobuf_to_vmalloc(&buf->vb);
+	unsigned long last_frame;
+	struct framei *frm;
+	last_frame = dev->last_frame[chn];
+	if ((last_frame != -1) && (vbuf != NULL)) {
+		frm = &dev->buffer[chn].frame[last_frame];
+		tmpbuf =
+		    (const char *)dev->buffer[chn].frame[last_frame].lpvbits;
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUYV) {
+			planar422p_to_yuy2((const unsigned char *)tmpbuf,
+					   vbuf, buf->vb.width, buf->vb.height);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_GREY) {
+			memcpy(vbuf, tmpbuf, buf->vb.width * buf->vb.height);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			memcpy(vbuf, tmpbuf,
+			       buf->vb.width * buf->vb.height * 2);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_RGB24) {
+			planar422p_to_rgb24(tmpbuf, vbuf, buf->vb.width,
+					    buf->vb.height, 1);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_BGR24) {
+			planar422p_to_rgb24(tmpbuf, vbuf, buf->vb.width,
+					    buf->vb.height, 0);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_RGB32) {
+			planar422p_to_rgb32(tmpbuf, vbuf, buf->vb.width,
+					    buf->vb.height, 0);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_BGR32) {
+			planar422p_to_rgb32(tmpbuf, vbuf, buf->vb.width,
+					    buf->vb.height, 1);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_RGB565) {
+			planar422p_to_rgb565(tmpbuf, vbuf, buf->vb.width,
+					     buf->vb.height, 1);
+		} else if (buf->fmt->fourcc == V4L2_PIX_FMT_RGB565X) {
+			planar422p_to_rgb565(tmpbuf, vbuf, buf->vb.width,
+					     buf->vb.height, 0);
+		} else {
+			printk(KERN_DEBUG "s2255: unknown format?\n");
+		}
+		dev->last_frame[chn] = -1;
+		/* done with the frame, free it */
+		frm->ulState = 0;
+		dprintk(4, "freeing buffer\n");
+	} else {
+		printk(KERN_ERR "s2255: =======no frame\n");
+		return;
+
+	}
+	dprintk(2, "s2255fill at : Buffer 0x%08lx size= %d\n",
+		(unsigned long)vbuf, pos);
+	/* tell v4l buffer was filled */
+	buf->vb.state = VIDEOBUF_DONE;
+	buf->vb.field_count++;
+	do_gettimeofday(&ts);
+	buf->vb.ts = ts;
+	list_del(&buf->vb.queue);
+	wake_up(&buf->vb.done);
+}
+
+
+static int restart_video_queue(struct s2255_dmaqueue *dma_q)
+{
+	struct s2255_buffer *buf, *prev;
+	dprintk(1, "%s dma_q=0x%08lx chan %d\n", __func__,
+		(unsigned long)dma_q, dma_q->channel);
+	if (!list_empty(&dma_q->active)) {
+		buf =
+		    list_entry(dma_q->active.next, struct s2255_buffer,
+			       vb.queue);
+		dprintk(2, "restart_queue [%p/%d]: restart dma\n", buf,
+			buf->vb.i);
+		/* cancel all outstanding capture requests */
+		list_for_each_entry_safe(buf, prev, &dma_q->active, vb.queue) {
+			list_del(&buf->vb.queue);
+			buf->vb.state = VIDEOBUF_ERROR;
+			wake_up(&buf->vb.done);
+		}
+		mod_timer(&dma_q->timeout, jiffies + BUFFER_TIMEOUT);
+
+		return 0;
+	}
+
+	prev = NULL;
+	for (;;) {
+		if (list_empty(&dma_q->queued)) {
+			dprintk(1, "exiting nothing queued\n");
+			return 0;
+		}
+		buf =
+		    list_entry(dma_q->queued.next, struct s2255_buffer,
+			       vb.queue);
+		if (NULL == prev) {
+			list_del(&buf->vb.queue);
+			list_add_tail(&buf->vb.queue, &dma_q->active);
+			buf->vb.state = VIDEOBUF_ACTIVE;
+			dprintk(2, "[%p/%d] restart_queue - first active\n",
+				buf, buf->vb.i);
+		} else if (prev->vb.width == buf->vb.width &&
+			   prev->vb.height == buf->vb.height &&
+			   prev->fmt == buf->fmt) {
+			list_del(&buf->vb.queue);
+			list_add_tail(&buf->vb.queue, &dma_q->active);
+			buf->vb.state = VIDEOBUF_ACTIVE;
+			mod_timer(&dma_q->timeout, jiffies + BUFFER_TIMEOUT);
+			dprintk(2, "[%p/%d] restart_queue - move to active\n",
+				buf, buf->vb.i);
+		} else {
+			return 0;
+		}
+		prev = buf;
+	}
+}
+
+static void s2255_vid_timeout(unsigned long data)
+{
+	struct s2255_dmaqueue *vidq = (struct s2255_dmaqueue *)data;
+	struct s2255_buffer *buf;
+	dprintk(1, "[%d]vid timeout %p\n", vidq->channel, vidq);
+	while (!list_empty(&vidq->active)) {
+		buf =
+		    list_entry(vidq->active.next, struct s2255_buffer,
+			       vb.queue);
+		list_del(&buf->vb.queue);
+		buf->vb.state = VIDEOBUF_ERROR;
+		wake_up(&buf->vb.done);
+	}
+	restart_video_queue(vidq);
+}
+
+/* ------------------------------------------------------------------
+   Videobuf operations
+   ------------------------------------------------------------------*/
+static int buffer_setup(struct videobuf_queue *vq, unsigned int *count,
+			unsigned int *size)
+{
+	struct s2255_fh *fh = vq->priv_data;
+
+	*size = fh->width * fh->height * (fh->fmt->depth >> 3);
+
+	if (0 == *count)
+		*count = 8;
+
+	while (*size * *count > vid_limit * 1024 * 1024)
+		(*count)--;
+
+	return 0;
+}
+
+static void free_buffer(struct videobuf_queue *vq, struct s2255_buffer *buf)
+{
+	dprintk(4, "%s\n", __func__);
+
+	videobuf_waiton(&buf->vb, 0, 0);
+	videobuf_vmalloc_free(&buf->vb);
+	buf->vb.state = VIDEOBUF_NEEDS_INIT;
+}
+
+static int buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
+			  enum v4l2_field field)
+{
+	struct s2255_fh *fh = vq->priv_data;
+	struct s2255_buffer *buf = container_of(vb, struct s2255_buffer, vb);
+	int rc, init_buffer = 0;
+	dprintk(4, "%s, field=%d\n", __func__, field);
+	if (fh->fmt == NULL)
+		return -EINVAL;
+	if ((fh->width < norm_minw(fh->dev->vdev[fh->channel])) ||
+	    (fh->width > norm_maxw(fh->dev->vdev[fh->channel])) ||
+	    (fh->height < norm_minh(fh->dev->vdev[fh->channel])) ||
+	    (fh->height > norm_maxh(fh->dev->vdev[fh->channel]))) {
+		dprintk(4, "invalid buffer prepare\n");
+		return -EINVAL;
+	}
+
+	buf->vb.size = fh->width * fh->height * (fh->fmt->depth >> 3);
+
+	if (0 != buf->vb.baddr && buf->vb.bsize < buf->vb.size) {
+		dprintk(4, "invalid buffer prepare\n");
+		return -EINVAL;
+	}
+
+	if (buf->fmt != fh->fmt ||
+	    buf->vb.width != fh->width ||
+	    buf->vb.height != fh->height || buf->vb.field != field) {
+		buf->fmt = fh->fmt;
+		buf->vb.width = fh->width;
+		buf->vb.height = fh->height;
+		buf->vb.field = field;
+		init_buffer = 1;
+	}
+
+	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
+		rc = videobuf_iolock(vq, &buf->vb, NULL);
+		if (rc)
+			goto fail;
+	}
+
+	buf->vb.state = VIDEOBUF_PREPARED;
+	return 0;
+fail:
+	free_buffer(vq, buf);
+	return rc;
+}
+
+static void buffer_queue(struct videobuf_queue *vq, struct videobuf_buffer *vb)
+{
+	struct s2255_buffer *buf = container_of(vb, struct s2255_buffer, vb);
+	struct s2255_fh *fh = vq->priv_data;
+	struct s2255_dev *dev = fh->dev;
+	struct s2255_dmaqueue *vidq = &dev->vidq[fh->channel];
+	struct s2255_buffer *prev;
+
+	if (!list_empty(&vidq->queued)) {
+		dprintk(1, "adding vb queue=0x%08lx\n",
+			(unsigned long)&buf->vb.queue);
+		list_add_tail(&buf->vb.queue, &vidq->queued);
+		buf->vb.state = VIDEOBUF_QUEUED;
+		dprintk(2, "[%p/%d] buffer_queue - append to queued\n",
+			buf, buf->vb.i);
+	} else if (list_empty(&vidq->active)) {
+		list_add_tail(&buf->vb.queue, &vidq->active);
+		buf->vb.state = VIDEOBUF_ACTIVE;
+		dprintk(2, "[%p/%d] buffer_queue - first active\n",
+			buf, buf->vb.i);
+	} else {
+		prev =
+		    list_entry(vidq->active.prev, struct s2255_buffer,
+			       vb.queue);
+		if (prev->vb.width == buf->vb.width
+		    && prev->vb.height == buf->vb.height
+		    && prev->fmt == buf->fmt) {
+			list_add_tail(&buf->vb.queue, &vidq->active);
+			buf->vb.state = VIDEOBUF_ACTIVE;
+			dprintk(2, "[%p/%d] buffer_queue - append to active\n",
+				buf, buf->vb.i);
+
+		} else {
+			list_add_tail(&buf->vb.queue, &vidq->queued);
+			buf->vb.state = VIDEOBUF_QUEUED;
+			dprintk(2, "[%p/%d] buffer_queue - first queued\n",
+				buf, buf->vb.i);
+		}
+	}
+}
+
+static void buffer_release(struct videobuf_queue *vq,
+			   struct videobuf_buffer *vb)
+{
+	struct s2255_buffer *buf = container_of(vb, struct s2255_buffer, vb);
+	struct s2255_fh *fh = vq->priv_data;
+	dprintk(4, "%s %d\n", __func__, fh->channel);
+	free_buffer(vq, buf);
+}
+
+static struct videobuf_queue_ops s2255_video_qops = {
+	.buf_setup = buffer_setup,
+	.buf_prepare = buffer_prepare,
+	.buf_queue = buffer_queue,
+	.buf_release = buffer_release,
+};
+
+/* ------------------------------------------------------------------
+   IOCTL handling
+   ------------------------------------------------------------------*/
+
+static int res_get(struct s2255_dev *dev, struct s2255_fh *fh)
+{
+	/* is it free? */
+	mutex_lock(&dev->lock);
+	if (dev->resources[fh->channel]) {
+		/* no, someone else uses it */
+		mutex_unlock(&dev->lock);
+		return 0;
+	}
+	/* it's free, grab it */
+	dev->resources[fh->channel] = 1;
+	dprintk(1, "res: get\n");
+	mutex_unlock(&dev->lock);
+	return 1;
+}
+
+static int res_locked(struct s2255_dev *dev, struct s2255_fh *fh)
+{
+	return (dev->resources[fh->channel]);
+}
+
+static void res_free(struct s2255_dev *dev, struct s2255_fh *fh)
+{
+	dev->resources[fh->channel] = 0;
+	dprintk(1, "res: put\n");
+}
+
+/* ------------------------------------------------------------------
+   IOCTL vidioc handling
+   ------------------------------------------------------------------*/
+static int vidioc_querycap(struct file *file, void *priv,
+			   struct v4l2_capability *cap)
+{
+	strcpy(cap->driver, "s2255");
+	strcpy(cap->card, "s2255");
+	cap->version = S2255_VERSION;
+	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+	return 0;
+}
+
+static int vidioc_enum_fmt_cap(struct file *file, void *priv,
+			       struct v4l2_fmtdesc *f)
+{
+	int index = 0;
+	if (f)
+		index = f->index;
+
+	if (index >= ARRAY_SIZE(formats))
+		return -EINVAL;
+
+	dprintk(4, "name %s\n", formats[index].name);
+	strlcpy(f->description, formats[index].name, sizeof(f->description));
+	f->pixelformat = formats[index].fourcc;
+	return 0;
+}
+
+static int vidioc_g_fmt_cap(struct file *file, void *priv,
+			    struct v4l2_format *f)
+{
+	struct s2255_fh *fh = priv;
+
+	f->fmt.pix.width = fh->width;
+	f->fmt.pix.height = fh->height;
+	f->fmt.pix.field = fh->vb_vidq.field;
+	f->fmt.pix.pixelformat = fh->fmt->fourcc;
+	f->fmt.pix.bytesperline = f->fmt.pix.width * (fh->fmt->depth >> 3);
+	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
+
+	return (0);
+}
+
+static int vidioc_try_fmt_cap(struct file *file, void *priv,
+			      struct v4l2_format *f)
+{
+	const struct s2255_fmt *fmt;
+	enum v4l2_field field;
+	int  b_any_field = 0;
+	struct s2255_fh *fh = priv;
+	struct s2255_dev *dev = fh->dev;
+	int is_ntsc;
+
+	is_ntsc =
+	    (dev->vdev[fh->channel]->current_norm != V4L2_STD_PAL_B) ? 1 : 0;
+
+	fmt = format_by_fourcc(f->fmt.pix.pixelformat);
+
+	if (fmt == NULL)
+		return -EINVAL;
+
+	field = f->fmt.pix.field;
+	if (field == V4L2_FIELD_ANY)
+		b_any_field = 1;
+
+	dprintk(4, "try format %d \n", is_ntsc);
+	/* supports 3 sizes. see s2255drv.h */
+	dprintk(50, "width test %d, height %d\n",
+		f->fmt.pix.width, f->fmt.pix.height);
+	if (is_ntsc) {
+		/* NTSC */
+		if (f->fmt.pix.height >= NUM_LINES_1CIFS_NTSC * 2) {
+			f->fmt.pix.height = NUM_LINES_1CIFS_NTSC * 2;
+			if (b_any_field) {
+				field = V4L2_FIELD_SEQ_TB;
+			} else if (!((field == V4L2_FIELD_INTERLACED) ||
+				      (field == V4L2_FIELD_SEQ_TB) ||
+				      (field == V4L2_FIELD_INTERLACED_TB))) {
+				dprintk(1, "unsupported field setting\n");
+				return -EINVAL;
+			}
+		} else {
+			f->fmt.pix.height = NUM_LINES_1CIFS_NTSC;
+			if (b_any_field) {
+				field = V4L2_FIELD_TOP;
+			} else if (!((field == V4L2_FIELD_TOP) ||
+				      (field == V4L2_FIELD_BOTTOM))) {
+				dprintk(1, "unsupported field setting\n");
+				return -EINVAL;
+			}
+
+		}
+		if (f->fmt.pix.width >= LINE_SZ_4CIFS_NTSC)
+			f->fmt.pix.width = LINE_SZ_4CIFS_NTSC;
+		else if (f->fmt.pix.width >= LINE_SZ_2CIFS_NTSC)
+			f->fmt.pix.width = LINE_SZ_2CIFS_NTSC;
+		else if (f->fmt.pix.width >= LINE_SZ_1CIFS_NTSC)
+			f->fmt.pix.width = LINE_SZ_1CIFS_NTSC;
+		else
+			f->fmt.pix.width = LINE_SZ_1CIFS_NTSC;
+	} else {
+		/* PAL */
+		if (f->fmt.pix.height >= NUM_LINES_1CIFS_PAL * 2) {
+			f->fmt.pix.height = NUM_LINES_1CIFS_PAL * 2;
+			if (b_any_field) {
+				field = V4L2_FIELD_SEQ_TB;
+			} else if (!((field == V4L2_FIELD_INTERLACED) ||
+				      (field == V4L2_FIELD_SEQ_TB) ||
+				      (field == V4L2_FIELD_INTERLACED_TB))) {
+				dprintk(1, "unsupported field setting\n");
+				return -EINVAL;
+			}
+		} else {
+			f->fmt.pix.height = NUM_LINES_1CIFS_PAL;
+			if (b_any_field) {
+				field = V4L2_FIELD_TOP;
+			} else if (!((field == V4L2_FIELD_TOP) ||
+				     (field == V4L2_FIELD_BOTTOM))) {
+				dprintk(1, "unsupported field setting\n");
+				return -EINVAL;
+			}
+		}
+		if (f->fmt.pix.width >= LINE_SZ_4CIFS_PAL) {
+			dprintk(50, "pal 704\n");
+			f->fmt.pix.width = LINE_SZ_4CIFS_PAL;
+			field = V4L2_FIELD_SEQ_TB;
+		} else if (f->fmt.pix.width >= LINE_SZ_2CIFS_PAL) {
+			dprintk(50, "pal 352A\n");
+			f->fmt.pix.width = LINE_SZ_2CIFS_PAL;
+			field = V4L2_FIELD_TOP;
+		} else if (f->fmt.pix.width >= LINE_SZ_1CIFS_PAL) {
+			dprintk(50, "pal 352B\n");
+			f->fmt.pix.width = LINE_SZ_1CIFS_PAL;
+			field = V4L2_FIELD_TOP;
+		} else {
+			dprintk(50, "pal 352C\n");
+			f->fmt.pix.width = LINE_SZ_1CIFS_PAL;
+			field = V4L2_FIELD_TOP;
+		}
+	}
+
+	dprintk(50, "width %d height %d field %d \n", f->fmt.pix.width,
+		f->fmt.pix.height, f->fmt.pix.field);
+	f->fmt.pix.field = field;
+	f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3;
+	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
+	return 0;
+}
+
+/* FIXME: This seems to be generic enough to be at videodev2 */
+static int vidioc_s_fmt_cap(struct file *file, void *priv,
+			    struct v4l2_format *f)
+{
+	struct s2255_fh *fh = priv;
+	const struct s2255_fmt *fmt;
+	int ret;
+	int norm;
+
+	if (res_locked(fh->dev, fh)) {
+		dprintk(1, "can't change format after started\n");
+		return -EBUSY;
+	}
+
+	ret = vidioc_try_fmt_cap(file, fh, f);
+
+	if (ret < 0)
+		return (ret);
+
+	fmt = format_by_fourcc(f->fmt.pix.pixelformat);
+	if (fmt == NULL)
+		return -EINVAL;
+
+	fh->fmt = fmt;
+	fh->width = f->fmt.pix.width;
+	fh->height = f->fmt.pix.height;
+	fh->vb_vidq.field = f->fmt.pix.field;
+	fh->type = f->type;
+	norm = norm_minw(fh->dev->vdev[fh->channel]);
+	if (fh->width > norm_minw(fh->dev->vdev[fh->channel])) {
+		if (fh->height > norm_minh(fh->dev->vdev[fh->channel]))
+			fh->mode.scale = SCALE_4CIFS;
+		else
+			fh->mode.scale = SCALE_2CIFS;
+
+	} else {
+		fh->mode.scale = SCALE_1CIFS;
+	}
+
+	/* color mode */
+	if (fh->fmt->fourcc == V4L2_PIX_FMT_GREY) {
+		fh->mode.color = COLOR_Y8;
+	} else if (fh->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+		fh->mode.color = COLOR_YUVPL;
+	} else if (fh->fmt->fourcc == V4L2_PIX_FMT_YUYV) {
+		/* Note: software conversion from YUV422P to YUYV */
+		fh->mode.color = COLOR_YUVPK;
+	} else if ((fh->fmt->fourcc == V4L2_PIX_FMT_RGB24) ||
+		   (fh->fmt->fourcc == V4L2_PIX_FMT_BGR24) ||
+		   (fh->fmt->fourcc == V4L2_PIX_FMT_RGB32) ||
+		   (fh->fmt->fourcc == V4L2_PIX_FMT_RGB565) ||
+		   (fh->fmt->fourcc == V4L2_PIX_FMT_RGB565X) ||
+		   (fh->fmt->fourcc == V4L2_PIX_FMT_BGR32)) {
+		/* Note:software conversion from YUV422P to RGB(s) */
+		dprintk(2, "mode supported with software conversion.\n");
+		dprintk(2, "for lower CPU usage, use V4L2_PIX_FMT_YUV422P"
+			"V4L2_PIX_FMT_YUVV(minimal software reordering) or"
+			" V4L2_PIX_FMT_GREY\n");
+		fh->mode.color = COLOR_YUVPL;
+	}
+	return (0);
+}
+
+static int vidioc_reqbufs(struct file *file, void *priv,
+			  struct v4l2_requestbuffers *p)
+{
+	int rc;
+	struct s2255_fh *fh = priv;
+	rc = videobuf_reqbufs(&fh->vb_vidq, p);
+	return rc;
+}
+
+static int vidioc_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
+{
+	int rc;
+	struct s2255_fh *fh = priv;
+	rc = videobuf_querybuf(&fh->vb_vidq, p);
+	return rc;
+}
+
+static int vidioc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
+{
+	int rc;
+	struct s2255_fh *fh = priv;
+	rc = videobuf_qbuf(&fh->vb_vidq, p);
+	return rc;
+}
+
+static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
+{
+	int rc;
+	struct s2255_fh *fh = priv;
+	rc = videobuf_dqbuf(&fh->vb_vidq, p, file->f_flags & O_NONBLOCK);
+	return rc;
+}
+
+#ifdef CONFIG_VIDEO_V4L1_COMPAT
+static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)
+{
+	struct s2255_fh *fh = priv;
+	struct videobuf_queue *q = &fh->vb_vidq;
+	struct v4l2_requestbuffers req;
+	unsigned int i;
+	int ret;
+
+	req.type = q->type;
+	req.count = 8;
+	req.memory = V4L2_MEMORY_MMAP;
+	ret = videobuf_reqbufs(q, &req);
+	if (ret < 0)
+		return (ret);
+
+	mbuf->frames = req.count;
+	mbuf->size = 0;
+	for (i = 0; i < mbuf->frames; i++) {
+		mbuf->offsets[i] = q->bufs[i]->boff;
+		mbuf->size += q->bufs[i]->bsize;
+	}
+	return (0);
+}
+#endif
+
+
+#define EP_NUM_CONFIG 2
+/* write to the configuration pipe, synchronously */
+static int s2255_write_config(struct usb_device *udev, unsigned char *pbuf,
+			      int size)
+{
+	int pipe;
+	int done;
+	long retval = -1;
+	if (udev) {
+		pipe = usb_sndbulkpipe(udev, EP_NUM_CONFIG);
+		retval = usb_bulk_msg(udev, pipe, pbuf, size, &done, 500);
+	}
+	return retval;
+}
+
+static u32 get_transfer_size(struct mode2255i *mode)
+{
+	int linesPerFrame = LINE_SZ_DEF;
+	int pixelsPerLine = NUM_LINES_DEF;
+	u32 outImageSize;
+	u32 usbInSize;
+	unsigned int mask_mult;
+
+	if (mode == NULL)
+		return 0;
+
+	if (mode->format == FORMAT_NTSC) {
+		switch (mode->scale) {
+		case SCALE_4CIFS:
+			linesPerFrame = NUM_LINES_4CIFS_NTSC * 2;
+			pixelsPerLine = LINE_SZ_4CIFS_NTSC;
+			break;
+		case SCALE_2CIFS:
+			linesPerFrame = NUM_LINES_2CIFS_NTSC;
+			pixelsPerLine = LINE_SZ_2CIFS_NTSC;
+			break;
+		case SCALE_1CIFS:
+			linesPerFrame = NUM_LINES_1CIFS_NTSC;
+			pixelsPerLine = LINE_SZ_1CIFS_NTSC;
+			break;
+		default:
+			break;
+		}
+	} else if (mode->format == FORMAT_PAL) {
+		switch (mode->scale) {
+		case SCALE_4CIFS:
+			linesPerFrame = NUM_LINES_4CIFS_PAL * 2;
+			pixelsPerLine = LINE_SZ_4CIFS_PAL;
+			break;
+		case SCALE_2CIFS:
+			linesPerFrame = NUM_LINES_2CIFS_PAL;
+			pixelsPerLine = LINE_SZ_2CIFS_PAL;
+			break;
+		case SCALE_1CIFS:
+			linesPerFrame = NUM_LINES_1CIFS_PAL;
+			pixelsPerLine = LINE_SZ_1CIFS_PAL;
+			break;
+		default:
+			break;
+		}
+	}
+	outImageSize = linesPerFrame * pixelsPerLine;
+	if (mode->color != COLOR_Y8) {
+		/* 2 bytes/pixel if not monochrome */
+		outImageSize *= 2;
+	}
+
+	/* total bytes to send including prefix and 4K padding;
+	   must be a multiple of USB_READ_SIZE */
+	usbInSize = outImageSize + PREFIX_SIZE;	/* always send prefix */
+	mask_mult = 0xffffffffUL - DEF_USB_BLOCK + 1;
+	/* if size not a multiple of USB_READ_SIZE */
+	if (usbInSize & ~mask_mult)
+		usbInSize = (usbInSize & mask_mult) + (DEF_USB_BLOCK);
+	return usbInSize;
+}
+
+static void dump_verify_mode(struct s2255_dev *sdev, struct mode2255i *mode)
+{
+	struct device *dev = &sdev->udev->dev;
+	dev_info(dev, "------------------------------------------------\n");
+	dev_info(dev, "verify mode\n");
+	dev_info(dev, "format: %d\n", mode->format);
+	dev_info(dev, "scale: %d\n", mode->scale);
+	dev_info(dev, "fdec: %d\n", mode->fdec);
+	dev_info(dev, "color: %d\n", mode->color);
+	dev_info(dev, "bright: 0x%x\n", mode->bright);
+	dev_info(dev, "restart: 0x%x\n", mode->restart);
+	dev_info(dev, "usb_block: 0x%x\n", mode->usb_block);
+	dev_info(dev, "single: 0x%x\n", mode->single);
+	dev_info(dev, "------------------------------------------------\n");
+}
+
+/*
+ * set mode is the function which controls the DSP.
+ * the restart parameter in struct mode2255i should be set whenever
+ * the image size could change via color format, video system or image
+ * size.
+ * When the restart parameter is set, we sleep for ONE frame to allow the
+ * DSP time to get the new frame
+ */
+static int s2255_set_mode(struct s2255_dev *dev, unsigned long chn,
+			  struct mode2255i *mode)
+{
+	int res;
+	u32 *buffer;
+	unsigned long chn_rev;
+
+	chn_rev = G_chnmap[chn];
+	dprintk(3, "mode scale [%ld] %p %d\n", chn, mode, mode->scale);
+	dprintk(3, "mode scale [%ld] %p %d\n", chn, &dev->mode[chn],
+		dev->mode[chn].scale);
+	dprintk(2, "mode contrast %x\n", mode->contrast);
+
+	/* save the mode */
+	dev->mode[chn] = *mode;
+	dev->req_image_size[chn] = get_transfer_size(mode);
+	dprintk(1, "transfer size %ld\n", dev->req_image_size[chn]);
+
+	buffer = kzalloc(512, GFP_KERNEL);
+	if (buffer == NULL) {
+		dev_err(&dev->udev->dev, "out of mem\n");
+		return -ENOMEM;
+	}
+
+	/* set the mode */
+	buffer[0] = IN_DATA_TOKEN;
+	buffer[1] = (u32) chn_rev;
+	buffer[2] = CMD_SET_MODE;
+	memcpy(&buffer[3], &dev->mode[chn], sizeof(struct mode2255i));
+	res = s2255_write_config(dev->udev, (unsigned char *)buffer, 512);
+	if (debug)
+		dump_verify_mode(dev, mode);
+	kfree(buffer);
+	dprintk(1, "set mode done chn %lu, %d\n", chn, res);
+
+	/* wait at least 3 frames before continuing */
+	if (mode->restart)
+		msleep(125);
+
+	/* clear the restart flag */
+	dev->mode[chn].restart = 0;
+
+	return res;
+}
+
+static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
+{
+	int res;
+	struct s2255_fh *fh = priv;
+	struct s2255_dev *dev = fh->dev;
+	struct s2255_dmaqueue *dmaq = &dev->vidq[fh->channel];
+	struct mode2255i *new_mode;
+	struct mode2255i *old_mode;
+	int chn;
+	int j;
+	dprintk(4, "%s\n", __func__);
+	if (fh->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+		dev_err(&dev->udev->dev, "invalid fh type0\n");
+		return -EINVAL;
+	}
+	if (i != fh->type) {
+		dev_err(&dev->udev->dev, "invalid fh type1\n");
+		return -EINVAL;
+	}
+
+	if (!res_get(dev, fh)) {
+		dev_err(&dev->udev->dev, "res get busy\n");
+		return -EBUSY;
+	}
+	/* send a set mode command everytime with restart.
+	   in case we switch resolutions or other parameters */
+	chn = fh->channel;
+	new_mode = &fh->mode;
+	old_mode = &fh->dev->mode[chn];
+
+	if (new_mode->color != old_mode->color)
+		new_mode->restart = 1;
+	else if (new_mode->scale != old_mode->scale)
+		new_mode->restart = 1;
+	else if (new_mode->format != old_mode->format)
+		new_mode->restart = 1;
+
+	s2255_set_mode(dev, chn, new_mode);
+	new_mode->restart = 0;
+	*old_mode = *new_mode;
+	dev->cur_fmt[chn] = fh->fmt;
+	dprintk(1, "%s[%d]\n", __func__, chn);
+	dev->last_frame[chn] = -1;
+	dev->bad_payload[chn] = 0;
+	dev->cur_frame[chn] = 0;
+	for (j = 0; j < SYS_FRAMES; j++) {
+		dev->buffer[chn].frame[j].ulState = 0;
+		dev->buffer[chn].frame[j].cur_size = 0;
+	}
+	mod_timer(&dmaq->timeout, jiffies + BUFFER_TIMEOUT);
+	res = videobuf_streamon(&fh->vb_vidq);
+	if (res == 0) {
+		s2255_start_acquire(dev, chn);
+		dev->b_acquire[chn] = 1;
+	} else {
+		res_free(dev, fh);
+	}
+	return res;
+}
+
+static int vidioc_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
+{
+	int res;
+	struct s2255_fh *fh = priv;
+	struct s2255_dev *dev = fh->dev;
+	struct s2255_dmaqueue *dmaq = &dev->vidq[fh->channel];
+	dprintk(4, "%s\n, channel: %d", __func__, fh->channel);
+	if (fh->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+		printk(KERN_ERR "invalid fh type0\n");
+		return -EINVAL;
+	}
+	if (i != fh->type) {
+		printk(KERN_ERR "invalid type i\n");
+		return -EINVAL;
+	}
+	s2255_stop_acquire(dev, fh->channel);
+	/* give time for last frame to be received/flush in the usb
+	   receive pipe*/
+	msleep(50);
+	res = videobuf_streamoff(&fh->vb_vidq);
+	res_free(dev, fh);
+	/* expire the queue timer now in case still active */
+	mod_timer(&dmaq->timeout, jiffies + HZ);
+	return res;
+}
+
+static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id *i)
+{
+	struct s2255_fh *fh = priv;
+	struct mode2255i *mode;
+
+	if (res_locked(fh->dev, fh)) {
+		dprintk(1, "can't change standard after started\n");
+		return -EBUSY;
+	}
+	mode = &fh->mode;
+
+	if (*i == V4L2_STD_NTSC_M) {
+		dprintk(4, "vidioc_s_std NTSC\n");
+		mode->format = FORMAT_NTSC;
+	} else if (*i == V4L2_STD_PAL_B) {
+		dprintk(4, "vidioc_s_std PAL\n");
+		mode->format = FORMAT_PAL;
+	} else {
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/* Sensoray 2255 is a multiple channel capture device.
+   It does not have a "crossbar" of inputs.
+   We use one V4L device per channel. The user must
+   be aware that certain combinations are not allowed.
+   For instance, you cannot do full FPS on more than 2 channels(2 videodevs)
+   at once in color(you can do full fps on 4 channels with greyscale.
+*/
+static int vidioc_enum_input(struct file *file, void *priv,
+			     struct v4l2_input *inp)
+{
+	if (inp->index != 0)
+		return -EINVAL;
+
+	inp->type = V4L2_INPUT_TYPE_CAMERA;
+	inp->std = S2255_NORMS;
+	strcpy(inp->name, "Camera");
+	return (0);
+}
+
+static int vidioc_g_input(struct file *file, void *priv, unsigned int *i)
+{
+	*i = 0;
+	return 0;
+}
+static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
+{
+	if (i > 0)
+		return -EINVAL;
+	return 0;
+}
+
+/* --- controls ---------------------------------------------- */
+static int vidioc_queryctrl(struct file *file, void *priv,
+			    struct v4l2_queryctrl *qc)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s2255_qctrl); i++)
+		if (qc->id && qc->id == s2255_qctrl[i].id) {
+			memcpy(qc, &(s2255_qctrl[i]), sizeof(*qc));
+			return (0);
+		}
+
+	dprintk(4, "query_ctrl -EINVAL %d\n", qc->id);
+	return -EINVAL;
+}
+
+static int vidioc_g_ctrl(struct file *file, void *priv,
+			 struct v4l2_control *ctrl)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s2255_qctrl); i++)
+		if (ctrl->id == s2255_qctrl[i].id) {
+			ctrl->value = qctl_regs[i];
+			return (0);
+		}
+	dprintk(4, "g_ctrl -EINVAL\n");
+
+	return -EINVAL;
+}
+
+static int vidioc_s_ctrl(struct file *file, void *priv,
+			 struct v4l2_control *ctrl)
+{
+	int i;
+	struct s2255_fh *fh = priv;
+	struct s2255_dev *dev = fh->dev;
+	struct mode2255i *mode;
+	mode = &fh->mode;
+	dprintk(4, "vidioc_s_ctrl\n");
+	for (i = 0; i < ARRAY_SIZE(s2255_qctrl); i++) {
+		if (ctrl->id == s2255_qctrl[i].id) {
+			if (ctrl->value < s2255_qctrl[i].minimum ||
+			    ctrl->value > s2255_qctrl[i].maximum)
+				return (-ERANGE);
+
+			qctl_regs[i] = ctrl->value;
+			/* update the mode to the corresponding value */
+			if (ctrl->id == V4L2_CID_BRIGHTNESS)
+				mode->bright = ctrl->value;
+			else if (ctrl->id == V4L2_CID_CONTRAST)
+				mode->contrast = ctrl->value;
+			else if (ctrl->id == V4L2_CID_HUE)
+				mode->hue = ctrl->value;
+			else if (ctrl->id == V4L2_CID_SATURATION)
+				mode->saturation = ctrl->value;
+#if 1
+			mode->restart = 0;
+			/* set mode here.  Note: stream does not need restarted.
+			   some V4L programs restart stream unnecessarily
+			   after a s_crtl.
+			 */
+			s2255_set_mode(dev, fh->channel, mode);
+#endif
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+static int s2255_open_v4l(struct inode *inode, struct file *file)
+{
+	int minor = iminor(inode);
+	struct s2255_dev *h, *dev = NULL;
+	struct s2255_fh *fh;
+	struct list_head *list;
+	enum v4l2_buf_type type = 0;
+	int i = 0;
+	int cur_channel = -1;
+	dprintk(1, "s2255: open called (minor=%d)\n", minor);
+	list_for_each(list, &s2255_devlist) {
+		h = list_entry(list, struct s2255_dev, s2255_devlist);
+		for (i = 0; i < MAX_CHANNELS; i++) {
+			if (h->vdev[i]->minor == minor) {
+				cur_channel = i;
+				dev = h;
+				type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+			}
+		}
+	}
+
+	if ((NULL == dev) || (cur_channel == -1)) {
+		dprintk(1, "s2255: openv4l no dev\n");
+		return -ENODEV;
+	}
+
+	mutex_lock(&usb_s2255_open_mutex);
+
+
+
+	if (dev->fw_data->fw_state == FWSTATE_FAILED) {
+		err("2255 firmware wasn't loaded\n");
+		mutex_unlock(&usb_s2255_open_mutex);
+		return -ENODEV;
+	}
+
+	if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
+		/* give 1 second for firmware to load in case
+		   driver loaded and then device immediately opened */
+		msleep(1000);
+		if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
+			err("2255 firmware loading stalled\n");
+			mutex_unlock(&usb_s2255_open_mutex);
+			return -EAGAIN;
+		}
+	}
+	/* if first open after firmware loaded, release the firmware */
+	if (dev->fw_data->fw) {
+		release_firmware(dev->fw_data->fw);
+		dev->fw_data->fw = NULL;
+	}
+
+	dev->users[cur_channel]++;
+
+	if (dev->users[cur_channel] > 1) {
+		dev->users[cur_channel]--;
+		dev_err(&dev->udev->dev, "one user at a time\n");
+		mutex_unlock(&usb_s2255_open_mutex);
+		return -EAGAIN;
+	}
+
+	dprintk(1, "open minor=%d type=%s users=%d\n",
+		minor, v4l2_type_names[type], dev->users[cur_channel]);
+
+	/* allocate + initialize per filehandle data */
+	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+	if (NULL == fh) {
+		dev->users[cur_channel]--;
+		mutex_unlock(&usb_s2255_open_mutex);
+		return -ENOMEM;
+	}
+
+	file->private_data = fh;
+	fh->dev = dev;
+	fh->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	fh->mode = dev->mode[cur_channel];
+	fh->fmt = dev->cur_fmt[cur_channel];
+	/* default 1CIF NTSC */
+	fh->width = 640;
+	fh->height = 480;
+	fh->channel = cur_channel;
+
+	/* Put all controls at a sane state */
+	for (i = 0; i < ARRAY_SIZE(s2255_qctrl); i++)
+		qctl_regs[i] = s2255_qctrl[i].default_value;
+
+	dprintk(1, "Open: fh=0x%08lx, dev=0x%08lx, dev->vidq=0x%08lx\n",
+		(unsigned long)fh, (unsigned long)dev,
+		(unsigned long)&dev->vidq[cur_channel]);
+	dprintk(1, "Open: list_empty queued=%d\n",
+		list_empty(&dev->vidq[cur_channel].queued));
+	dprintk(1, "Open: list_empty active=%d\n",
+		list_empty(&dev->vidq[cur_channel].active));
+	dprintk(1, "s2255core_board_open\n");
+
+	videobuf_queue_vmalloc_init(&fh->vb_vidq, &s2255_video_qops,
+				    NULL, NULL,
+				    fh->type,
+				    V4L2_FIELD_INTERLACED,
+				    sizeof(struct s2255_buffer), fh);
+
+	kref_get(&dev->kref);
+	mutex_unlock(&usb_s2255_open_mutex);
+	dprintk(2, "v4l open done\n");
+	return 0;
+}
+
+
+static unsigned int s2255_poll(struct file *file,
+			       struct poll_table_struct *wait)
+{
+	struct s2255_fh *fh = file->private_data;
+	int rc;
+	dprintk(100, "%s\n", __func__);
+
+	if (V4L2_BUF_TYPE_VIDEO_CAPTURE != fh->type)
+		return POLLERR;
+
+	rc = videobuf_poll_stream(file, &fh->vb_vidq, wait);
+	return rc;
+}
+
+static void s2255_destroy(struct kref *kref)
+{
+	struct s2255_dev *dev = to_s2255_dev(kref);
+	usb_put_dev(dev->udev);
+	dprintk(1, "s2255_destroy\n");
+	kfree(dev);
+}
+
+static int s2255_release_v4l(struct inode *inode, struct file *file)
+{
+	struct s2255_fh *fh = file->private_data;
+	struct s2255_dev *dev = fh->dev;
+	int minor = iminor(inode);
+
+	dev->users[fh->channel]--;
+	if (dev->b_acquire[fh->channel])
+		s2255_stop_acquire(dev, fh->channel);
+	res_free(dev, fh);
+	videobuf_mmap_free(&fh->vb_vidq);
+	kfree(fh);
+	kref_put(&dev->kref, s2255_destroy);
+	dprintk(1, "s2255: close called (minor=%d, users=%d)\n", minor,
+	       dev->users[fh->channel]);
+	return 0;
+}
+
+static int s2255_mmap_v4l(struct file *file, struct vm_area_struct *vma)
+{
+	struct s2255_fh *fh = file->private_data;
+	int ret;
+
+	dprintk(4, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
+
+	ret = videobuf_mmap_mapper(&fh->vb_vidq, vma);
+
+	dprintk(4, "vma start=0x%08lx, size=%ld, ret=%d\n",
+		(unsigned long)vma->vm_start,
+		(unsigned long)vma->vm_end - (unsigned long)vma->vm_start, ret);
+
+	return ret;
+}
+
+static const struct file_operations s2255_fops_v4l = {
+	.owner = THIS_MODULE,
+	.open = s2255_open_v4l,
+	.release = s2255_release_v4l,
+	.poll = s2255_poll,
+	.ioctl = video_ioctl2,	/* V4L2 ioctl handler */
+	.mmap = s2255_mmap_v4l,
+	.llseek = no_llseek,
+};
+
+static struct video_device template = {
+	.name = "s2255v",
+	.type = VID_TYPE_CAPTURE,
+	.fops = &s2255_fops_v4l,
+	.minor = -1,
+	.vidioc_querycap = vidioc_querycap,
+	.vidioc_enum_fmt_cap = vidioc_enum_fmt_cap,
+	.vidioc_g_fmt_cap = vidioc_g_fmt_cap,
+	.vidioc_try_fmt_cap = vidioc_try_fmt_cap,
+	.vidioc_s_fmt_cap = vidioc_s_fmt_cap,
+	.vidioc_reqbufs = vidioc_reqbufs,
+	.vidioc_querybuf = vidioc_querybuf,
+	.vidioc_qbuf = vidioc_qbuf,
+	.vidioc_dqbuf = vidioc_dqbuf,
+	.vidioc_s_std = vidioc_s_std,
+	.vidioc_enum_input = vidioc_enum_input,
+	.vidioc_g_input = vidioc_g_input,
+	.vidioc_s_input = vidioc_s_input,
+	.vidioc_queryctrl = vidioc_queryctrl,
+	.vidioc_g_ctrl = vidioc_g_ctrl,
+	.vidioc_s_ctrl = vidioc_s_ctrl,
+	.vidioc_streamon = vidioc_streamon,
+	.vidioc_streamoff = vidioc_streamoff,
+#ifdef CONFIG_VIDEO_V4L1_COMPAT
+	.vidiocgmbuf = vidiocgmbuf,
+#endif
+	.tvnorms = S2255_NORMS,
+	.current_norm = V4L2_STD_NTSC_M,
+};
+
+static int s2255_probe_v4l(struct s2255_dev *dev)
+{
+	int ret;
+	int i;
+	int cur_nr = video_nr;
+
+	/* initialize all video 4 linux */
+	list_add_tail(&dev->s2255_devlist, &s2255_devlist);
+	/* register 4 video devices */
+	for (i = 0; i < MAX_CHANNELS; i++) {
+		INIT_LIST_HEAD(&dev->vidq[i].active);
+		INIT_LIST_HEAD(&dev->vidq[i].queued);
+		dev->vidq[i].timeout.function = s2255_vid_timeout;
+		dev->vidq[i].timeout.data = (unsigned long)&dev->vidq[i];
+		dev->vidq[i].dev = dev;
+		dev->vidq[i].channel = i;
+		dev->vidq[i].kthread = NULL;
+		init_timer(&dev->vidq[i].timeout);
+		/* register 4 video devices */
+		dev->vdev[i] = video_device_alloc();
+		memcpy(dev->vdev[i], &template, sizeof(struct video_device));
+		dev->vdev[i]->dev = &dev->interface->dev;
+		if (video_nr == -1)
+			ret = video_register_device(dev->vdev[i],
+						    VFL_TYPE_GRABBER,
+						    video_nr);
+		else
+			ret = video_register_device(dev->vdev[i],
+						    VFL_TYPE_GRABBER,
+						    cur_nr + i);
+		dev->vdev[i]->priv = dev;
+
+		if (ret != 0) {
+			dev_err(&dev->udev->dev,
+				"failed to register video device!\n");
+			return ret;
+		}
+	}
+	printk(KERN_INFO "Sensoray 2255 V4L driver\n");
+	return ret;
+}
+
+static void s2255_exit_v4l(struct s2255_dev *dev)
+{
+	struct list_head *list;
+	int i;
+	/* unregister the video devices */
+	while (!list_empty(&s2255_devlist)) {
+		list = s2255_devlist.next;
+		list_del(list);
+	}
+
+	for (i = 0; i < MAX_CHANNELS; i++) {
+		video_unregister_device(dev->vdev[i]);
+		del_timer(&dev->vidq[i].timeout);
+	}
+}
+
+/* this function moves the usb stream read pipe data
+ * into the system buffers.
+ * returns 0 on success, EAGAIN if more data to process( call this
+ * function again).
+ *
+ * Received frame structure:
+ * bytes 0-3:  marker : 0x2255DA4AL (FRAME_MARKER)
+ * bytes 4-7:  channel: 0-3
+ * bytes 8-11: payload size:  size of the frame
+ * bytes 12-payloadsize+12:  frame data
+ */
+static int save_frame(struct s2255_dev *dev, struct s2255_pipeinfo *pipe_info)
+{
+	static int dbgsync; /* = 0; */
+	char *pdest;
+	u32 offset = 0;
+	int bsync = 0;
+	int btrunc = 0;
+	char *psrc;
+	unsigned long copy_size;
+	unsigned long size;
+	s32 idx = -1;
+	struct framei *frm;
+	unsigned char *pdata;
+	unsigned long cur_size;
+	int bsearch = 0;
+	struct bufferi *buf;
+	dprintk(100, "buffer to user\n");
+
+	idx = dev->cur_frame[dev->cc];
+	buf = &dev->buffer[dev->cc];
+	frm = &buf->frame[idx];
+
+	if (frm->ulState == 0) {
+		frm->ulState = 1;
+		frm->cur_size = 0;
+		bsearch = 1;
+	} else if (frm->ulState == 2) {
+		/* system frame was not freed */
+		dprintk(2, "sys frame not free.  overrun ringbuf\n");
+		bsearch = 1;
+		frm->ulState = 1;
+		frm->cur_size = 0;
+	}
+
+	if (bsearch) {
+		if (*(s32 *) pipe_info->transfer_buffer != FRAME_MARKER) {
+			u32 jj;
+			if (dbgsync == 0) {
+				dprintk(3, "not synched, discarding all packets"
+					"until marker\n");
+
+				dbgsync++;
+			}
+			pdata = (unsigned char *)pipe_info->transfer_buffer;
+			for (jj = 0; jj < (pipe_info->cur_transfer_size - 12);
+			     jj++) {
+				if (*(s32 *) pdata == FRAME_MARKER) {
+					int cc;
+					dprintk(3,
+						"found frame marker at offset:"
+						" %d [%x %x]\n", jj, pdata[0],
+						pdata[1]);
+					offset = jj;
+					bsync = 1;
+					cc = *(u32 *) (pdata + sizeof(u32));
+					if (cc >= MAX_CHANNELS) {
+						printk(KERN_ERR
+						       "bad channel\n");
+						return -EINVAL;
+					}
+					/* reverse it */
+					dev->cc = G_chnmap[cc];
+					break;
+				}
+				pdata++;
+			}
+			if (bsync == 0)
+				return -EINVAL;
+		} else {
+			u32 *pword;
+			u32 payload;
+			int cc;
+			dbgsync = 0;
+			bsync = 1;
+			pword = (u32 *) pipe_info->transfer_buffer;
+			cc = pword[1];
+
+			if (cc >= MAX_CHANNELS) {
+				printk("invalid channel found. "
+					"throwing out data!\n");
+				return -EINVAL;
+			}
+			dev->cc = G_chnmap[cc];
+			payload = pword[2];
+			if (payload != dev->req_image_size[dev->cc]) {
+				dprintk(1, "[%d][%d]unexpected payload: %d"
+					"required: %lu \n", cc, dev->cc,
+					payload, dev->req_image_size[dev->cc]);
+				dev->bad_payload[dev->cc]++;
+				/* discard the bad frame */
+				return -EINVAL;
+			}
+
+		}
+	}
+	/* search done.  now find out if should be acquiring
+	   on this channel */
+	if (!dev->b_acquire[dev->cc]) {
+		frm->ulState = 0;
+		return -EINVAL;
+	}
+
+	idx = dev->cur_frame[dev->cc];
+	frm = &dev->buffer[dev->cc].frame[idx];
+
+	if (frm->ulState == 0) {
+		frm->ulState = 1;
+		frm->cur_size = 0;
+	} else if (frm->ulState == 2) {
+		/* system frame ring buffer overrun */
+		dprintk(2, "sys frame overrun.  overwriting frame %d %d\n",
+			dev->cc, idx);
+		frm->ulState = 1;
+		frm->cur_size = 0;
+	}
+
+	if (bsync) {
+		/* skip the marker 512 bytes (and offset if out of sync) */
+		psrc = (u8 *)pipe_info->transfer_buffer + offset + PREFIX_SIZE;
+	} else {
+		psrc = (u8 *)pipe_info->transfer_buffer;
+	}
+
+	if (frm->lpvbits == NULL) {
+		dprintk(1, "s2255 frame buffer == NULL.%p %p %d %d",
+			frm, dev, dev->cc, idx);
+		return -ENOMEM;
+	}
+
+	pdest = frm->lpvbits + frm->cur_size;
+
+	if (bsync) {
+		copy_size =
+		    (pipe_info->cur_transfer_size - offset) - PREFIX_SIZE;
+		if (copy_size > pipe_info->cur_transfer_size) {
+			printk("invalid copy size, overflow!\n");
+			return -ENOMEM;
+		}
+	} else {
+		copy_size = pipe_info->cur_transfer_size;
+	}
+
+	cur_size = frm->cur_size;
+	size = dev->req_image_size[dev->cc];
+
+	if ((copy_size + cur_size) > size) {
+		copy_size = size - cur_size;
+		btrunc = 1;
+	}
+
+	memcpy(pdest, psrc, copy_size);
+	cur_size += copy_size;
+	frm->cur_size += copy_size;
+	dprintk(50, "cur_size size %lu size %lu \n", cur_size, size);
+
+	if (cur_size >= (size - PREFIX_SIZE)) {
+		u32 cc = dev->cc;
+		frm->ulState = 2;
+		dprintk(2, "****************[%d]Buffer[%d]full*************\n",
+			cc, idx);
+		dev->last_frame[cc] = dev->cur_frame[cc];
+		dev->cur_frame[cc]++;
+		/* end of system frame ring buffer, start at zero */
+		if ((dev->cur_frame[cc] == SYS_FRAMES) ||
+		    (dev->cur_frame[cc] == dev->buffer[cc].dwFrames))
+			dev->cur_frame[cc] = 0;
+
+		/* signal the semaphore for this channel */
+		if (dev->b_acquire[cc])
+			s2255_got_frame(dev, cc);
+		dev->frame_count[cc]++;
+	}
+	/* frame was truncated */
+	if (btrunc) {
+		/* return more data to process */
+		return EAGAIN;
+	}
+	/* done successfully */
+	return 0;
+}
+
+static void s2255_read_video_callback(struct s2255_dev *dev,
+				      struct s2255_pipeinfo *pipe_info)
+{
+	int res;
+	dprintk(50, "callback read video \n");
+
+	if (dev->cc >= MAX_CHANNELS) {
+		dev->cc = 0;
+		dev_err(&dev->udev->dev, "invalid channel\n");
+		return;
+	}
+	/* otherwise copy to the system buffers */
+	res = save_frame(dev, pipe_info);
+	if (res == EAGAIN)
+		save_frame(dev, pipe_info);
+
+	dprintk(50, "callback read video done\n");
+	return;
+}
+
+static long s2255_vendor_req(struct s2255_dev *dev, unsigned char Request,
+			     u16 Index, u16 Value, void *TransferBuffer,
+			     s32 TransferBufferLength, int bOut)
+{
+	int r;
+	if (!bOut) {
+		r = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
+				    Request,
+				    USB_TYPE_VENDOR | USB_RECIP_DEVICE |
+				    USB_DIR_IN,
+				    Value, Index, TransferBuffer,
+				    TransferBufferLength, HZ * 5);
+	} else {
+		r = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
+				    Request, USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+				    Value, Index, TransferBuffer,
+				    TransferBufferLength, HZ * 5);
+	}
+	return r;
+}
+
+/*
+ * retrieve FX2 firmware version. future use.
+ * @param dev pointer to device extension
+ * @return -1 for fail, else returns firmware version as an int(16 bits)
+ */
+static int s2255_get_fx2fw(struct s2255_dev *dev)
+{
+	int fw;
+	int ret;
+	unsigned char transBuffer[64];
+	ret = s2255_vendor_req(dev, VX_FW, 0, 0, transBuffer, 2, DIR_IN);
+	if (ret < 0)
+		dprintk(2, "get fw error: %x\n", ret);
+	fw = transBuffer[0] + (transBuffer[1] << 8);
+	dprintk(2, "Get FW %x %x\n", transBuffer[0], transBuffer[1]);
+	return fw;
+}
+
+/*
+ * Create the system ring buffer to copy frames into from the
+ * usb read pipe.
+ */
+static int s2255_create_sys_buffers(struct s2255_dev *dev, unsigned long chn)
+{
+	unsigned long i;
+	unsigned long reqsize;
+	dprintk(1, "create sys buffers\n");
+	if (chn >= MAX_CHANNELS)
+		return -1;
+
+	dev->buffer[chn].dwFrames = SYS_FRAMES;
+
+	/* always allocate maximum size(PAL) for system buffers */
+	reqsize = SYS_FRAMES_MAXSIZE;
+
+	if (reqsize > SYS_FRAMES_MAXSIZE)
+		reqsize = SYS_FRAMES_MAXSIZE;
+
+	for (i = 0; i < SYS_FRAMES; i++) {
+		/* allocate the frames */
+		dev->buffer[chn].frame[i].lpvbits = vmalloc(reqsize);
+
+		dprintk(1, "valloc %p chan %lu, idx %lu, pdata %p\n",
+			&dev->buffer[chn].frame[i], chn, i,
+			dev->buffer[chn].frame[i].lpvbits);
+		dev->buffer[chn].frame[i].size = reqsize;
+		if (dev->buffer[chn].frame[i].lpvbits == NULL) {
+			printk(KERN_INFO "out of memory.  using less frames\n");
+			dev->buffer[chn].dwFrames = i;
+			break;
+		}
+	}
+
+	/* make sure internal states are set */
+	for (i = 0; i < SYS_FRAMES; i++) {
+		dev->buffer[chn].frame[i].ulState = 0;
+		dev->buffer[chn].frame[i].cur_size = 0;
+	}
+
+	dev->cur_frame[chn] = 0;
+	dev->last_frame[chn] = -1;
+	return 0;
+}
+
+static int s2255_release_sys_buffers(struct s2255_dev *dev,
+				     unsigned long channel)
+{
+	unsigned long i;
+	dprintk(1, "release sys buffers\n");
+	for (i = 0; i < SYS_FRAMES; i++) {
+		if (dev->buffer[channel].frame[i].lpvbits) {
+			dprintk(1, "vfree %p\n",
+				dev->buffer[channel].frame[i].lpvbits);
+			vfree(dev->buffer[channel].frame[i].lpvbits);
+		}
+		dev->buffer[channel].frame[i].lpvbits = NULL;
+	}
+	return 0;
+}
+
+static int s2255_board_init(struct s2255_dev *dev)
+{
+	int j;
+	struct mode2255i mode_def = { DEF_MODEI_NTSC_CONT };
+	int fw_ver;
+	dprintk(4, "board init: %p", dev);
+
+	for (j = 0; j < MAX_PIPE_BUFFERS; j++) {
+		struct s2255_pipeinfo *pipe = &dev->pipes[j];
+
+		memset(pipe, 0, sizeof(*pipe));
+		pipe->dev = dev;
+		pipe->cur_transfer_size = DEFAULT_PIPE_USBBLOCK;
+		pipe->max_transfer_size = MAX_PIPE_USBBLOCK;
+
+		if (pipe->cur_transfer_size > pipe->max_transfer_size)
+			pipe->cur_transfer_size = pipe->max_transfer_size;
+		pipe->transfer_buffer = kzalloc(pipe->max_transfer_size,
+						GFP_KERNEL);
+		if (pipe->transfer_buffer == NULL) {
+			dprintk(1, "out of memory!\n");
+			return -ENOMEM;
+		}
+
+	}
+
+	/* query the firmware */
+	fw_ver = s2255_get_fx2fw(dev);
+
+	printk(KERN_INFO "2255 usb firmware version %d \n", fw_ver);
+	if (fw_ver < CUR_USB_FWVER)
+		err("usb firmware not up to date %d\n", fw_ver);
+
+	for (j = 0; j < MAX_CHANNELS; j++) {
+		dev->b_acquire[j] = 0;
+		dev->mode[j] = mode_def;
+		dev->cur_fmt[j] = &formats[0];
+		dev->mode[j].restart = 1;
+		dev->req_image_size[j] = get_transfer_size(&mode_def);
+		dev->frame_count[j] = 0;
+		/* create the system buffers */
+		s2255_create_sys_buffers(dev, j);
+	}
+	/* start read pipe */
+	s2255_start_readpipe(dev);
+
+	dprintk(1, "S2255: board initialized\n");
+	return 0;
+}
+
+static int s2255_board_shutdown(struct s2255_dev *dev)
+{
+	u32 i;
+
+	dprintk(1, "S2255: board shutdown: %p", dev);
+
+	for (i = 0; i < MAX_CHANNELS; i++) {
+		if (dev->b_acquire[i])
+			s2255_stop_acquire(dev, i);
+	}
+
+	s2255_stop_readpipe(dev);
+
+	for (i = 0; i < MAX_CHANNELS; i++)
+		s2255_release_sys_buffers(dev, i);
+
+	/* release transfer buffers */
+	for (i = 0; i < MAX_PIPE_BUFFERS; i++) {
+		struct s2255_pipeinfo *pipe = &dev->pipes[i];
+		kfree(pipe->transfer_buffer);
+	}
+	return 0;
+}
+
+static void read_pipe_completion(struct urb *purb)
+{
+	struct s2255_pipeinfo *pipe_info;
+	struct s2255_dev *dev;
+	int status;
+	int pipe;
+
+	pipe_info = purb->context;
+	dprintk(100, "read pipe completion %p, status %d\n", purb,
+		purb->status);
+	if (pipe_info == NULL) {
+		err("no context !");
+		return;
+	}
+
+	dev = pipe_info->dev;
+	if (dev == NULL) {
+		err("no context !");
+		return;
+	}
+	status = purb->status;
+	if (status != 0) {
+		dprintk(2, "read_pipe_completion: err\n");
+		return;
+	}
+
+	if (pipe_info->state == 0) {
+		dprintk(2, "exiting USB pipe");
+		return;
+	}
+
+	s2255_read_video_callback(dev, pipe_info);
+
+	pipe_info->err_count = 0;
+	pipe = usb_rcvbulkpipe(dev->udev, dev->read_endpoint);
+	/* reuse urb */
+	usb_fill_bulk_urb(pipe_info->stream_urb, dev->udev,
+			  pipe,
+			  pipe_info->transfer_buffer,
+			  pipe_info->cur_transfer_size,
+			  read_pipe_completion, pipe_info);
+
+	if (pipe_info->state != 0) {
+		if (usb_submit_urb(pipe_info->stream_urb, GFP_KERNEL)) {
+			dev_err(&dev->udev->dev, "error submitting urb\n");
+			usb_free_urb(pipe_info->stream_urb);
+		}
+	} else {
+		dprintk(2, "read pipe complete state 0\n");
+	}
+	return;
+}
+
+static int s2255_start_readpipe(struct s2255_dev *dev)
+{
+	int pipe;
+	int retval;
+	int i;
+	struct s2255_pipeinfo *pipe_info = dev->pipes;
+	pipe = usb_rcvbulkpipe(dev->udev, dev->read_endpoint);
+	dprintk(2, "start pipe IN %d\n", dev->read_endpoint);
+
+	for (i = 0; i < MAX_PIPE_BUFFERS; i++) {
+		pipe_info->state = 1;
+		pipe_info->buf_index = (u32) i;
+		pipe_info->priority_set = 0;
+		pipe_info->stream_urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!pipe_info->stream_urb) {
+			dev_err(&dev->udev->dev,
+				"ReadStream: Unable to alloc URB");
+			return -ENOMEM;
+		}
+		/* transfer buffer allocated in board_init */
+		usb_fill_bulk_urb(pipe_info->stream_urb, dev->udev,
+				  pipe,
+				  pipe_info->transfer_buffer,
+				  pipe_info->cur_transfer_size,
+				  read_pipe_completion, pipe_info);
+
+		pipe_info->urb_size = sizeof(pipe_info->stream_urb);
+		dprintk(4, "submitting URB %p\n", pipe_info->stream_urb);
+		retval = usb_submit_urb(pipe_info->stream_urb, GFP_KERNEL);
+		if (retval) {
+			printk(KERN_ERR "s2255: start read pipe failed\n");
+			return retval;
+		}
+	}
+
+	return 0;
+}
+
+/* starts acquisition process */
+static int s2255_start_acquire(struct s2255_dev *dev, unsigned long chn)
+{
+	unsigned char *buffer;
+	int res;
+	unsigned long chn_rev;
+	int j;
+	if (chn >= MAX_CHANNELS) {
+		dprintk(2, "start acquire failed, bad channel %lu\n", chn);
+		return -1;
+	}
+
+	chn_rev = G_chnmap[chn];
+	dprintk(1, "S2255: start acquire %lu \n", chn);
+
+	buffer = kzalloc(512, GFP_KERNEL);
+	if (buffer == NULL) {
+		dev_err(&dev->udev->dev, "out of mem\n");
+		return -ENOMEM;
+	}
+
+	dev->last_frame[chn] = -1;
+	dev->bad_payload[chn] = 0;
+	dev->cur_frame[chn] = 0;
+	for (j = 0; j < SYS_FRAMES; j++) {
+		dev->buffer[chn].frame[j].ulState = 0;
+		dev->buffer[chn].frame[j].cur_size = 0;
+	}
+
+	/* send the start command */
+	*(u32 *) buffer = IN_DATA_TOKEN;
+	*((u32 *) buffer + 1) = (u32) chn_rev;
+	*((u32 *) buffer + 2) = (u32) CMD_START;
+	res = s2255_write_config(dev->udev, (unsigned char *)buffer, 512);
+	if (res != 0)
+		dev_err(&dev->udev->dev, "CMD_START error\n");
+
+	dprintk(2, "start acquire exit[%lu] %d \n", chn, res);
+	kfree(buffer);
+	return 0;
+}
+
+static int s2255_stop_acquire(struct s2255_dev *dev, unsigned long chn)
+{
+	unsigned char *buffer;
+	int res;
+	unsigned long chn_rev;
+
+	if (chn >= MAX_CHANNELS) {
+		dprintk(2, "stop acquire failed, bad channel %lu\n", chn);
+		return -1;
+	}
+	chn_rev = G_chnmap[chn];
+
+	buffer = kzalloc(512, GFP_KERNEL);
+	if (buffer == NULL) {
+		dev_err(&dev->udev->dev, "out of mem\n");
+		return -ENOMEM;
+	}
+
+	/* send the stop command */
+	dprintk(4, "stop acquire %lu\n", chn);
+	*(u32 *) buffer = IN_DATA_TOKEN;
+	*((u32 *) buffer + 1) = (u32) chn_rev;
+	*((u32 *) buffer + 2) = CMD_STOP;
+	res = s2255_write_config(dev->udev, (unsigned char *)buffer, 512);
+
+	if (res != 0)
+		dev_err(&dev->udev->dev, "CMD_STOP error\n");
+
+	dprintk(4, "stop acquire: releasing states \n");
+
+	kfree(buffer);
+	dev->b_acquire[chn] = 0;
+
+	return 0;
+}
+
+static void s2255_stop_readpipe(struct s2255_dev *dev)
+{
+	int j;
+
+	if (dev == NULL) {
+		err("s2255: invalid device");
+		return;
+	}
+	dprintk(4, "stop read pipe\n");
+	for (j = 0; j < MAX_PIPE_BUFFERS; j++) {
+		struct s2255_pipeinfo *pipe_info = &dev->pipes[j];
+		if (pipe_info) {
+			if (pipe_info->state == 0)
+				continue;
+			pipe_info->state = 0;
+			pipe_info->prev_state = 1;
+
+		}
+	}
+
+	for (j = 0; j < MAX_PIPE_BUFFERS; j++) {
+		struct s2255_pipeinfo *pipe_info = &dev->pipes[j];
+		if (pipe_info->stream_urb) {
+			/* cancel urb */
+			usb_kill_urb(pipe_info->stream_urb);
+			usb_free_urb(pipe_info->stream_urb);
+			pipe_info->stream_urb = NULL;
+		}
+	}
+	dprintk(2, "s2255 stop read pipe: %d\n", j);
+	return;
+}
+
+/* standard usb probe function */
+static int s2255_probe(struct usb_interface *interface,
+		       const struct usb_device_id *id)
+{
+	struct s2255_dev *dev = NULL;
+	struct usb_host_interface *iface_desc;
+	struct usb_endpoint_descriptor *endpoint;
+	int i;
+	int retval = -ENOMEM;
+
+	dprintk(100, "s2255: probe\n");
+
+	/* allocate memory for our device state and initialize it to zero */
+	dev = kzalloc(sizeof(struct s2255_dev), GFP_KERNEL);
+	if (dev == NULL) {
+		err("s2255: out of memory");
+		goto error;
+	}
+
+	/* grab usb_device and save it */
+	dev->udev = usb_get_dev(interface_to_usbdev(interface));
+	if (dev->udev == NULL) {
+		dev_err(&interface->dev, "null usb device\n");
+		goto error;
+	}
+
+	kref_init(&dev->kref);
+	dprintk(1, "dev: %p, kref: %p udev %p interface %p\n", dev, &dev->kref,
+		dev->udev, interface);
+	dev->interface = interface;
+	/* set up the endpoint information  */
+	iface_desc = interface->cur_altsetting;
+	dprintk(1, "num endpoints %d\n", iface_desc->desc.bNumEndpoints);
+	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
+		endpoint = &iface_desc->endpoint[i].desc;
+		if (!dev->read_endpoint && usb_endpoint_is_bulk_in(endpoint)) {
+			/* we found the bulk in endpoint */
+			dev->read_endpoint = endpoint->bEndpointAddress;
+		}
+	}
+
+	if (!dev->read_endpoint) {
+		dev_err(&interface->dev, "Could not find bulk-in endpoint");
+		goto error;
+	}
+
+	/* set intfdata */
+	usb_set_intfdata(interface, dev);
+
+	dprintk(100, "after intfdata %p\n", dev);
+
+	/* initialize device mutex */
+	mutex_init(&dev->lock);
+
+	init_timer(&dev->timer);
+	dev->timer.function = s2255_timer;
+	dev->fw_data = kzalloc(sizeof(struct complete_data), GFP_KERNEL);
+	if (!dev->fw_data)
+		goto error;
+
+	dev->timer.data = (unsigned long)dev->fw_data;
+	dev->fw_data->fw_urb = usb_alloc_urb(0, GFP_KERNEL);
+
+	if (!dev->fw_data->fw_urb) {
+		dev_err(&interface->dev, "out of memory!\n");
+		goto error;
+	}
+	dev->fw_data->pfw_data = kzalloc(CHUNK_SIZE, GFP_KERNEL);
+	if (!dev->fw_data->pfw_data) {
+		dev_err(&interface->dev, "out of memory!\n");
+		goto error;
+	}
+	/* load the first chunk */
+	if (request_firmware(&dev->fw_data->fw,
+			     FIRMWARE_FILE_NAME, &dev->udev->dev)) {
+		printk(KERN_ERR "sensoray 2255 failed to get firmware\n");
+		goto error;
+	}
+
+	dev->fw_data->fw_size = dev->fw_data->fw->size;
+	memcpy(dev->fw_data->pfw_data, dev->fw_data->fw->data, CHUNK_SIZE);
+	dev->fw_data->fw_loaded = CHUNK_SIZE;
+	usb_fill_bulk_urb(dev->fw_data->fw_urb, dev->udev,
+			  usb_sndbulkpipe(dev->udev, 2), dev->fw_data->pfw_data,
+			  CHUNK_SIZE, s2255_fwchunk_complete, dev->fw_data);
+	/* loads v4l specific */
+	s2255_probe_v4l(dev);
+	/* load 2255 board specific */
+	s2255_board_init(dev);
+
+	dev_info(&interface->dev, "Sensoray 2255 successfully loaded\n");
+	dprintk(4, "before probe done %p\n", dev);
+
+	mod_timer(&dev->timer, jiffies + HZ);
+	spin_lock_init(&dev->slock);
+	kref_get(&dev->kref);
+	return 0;
+error:
+	return retval;
+}
+
+/* disconnect routine. when board is removed physically or with rmmod */
+static void s2255_disconnect(struct usb_interface *interface)
+{
+	struct s2255_dev *dev = NULL;
+
+	/* lock to prevent s2255_open() from racing s2255_disconnect() */
+	mutex_lock(&usb_s2255_open_mutex);
+	dprintk(1, "s2255: disconnect interface %p\n", interface);
+	dev = usb_get_intfdata(interface);
+	s2255_board_shutdown(dev);
+	if (dev->fw_data->fw) {
+		release_firmware(dev->fw_data->fw);
+		dev->fw_data->fw = NULL;
+	}
+	if (dev->fw_data->fw_urb) {
+		dprintk(2, "kill URB\n");
+		usb_kill_urb(dev->fw_data->fw_urb);
+		usb_free_urb(dev->fw_data->fw_urb);
+
+	}
+	s2255_exit_v4l(dev);
+	if (dev->fw_data) {
+		kfree(dev->fw_data->pfw_data);
+		kfree(dev->fw_data);
+	}
+	usb_set_intfdata(interface, NULL);
+	kref_put(&dev->kref, s2255_destroy);
+	mutex_unlock(&usb_s2255_open_mutex);
+	dev_info(&interface->dev, "s2255usb now disconnected\n");
+}
+
+static struct usb_driver s2255_driver = {
+	.name = "s2255",
+	.probe = s2255_probe,
+	.disconnect = s2255_disconnect,
+	.id_table = s2255_table,
+};
+
+static int __init usb_s2255_init(void)
+{
+	int result;
+
+	/* register this driver with the USB subsystem */
+	result = usb_register(&s2255_driver);
+
+	if (result)
+		err("usb_register failed. Error number %d", result);
+
+	dprintk(2, "s2255_init: done\n");
+	return result;
+}
+
+static void __exit usb_s2255_exit(void)
+{
+	usb_deregister(&s2255_driver);
+}
+
+module_init(usb_s2255_init);
+module_exit(usb_s2255_exit);
+MODULE_DESCRIPTION("Sensoray 2255 Video for Linux driver");
+MODULE_AUTHOR("D.A.(Sensoray)");
+MODULE_LICENSE("GPL");



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-14 20:59 [PATCH] USB: add Sensoray 2255 v4l driver Greg KH
@ 2008-05-15  1:17   ` Markus Rechberger
  2008-05-15 11:38 ` Oliver Neukum
  2008-05-16  2:51   ` Mauro Carvalho Chehab
  2 siblings, 0 replies; 26+ messages in thread
From: Markus Rechberger @ 2008-05-15  1:17 UTC (permalink / raw)
  To: Greg KH
  Cc: mchehab, v4l-dvb-maintainer, video4linux-list, linux-usb, linux-kernel

Hi Dean, Greg,

On 5/14/08, Greg KH <greg@kroah.com> wrote:
> From: Dean Anderson <dean@sensoray.com>
>
> +static int norm_maxw(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    LINE_SZ_4CIFS_NTSC : LINE_SZ_4CIFS_PAL;
> +}
> +
> +static int norm_maxh(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    (NUM_LINES_1CIFS_NTSC * 2) : (NUM_LINES_1CIFS_PAL * 2);
> +}
> +
> +static int norm_minw(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    LINE_SZ_1CIFS_NTSC : LINE_SZ_1CIFS_PAL;
> +}
> +
> +static int norm_minh(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    (NUM_LINES_1CIFS_NTSC) : (NUM_LINES_1CIFS_PAL);
> +}
> +
> +/*
> + * convert from YUV(YCrCb) to RGB
> + * 65536 R = 76533(Y-16) + 104936 * (Cr-128)
> + * 65536 G = 76533(Y-16) - 53451(Cr-128) - 25703(Cb -128)
> + * 65536 B = 76533(Y-16) + 132677(Cb-128)
> + */
> +static void YCrCb2RGB(int Y, int Cr, int Cb, unsigned char *pR,
> +		      unsigned char *pG, unsigned char *pB)
> +{
> +	int R, G, B;
> +
> +	Y = Y - 16;
> +	Cr = Cr - 128;
> +	Cb = Cb - 128;
> +
> +	R = (76533 * Y + 104936 * Cr) >> 16;
> +	G = ((76533 * Y) - (53451 * Cr) - (25703 * Cb)) >> 16;
> +	B = ((76533 * Y) + (132677 * Cb)) >> 16;
> +	/* even with proper conversion, some values still need clipping. */
> +	if (R > 255)
> +		R = 255;
> +	if (G > 255)
> +		G = 255;
> +	if (B > 255)
> +		B = 255;
> +	if (R < 0)
> +		R = 0;
> +	if (G < 0)
> +		G = 0;
> +	if (B < 0)
> +		B = 0;
> +	*pR = R;
> +	*pG = G;
> +	*pB = B;
> +	return;
> +}
> +
> +/* converts 2255 planar format to yuyv */
> +static void planar422p_to_yuy2(const unsigned char *in, unsigned char *out,
> +			       int width, int height)
> +{
> +	unsigned char *pY;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned long size = height * width;
> +	unsigned int i;
> +	pY = (unsigned char *)in;
> +	pCr = (unsigned char *)in + height * width;
> +	pCb = (unsigned char *)in + height * width + (height * width / 2);
> +	for (i = 0; i < size * 2; i += 4) {
> +		out[i] = *pY++;
> +		out[i + 1] = *pCr++;
> +		out[i + 2] = *pY++;
> +		out[i + 3] = *pCb++;
> +	}
> +	return;
> +}
> +
> +/*
> + * basic 422 planar to RGB24 or BGR24 software conversion.
> + * This is best done with MMX. Update to kernel function
> + * when image conversion functions added to kernel.
> + */
> +static void planar422p_to_rgb24(const unsigned char *in,
> +				unsigned char *out, int width,
> +				int height, int rev_order)
> +{
> +	unsigned char *pY;
> +	unsigned char *pYEND;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned char Cr, Cb, Y, r, g, b;
> +	unsigned long k = 0;
> +	pY = (unsigned char *)in;
> +	pCb = (unsigned char *)in + (height * width);
> +	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
> +	pYEND = pCb;
> +	while (pY < pYEND) {
> +		Y = *pY++;
> +		Cr = *pCr;
> +		Cb = *pCb;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = !rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = !rev_order ? r : b;
> +		if (pY >= pYEND)
> +			break;
> +		Y = *pY++;
> +		Cr = *pCr++;
> +		Cb = *pCb++;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = !rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = !rev_order ? r : b;
> +	}
> +	return;
> +}
> +
> +static void planar422p_to_rgb32(const unsigned char *in, unsigned char
> *out,
> +				int width, int height, int rev_order)
> +{
> +	unsigned char *pY;
> +	unsigned char *pYEND;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned char Cr, Cb, Y, r, g, b;
> +	unsigned long k = 0;
> +	pY = (unsigned char *)in;
> +	pCb = (unsigned char *)in + (height * width);
> +	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
> +	pYEND = pCb;
> +	while (pY < pYEND) {
> +		Y = *pY++;
> +		Cr = *pCr;
> +		Cb = *pCb;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = rev_order ? r : b;
> +		out[k++] = 0;
> +		if (pY >= pYEND)
> +			break;
> +		Y = *pY++;
> +		Cr = *pCr++;
> +		Cb = *pCb++;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = rev_order ? r : b;
> +		out[k++] = 0;
> +	}
> +
> +	return;
> +}
> +
> +static void planar422p_to_rgb565(unsigned char const *in, unsigned char
> *out,
> +				 int width, int height, int rev_order)
> +{
> +	unsigned char *pY;
> +	unsigned char *pYEND;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned char Cr, Cb, Y, r, g, b;
> +	unsigned long k = 0;
> +	unsigned short rgbbytes;
> +	pY = (unsigned char *)in;
> +	pCb = (unsigned char *)in + (height * width);
> +	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
> +	pYEND = pCb;
> +	while (pY < pYEND) {
> +		Y = *pY++;
> +		Cr = *pCr;
> +		Cb = *pCb;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		r = r >> 3;
> +		g = g >> 2;
> +		b = b >> 3;
> +		if (rev_order)
> +			rgbbytes = b + (g << 5) + (r << (5 + 6));
> +		else
> +			rgbbytes = r + (g << 5) + (b << (5 + 6));
> +		out[k++] = rgbbytes & 0xff;
> +		out[k++] = (rgbbytes >> 8) & 0xff;
> +		Y = *pY++;
> +		Cr = *pCr++;
> +		Cb = *pCb++;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		r = r >> 3;
> +		g = g >> 2;
> +		b = b >> 3;
> +		if (rev_order)
> +			rgbbytes = b + (g << 5) + (r << (5 + 6));
> +		else
> +			rgbbytes = r + (g << 5) + (b << (5 + 6));
> +		out[k++] = rgbbytes & 0xff;
> +		out[k++] = (rgbbytes >> 8) & 0xff;
> +	}
> +	return;
> +}
> +


Why do you do those conversions in kernelspace?
ffmpeg/libswscale has optimized code for colourspace conversions.
I know a few drivers do that in kernelspace but it's way more flexible
in userspace and depending on the optimization requires less CPU
power.

Markus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-15  1:17   ` Markus Rechberger
  0 siblings, 0 replies; 26+ messages in thread
From: Markus Rechberger @ 2008-05-15  1:17 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel, v4l-dvb-maintainer, linux-usb, video4linux-list, mchehab

Hi Dean, Greg,

On 5/14/08, Greg KH <greg@kroah.com> wrote:
> From: Dean Anderson <dean@sensoray.com>
>
> +static int norm_maxw(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    LINE_SZ_4CIFS_NTSC : LINE_SZ_4CIFS_PAL;
> +}
> +
> +static int norm_maxh(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    (NUM_LINES_1CIFS_NTSC * 2) : (NUM_LINES_1CIFS_PAL * 2);
> +}
> +
> +static int norm_minw(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    LINE_SZ_1CIFS_NTSC : LINE_SZ_1CIFS_PAL;
> +}
> +
> +static int norm_minh(struct video_device *vdev)
> +{
> +	return (vdev->current_norm != V4L2_STD_PAL_B) ?
> +	    (NUM_LINES_1CIFS_NTSC) : (NUM_LINES_1CIFS_PAL);
> +}
> +
> +/*
> + * convert from YUV(YCrCb) to RGB
> + * 65536 R = 76533(Y-16) + 104936 * (Cr-128)
> + * 65536 G = 76533(Y-16) - 53451(Cr-128) - 25703(Cb -128)
> + * 65536 B = 76533(Y-16) + 132677(Cb-128)
> + */
> +static void YCrCb2RGB(int Y, int Cr, int Cb, unsigned char *pR,
> +		      unsigned char *pG, unsigned char *pB)
> +{
> +	int R, G, B;
> +
> +	Y = Y - 16;
> +	Cr = Cr - 128;
> +	Cb = Cb - 128;
> +
> +	R = (76533 * Y + 104936 * Cr) >> 16;
> +	G = ((76533 * Y) - (53451 * Cr) - (25703 * Cb)) >> 16;
> +	B = ((76533 * Y) + (132677 * Cb)) >> 16;
> +	/* even with proper conversion, some values still need clipping. */
> +	if (R > 255)
> +		R = 255;
> +	if (G > 255)
> +		G = 255;
> +	if (B > 255)
> +		B = 255;
> +	if (R < 0)
> +		R = 0;
> +	if (G < 0)
> +		G = 0;
> +	if (B < 0)
> +		B = 0;
> +	*pR = R;
> +	*pG = G;
> +	*pB = B;
> +	return;
> +}
> +
> +/* converts 2255 planar format to yuyv */
> +static void planar422p_to_yuy2(const unsigned char *in, unsigned char *out,
> +			       int width, int height)
> +{
> +	unsigned char *pY;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned long size = height * width;
> +	unsigned int i;
> +	pY = (unsigned char *)in;
> +	pCr = (unsigned char *)in + height * width;
> +	pCb = (unsigned char *)in + height * width + (height * width / 2);
> +	for (i = 0; i < size * 2; i += 4) {
> +		out[i] = *pY++;
> +		out[i + 1] = *pCr++;
> +		out[i + 2] = *pY++;
> +		out[i + 3] = *pCb++;
> +	}
> +	return;
> +}
> +
> +/*
> + * basic 422 planar to RGB24 or BGR24 software conversion.
> + * This is best done with MMX. Update to kernel function
> + * when image conversion functions added to kernel.
> + */
> +static void planar422p_to_rgb24(const unsigned char *in,
> +				unsigned char *out, int width,
> +				int height, int rev_order)
> +{
> +	unsigned char *pY;
> +	unsigned char *pYEND;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned char Cr, Cb, Y, r, g, b;
> +	unsigned long k = 0;
> +	pY = (unsigned char *)in;
> +	pCb = (unsigned char *)in + (height * width);
> +	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
> +	pYEND = pCb;
> +	while (pY < pYEND) {
> +		Y = *pY++;
> +		Cr = *pCr;
> +		Cb = *pCb;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = !rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = !rev_order ? r : b;
> +		if (pY >= pYEND)
> +			break;
> +		Y = *pY++;
> +		Cr = *pCr++;
> +		Cb = *pCb++;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = !rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = !rev_order ? r : b;
> +	}
> +	return;
> +}
> +
> +static void planar422p_to_rgb32(const unsigned char *in, unsigned char
> *out,
> +				int width, int height, int rev_order)
> +{
> +	unsigned char *pY;
> +	unsigned char *pYEND;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned char Cr, Cb, Y, r, g, b;
> +	unsigned long k = 0;
> +	pY = (unsigned char *)in;
> +	pCb = (unsigned char *)in + (height * width);
> +	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
> +	pYEND = pCb;
> +	while (pY < pYEND) {
> +		Y = *pY++;
> +		Cr = *pCr;
> +		Cb = *pCb;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = rev_order ? r : b;
> +		out[k++] = 0;
> +		if (pY >= pYEND)
> +			break;
> +		Y = *pY++;
> +		Cr = *pCr++;
> +		Cb = *pCb++;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		out[k++] = rev_order ? b : r;
> +		out[k++] = g;
> +		out[k++] = rev_order ? r : b;
> +		out[k++] = 0;
> +	}
> +
> +	return;
> +}
> +
> +static void planar422p_to_rgb565(unsigned char const *in, unsigned char
> *out,
> +				 int width, int height, int rev_order)
> +{
> +	unsigned char *pY;
> +	unsigned char *pYEND;
> +	unsigned char *pCb;
> +	unsigned char *pCr;
> +	unsigned char Cr, Cb, Y, r, g, b;
> +	unsigned long k = 0;
> +	unsigned short rgbbytes;
> +	pY = (unsigned char *)in;
> +	pCb = (unsigned char *)in + (height * width);
> +	pCr = (unsigned char *)in + (height * width) + (height * width / 2);
> +	pYEND = pCb;
> +	while (pY < pYEND) {
> +		Y = *pY++;
> +		Cr = *pCr;
> +		Cb = *pCb;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		r = r >> 3;
> +		g = g >> 2;
> +		b = b >> 3;
> +		if (rev_order)
> +			rgbbytes = b + (g << 5) + (r << (5 + 6));
> +		else
> +			rgbbytes = r + (g << 5) + (b << (5 + 6));
> +		out[k++] = rgbbytes & 0xff;
> +		out[k++] = (rgbbytes >> 8) & 0xff;
> +		Y = *pY++;
> +		Cr = *pCr++;
> +		Cb = *pCb++;
> +		YCrCb2RGB(Y, Cr, Cb, &r, &g, &b);
> +		r = r >> 3;
> +		g = g >> 2;
> +		b = b >> 3;
> +		if (rev_order)
> +			rgbbytes = b + (g << 5) + (r << (5 + 6));
> +		else
> +			rgbbytes = r + (g << 5) + (b << (5 + 6));
> +		out[k++] = rgbbytes & 0xff;
> +		out[k++] = (rgbbytes >> 8) & 0xff;
> +	}
> +	return;
> +}
> +


Why do you do those conversions in kernelspace?
ffmpeg/libswscale has optimized code for colourspace conversions.
I know a few drivers do that in kernelspace but it's way more flexible
in userspace and depending on the optimization requires less CPU
power.

Markus

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15  1:17   ` Markus Rechberger
  (?)
@ 2008-05-15  2:41   ` Greg KH
  2008-05-15  3:12       ` Trent Piepho
  -1 siblings, 1 reply; 26+ messages in thread
From: Greg KH @ 2008-05-15  2:41 UTC (permalink / raw)
  To: Markus Rechberger
  Cc: mchehab, v4l-dvb-maintainer, video4linux-list, linux-usb,
	linux-kernel, dean

On Thu, May 15, 2008 at 03:17:42AM +0200, Markus Rechberger wrote:
> Hi Dean, Greg,

Adding dean to the cc: line...  :)
> 
> On 5/14/08, Greg KH <greg@kroah.com> wrote:
> > From: Dean Anderson <dean@sensoray.com>
> >

<big patch snipped>

> Why do you do those conversions in kernelspace?
> ffmpeg/libswscale has optimized code for colourspace conversions.
> I know a few drivers do that in kernelspace but it's way more flexible
> in userspace and depending on the optimization requires less CPU
> power.

I thought they were there as needed by some V4L1 applications, but that
code was recently removed by Dean I think.  If they don't need to be
there, and userspace apps can properly handle the different colorspace,
then I'll be glad to remove them.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15  2:41   ` Greg KH
@ 2008-05-15  3:12       ` Trent Piepho
  0 siblings, 0 replies; 26+ messages in thread
From: Trent Piepho @ 2008-05-15  3:12 UTC (permalink / raw)
  To: Greg KH
  Cc: Markus Rechberger, video4linux-list, linux-usb, dean,
	linux-kernel, mchehab, v4l-dvb-maintainer

On Wed, 14 May 2008, Greg KH wrote:
> On Thu, May 15, 2008 at 03:17:42AM +0200, Markus Rechberger wrote:
> > Hi Dean, Greg,
>
> Adding dean to the cc: line...  :)
> >
> > On 5/14/08, Greg KH <greg@kroah.com> wrote:
> > > From: Dean Anderson <dean@sensoray.com>
> > >
>
> <big patch snipped>
>
> > Why do you do those conversions in kernelspace?
> > ffmpeg/libswscale has optimized code for colourspace conversions.
> > I know a few drivers do that in kernelspace but it's way more flexible
> > in userspace and depending on the optimization requires less CPU
> > power.
>
> I thought they were there as needed by some V4L1 applications, but that
> code was recently removed by Dean I think.  If they don't need to be
> there, and userspace apps can properly handle the different colorspace,
> then I'll be glad to remove them.

Virtually all apps (V4L1 & 2) can handle YUV and RGB colorspaces.
Certainly all the major ones do and all the major libraries as well.

The problem is when the device only supports some vendor specific or
otherwise very uncommon format.  In that case not doing the conversion in
the kernel means the device won't work with any existing software without
patches.  In this case, while it's not "the right way", drivers often end
up including an in kernel conversion for pragmatic reasons.

This was a problem with the bayer format, but now userspace support for
that format is more common.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-15  3:12       ` Trent Piepho
  0 siblings, 0 replies; 26+ messages in thread
From: Trent Piepho @ 2008-05-15  3:12 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel, linux-usb, dean, video4linux-list, mchehab,
	v4l-dvb-maintainer

On Wed, 14 May 2008, Greg KH wrote:
> On Thu, May 15, 2008 at 03:17:42AM +0200, Markus Rechberger wrote:
> > Hi Dean, Greg,
>
> Adding dean to the cc: line...  :)
> >
> > On 5/14/08, Greg KH <greg@kroah.com> wrote:
> > > From: Dean Anderson <dean@sensoray.com>
> > >
>
> <big patch snipped>
>
> > Why do you do those conversions in kernelspace?
> > ffmpeg/libswscale has optimized code for colourspace conversions.
> > I know a few drivers do that in kernelspace but it's way more flexible
> > in userspace and depending on the optimization requires less CPU
> > power.
>
> I thought they were there as needed by some V4L1 applications, but that
> code was recently removed by Dean I think.  If they don't need to be
> there, and userspace apps can properly handle the different colorspace,
> then I'll be glad to remove them.

Virtually all apps (V4L1 & 2) can handle YUV and RGB colorspaces.
Certainly all the major ones do and all the major libraries as well.

The problem is when the device only supports some vendor specific or
otherwise very uncommon format.  In that case not doing the conversion in
the kernel means the device won't work with any existing software without
patches.  In this case, while it's not "the right way", drivers often end
up including an in kernel conversion for pragmatic reasons.

This was a problem with the bayer format, but now userspace support for
that format is more common.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-14 20:59 [PATCH] USB: add Sensoray 2255 v4l driver Greg KH
  2008-05-15  1:17   ` Markus Rechberger
@ 2008-05-15 11:38 ` Oliver Neukum
  2008-05-15 12:03   ` Oliver Neukum
  2008-05-16  2:51   ` Mauro Carvalho Chehab
  2 siblings, 1 reply; 26+ messages in thread
From: Oliver Neukum @ 2008-05-15 11:38 UTC (permalink / raw)
  To: Greg KH
  Cc: mchehab, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

Hi,

1. how about a *.h file?

2. You can inline these.

+static int norm_maxw(struct video_device *vdev)
+{
+       return (vdev->current_norm != V4L2_STD_PAL_B) ?
+           LINE_SZ_4CIFS_NTSC : LINE_SZ_4CIFS_PAL;
+}

3. The firmware stuff. That's an interesting solution. However:

a - if you don't need that delay, use a work queue

b - that's mean, use interruptible sleep

+               /* give 1 second for firmware to load in case
+                  driver loaded and then device immediately opened */
+               msleep(1000);

particularly you'd stall khubd processing an early unplug

c - obviously loading the firmware might have failed after waiting

+               if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
+                       err("2255 firmware loading stalled\n");
+                       mutex_unlock(&usb_s2255_open_mutex);
+                       return -EAGAIN;
+               }
+       }

you need a check for failure in an else branch

d - so you'll never release firmware in the error case unless you unplug

+       /* if first open after firmware loaded, release the firmware */
+       if (dev->fw_data->fw) {
+               release_firmware(dev->fw_data->fw);
+               dev->fw_data->fw = NULL;
+       }

e - you need to report errors

+static void s2255_timer(unsigned long user_data)
+{
+       struct complete_data *data = (struct complete_data *)user_data;
+       dprintk(100, "s2255 timer\n");
+       if (usb_submit_urb(data->fw_urb, GFP_ATOMIC) < 0) {
+               printk(KERN_ERR "s2255: can't submit urb\n");
+               if (data->fw) {
+                       release_firmware(data->fw);
+                       data->fw = NULL;
+               }
+               return;
+       }
+}

f - also here

+static void s2255_fwchunk_complete(struct urb *urb)
+{
+       struct complete_data *data = urb->context;
+       struct usb_device *udev = urb->dev;
+       int len;
+       dprintk(100, "udev %p urb %p", udev, urb);
+
+       if (urb->status) {
+               dev_err(&udev->dev, "URB failed with status %d", urb->status);
+               return;
+       }

4. as a rule, init all locks before you start a timer

+       mod_timer(&dev->timer, jiffies + HZ);
+       spin_lock_init(&dev->slock);

5. Unnecessary init

+static void s2255_disconnect(struct usb_interface *interface)
+{
+       struct s2255_dev *dev = NULL;

6. The initial firmware timer may still be ticking

+       if (dev->fw_data->fw_urb) {
+               dprintk(2, "kill URB\n");
+               usb_kill_urb(dev->fw_data->fw_urb);
+               usb_free_urb(dev->fw_data->fw_urb);

You need to delete that timer and kill the firmware urb after that.

7. That's not an error you want to return in that case. It may livelock

+       if (dev->users[cur_channel] > 1) {
+               dev->users[cur_channel]--;
+               dev_err(&dev->udev->dev, "one user at a time\n");
+               mutex_unlock(&usb_s2255_open_mutex);
+               return -EAGAIN;
+       }

8. Bogus check

+       if (data->fw_urb == NULL) {
+               dev_err(&udev->dev, "early disconncect\n");
+               return;
+       }

9. This can be computed directly

+       while (*size * *count > vid_limit * 1024 * 1024)
+               (*count)--;

10. This is fishy.

+static int res_locked(struct s2255_dev *dev, struct s2255_fh *fh)
+{
+       return (dev->resources[fh->channel]);
+}
+
+static void res_free(struct s2255_dev *dev, struct s2255_fh *fh)
+{
+       dev->resources[fh->channel] = 0;
+       dprintk(1, "res: put\n");
+}

In theory out of order memory access might return false positives.
Better use memory barriers or take the mutex.

11. Coding style

+       return (0);

12. This is close to obfuscated code

+       is_ntsc =
+           (dev->vdev[fh->channel]->current_norm != V4L2_STD_PAL_B) ? 1 : 0;

13. Coding style

+       if (ret < 0)
+               return (ret);

14. GFP_KERNEL in interrupt context

+
+       if (pipe_info->state != 0) {
+               if (usb_submit_urb(pipe_info->stream_urb, GFP_KERNEL)) {

15. Error handling in s2255_probe() is a gigantic resource leak

+       if (!dev->fw_data->fw_urb) {
+               dev_err(&interface->dev, "out of memory!\n");
+               goto error;
+       }

You must free what you allocate

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 11:38 ` Oliver Neukum
@ 2008-05-15 12:03   ` Oliver Neukum
  2008-05-15 18:44     ` Greg KH
  0 siblings, 1 reply; 26+ messages in thread
From: Oliver Neukum @ 2008-05-15 12:03 UTC (permalink / raw)
  To: Greg KH
  Cc: mchehab, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

Am Donnerstag 15 Mai 2008 13:38:37 schrieb Oliver Neukum:
> 3. The firmware stuff. That's an interesting solution. However:

Actually, on second thought, I take that back. It's a bad solution.
If you don't want to do it in probe(), the only other sensible place
is in open(). That way you can avoid the whole trouble if nobody
opens the device. And you need to handle the case of unloaded
firmware anyway, so you can trigger firmware load there.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15  3:12       ` Trent Piepho
  (?)
@ 2008-05-15 15:34       ` Dean Anderson
  2008-05-15 16:57           ` Markus Rechberger
  2008-05-16  2:59         ` Mauro Carvalho Chehab
  -1 siblings, 2 replies; 26+ messages in thread
From: Dean Anderson @ 2008-05-15 15:34 UTC (permalink / raw)
  To: Trent Piepho
  Cc: Greg KH, Markus Rechberger, video4linux-list, linux-usb,
	linux-kernel, mchehab, v4l-dvb-maintainer

Trent Piepho wrote:
> On Wed, 14 May 2008, Greg KH wrote:
>   
>> On Thu, May 15, 2008 at 03:17:42AM +0200, Markus Rechberger wrote:
>>     
>>> Hi Dean, Greg,
>>>       
>> Adding dean to the cc: line...  :)
>>     
>>> On 5/14/08, Greg KH <greg@kroah.com> wrote:
>>>       
>>>> From: Dean Anderson <dean@sensoray.com>
>>>>
>>>>         
>> <big patch snipped>
>>
>>     
>>> Why do you do those conversions in kernelspace?
>>> ffmpeg/libswscale has optimized code for colourspace conversions.
>>> I know a few drivers do that in kernelspace but it's way more flexible
>>> in userspace and depending on the optimization requires less CPU
>>> power.
>>>       
>> I thought they were there as needed by some V4L1 applications, but that
>> code was recently removed by Dean I think.  If they don't need to be
>> there, and userspace apps can properly handle the different colorspace,
>> then I'll be glad to remove them.
>>     
>
> Virtually all apps (V4L1 & 2) can handle YUV and RGB colorspaces.
> Certainly all the major ones do and all the major libraries as well.
>
> The problem is when the device only supports some vendor specific or
> otherwise very uncommon format.  In that case not doing the conversion in
> the kernel means the device won't work with any existing software without
> patches.  In this case, while it's not "the right way", drivers often end
> up including an in kernel conversion for pragmatic reasons.
>
> This was a problem with the bayer format, but now userspace support for
> that format is more common.
>   

I agree the conversions don't belong in a driver. For the record, the 
following are done in the 2255 hardware: V4L2_PIX_FMT_GREY and 
V4L2_PIX_FMT_YUV422P.

Since planar YUV formats such as V4L2_PIX_FMT_YUV422P are still not that 
well supported, is it possible to keep at least one packed YUV 
format(V4L2_PIX_FMT_YUYV) in the driver?  If not, let me know.  I will 
strongly suggest that the hardware Engineers add YUY2 or YUYV on board 
in the DSP firmware.  Thanks, Dean











^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 15:34       ` Dean Anderson
@ 2008-05-15 16:57           ` Markus Rechberger
  2008-05-16  2:59         ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 26+ messages in thread
From: Markus Rechberger @ 2008-05-15 16:57 UTC (permalink / raw)
  To: Dean Anderson
  Cc: Trent Piepho, Greg KH, video4linux-list, linux-usb, linux-kernel,
	mchehab, v4l-dvb-maintainer

On 5/15/08, Dean Anderson <dean@sensoray.com> wrote:
>>
>> Virtually all apps (V4L1 & 2) can handle YUV and RGB colorspaces.
>> Certainly all the major ones do and all the major libraries as well.
>>
>> The problem is when the device only supports some vendor specific or
>> otherwise very uncommon format.  In that case not doing the conversion in
>> the kernel means the device won't work with any existing software without
>> patches.  In this case, while it's not "the right way", drivers often end
>> up including an in kernel conversion for pragmatic reasons.
>>
>> This was a problem with the bayer format, but now userspace support for
>> that format is more common.
>>
>
> I agree the conversions don't belong in a driver. For the record, the
> following are done in the 2255 hardware: V4L2_PIX_FMT_GREY and
> V4L2_PIX_FMT_YUV422P.
>
> Since planar YUV formats such as V4L2_PIX_FMT_YUV422P are still not that
> well supported, is it possible to keep at least one packed YUV
> format(V4L2_PIX_FMT_YUYV) in the driver?  If not, let me know.  I will
> strongly suggest that the hardware Engineers add YUY2 or YUYV on board
> in the DSP firmware.  Thanks, Dean
>

Maybe it's better to fix up the corresponding application? If someone
wants to get those devices work he already either has to upgrade his
system or compile it manually at the moment.
With libswscale it's just a few lines of code to convert the formats
with a decent performance.
Seems like the demand of conversions is also growing for the future.

Markus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-15 16:57           ` Markus Rechberger
  0 siblings, 0 replies; 26+ messages in thread
From: Markus Rechberger @ 2008-05-15 16:57 UTC (permalink / raw)
  To: Dean Anderson
  Cc: video4linux-list, Greg KH, linux-usb, Trent Piepho, mchehab,
	v4l-dvb-maintainer, linux-kernel

On 5/15/08, Dean Anderson <dean@sensoray.com> wrote:
>>
>> Virtually all apps (V4L1 & 2) can handle YUV and RGB colorspaces.
>> Certainly all the major ones do and all the major libraries as well.
>>
>> The problem is when the device only supports some vendor specific or
>> otherwise very uncommon format.  In that case not doing the conversion in
>> the kernel means the device won't work with any existing software without
>> patches.  In this case, while it's not "the right way", drivers often end
>> up including an in kernel conversion for pragmatic reasons.
>>
>> This was a problem with the bayer format, but now userspace support for
>> that format is more common.
>>
>
> I agree the conversions don't belong in a driver. For the record, the
> following are done in the 2255 hardware: V4L2_PIX_FMT_GREY and
> V4L2_PIX_FMT_YUV422P.
>
> Since planar YUV formats such as V4L2_PIX_FMT_YUV422P are still not that
> well supported, is it possible to keep at least one packed YUV
> format(V4L2_PIX_FMT_YUYV) in the driver?  If not, let me know.  I will
> strongly suggest that the hardware Engineers add YUY2 or YUYV on board
> in the DSP firmware.  Thanks, Dean
>

Maybe it's better to fix up the corresponding application? If someone
wants to get those devices work he already either has to upgrade his
system or compile it manually at the moment.
With libswscale it's just a few lines of code to convert the formats
with a decent performance.
Seems like the demand of conversions is also growing for the future.

Markus

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 12:03   ` Oliver Neukum
@ 2008-05-15 18:44     ` Greg KH
  2008-05-15 19:54       ` Oliver Neukum
  0 siblings, 1 reply; 26+ messages in thread
From: Greg KH @ 2008-05-15 18:44 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: mchehab, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

On Thu, May 15, 2008 at 02:03:18PM +0200, Oliver Neukum wrote:
> Am Donnerstag 15 Mai 2008 13:38:37 schrieb Oliver Neukum:
> > 3. The firmware stuff. That's an interesting solution. However:
> 
> Actually, on second thought, I take that back. It's a bad solution.
> If you don't want to do it in probe(), the only other sensible place
> is in open(). That way you can avoid the whole trouble if nobody
> opens the device. And you need to handle the case of unloaded
> firmware anyway, so you can trigger firmware load there.

No, we want to do firmware load on probe, I'll change it to be async so
that probe can continue on.  Need to see if I can disconnect the driver
from device after probe succeeds in case the firmware fails.

thanks for the review, I appreciate it and will fix them up later today
or tomorrow.

greg k-h

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 18:44     ` Greg KH
@ 2008-05-15 19:54       ` Oliver Neukum
  2008-05-15 20:10         ` Greg KH
  0 siblings, 1 reply; 26+ messages in thread
From: Oliver Neukum @ 2008-05-15 19:54 UTC (permalink / raw)
  To: Greg KH
  Cc: mchehab, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

Am Donnerstag 15 Mai 2008 20:44:24 schrieb Greg KH:
> On Thu, May 15, 2008 at 02:03:18PM +0200, Oliver Neukum wrote:
> > Am Donnerstag 15 Mai 2008 13:38:37 schrieb Oliver Neukum:
> > > 3. The firmware stuff. That's an interesting solution. However:
> > 
> > Actually, on second thought, I take that back. It's a bad solution.
> > If you don't want to do it in probe(), the only other sensible place
> > is in open(). That way you can avoid the whole trouble if nobody
> > opens the device. And you need to handle the case of unloaded
> > firmware anyway, so you can trigger firmware load there.
> 
> No, we want to do firmware load on probe, I'll change it to be async so

Could you state your reasons for that preference?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 19:54       ` Oliver Neukum
@ 2008-05-15 20:10         ` Greg KH
  2008-05-15 20:13           ` Oliver Neukum
  0 siblings, 1 reply; 26+ messages in thread
From: Greg KH @ 2008-05-15 20:10 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: mchehab, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

On Thu, May 15, 2008 at 09:54:12PM +0200, Oliver Neukum wrote:
> Am Donnerstag 15 Mai 2008 20:44:24 schrieb Greg KH:
> > On Thu, May 15, 2008 at 02:03:18PM +0200, Oliver Neukum wrote:
> > > Am Donnerstag 15 Mai 2008 13:38:37 schrieb Oliver Neukum:
> > > > 3. The firmware stuff. That's an interesting solution. However:
> > > 
> > > Actually, on second thought, I take that back. It's a bad solution.
> > > If you don't want to do it in probe(), the only other sensible place
> > > is in open(). That way you can avoid the whole trouble if nobody
> > > opens the device. And you need to handle the case of unloaded
> > > firmware anyway, so you can trigger firmware load there.
> > 
> > No, we want to do firmware load on probe, I'll change it to be async so
> 
> Could you state your reasons for that preference?

I don't want to create the device nodes, and have userspace think the
device really is working, only to have everything fall down and die if
open() is called and the firmware isn't present.  I'd rather not create
the video devices if we know this isn't going to work at all.

Just trying to be nicer to users who would get very confused, "but wait,
the device nodes are there, and the application finds the device, why is
it not working properly?"

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 20:10         ` Greg KH
@ 2008-05-15 20:13           ` Oliver Neukum
  0 siblings, 0 replies; 26+ messages in thread
From: Oliver Neukum @ 2008-05-15 20:13 UTC (permalink / raw)
  To: Greg KH
  Cc: mchehab, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

Am Donnerstag 15 Mai 2008 22:10:49 schrieb Greg KH:
> On Thu, May 15, 2008 at 09:54:12PM +0200, Oliver Neukum wrote:
> > Am Donnerstag 15 Mai 2008 20:44:24 schrieb Greg KH:
> > > On Thu, May 15, 2008 at 02:03:18PM +0200, Oliver Neukum wrote:
> > > > Am Donnerstag 15 Mai 2008 13:38:37 schrieb Oliver Neukum:
> > > > > 3. The firmware stuff. That's an interesting solution. However:
> > > > 
> > > > Actually, on second thought, I take that back. It's a bad solution.
> > > > If you don't want to do it in probe(), the only other sensible place
> > > > is in open(). That way you can avoid the whole trouble if nobody
> > > > opens the device. And you need to handle the case of unloaded
> > > > firmware anyway, so you can trigger firmware load there.
> > > 
> > > No, we want to do firmware load on probe, I'll change it to be async so
> > 
> > Could you state your reasons for that preference?
> 
> I don't want to create the device nodes, and have userspace think the
> device really is working, only to have everything fall down and die if
> open() is called and the firmware isn't present.  I'd rather not create
> the video devices if we know this isn't going to work at all.

Yes, that makes sense. However, then you might want to look into
spawning a thread per bus rather than a delayed failure mechanism.

	Regards
		Oliver



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-14 20:59 [PATCH] USB: add Sensoray 2255 v4l driver Greg KH
@ 2008-05-16  2:51   ` Mauro Carvalho Chehab
  2008-05-15 11:38 ` Oliver Neukum
  2008-05-16  2:51   ` Mauro Carvalho Chehab
  2 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16  2:51 UTC (permalink / raw)
  To: Greg KH
  Cc: v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list,
	Dean Anderson

Hi Greg and Dean,

In general, the driver seems sane and almost ready for their kernel inclusion.
However, there were several videobuf changes affecting mostly the users of
videobuf-vmalloc during the last kernel development cycle, to fix lock issues.
The code should be reviewed at the light of the latest version, and tested. The
good news is that the videobuf handler will be simpler, but a lock is now required.

I have other comments to help improving the driver quality.

Btw, I noticed the lack of Dean's SOB. Is this intentional?

Cheers,
Mauro.

On Wed, 14 May 2008 13:59:27 -0700
Greg KH <greg@kroah.com> wrote:

> From: Dean Anderson <dean@sensoray.com>
> 
> This driver adds support for the Sensoray 2255 devices.
> 
> It was primarily developed by Dean Anderson with only a little bit of
> guidance and cleanup by Greg.

> +#define DIR_IN			0
> +#define DIR_OUT			1

It seems a little dangerous to use just DIR_IN/DIR_OUT, since a future change
on a kernel header could cause namespace conflict. IMO, the better is to rename
those macros to something more specific, like S2255_DIR_IN.

The same comment is also true to struct and other namespace definitions, like
"framei", "bufferi", etc.

> +
> +/* buffer for one video frame */
> +struct s2255_buffer {
> +	/* common v4l buffer stuff -- must be first */
> +	struct videobuf_buffer vb;
> +	const struct s2255_fmt *fmt;
> +	/* future use */
> +	int reserved[32];

Why to reserve 32 integers here? While this may have some sense at userspace
API, I can't see any reason to use this internally.

> +#define S2255_NORMS		(V4L2_STD_PAL_B | V4L2_STD_NTSC_M)

Just those two norms? Are you sure it doesn't support the other european PAL
variants (PAL/G, PAL/D, PAL/K)?

> +static DEFINE_MUTEX(usb_s2255_open_mutex);

Hmmm... what happens if the user plugs two or more of such devices?

> +/* supported controls */
> +static struct v4l2_queryctrl s2255_qctrl[] = {
> +	{
> +	.id = V4L2_CID_BRIGHTNESS,
> +	.type = V4L2_CTRL_TYPE_INTEGER,
> +	.name = "Brightness",
> +	.minimum = -127,
> +	.maximum = 128,
> +	.step = 1,
> +	.default_value = 0,
> +	.flags = 0,
> +	},
> +	{

This seems to be violating CodingStyle. It should be something like:
	{
		.id = foo,
<snip/>
	}, {

The same applies also to other structs. The better is to check the patch with
checkpatch.pl, since other violations might be present.

> +
> +/*
> + * convert from YUV(YCrCb) to RGB
> + * 65536 R = 76533(Y-16) + 104936 * (Cr-128)
> + * 65536 G = 76533(Y-16) - 53451(Cr-128) - 25703(Cb -128)
> + * 65536 B = 76533(Y-16) + 132677(Cb-128)
> + */

As already discussed, the driver should support only the video standards
supported by the hardware. colorspace conversion can happen at userspace apps.

> +/* this loads the firmware asynchronously.
> +   Originally this was done synchroously in probe.
> +   But it is better to load it asynchronously here than block
> +   inside the probe function. Blocking inside probe affects boot time.
> +   FW loading is triggered by the timer in the probe function
> +*/

I saw another discussion about this.

I like the idea of loading firmware asynchronously, provided that you block at
open() or at ioctl(), if the user requested to open before having the firmware
loaded.

> +static int restart_video_queue(struct s2255_dmaqueue *dma_q)

This is probably obsolete. We've cleaned up those code. The better is to take a
look at videobuf implementation at em28xx-video, where some lock bugs were fixed and the
code were simplified. I don't have your hardware to test, but I suspect that
the driver suffers some bugs at videobuf implementation that we've removed recently.

> +/* ------------------------------------------------------------------
> +   IOCTL vidioc handling
> +   ------------------------------------------------------------------*/
> +static int vidioc_querycap(struct file *file, void *priv,
> +			   struct v4l2_capability *cap)
> +{
> +	strcpy(cap->driver, "s2255");
> +	strcpy(cap->card, "s2255");
> +	cap->version = S2255_VERSION;
> +	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	return 0;
> +}

Better to use "strlcpy" instead of just "strcpy", to avoid the risk of buffer
overflow. Ok, currently, there's no risk, but, if tomorrow someone changes
something here, the bug will emerge.

Also, please fill bus info, with something like:

	strlcpy(cap->bus_info, dev->udev->dev.bus_id, sizeof(cap->bus_info));

This is required for some udev implementations to properly work.

> +/* FIXME: This seems to be generic enough to be at videodev2 */
> +static int vidioc_s_fmt_cap(struct file *file, void *priv,
> +			    struct v4l2_format *f)

The implementation you did is not generic enough for videodev2. I agree that we
may improve the implementation of try_fmt/s_fmt at videobuf2. Feel free to
propose such improvements.
> +#ifdef CONFIG_VIDEO_V4L1_COMPAT
> +static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)

There are now a videobuf method to implement vidiocgmbuf. The better is to use
it, instead of duplicating the code.

> +#define EP_NUM_CONFIG 2

This define here sounds strange. I would move it to the beginning at the
driver. Better to write a short comment about this magic number.

> +	s2255_stop_acquire(dev, fh->channel);
> +	/* give time for last frame to be received/flush in the usb
> +	   receive pipe*/
> +	msleep(50);

The sleep above seems a hack to my eyes. The better is to protect with a
spinlock. The current code, at vivi and em28xx already prevents such issues (at
least, it went fine at the tests we did).

> +	res = videobuf_streamoff(&fh->vb_vidq);
> +	res_free(dev, fh);
> +	/* expire the queue timer now in case still active */
> +	mod_timer(&dmaq->timeout, jiffies + HZ);

Also, the timeout is obsolete

> +	return res;
> +}
> +	if (*i == V4L2_STD_NTSC_M) {
> +		dprintk(4, "vidioc_s_std NTSC\n");
> +		mode->format = FORMAT_NTSC;

No. NTSC/M is just one variant of NTSC, used on US/Canada. There are other
variants at Asia, in Korea and Japan. The difference is at the audio carrier.

> +	} else if (*i == V4L2_STD_PAL_B) {
> +		dprintk(4, "vidioc_s_std PAL\n");

Also, PAL/B is just one variant. 

A proper code would be to do this:
	if (*i & V4L2_STD_PAL)

(notice that the equal operator were replaced by the logical operation _AND_.
This covers all european PAL. However, this mask doesn't cover PAL/N, PAL/Nc
and PAL/M.

If all you need is to check if the video mode is 50Hz/60Hz, the proper code would be, instead:

	if (*i & V4L2_STD_525_60)
		/* some logic for NTSC/M, PAL/M and PAL/60 */
	else
		/* some logic for the other PAL variations and SECAM */

> +/* Sensoray 2255 is a multiple channel capture device.
> +   It does not have a "crossbar" of inputs.
> +   We use one V4L device per channel. The user must
> +   be aware that certain combinations are not allowed.
> +   For instance, you cannot do full FPS on more than 2 channels(2 videodevs)
> +   at once in color(you can do full fps on 4 channels with greyscale.
> +*/

The invalid combinations should be tested, to avoid OOPSes and other bad behaviours.

> +			/* update the mode to the corresponding value */
> +			if (ctrl->id == V4L2_CID_BRIGHTNESS)
> +				mode->bright = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_CONTRAST)
> +				mode->contrast = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_HUE)
> +				mode->hue = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_SATURATION)
> +				mode->saturation = ctrl->value;

You have just a few controls. However, the better would be to use a switch(),
since the compiler can reorder the operations to improve the code speed and
size.

> +	if (dev->fw_data->fw_state == FWSTATE_FAILED) {
> +		err("2255 firmware wasn't loaded\n");
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -ENODEV;
> +	}

It seems too hard to return ENODEV. Couldn't the driver try again to load the
firmware here?

> +	if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
> +		/* give 1 second for firmware to load in case
> +		   driver loaded and then device immediately opened */
> +		msleep(1000);

Argh. I would use a wait_event_timeout() instead, and use a higher timeout value. 

Also, it seems that fw_state needs to be atomic.

> +		if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
> +			err("2255 firmware loading stalled\n");
> +			mutex_unlock(&usb_s2255_open_mutex);
> +			return -EAGAIN;

Hmm... I'm in doubt if we should return, instead, -EBUSY.

> +	/* if first open after firmware loaded, release the firmware */
> +	if (dev->fw_data->fw) {
> +		release_firmware(dev->fw_data->fw);
> +		dev->fw_data->fw = NULL;
> +	}

This seems weird. Why to release the firmware?

> +
> +	dev->users[cur_channel]++;
> +
> +	if (dev->users[cur_channel] > 1) {
> +		dev->users[cur_channel]--;
> +		dev_err(&dev->udev->dev, "one user at a time\n");
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -EAGAIN;

In this case, it should return -EBUSY.

> +	}
> +
> +	dprintk(1, "open minor=%d type=%s users=%d\n",
> +		minor, v4l2_type_names[type], dev->users[cur_channel]);
> +
> +	/* allocate + initialize per filehandle data */
> +	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> +	if (NULL == fh) {
> +		dev->users[cur_channel]--;
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -ENOMEM;
> +	}


Instead of incrementing the usage, printing the open message just to discover
that you don't have enough memory, I would move the users increment and the
dprintk to happen after the allocation of fh.

> +	videobuf_queue_vmalloc_init(&fh->vb_vidq, &s2255_video_qops,
> +				    NULL, NULL,
> +				    fh->type,
> +				    V4L2_FIELD_INTERLACED,
> +				    sizeof(struct s2255_buffer), fh);

You'll need to define a spinlock for videoqueue, otherwise the kernel will now
complain. Please test your code against the latest development version (or the
latest code at mainstream).

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-16  2:51   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16  2:51 UTC (permalink / raw)
  To: Greg KH
  Cc: video4linux-list, v4l-dvb-maintainer, linux-usb, Dean Anderson,
	linux-kernel

Hi Greg and Dean,

In general, the driver seems sane and almost ready for their kernel inclusion.
However, there were several videobuf changes affecting mostly the users of
videobuf-vmalloc during the last kernel development cycle, to fix lock issues.
The code should be reviewed at the light of the latest version, and tested. The
good news is that the videobuf handler will be simpler, but a lock is now required.

I have other comments to help improving the driver quality.

Btw, I noticed the lack of Dean's SOB. Is this intentional?

Cheers,
Mauro.

On Wed, 14 May 2008 13:59:27 -0700
Greg KH <greg@kroah.com> wrote:

> From: Dean Anderson <dean@sensoray.com>
> 
> This driver adds support for the Sensoray 2255 devices.
> 
> It was primarily developed by Dean Anderson with only a little bit of
> guidance and cleanup by Greg.

> +#define DIR_IN			0
> +#define DIR_OUT			1

It seems a little dangerous to use just DIR_IN/DIR_OUT, since a future change
on a kernel header could cause namespace conflict. IMO, the better is to rename
those macros to something more specific, like S2255_DIR_IN.

The same comment is also true to struct and other namespace definitions, like
"framei", "bufferi", etc.

> +
> +/* buffer for one video frame */
> +struct s2255_buffer {
> +	/* common v4l buffer stuff -- must be first */
> +	struct videobuf_buffer vb;
> +	const struct s2255_fmt *fmt;
> +	/* future use */
> +	int reserved[32];

Why to reserve 32 integers here? While this may have some sense at userspace
API, I can't see any reason to use this internally.

> +#define S2255_NORMS		(V4L2_STD_PAL_B | V4L2_STD_NTSC_M)

Just those two norms? Are you sure it doesn't support the other european PAL
variants (PAL/G, PAL/D, PAL/K)?

> +static DEFINE_MUTEX(usb_s2255_open_mutex);

Hmmm... what happens if the user plugs two or more of such devices?

> +/* supported controls */
> +static struct v4l2_queryctrl s2255_qctrl[] = {
> +	{
> +	.id = V4L2_CID_BRIGHTNESS,
> +	.type = V4L2_CTRL_TYPE_INTEGER,
> +	.name = "Brightness",
> +	.minimum = -127,
> +	.maximum = 128,
> +	.step = 1,
> +	.default_value = 0,
> +	.flags = 0,
> +	},
> +	{

This seems to be violating CodingStyle. It should be something like:
	{
		.id = foo,
<snip/>
	}, {

The same applies also to other structs. The better is to check the patch with
checkpatch.pl, since other violations might be present.

> +
> +/*
> + * convert from YUV(YCrCb) to RGB
> + * 65536 R = 76533(Y-16) + 104936 * (Cr-128)
> + * 65536 G = 76533(Y-16) - 53451(Cr-128) - 25703(Cb -128)
> + * 65536 B = 76533(Y-16) + 132677(Cb-128)
> + */

As already discussed, the driver should support only the video standards
supported by the hardware. colorspace conversion can happen at userspace apps.

> +/* this loads the firmware asynchronously.
> +   Originally this was done synchroously in probe.
> +   But it is better to load it asynchronously here than block
> +   inside the probe function. Blocking inside probe affects boot time.
> +   FW loading is triggered by the timer in the probe function
> +*/

I saw another discussion about this.

I like the idea of loading firmware asynchronously, provided that you block at
open() or at ioctl(), if the user requested to open before having the firmware
loaded.

> +static int restart_video_queue(struct s2255_dmaqueue *dma_q)

This is probably obsolete. We've cleaned up those code. The better is to take a
look at videobuf implementation at em28xx-video, where some lock bugs were fixed and the
code were simplified. I don't have your hardware to test, but I suspect that
the driver suffers some bugs at videobuf implementation that we've removed recently.

> +/* ------------------------------------------------------------------
> +   IOCTL vidioc handling
> +   ------------------------------------------------------------------*/
> +static int vidioc_querycap(struct file *file, void *priv,
> +			   struct v4l2_capability *cap)
> +{
> +	strcpy(cap->driver, "s2255");
> +	strcpy(cap->card, "s2255");
> +	cap->version = S2255_VERSION;
> +	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	return 0;
> +}

Better to use "strlcpy" instead of just "strcpy", to avoid the risk of buffer
overflow. Ok, currently, there's no risk, but, if tomorrow someone changes
something here, the bug will emerge.

Also, please fill bus info, with something like:

	strlcpy(cap->bus_info, dev->udev->dev.bus_id, sizeof(cap->bus_info));

This is required for some udev implementations to properly work.

> +/* FIXME: This seems to be generic enough to be at videodev2 */
> +static int vidioc_s_fmt_cap(struct file *file, void *priv,
> +			    struct v4l2_format *f)

The implementation you did is not generic enough for videodev2. I agree that we
may improve the implementation of try_fmt/s_fmt at videobuf2. Feel free to
propose such improvements.
> +#ifdef CONFIG_VIDEO_V4L1_COMPAT
> +static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)

There are now a videobuf method to implement vidiocgmbuf. The better is to use
it, instead of duplicating the code.

> +#define EP_NUM_CONFIG 2

This define here sounds strange. I would move it to the beginning at the
driver. Better to write a short comment about this magic number.

> +	s2255_stop_acquire(dev, fh->channel);
> +	/* give time for last frame to be received/flush in the usb
> +	   receive pipe*/
> +	msleep(50);

The sleep above seems a hack to my eyes. The better is to protect with a
spinlock. The current code, at vivi and em28xx already prevents such issues (at
least, it went fine at the tests we did).

> +	res = videobuf_streamoff(&fh->vb_vidq);
> +	res_free(dev, fh);
> +	/* expire the queue timer now in case still active */
> +	mod_timer(&dmaq->timeout, jiffies + HZ);

Also, the timeout is obsolete

> +	return res;
> +}
> +	if (*i == V4L2_STD_NTSC_M) {
> +		dprintk(4, "vidioc_s_std NTSC\n");
> +		mode->format = FORMAT_NTSC;

No. NTSC/M is just one variant of NTSC, used on US/Canada. There are other
variants at Asia, in Korea and Japan. The difference is at the audio carrier.

> +	} else if (*i == V4L2_STD_PAL_B) {
> +		dprintk(4, "vidioc_s_std PAL\n");

Also, PAL/B is just one variant. 

A proper code would be to do this:
	if (*i & V4L2_STD_PAL)

(notice that the equal operator were replaced by the logical operation _AND_.
This covers all european PAL. However, this mask doesn't cover PAL/N, PAL/Nc
and PAL/M.

If all you need is to check if the video mode is 50Hz/60Hz, the proper code would be, instead:

	if (*i & V4L2_STD_525_60)
		/* some logic for NTSC/M, PAL/M and PAL/60 */
	else
		/* some logic for the other PAL variations and SECAM */

> +/* Sensoray 2255 is a multiple channel capture device.
> +   It does not have a "crossbar" of inputs.
> +   We use one V4L device per channel. The user must
> +   be aware that certain combinations are not allowed.
> +   For instance, you cannot do full FPS on more than 2 channels(2 videodevs)
> +   at once in color(you can do full fps on 4 channels with greyscale.
> +*/

The invalid combinations should be tested, to avoid OOPSes and other bad behaviours.

> +			/* update the mode to the corresponding value */
> +			if (ctrl->id == V4L2_CID_BRIGHTNESS)
> +				mode->bright = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_CONTRAST)
> +				mode->contrast = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_HUE)
> +				mode->hue = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_SATURATION)
> +				mode->saturation = ctrl->value;

You have just a few controls. However, the better would be to use a switch(),
since the compiler can reorder the operations to improve the code speed and
size.

> +	if (dev->fw_data->fw_state == FWSTATE_FAILED) {
> +		err("2255 firmware wasn't loaded\n");
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -ENODEV;
> +	}

It seems too hard to return ENODEV. Couldn't the driver try again to load the
firmware here?

> +	if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
> +		/* give 1 second for firmware to load in case
> +		   driver loaded and then device immediately opened */
> +		msleep(1000);

Argh. I would use a wait_event_timeout() instead, and use a higher timeout value. 

Also, it seems that fw_state needs to be atomic.

> +		if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
> +			err("2255 firmware loading stalled\n");
> +			mutex_unlock(&usb_s2255_open_mutex);
> +			return -EAGAIN;

Hmm... I'm in doubt if we should return, instead, -EBUSY.

> +	/* if first open after firmware loaded, release the firmware */
> +	if (dev->fw_data->fw) {
> +		release_firmware(dev->fw_data->fw);
> +		dev->fw_data->fw = NULL;
> +	}

This seems weird. Why to release the firmware?

> +
> +	dev->users[cur_channel]++;
> +
> +	if (dev->users[cur_channel] > 1) {
> +		dev->users[cur_channel]--;
> +		dev_err(&dev->udev->dev, "one user at a time\n");
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -EAGAIN;

In this case, it should return -EBUSY.

> +	}
> +
> +	dprintk(1, "open minor=%d type=%s users=%d\n",
> +		minor, v4l2_type_names[type], dev->users[cur_channel]);
> +
> +	/* allocate + initialize per filehandle data */
> +	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> +	if (NULL == fh) {
> +		dev->users[cur_channel]--;
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -ENOMEM;
> +	}


Instead of incrementing the usage, printing the open message just to discover
that you don't have enough memory, I would move the users increment and the
dprintk to happen after the allocation of fh.

> +	videobuf_queue_vmalloc_init(&fh->vb_vidq, &s2255_video_qops,
> +				    NULL, NULL,
> +				    fh->type,
> +				    V4L2_FIELD_INTERLACED,
> +				    sizeof(struct s2255_buffer), fh);

You'll need to define a spinlock for videoqueue, otherwise the kernel will now
complain. Please test your code against the latest development version (or the
latest code at mainstream).

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [v4l-dvb-maintainer] [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-15 15:34       ` Dean Anderson
  2008-05-15 16:57           ` Markus Rechberger
@ 2008-05-16  2:59         ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16  2:59 UTC (permalink / raw)
  To: Dean Anderson
  Cc: Trent Piepho, Greg KH, Markus Rechberger, video4linux-list,
	linux-usb, linux-kernel, v4l-dvb-maintainer


> Since planar YUV formats such as V4L2_PIX_FMT_YUV422P are still not that 
> well supported, is it possible to keep at least one packed YUV 
> format(V4L2_PIX_FMT_YUYV) in the driver?  If not, let me know.  I will 
> strongly suggest that the hardware Engineers add YUY2 or YUYV on board 
> in the DSP firmware.  Thanks, Dean

The better is to have YUY2 and YUYV formats at the firmware and/or hardware.
Those are the more common formats, and userspace apps generally try one of
those first. Also, as it is common on several devices, the userspace handlers
are more tested.

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-16  2:51   ` Mauro Carvalho Chehab
  (?)
@ 2008-05-16  6:28   ` Oliver Neukum
  2008-05-16 15:57     ` dean
  -1 siblings, 1 reply; 26+ messages in thread
From: Oliver Neukum @ 2008-05-16  6:28 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Greg KH, v4l-dvb-maintainer, linux-usb, linux-kernel,
	video4linux-list, Dean Anderson

Am Freitag 16 Mai 2008 04:51:02 schrieb Mauro Carvalho Chehab:
> > +static DEFINE_MUTEX(usb_s2255_open_mutex);
> 
> Hmmm... what happens if the user plugs two or more of such devices?

Concurrent calls to open() will be delayed. Not a problem, if the
firmware issue is fixed as it needs to be.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-16  2:51   ` Mauro Carvalho Chehab
  (?)
  (?)
@ 2008-05-16 14:53   ` dean
  2008-05-16 15:34       ` Mauro Carvalho Chehab
  -1 siblings, 1 reply; 26+ messages in thread
From: dean @ 2008-05-16 14:53 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Greg KH, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

>Btw, I noticed the lack of Dean's SOB. Is this intentional?


It's not intentional, I can sign off on it.  The suggestions look good.

I have a few other questions.  First, is Video for Linux version 1 going 
to be obsoleted soon?  Do the V4L1 compatibility routines still work in 
the latest driver?  I had problems running the VIVI (virtual video 
driver) driver with VideoLan/VLC 0.8.6a-f, but it worked with VLC 9.0 
with the new V4L2 interface.

"videodev: "s2255v" has no release callback. Please fix your driver for 
proper sysfs support, see http://lwn.net/Articles/36850/"

Should we get rid of the warning message above? It's also been present 
in VIVI for quite a few kernel releases.




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-16 14:53   ` dean
@ 2008-05-16 15:34       ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16 15:34 UTC (permalink / raw)
  To: dean
  Cc: Greg KH, v4l-dvb-maintainer, linux-usb, linux-kernel, video4linux-list

On Fri, 16 May 2008 07:53:46 -0700
dean <dean@sensoray.com> wrote:

> >Btw, I noticed the lack of Dean's SOB. Is this intentional?  
> It's not intentional, I can sign off on it. 

Thanks. Please send your SOB at the next version.

> I have a few other questions.  First, is Video for Linux version 1 going 
> to be obsoleted soon?

We intend to, but people are currently lacking time to port old drivers to V4L2.

>  Do the V4L1 compatibility routines still work in 
> the latest driver?

V4L1 compat will still be kept for some time after the end of V4L1 drivers.

>  I had problems running the VIVI (virtual video 
> driver) driver with VideoLan/VLC 0.8.6a-f, but it worked with VLC 9.0 
> with the new V4L2 interface.

VLC V4L1 implementation were broken. It first starts DMA and streaming, then,
it calls some ioctls that changes the buffer size. The compat handler doesn't
accept this behaviour, since it would cause buffer overflow. AFAIK, only bttv
driver used to support this behaviour. On V4L1 mode, bttv were allocating
enough memory for the maximum resolution. So, subsequent buffer changes works
properly. 

It would be valuable if you could work on a safe way to implement backward
compat for this broken behaviour. In this case, you would need to change the
compat implementation at videobuf, and let v4l1-compat module to be aware that
it is safe to allow buffer size changes.

Yet, this seems to much work for something that should be already removed from
kernel (V4L1).

> "videodev: "s2255v" has no release callback. Please fix your driver for 
> proper sysfs support, see http://lwn.net/Articles/36850/"

> Should we get rid of the warning message above? It's also been present 
> in VIVI for quite a few kernel releases.

The message doesn't cause any harm, but the better is to fix this also. This
were already corrected at the latest vivi versions.

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-16 15:34       ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16 15:34 UTC (permalink / raw)
  To: dean
  Cc: Greg KH, v4l-dvb-maintainer, linux-usb, video4linux-list, linux-kernel

On Fri, 16 May 2008 07:53:46 -0700
dean <dean@sensoray.com> wrote:

> >Btw, I noticed the lack of Dean's SOB. Is this intentional?  
> It's not intentional, I can sign off on it. 

Thanks. Please send your SOB at the next version.

> I have a few other questions.  First, is Video for Linux version 1 going 
> to be obsoleted soon?

We intend to, but people are currently lacking time to port old drivers to V4L2.

>  Do the V4L1 compatibility routines still work in 
> the latest driver?

V4L1 compat will still be kept for some time after the end of V4L1 drivers.

>  I had problems running the VIVI (virtual video 
> driver) driver with VideoLan/VLC 0.8.6a-f, but it worked with VLC 9.0 
> with the new V4L2 interface.

VLC V4L1 implementation were broken. It first starts DMA and streaming, then,
it calls some ioctls that changes the buffer size. The compat handler doesn't
accept this behaviour, since it would cause buffer overflow. AFAIK, only bttv
driver used to support this behaviour. On V4L1 mode, bttv were allocating
enough memory for the maximum resolution. So, subsequent buffer changes works
properly. 

It would be valuable if you could work on a safe way to implement backward
compat for this broken behaviour. In this case, you would need to change the
compat implementation at videobuf, and let v4l1-compat module to be aware that
it is safe to allow buffer size changes.

Yet, this seems to much work for something that should be already removed from
kernel (V4L1).

> "videodev: "s2255v" has no release callback. Please fix your driver for 
> proper sysfs support, see http://lwn.net/Articles/36850/"

> Should we get rid of the warning message above? It's also been present 
> in VIVI for quite a few kernel releases.

The message doesn't cause any harm, but the better is to fix this also. This
were already corrected at the latest vivi versions.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-16  6:28   ` Oliver Neukum
@ 2008-05-16 15:57     ` dean
  2008-05-16 16:04       ` Oliver Neukum
  2008-05-16 18:17         ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 26+ messages in thread
From: dean @ 2008-05-16 15:57 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Mauro Carvalho Chehab, Greg KH, v4l-dvb-maintainer, linux-usb,
	linux-kernel, video4linux-list

Hi Oliver,

Just to be clear.  You mean fixing the loading issues(ugly msleep, 
etc...) that Mauro mentioned, not the YUV formats(which seem unrelated)? 

Would it be ok to have just 422P and greyscale in the driver for now if 
we can't get a firmware update in a reasonable timeframe? It's certainly 
not ideal, but we could direct our customers to the libraries, at least 
until we get a proper HW fix.

Best regards,

Dean


Oliver Neukum wrote:
> Am Freitag 16 Mai 2008 04:51:02 schrieb Mauro Carvalho Chehab:
>   
>>> +static DEFINE_MUTEX(usb_s2255_open_mutex);
>>>       
>> Hmmm... what happens if the user plugs two or more of such devices?
>>     
>
> Concurrent calls to open() will be delayed. Not a problem, if the
> firmware issue is fixed as it needs to be.
>
> 	Regards
> 		Oliver
>
>
>   


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-16 15:57     ` dean
@ 2008-05-16 16:04       ` Oliver Neukum
  2008-05-16 18:17         ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 26+ messages in thread
From: Oliver Neukum @ 2008-05-16 16:04 UTC (permalink / raw)
  To: dean
  Cc: Mauro Carvalho Chehab, Greg KH, v4l-dvb-maintainer, linux-usb,
	linux-kernel, video4linux-list

Am Freitag 16 Mai 2008 17:57:15 schrieb dean:
> Hi Oliver,
> 
> Just to be clear.  You mean fixing the loading issues(ugly msleep,

Yes.
 
> etc...) that Mauro mentioned, not the YUV formats(which seem unrelated)? 
> 
> Would it be ok to have just 422P and greyscale in the driver for now if 
> we can't get a firmware update in a reasonable timeframe? It's certainly 
> not ideal, but we could direct our customers to the libraries, at least 
> until we get a proper HW fix.

I am incompetent to answer that.

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
  2008-05-16 15:57     ` dean
@ 2008-05-16 18:17         ` Mauro Carvalho Chehab
  2008-05-16 18:17         ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16 18:17 UTC (permalink / raw)
  To: dean
  Cc: Oliver Neukum, Greg KH, v4l-dvb-maintainer, linux-usb,
	linux-kernel, video4linux-list

Dean,

On Fri, 16 May 2008 08:57:15 -0700
dean <dean@sensoray.com> wrote:

> Hi Oliver,
> 
> Just to be clear.  You mean fixing the loading issues(ugly msleep, 
> etc...) that Mauro mentioned, not the YUV formats(which seem unrelated)? 
> 
> Would it be ok to have just 422P and greyscale in the driver for now if 
> we can't get a firmware update in a reasonable timeframe? It's certainly 
> not ideal, but we could direct our customers to the libraries, at least 
> until we get a proper HW fix.

Feature regression is something that shouldn't happen. If we add the driver
with a color conversion inside, we should somehow keep this feature available
on newer versions. That's said, I don't see any issue if the later versions do
this implementation inside hardware/firmware, since the end result is that the
driver will keep supporting those standards.

>From my side, I'm ok on having this color conversion for a short timeframe (one
or two kernel releases), if you are sure that those features will be present
on the next firmwares.

The better is to mark the driver as EXPERIMENTAL, at Kconfig.

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] USB: add Sensoray 2255 v4l driver
@ 2008-05-16 18:17         ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2008-05-16 18:17 UTC (permalink / raw)
  To: dean
  Cc: video4linux-list, linux-usb, Greg KH, Oliver Neukum,
	linux-kernel, v4l-dvb-maintainer

Dean,

On Fri, 16 May 2008 08:57:15 -0700
dean <dean@sensoray.com> wrote:

> Hi Oliver,
> 
> Just to be clear.  You mean fixing the loading issues(ugly msleep, 
> etc...) that Mauro mentioned, not the YUV formats(which seem unrelated)? 
> 
> Would it be ok to have just 422P and greyscale in the driver for now if 
> we can't get a firmware update in a reasonable timeframe? It's certainly 
> not ideal, but we could direct our customers to the libraries, at least 
> until we get a proper HW fix.

Feature regression is something that shouldn't happen. If we add the driver
with a color conversion inside, we should somehow keep this feature available
on newer versions. That's said, I don't see any issue if the later versions do
this implementation inside hardware/firmware, since the end result is that the
driver will keep supporting those standards.

>From my side, I'm ok on having this color conversion for a short timeframe (one
or two kernel releases), if you are sure that those features will be present
on the next firmwares.

The better is to mark the driver as EXPERIMENTAL, at Kconfig.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2008-05-16 18:18 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-14 20:59 [PATCH] USB: add Sensoray 2255 v4l driver Greg KH
2008-05-15  1:17 ` [v4l-dvb-maintainer] " Markus Rechberger
2008-05-15  1:17   ` Markus Rechberger
2008-05-15  2:41   ` Greg KH
2008-05-15  3:12     ` Trent Piepho
2008-05-15  3:12       ` Trent Piepho
2008-05-15 15:34       ` Dean Anderson
2008-05-15 16:57         ` Markus Rechberger
2008-05-15 16:57           ` Markus Rechberger
2008-05-16  2:59         ` Mauro Carvalho Chehab
2008-05-15 11:38 ` Oliver Neukum
2008-05-15 12:03   ` Oliver Neukum
2008-05-15 18:44     ` Greg KH
2008-05-15 19:54       ` Oliver Neukum
2008-05-15 20:10         ` Greg KH
2008-05-15 20:13           ` Oliver Neukum
2008-05-16  2:51 ` Mauro Carvalho Chehab
2008-05-16  2:51   ` Mauro Carvalho Chehab
2008-05-16  6:28   ` Oliver Neukum
2008-05-16 15:57     ` dean
2008-05-16 16:04       ` Oliver Neukum
2008-05-16 18:17       ` Mauro Carvalho Chehab
2008-05-16 18:17         ` Mauro Carvalho Chehab
2008-05-16 14:53   ` dean
2008-05-16 15:34     ` Mauro Carvalho Chehab
2008-05-16 15:34       ` Mauro Carvalho Chehab

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.