[PATCH v2 00/21] em28xx: add support fur USB bulk transfers

[PATCH v2 00/21] em28xx: add support fur USB bulk transfers

[Frank Schäfer]

This patch series adds support for USB bulk transfers to the em28xx driver.

Patch 1 is a bugfix for the image data processing with non-interlaced
devices (webcams) that should be considered for stable (see commit message).

Patches 2-21 extend the driver to support USB bulk transfers.
USB endpoint mapping had to be extended and is a bit tricky.
It might still not be sufficient to handle ALL isoc/bulk endpoints 
of ALL existing devices, but it should work with the devices we have 
seen so far and (most important !) preserves backwards compatibility to 
the current driver behavior.
Isoc endpoints/transfers are preffered by default, patch 21 adds a 
module parameter to change this behavior.

Changes in v2:
- remove warnings about untestet changes from the commit messages of
  patches 14 and 15 (meanwhile changes have been tested)
- fix interpretation of the new module parameter prefer_bulk (patch 21)
- drop patches 22 and 23 (they are not really related to USB transfers)



Frank Schäfer (21):
  em28xx: fix wrong data offset for non-interlaced mode in
    em28xx_copy_video
  em28xx: clarify meaning of field 'progressive' in struct em28xx
  em28xx: rename isoc packet number constants and parameters
  em28xx: rename struct em28xx_usb_isoc_bufs to em28xx_usb_bufs
  em28xx: rename struct em28xx_usb_isoc_ctl to em28xx_usb_ctl
  em28xx: remove obsolete #define EM28XX_URB_TIMEOUT
  em28xx: update description of em28xx_irq_callback
  em28xx: rename function em28xx_uninit_isoc to em28xx_uninit_usb_xfer
  em28xx: create a common function for isoc and bulk URB allocation and
    setup
  em28xx: create a common function for isoc and bulk USB transfer
    initialization
  em28xx: clear USB halt/stall condition in em28xx_init_usb_xfer when
    using bulk transfers
  em28xx: remove double checks for urb->status == -ENOENT in
    urb_data_copy functions
  em28xx: rename function em28xx_isoc_copy and extend for USB bulk
    transfers
  em28xx: rename function em28xx_isoc_copy_vbi and extend for USB bulk
    transfers
  em28xx: rename function em28xx_dvb_isoc_copy and extend for USB bulk
    transfers
  em28xx: rename usb debugging module parameter and macro
  em28xx: rename some USB parameter fields in struct em28xx to clarify
    their role
  em28xx: add fields for analog and DVB USB transfer type selection to
    struct em28xx
  em28xx: set USB alternate settings for analog video bulk transfers
    properly
  em28xx: improve USB endpoint logic, also use bulk transfers
  em28xx: add module parameter for selection of the preferred USB
    transfer type

 drivers/media/usb/em28xx/em28xx-cards.c |  116 +++++++++++---
 drivers/media/usb/em28xx/em28xx-core.c  |  247 +++++++++++++++++------------
 drivers/media/usb/em28xx/em28xx-dvb.c   |   85 ++++++----
 drivers/media/usb/em28xx/em28xx-reg.h   |    4 +-
 drivers/media/usb/em28xx/em28xx-vbi.c   |    4 +-
 drivers/media/usb/em28xx/em28xx-video.c |  259 +++++++++++++++++--------------
 drivers/media/usb/em28xx/em28xx.h       |   78 ++++++----
 7 Dateien geändert, 494 Zeilen hinzugefügt(+), 299 Zeilen entfernt(-)

-- 
1.7.10.4

[PATCH v2 01/21] em28xx: fix wrong data offset for non-interlaced mode in em28xx_copy_video

[Frank Schäfer]

em28xx_copy_video uses a wrong offset for the target buffer
when copying the data from an USB isoc packet. This happens
only for the second and all following lines in the packet.

The reason why this bug doesn't cause image corruption with
my test device (SilverCrest Webcam 1.3 MPix) is, that this
device never sends any packets that cross the end of a line.
I don't know if all devices behave like this, so this patch
should be considered for stable.

With the upcoming patches to add support for USB bulk transfers,
em28xx_copy_video will be called once per URB, which will
always trigger this bug.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   16 +++++++---------
 1 Datei geändert, 7 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 1e553d3..7c88a40 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -207,15 +207,10 @@ static void em28xx_copy_video(struct em28xx *dev,
 	startread = p;
 	remain = len;
 
-	if (dev->progressive)
+	if (dev->progressive || buf->top_field)
 		fieldstart = outp;
-	else {
-		/* Interlaces two half frames */
-		if (buf->top_field)
-			fieldstart = outp;
-		else
-			fieldstart = outp + bytesperline;
-	}
+	else /* interlaced mode, even nr. of lines */
+		fieldstart = outp + bytesperline;
 
 	linesdone = dma_q->pos / bytesperline;
 	currlinedone = dma_q->pos % bytesperline;
@@ -243,7 +238,10 @@ static void em28xx_copy_video(struct em28xx *dev,
 	remain -= lencopy;
 
 	while (remain > 0) {
-		startwrite += lencopy + bytesperline;
+		if (dev->progressive)
+			startwrite += lencopy;
+		else
+			startwrite += lencopy + bytesperline;
 		startread += lencopy;
 		if (bytesperline > remain)
 			lencopy = remain;
-- 
1.7.10.4

[PATCH v2 02/21] em28xx: clarify meaning of field 'progressive' in struct em28xx

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx.h |    2 +-
 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 86e90d8..ad9eec0 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -497,7 +497,7 @@ struct em28xx {
 	int sensor_xres, sensor_yres;
 	int sensor_xtal;
 
-	/* Allows progressive (e. g. non-interlaced) mode */
+	/* Progressive (non-interlaced) mode */
 	int progressive;
 
 	/* Vinmode/Vinctl used at the driver */
-- 
1.7.10.4

[PATCH v2 03/21] em28xx: rename isoc packet number constants and parameters

[Frank Schäfer]

Rename EM28XX_NUM_PACKETS to EM28XX_NUM_ISOC_PACKETS and
EM28XX_DVB_MAX_PACKETS to EM28XX_DVB_NUM_ISOC_PACKETS to
clarify that these values are used only for isoc usb transfers.
Also use the term num_packets instead of max_packets, as this
is how these values are used and called in struct urb.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    8 ++++----
 drivers/media/usb/em28xx/em28xx-dvb.c   |    5 +++--
 drivers/media/usb/em28xx/em28xx-video.c |    4 ++--
 drivers/media/usb/em28xx/em28xx.h       |   10 +++++-----
 5 Dateien geändert, 15 Zeilen hinzugefügt(+), 14 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 619bffb..7cd2faf 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3323,7 +3323,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	if (has_dvb) {
 		/* pre-allocate DVB isoc transfer buffers */
 		retval = em28xx_alloc_isoc(dev, EM28XX_DIGITAL_MODE,
-					   EM28XX_DVB_MAX_PACKETS,
+					   EM28XX_DVB_NUM_ISOC_PACKETS,
 					   EM28XX_DVB_NUM_BUFS,
 					   dev->dvb_max_pkt_size);
 		if (retval) {
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index bed07a6..2520a16 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -1034,7 +1034,7 @@ EXPORT_SYMBOL_GPL(em28xx_stop_urbs);
  * Allocate URBs
  */
 int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		      int max_packets, int num_bufs, int max_pkt_size)
+		      int num_packets, int num_bufs, int max_pkt_size)
 {
 	struct em28xx_usb_isoc_bufs *isoc_bufs;
 	int i;
@@ -1069,7 +1069,7 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	}
 
 	isoc_bufs->max_pkt_size = max_pkt_size;
-	isoc_bufs->num_packets = max_packets;
+	isoc_bufs->num_packets = num_packets;
 	dev->isoc_ctl.vid_buf = NULL;
 	dev->isoc_ctl.vbi_buf = NULL;
 
@@ -1129,7 +1129,7 @@ EXPORT_SYMBOL_GPL(em28xx_alloc_isoc);
  * Allocate URBs and start IRQ
  */
 int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		     int max_packets, int num_bufs, int max_pkt_size,
+		     int num_packets, int num_bufs, int max_pkt_size,
 		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb))
 {
 	struct em28xx_dmaqueue *dma_q = &dev->vidq;
@@ -1153,7 +1153,7 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	}
 
 	if (alloc) {
-		rc = em28xx_alloc_isoc(dev, mode, max_packets,
+		rc = em28xx_alloc_isoc(dev, mode, num_packets,
 				       num_bufs, max_pkt_size);
 		if (rc)
 			return rc;
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 63f2e70..833f10b 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -173,11 +173,12 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		return max_dvb_packet_size;
 	dprintk(1, "Using %d buffers each with %d x %d bytes\n",
 		EM28XX_DVB_NUM_BUFS,
-		EM28XX_DVB_MAX_PACKETS,
+		EM28XX_DVB_NUM_ISOC_PACKETS,
 		max_dvb_packet_size);
 
 	return em28xx_init_isoc(dev, EM28XX_DIGITAL_MODE,
-				EM28XX_DVB_MAX_PACKETS, EM28XX_DVB_NUM_BUFS,
+				EM28XX_DVB_NUM_ISOC_PACKETS,
+				EM28XX_DVB_NUM_BUFS,
 				max_dvb_packet_size, em28xx_dvb_isoc_copy);
 }
 
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 7c88a40..e51284c 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -764,13 +764,13 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 	if (urb_init) {
 		if (em28xx_vbi_supported(dev) == 1)
 			rc = em28xx_init_isoc(dev, EM28XX_ANALOG_MODE,
-					      EM28XX_NUM_PACKETS,
+					      EM28XX_NUM_ISOC_PACKETS,
 					      EM28XX_NUM_BUFS,
 					      dev->max_pkt_size,
 					      em28xx_isoc_copy_vbi);
 		else
 			rc = em28xx_init_isoc(dev, EM28XX_ANALOG_MODE,
-					      EM28XX_NUM_PACKETS,
+					      EM28XX_NUM_ISOC_PACKETS,
 					      EM28XX_NUM_BUFS,
 					      dev->max_pkt_size,
 					      em28xx_isoc_copy);
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index ad9eec0..36a7864 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -157,12 +157,12 @@
 #define EM28XX_NUM_BUFS 5
 #define EM28XX_DVB_NUM_BUFS 5
 
-/* number of packets for each buffer
+/* isoc transfers: number of packets for each buffer
    windows requests only 64 packets .. so we better do the same
    this is what I found out for all alternate numbers there!
  */
-#define EM28XX_NUM_PACKETS 64
-#define EM28XX_DVB_MAX_PACKETS 64
+#define EM28XX_NUM_ISOC_PACKETS 64
+#define EM28XX_DVB_NUM_ISOC_PACKETS 64
 
 #define EM28XX_INTERLACED_DEFAULT 1
 
@@ -667,9 +667,9 @@ int em28xx_set_outfmt(struct em28xx *dev);
 int em28xx_resolution_set(struct em28xx *dev);
 int em28xx_set_alternate(struct em28xx *dev);
 int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		      int max_packets, int num_bufs, int max_pkt_size);
+		      int num_packets, int num_bufs, int max_pkt_size);
 int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		     int max_packets, int num_bufs, int max_pkt_size,
+		     int num_packets, int num_bufs, int max_pkt_size,
 		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb));
 void em28xx_uninit_isoc(struct em28xx *dev, enum em28xx_mode mode);
 void em28xx_stop_urbs(struct em28xx *dev);
-- 
1.7.10.4

[PATCH v2 04/21] em28xx: rename struct em28xx_usb_isoc_bufs to em28xx_usb_bufs

[Frank Schäfer]

It will be used for USB bulk transfers, too.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |    8 ++++----
 drivers/media/usb/em28xx/em28xx.h      |   10 +++++-----
 2 Dateien geändert, 9 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 2520a16..b250a63 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -964,7 +964,7 @@ static void em28xx_irq_callback(struct urb *urb)
 void em28xx_uninit_isoc(struct em28xx *dev, enum em28xx_mode mode)
 {
 	struct urb *urb;
-	struct em28xx_usb_isoc_bufs *isoc_bufs;
+	struct em28xx_usb_bufs *isoc_bufs;
 	int i;
 
 	em28xx_isocdbg("em28xx: called em28xx_uninit_isoc in mode %d\n", mode);
@@ -1012,7 +1012,7 @@ void em28xx_stop_urbs(struct em28xx *dev)
 {
 	int i;
 	struct urb *urb;
-	struct em28xx_usb_isoc_bufs *isoc_bufs = &dev->isoc_ctl.digital_bufs;
+	struct em28xx_usb_bufs *isoc_bufs = &dev->isoc_ctl.digital_bufs;
 
 	em28xx_isocdbg("em28xx: called em28xx_stop_urbs\n");
 
@@ -1036,7 +1036,7 @@ EXPORT_SYMBOL_GPL(em28xx_stop_urbs);
 int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 		      int num_packets, int num_bufs, int max_pkt_size)
 {
-	struct em28xx_usb_isoc_bufs *isoc_bufs;
+	struct em28xx_usb_bufs *isoc_bufs;
 	int i;
 	int sb_size, pipe;
 	struct urb *urb;
@@ -1134,7 +1134,7 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 {
 	struct em28xx_dmaqueue *dma_q = &dev->vidq;
 	struct em28xx_dmaqueue *vbi_dma_q = &dev->vbiq;
-	struct em28xx_usb_isoc_bufs *isoc_bufs;
+	struct em28xx_usb_bufs *isoc_bufs;
 	int i;
 	int rc;
 	int alloc;
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 36a7864..e062a27 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -203,7 +203,7 @@ enum em28xx_mode {
 
 struct em28xx;
 
-struct em28xx_usb_isoc_bufs {
+struct em28xx_usb_bufs {
 		/* max packet size of isoc transaction */
 	int				max_pkt_size;
 
@@ -213,19 +213,19 @@ struct em28xx_usb_isoc_bufs {
 		/* number of allocated urbs */
 	int				num_bufs;
 
-		/* urb for isoc transfers */
+		/* urb for isoc/bulk transfers */
 	struct urb			**urb;
 
-		/* transfer buffers for isoc transfer */
+		/* transfer buffers for isoc/bulk transfer */
 	char				**transfer_buffer;
 };
 
 struct em28xx_usb_isoc_ctl {
 		/* isoc transfer buffers for analog mode */
-	struct em28xx_usb_isoc_bufs	analog_bufs;
+	struct em28xx_usb_bufs		analog_bufs;
 
 		/* isoc transfer buffers for digital mode */
-	struct em28xx_usb_isoc_bufs	digital_bufs;
+	struct em28xx_usb_bufs		digital_bufs;
 
 		/* Stores already requested buffers */
 	struct em28xx_buffer    	*vid_buf;
-- 
1.7.10.4

[PATCH v2 05/21] em28xx: rename struct em28xx_usb_isoc_ctl to em28xx_usb_ctl

[Frank Schäfer]

Also rename the corresponding field isoc_ctl in struct em28xx
to usb_ctl.
We will use this struct for USB bulk transfers, too.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c  |   24 ++++++++++++------------
 drivers/media/usb/em28xx/em28xx-vbi.c   |    4 ++--
 drivers/media/usb/em28xx/em28xx-video.c |   24 ++++++++++++------------
 drivers/media/usb/em28xx/em28xx.h       |   12 ++++++------
 4 Dateien geändert, 32 Zeilen hinzugefügt(+), 32 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index b250a63..0892d92 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -941,7 +941,7 @@ static void em28xx_irq_callback(struct urb *urb)
 
 	/* Copy data from URB */
 	spin_lock(&dev->slock);
-	dev->isoc_ctl.isoc_copy(dev, urb);
+	dev->usb_ctl.urb_data_copy(dev, urb);
 	spin_unlock(&dev->slock);
 
 	/* Reset urb buffers */
@@ -970,9 +970,9 @@ void em28xx_uninit_isoc(struct em28xx *dev, enum em28xx_mode mode)
 	em28xx_isocdbg("em28xx: called em28xx_uninit_isoc in mode %d\n", mode);
 
 	if (mode == EM28XX_DIGITAL_MODE)
-		isoc_bufs = &dev->isoc_ctl.digital_bufs;
+		isoc_bufs = &dev->usb_ctl.digital_bufs;
 	else
-		isoc_bufs = &dev->isoc_ctl.analog_bufs;
+		isoc_bufs = &dev->usb_ctl.analog_bufs;
 
 	for (i = 0; i < isoc_bufs->num_bufs; i++) {
 		urb = isoc_bufs->urb[i];
@@ -1012,7 +1012,7 @@ void em28xx_stop_urbs(struct em28xx *dev)
 {
 	int i;
 	struct urb *urb;
-	struct em28xx_usb_bufs *isoc_bufs = &dev->isoc_ctl.digital_bufs;
+	struct em28xx_usb_bufs *isoc_bufs = &dev->usb_ctl.digital_bufs;
 
 	em28xx_isocdbg("em28xx: called em28xx_stop_urbs\n");
 
@@ -1045,9 +1045,9 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	em28xx_isocdbg("em28xx: called em28xx_alloc_isoc in mode %d\n", mode);
 
 	if (mode == EM28XX_DIGITAL_MODE)
-		isoc_bufs = &dev->isoc_ctl.digital_bufs;
+		isoc_bufs = &dev->usb_ctl.digital_bufs;
 	else
-		isoc_bufs = &dev->isoc_ctl.analog_bufs;
+		isoc_bufs = &dev->usb_ctl.analog_bufs;
 
 	/* De-allocates all pending stuff */
 	em28xx_uninit_isoc(dev, mode);
@@ -1070,8 +1070,8 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 
 	isoc_bufs->max_pkt_size = max_pkt_size;
 	isoc_bufs->num_packets = num_packets;
-	dev->isoc_ctl.vid_buf = NULL;
-	dev->isoc_ctl.vbi_buf = NULL;
+	dev->usb_ctl.vid_buf = NULL;
+	dev->usb_ctl.vbi_buf = NULL;
 
 	sb_size = isoc_bufs->num_packets * isoc_bufs->max_pkt_size;
 
@@ -1079,7 +1079,7 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	for (i = 0; i < isoc_bufs->num_bufs; i++) {
 		urb = usb_alloc_urb(isoc_bufs->num_packets, GFP_KERNEL);
 		if (!urb) {
-			em28xx_err("cannot alloc isoc_ctl.urb %i\n", i);
+			em28xx_err("cannot alloc usb_ctl.urb %i\n", i);
 			em28xx_uninit_isoc(dev, mode);
 			return -ENOMEM;
 		}
@@ -1141,14 +1141,14 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 
 	em28xx_isocdbg("em28xx: called em28xx_init_isoc in mode %d\n", mode);
 
-	dev->isoc_ctl.isoc_copy = isoc_copy;
+	dev->usb_ctl.urb_data_copy = isoc_copy;
 
 	if (mode == EM28XX_DIGITAL_MODE) {
-		isoc_bufs = &dev->isoc_ctl.digital_bufs;
+		isoc_bufs = &dev->usb_ctl.digital_bufs;
 		/* no need to free/alloc isoc buffers in digital mode */
 		alloc = 0;
 	} else {
-		isoc_bufs = &dev->isoc_ctl.analog_bufs;
+		isoc_bufs = &dev->usb_ctl.analog_bufs;
 		alloc = 1;
 	}
 
diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c
index 2b4c9cb..d74713b 100644
--- a/drivers/media/usb/em28xx/em28xx-vbi.c
+++ b/drivers/media/usb/em28xx/em28xx-vbi.c
@@ -60,8 +60,8 @@ free_buffer(struct videobuf_queue *vq, struct em28xx_buffer *buf)
 	   VIDEOBUF_ACTIVE, it won't be, though.
 	*/
 	spin_lock_irqsave(&dev->slock, flags);
-	if (dev->isoc_ctl.vbi_buf == buf)
-		dev->isoc_ctl.vbi_buf = NULL;
+	if (dev->usb_ctl.vbi_buf == buf)
+		dev->usb_ctl.vbi_buf = NULL;
 	spin_unlock_irqrestore(&dev->slock, flags);
 
 	videobuf_vmalloc_free(&buf->vb);
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index e51284c..b334885 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -165,7 +165,7 @@ static inline void buffer_filled(struct em28xx *dev,
 	buf->vb.field_count++;
 	do_gettimeofday(&buf->vb.ts);
 
-	dev->isoc_ctl.vid_buf = NULL;
+	dev->usb_ctl.vid_buf = NULL;
 
 	list_del(&buf->vb.queue);
 	wake_up(&buf->vb.done);
@@ -182,7 +182,7 @@ static inline void vbi_buffer_filled(struct em28xx *dev,
 	buf->vb.field_count++;
 	do_gettimeofday(&buf->vb.ts);
 
-	dev->isoc_ctl.vbi_buf = NULL;
+	dev->usb_ctl.vbi_buf = NULL;
 
 	list_del(&buf->vb.queue);
 	wake_up(&buf->vb.done);
@@ -368,7 +368,7 @@ static inline void get_next_buf(struct em28xx_dmaqueue *dma_q,
 
 	if (list_empty(&dma_q->active)) {
 		em28xx_isocdbg("No active queue to serve\n");
-		dev->isoc_ctl.vid_buf = NULL;
+		dev->usb_ctl.vid_buf = NULL;
 		*buf = NULL;
 		return;
 	}
@@ -380,7 +380,7 @@ static inline void get_next_buf(struct em28xx_dmaqueue *dma_q,
 	outp = videobuf_to_vmalloc(&(*buf)->vb);
 	memset(outp, 0, (*buf)->vb.size);
 
-	dev->isoc_ctl.vid_buf = *buf;
+	dev->usb_ctl.vid_buf = *buf;
 
 	return;
 }
@@ -396,7 +396,7 @@ static inline void vbi_get_next_buf(struct em28xx_dmaqueue *dma_q,
 
 	if (list_empty(&dma_q->active)) {
 		em28xx_isocdbg("No active queue to serve\n");
-		dev->isoc_ctl.vbi_buf = NULL;
+		dev->usb_ctl.vbi_buf = NULL;
 		*buf = NULL;
 		return;
 	}
@@ -407,7 +407,7 @@ static inline void vbi_get_next_buf(struct em28xx_dmaqueue *dma_q,
 	outp = videobuf_to_vmalloc(&(*buf)->vb);
 	memset(outp, 0x00, (*buf)->vb.size);
 
-	dev->isoc_ctl.vbi_buf = *buf;
+	dev->usb_ctl.vbi_buf = *buf;
 
 	return;
 }
@@ -435,7 +435,7 @@ static inline int em28xx_isoc_copy(struct em28xx *dev, struct urb *urb)
 			return 0;
 	}
 
-	buf = dev->isoc_ctl.vid_buf;
+	buf = dev->usb_ctl.vid_buf;
 	if (buf != NULL)
 		outp = videobuf_to_vmalloc(&buf->vb);
 
@@ -531,11 +531,11 @@ static inline int em28xx_isoc_copy_vbi(struct em28xx *dev, struct urb *urb)
 			return 0;
 	}
 
-	buf = dev->isoc_ctl.vid_buf;
+	buf = dev->usb_ctl.vid_buf;
 	if (buf != NULL)
 		outp = videobuf_to_vmalloc(&buf->vb);
 
-	vbi_buf = dev->isoc_ctl.vbi_buf;
+	vbi_buf = dev->usb_ctl.vbi_buf;
 	if (vbi_buf != NULL)
 		vbioutp = videobuf_to_vmalloc(&vbi_buf->vb);
 
@@ -725,8 +725,8 @@ static void free_buffer(struct videobuf_queue *vq, struct em28xx_buffer *buf)
 	   VIDEOBUF_ACTIVE, it won't be, though.
 	*/
 	spin_lock_irqsave(&dev->slock, flags);
-	if (dev->isoc_ctl.vid_buf == buf)
-		dev->isoc_ctl.vid_buf = NULL;
+	if (dev->usb_ctl.vid_buf == buf)
+		dev->usb_ctl.vid_buf = NULL;
 	spin_unlock_irqrestore(&dev->slock, flags);
 
 	videobuf_vmalloc_free(&buf->vb);
@@ -758,7 +758,7 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 			goto fail;
 	}
 
-	if (!dev->isoc_ctl.analog_bufs.num_bufs)
+	if (!dev->usb_ctl.analog_bufs.num_bufs)
 		urb_init = 1;
 
 	if (urb_init) {
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index e062a27..17310e6 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -220,19 +220,19 @@ struct em28xx_usb_bufs {
 	char				**transfer_buffer;
 };
 
-struct em28xx_usb_isoc_ctl {
-		/* isoc transfer buffers for analog mode */
+struct em28xx_usb_ctl {
+		/* isoc/bulk transfer buffers for analog mode */
 	struct em28xx_usb_bufs		analog_bufs;
 
-		/* isoc transfer buffers for digital mode */
+		/* isoc/bulk transfer buffers for digital mode */
 	struct em28xx_usb_bufs		digital_bufs;
 
 		/* Stores already requested buffers */
 	struct em28xx_buffer    	*vid_buf;
 	struct em28xx_buffer    	*vbi_buf;
 
-		/* isoc urb callback */
-	int (*isoc_copy) (struct em28xx *dev, struct urb *urb);
+		/* copy data from URB */
+	int (*urb_data_copy) (struct em28xx *dev, struct urb *urb);
 
 };
 
@@ -582,7 +582,7 @@ struct em28xx {
 	/* Isoc control struct */
 	struct em28xx_dmaqueue vidq;
 	struct em28xx_dmaqueue vbiq;
-	struct em28xx_usb_isoc_ctl isoc_ctl;
+	struct em28xx_usb_ctl usb_ctl;
 	spinlock_t slock;
 
 	/* usb transfer */
-- 
1.7.10.4

[PATCH v2 06/21] em28xx: remove obsolete #define EM28XX_URB_TIMEOUT

[Frank Schäfer]

It isn't used anymore and uses constants which no longer exist.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx.h |    4 ----
 1 Datei geändert, 4 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 17310e6..6773ca8 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -187,10 +187,6 @@
 			Interval: 125us
 */
 
-/* time to wait when stopping the isoc transfer */
-#define EM28XX_URB_TIMEOUT \
-			msecs_to_jiffies(EM28XX_NUM_BUFS * EM28XX_NUM_PACKETS)
-
 /* time in msecs to wait for i2c writes to finish */
 #define EM2800_I2C_WRITE_TIMEOUT 20
 
-- 
1.7.10.4

[PATCH v2 07/21] em28xx: update description of em28xx_irq_callback

[Frank Schäfer]

em28xx_irq_callback can be used for isoc and bulk transfers.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |    3 ++-
 1 Datei geändert, 2 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 0892d92..8f50f5c 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -919,7 +919,7 @@ EXPORT_SYMBOL_GPL(em28xx_set_mode);
    ------------------------------------------------------------------*/
 
 /*
- * IRQ callback, called by URB callback
+ * URB completion handler for isoc/bulk transfers
  */
 static void em28xx_irq_callback(struct urb *urb)
 {
@@ -946,6 +946,7 @@ static void em28xx_irq_callback(struct urb *urb)
 
 	/* Reset urb buffers */
 	for (i = 0; i < urb->number_of_packets; i++) {
+		/* isoc only (bulk: number_of_packets = 0) */
 		urb->iso_frame_desc[i].status = 0;
 		urb->iso_frame_desc[i].actual_length = 0;
 	}
-- 
1.7.10.4

[PATCH v2 08/21] em28xx: rename function em28xx_uninit_isoc to em28xx_uninit_usb_xfer

[Frank Schäfer]

This function will be used to uninitialize USB bulk transfers, too.
Also rename the local variable isoc_bufs to usb_bufs.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |    4 +--
 drivers/media/usb/em28xx/em28xx-core.c  |   43 ++++++++++++++++---------------
 drivers/media/usb/em28xx/em28xx-video.c |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    2 +-
 4 Dateien geändert, 26 Zeilen hinzugefügt(+), 25 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 7cd2faf..e474ccf 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3394,7 +3394,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 		     video_device_node_name(dev->vdev));
 
 		dev->state |= DEV_MISCONFIGURED;
-		em28xx_uninit_isoc(dev, dev->mode);
+		em28xx_uninit_usb_xfer(dev, dev->mode);
 		dev->state |= DEV_DISCONNECTED;
 	} else {
 		dev->state |= DEV_DISCONNECTED;
@@ -3402,7 +3402,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 	}
 
 	/* free DVB isoc buffers */
-	em28xx_uninit_isoc(dev, EM28XX_DIGITAL_MODE);
+	em28xx_uninit_usb_xfer(dev, EM28XX_DIGITAL_MODE);
 
 	mutex_unlock(&dev->lock);
 
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 8f50f5c..a1ebd08 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -962,49 +962,50 @@ static void em28xx_irq_callback(struct urb *urb)
 /*
  * Stop and Deallocate URBs
  */
-void em28xx_uninit_isoc(struct em28xx *dev, enum em28xx_mode mode)
+void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode)
 {
 	struct urb *urb;
-	struct em28xx_usb_bufs *isoc_bufs;
+	struct em28xx_usb_bufs *usb_bufs;
 	int i;
 
-	em28xx_isocdbg("em28xx: called em28xx_uninit_isoc in mode %d\n", mode);
+	em28xx_isocdbg("em28xx: called em28xx_uninit_usb_xfer in mode %d\n",
+		       mode);
 
 	if (mode == EM28XX_DIGITAL_MODE)
-		isoc_bufs = &dev->usb_ctl.digital_bufs;
+		usb_bufs = &dev->usb_ctl.digital_bufs;
 	else
-		isoc_bufs = &dev->usb_ctl.analog_bufs;
+		usb_bufs = &dev->usb_ctl.analog_bufs;
 
-	for (i = 0; i < isoc_bufs->num_bufs; i++) {
-		urb = isoc_bufs->urb[i];
+	for (i = 0; i < usb_bufs->num_bufs; i++) {
+		urb = usb_bufs->urb[i];
 		if (urb) {
 			if (!irqs_disabled())
 				usb_kill_urb(urb);
 			else
 				usb_unlink_urb(urb);
 
-			if (isoc_bufs->transfer_buffer[i]) {
+			if (usb_bufs->transfer_buffer[i]) {
 				usb_free_coherent(dev->udev,
 					urb->transfer_buffer_length,
-					isoc_bufs->transfer_buffer[i],
+					usb_bufs->transfer_buffer[i],
 					urb->transfer_dma);
 			}
 			usb_free_urb(urb);
-			isoc_bufs->urb[i] = NULL;
+			usb_bufs->urb[i] = NULL;
 		}
-		isoc_bufs->transfer_buffer[i] = NULL;
+		usb_bufs->transfer_buffer[i] = NULL;
 	}
 
-	kfree(isoc_bufs->urb);
-	kfree(isoc_bufs->transfer_buffer);
+	kfree(usb_bufs->urb);
+	kfree(usb_bufs->transfer_buffer);
 
-	isoc_bufs->urb = NULL;
-	isoc_bufs->transfer_buffer = NULL;
-	isoc_bufs->num_bufs = 0;
+	usb_bufs->urb = NULL;
+	usb_bufs->transfer_buffer = NULL;
+	usb_bufs->num_bufs = 0;
 
 	em28xx_capture_start(dev, 0);
 }
-EXPORT_SYMBOL_GPL(em28xx_uninit_isoc);
+EXPORT_SYMBOL_GPL(em28xx_uninit_usb_xfer);
 
 /*
  * Stop URBs
@@ -1051,7 +1052,7 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 		isoc_bufs = &dev->usb_ctl.analog_bufs;
 
 	/* De-allocates all pending stuff */
-	em28xx_uninit_isoc(dev, mode);
+	em28xx_uninit_usb_xfer(dev, mode);
 
 	isoc_bufs->num_bufs = num_bufs;
 
@@ -1081,7 +1082,7 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 		urb = usb_alloc_urb(isoc_bufs->num_packets, GFP_KERNEL);
 		if (!urb) {
 			em28xx_err("cannot alloc usb_ctl.urb %i\n", i);
-			em28xx_uninit_isoc(dev, mode);
+			em28xx_uninit_usb_xfer(dev, mode);
 			return -ENOMEM;
 		}
 		isoc_bufs->urb[i] = urb;
@@ -1093,7 +1094,7 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 					" buffer %i%s\n",
 					sb_size, i,
 					in_interrupt() ? " while in int" : "");
-			em28xx_uninit_isoc(dev, mode);
+			em28xx_uninit_usb_xfer(dev, mode);
 			return -ENOMEM;
 		}
 		memset(isoc_bufs->transfer_buffer[i], 0, sb_size);
@@ -1171,7 +1172,7 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 		if (rc) {
 			em28xx_err("submit of urb %i failed (error=%i)\n", i,
 				   rc);
-			em28xx_uninit_isoc(dev, mode);
+			em28xx_uninit_usb_xfer(dev, mode);
 			return rc;
 		}
 	}
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index b334885..1207a73 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2272,7 +2272,7 @@ static int em28xx_v4l2_close(struct file *filp)
 		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
 
 		/* do this before setting alternate! */
-		em28xx_uninit_isoc(dev, EM28XX_ANALOG_MODE);
+		em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
 		em28xx_set_mode(dev, EM28XX_SUSPEND);
 
 		/* set alternate 0 */
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 6773ca8..8fb3504 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -667,7 +667,7 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 		     int num_packets, int num_bufs, int max_pkt_size,
 		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb));
-void em28xx_uninit_isoc(struct em28xx *dev, enum em28xx_mode mode);
+void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode);
 void em28xx_stop_urbs(struct em28xx *dev);
 int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev);
 int em28xx_set_mode(struct em28xx *dev, enum em28xx_mode set_mode);
-- 
1.7.10.4

[PATCH v2 09/21] em28xx: create a common function for isoc and bulk URB allocation and setup

[Frank Schäfer]

Rename the existing function for isoc transfers em28xx_init_isoc
to em28xx_init_usb_xfer and extend it.
URB allocation and setup is now done depending on the USB
transfer type, which is selected with a new function parameter.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |    6 +-
 drivers/media/usb/em28xx/em28xx-core.c  |  101 +++++++++++++++++--------------
 drivers/media/usb/em28xx/em28xx.h       |    4 +-
 3 Dateien geändert, 61 Zeilen hinzugefügt(+), 50 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index e474ccf..bfce34d 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3322,10 +3322,10 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 
 	if (has_dvb) {
 		/* pre-allocate DVB isoc transfer buffers */
-		retval = em28xx_alloc_isoc(dev, EM28XX_DIGITAL_MODE,
-					   EM28XX_DVB_NUM_ISOC_PACKETS,
+		retval = em28xx_alloc_urbs(dev, EM28XX_DIGITAL_MODE, 0,
 					   EM28XX_DVB_NUM_BUFS,
-					   dev->dvb_max_pkt_size);
+					   dev->dvb_max_pkt_size,
+					   EM28XX_DVB_NUM_ISOC_PACKETS);
 		if (retval) {
 			goto unlock_and_free;
 		}
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index a1ebd08..42388de 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -5,6 +5,7 @@
 		      Markus Rechberger <mrechberger@gmail.com>
 		      Mauro Carvalho Chehab <mchehab@infradead.org>
 		      Sascha Sommer <saschasommer@freenet.de>
+   Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
 
    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
@@ -1035,10 +1036,10 @@ EXPORT_SYMBOL_GPL(em28xx_stop_urbs);
 /*
  * Allocate URBs
  */
-int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		      int num_packets, int num_bufs, int max_pkt_size)
+int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
+		      int num_bufs, int max_pkt_size, int packet_multiplier)
 {
-	struct em28xx_usb_bufs *isoc_bufs;
+	struct em28xx_usb_bufs *usb_bufs;
 	int i;
 	int sb_size, pipe;
 	struct urb *urb;
@@ -1047,49 +1048,52 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	em28xx_isocdbg("em28xx: called em28xx_alloc_isoc in mode %d\n", mode);
 
 	if (mode == EM28XX_DIGITAL_MODE)
-		isoc_bufs = &dev->usb_ctl.digital_bufs;
+		usb_bufs = &dev->usb_ctl.digital_bufs;
 	else
-		isoc_bufs = &dev->usb_ctl.analog_bufs;
+		usb_bufs = &dev->usb_ctl.analog_bufs;
 
 	/* De-allocates all pending stuff */
 	em28xx_uninit_usb_xfer(dev, mode);
 
-	isoc_bufs->num_bufs = num_bufs;
+	usb_bufs->num_bufs = num_bufs;
 
-	isoc_bufs->urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
-	if (!isoc_bufs->urb) {
+	usb_bufs->urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
+	if (!usb_bufs->urb) {
 		em28xx_errdev("cannot alloc memory for usb buffers\n");
 		return -ENOMEM;
 	}
 
-	isoc_bufs->transfer_buffer = kzalloc(sizeof(void *)*num_bufs,
+	usb_bufs->transfer_buffer = kzalloc(sizeof(void *)*num_bufs,
 					     GFP_KERNEL);
-	if (!isoc_bufs->transfer_buffer) {
+	if (!usb_bufs->transfer_buffer) {
 		em28xx_errdev("cannot allocate memory for usb transfer\n");
-		kfree(isoc_bufs->urb);
+		kfree(usb_bufs->urb);
 		return -ENOMEM;
 	}
 
-	isoc_bufs->max_pkt_size = max_pkt_size;
-	isoc_bufs->num_packets = num_packets;
+	usb_bufs->max_pkt_size = max_pkt_size;
+	if (xfer_bulk)
+		usb_bufs->num_packets = 0;
+	else
+		usb_bufs->num_packets = packet_multiplier;
 	dev->usb_ctl.vid_buf = NULL;
 	dev->usb_ctl.vbi_buf = NULL;
 
-	sb_size = isoc_bufs->num_packets * isoc_bufs->max_pkt_size;
+	sb_size = packet_multiplier * usb_bufs->max_pkt_size;
 
 	/* allocate urbs and transfer buffers */
-	for (i = 0; i < isoc_bufs->num_bufs; i++) {
-		urb = usb_alloc_urb(isoc_bufs->num_packets, GFP_KERNEL);
+	for (i = 0; i < usb_bufs->num_bufs; i++) {
+		urb = usb_alloc_urb(usb_bufs->num_packets, GFP_KERNEL);
 		if (!urb) {
 			em28xx_err("cannot alloc usb_ctl.urb %i\n", i);
 			em28xx_uninit_usb_xfer(dev, mode);
 			return -ENOMEM;
 		}
-		isoc_bufs->urb[i] = urb;
+		usb_bufs->urb[i] = urb;
 
-		isoc_bufs->transfer_buffer[i] = usb_alloc_coherent(dev->udev,
+		usb_bufs->transfer_buffer[i] = usb_alloc_coherent(dev->udev,
 			sb_size, GFP_KERNEL, &urb->transfer_dma);
-		if (!isoc_bufs->transfer_buffer[i]) {
+		if (!usb_bufs->transfer_buffer[i]) {
 			em28xx_err("unable to allocate %i bytes for transfer"
 					" buffer %i%s\n",
 					sb_size, i,
@@ -1097,35 +1101,42 @@ int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
 			em28xx_uninit_usb_xfer(dev, mode);
 			return -ENOMEM;
 		}
-		memset(isoc_bufs->transfer_buffer[i], 0, sb_size);
-
-		/* FIXME: this is a hack - should be
-			'desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK'
-			should also be using 'desc.bInterval'
-		 */
-		pipe = usb_rcvisocpipe(dev->udev,
-				       mode == EM28XX_ANALOG_MODE ?
-				       EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);
-
-		usb_fill_int_urb(urb, dev->udev, pipe,
-				 isoc_bufs->transfer_buffer[i], sb_size,
-				 em28xx_irq_callback, dev, 1);
-
-		urb->number_of_packets = isoc_bufs->num_packets;
-		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-
-		k = 0;
-		for (j = 0; j < isoc_bufs->num_packets; j++) {
-			urb->iso_frame_desc[j].offset = k;
-			urb->iso_frame_desc[j].length =
-						isoc_bufs->max_pkt_size;
-			k += isoc_bufs->max_pkt_size;
+		memset(usb_bufs->transfer_buffer[i], 0, sb_size);
+
+		if (xfer_bulk) { /* bulk */
+			pipe = usb_rcvbulkpipe(dev->udev,
+					       mode == EM28XX_ANALOG_MODE ?
+					       EM28XX_EP_ANALOG :
+					       EM28XX_EP_DIGITAL);
+			usb_fill_bulk_urb(urb, dev->udev, pipe,
+					  usb_bufs->transfer_buffer[i], sb_size,
+					  em28xx_irq_callback, dev);
+			urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+		} else { /* isoc */
+			pipe = usb_rcvisocpipe(dev->udev,
+					       mode == EM28XX_ANALOG_MODE ?
+					       EM28XX_EP_ANALOG :
+					       EM28XX_EP_DIGITAL);
+			usb_fill_int_urb(urb, dev->udev, pipe,
+					 usb_bufs->transfer_buffer[i], sb_size,
+					 em28xx_irq_callback, dev, 1);
+			urb->transfer_flags = URB_ISO_ASAP |
+					      URB_NO_TRANSFER_DMA_MAP;
+			k = 0;
+			for (j = 0; j < usb_bufs->num_packets; j++) {
+				urb->iso_frame_desc[j].offset = k;
+				urb->iso_frame_desc[j].length =
+							usb_bufs->max_pkt_size;
+				k += usb_bufs->max_pkt_size;
+			}
 		}
+
+		urb->number_of_packets = usb_bufs->num_packets;
 	}
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(em28xx_alloc_isoc);
+EXPORT_SYMBOL_GPL(em28xx_alloc_urbs);
 
 /*
  * Allocate URBs and start IRQ
@@ -1155,8 +1166,8 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	}
 
 	if (alloc) {
-		rc = em28xx_alloc_isoc(dev, mode, num_packets,
-				       num_bufs, max_pkt_size);
+		rc = em28xx_alloc_urbs(dev, mode, 0, num_bufs,
+				       max_pkt_size, num_packets);
 		if (rc)
 			return rc;
 	}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 8fb3504..7bc2ddd 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -662,8 +662,8 @@ int em28xx_vbi_supported(struct em28xx *dev);
 int em28xx_set_outfmt(struct em28xx *dev);
 int em28xx_resolution_set(struct em28xx *dev);
 int em28xx_set_alternate(struct em28xx *dev);
-int em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		      int num_packets, int num_bufs, int max_pkt_size);
+int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
+		      int num_bufs, int max_pkt_size, int packet_multiplier);
 int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 		     int num_packets, int num_bufs, int max_pkt_size,
 		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb));
-- 
1.7.10.4

[PATCH v2 10/21] em28xx: create a common function for isoc and bulk USB transfer initialization

[Frank Schäfer]

- rename em28xx_init_isoc to em28xx_init_usb_xfer
- add parameter for isoc/bulk transfer selection which is passed to em28xx_alloc_urbs
- rename local variable isoc_buf to usb_bufs

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c  |   30 ++++++++++++++++--------------
 drivers/media/usb/em28xx/em28xx-dvb.c   |    9 +++++----
 drivers/media/usb/em28xx/em28xx-video.c |   20 ++++++++++----------
 drivers/media/usb/em28xx/em28xx.h       |    8 +++++---
 4 Dateien geändert, 36 Zeilen hinzugefügt(+), 31 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 42388de..d8a8e8b 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -1141,33 +1141,35 @@ EXPORT_SYMBOL_GPL(em28xx_alloc_urbs);
 /*
  * Allocate URBs and start IRQ
  */
-int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		     int num_packets, int num_bufs, int max_pkt_size,
-		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb))
+int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode,
+		    int xfer_bulk, int num_bufs, int max_pkt_size,
+		    int packet_multiplier,
+		    int (*urb_data_copy) (struct em28xx *dev, struct urb *urb))
 {
 	struct em28xx_dmaqueue *dma_q = &dev->vidq;
 	struct em28xx_dmaqueue *vbi_dma_q = &dev->vbiq;
-	struct em28xx_usb_bufs *isoc_bufs;
+	struct em28xx_usb_bufs *usb_bufs;
 	int i;
 	int rc;
 	int alloc;
 
-	em28xx_isocdbg("em28xx: called em28xx_init_isoc in mode %d\n", mode);
+	em28xx_isocdbg("em28xx: called em28xx_init_usb_xfer in mode %d\n",
+		       mode);
 
-	dev->usb_ctl.urb_data_copy = isoc_copy;
+	dev->usb_ctl.urb_data_copy = urb_data_copy;
 
 	if (mode == EM28XX_DIGITAL_MODE) {
-		isoc_bufs = &dev->usb_ctl.digital_bufs;
-		/* no need to free/alloc isoc buffers in digital mode */
+		usb_bufs = &dev->usb_ctl.digital_bufs;
+		/* no need to free/alloc usb buffers in digital mode */
 		alloc = 0;
 	} else {
-		isoc_bufs = &dev->usb_ctl.analog_bufs;
+		usb_bufs = &dev->usb_ctl.analog_bufs;
 		alloc = 1;
 	}
 
 	if (alloc) {
-		rc = em28xx_alloc_urbs(dev, mode, 0, num_bufs,
-				       max_pkt_size, num_packets);
+		rc = em28xx_alloc_urbs(dev, mode, xfer_bulk, num_bufs,
+				       max_pkt_size, packet_multiplier);
 		if (rc)
 			return rc;
 	}
@@ -1178,8 +1180,8 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 	em28xx_capture_start(dev, 1);
 
 	/* submit urbs and enables IRQ */
-	for (i = 0; i < isoc_bufs->num_bufs; i++) {
-		rc = usb_submit_urb(isoc_bufs->urb[i], GFP_ATOMIC);
+	for (i = 0; i < usb_bufs->num_bufs; i++) {
+		rc = usb_submit_urb(usb_bufs->urb[i], GFP_ATOMIC);
 		if (rc) {
 			em28xx_err("submit of urb %i failed (error=%i)\n", i,
 				   rc);
@@ -1190,7 +1192,7 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(em28xx_init_isoc);
+EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
 
 /*
  * em28xx_wake_i2c()
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 833f10b..eeabc25 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -176,10 +176,11 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		EM28XX_DVB_NUM_ISOC_PACKETS,
 		max_dvb_packet_size);
 
-	return em28xx_init_isoc(dev, EM28XX_DIGITAL_MODE,
-				EM28XX_DVB_NUM_ISOC_PACKETS,
-				EM28XX_DVB_NUM_BUFS,
-				max_dvb_packet_size, em28xx_dvb_isoc_copy);
+	return em28xx_init_usb_xfer(dev, EM28XX_DIGITAL_MODE, 0,
+				    EM28XX_DVB_NUM_BUFS,
+				    max_dvb_packet_size,
+				    EM28XX_DVB_NUM_ISOC_PACKETS,
+				    em28xx_dvb_isoc_copy);
 }
 
 static int em28xx_stop_streaming(struct em28xx_dvb *dvb)
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 1207a73..4024dfc 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -763,17 +763,17 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 
 	if (urb_init) {
 		if (em28xx_vbi_supported(dev) == 1)
-			rc = em28xx_init_isoc(dev, EM28XX_ANALOG_MODE,
-					      EM28XX_NUM_ISOC_PACKETS,
-					      EM28XX_NUM_BUFS,
-					      dev->max_pkt_size,
-					      em28xx_isoc_copy_vbi);
+			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE, 0,
+						  EM28XX_NUM_BUFS,
+						  dev->max_pkt_size,
+						  EM28XX_NUM_ISOC_PACKETS,
+						  em28xx_isoc_copy_vbi);
 		else
-			rc = em28xx_init_isoc(dev, EM28XX_ANALOG_MODE,
-					      EM28XX_NUM_ISOC_PACKETS,
-					      EM28XX_NUM_BUFS,
-					      dev->max_pkt_size,
-					      em28xx_isoc_copy);
+			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE, 0,
+						  EM28XX_NUM_BUFS,
+						  dev->max_pkt_size,
+						  EM28XX_NUM_ISOC_PACKETS,
+						  em28xx_isoc_copy);
 		if (rc < 0)
 			goto fail;
 	}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 7bc2ddd..950a717 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -664,9 +664,11 @@ int em28xx_resolution_set(struct em28xx *dev);
 int em28xx_set_alternate(struct em28xx *dev);
 int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
 		      int num_bufs, int max_pkt_size, int packet_multiplier);
-int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
-		     int num_packets, int num_bufs, int max_pkt_size,
-		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb));
+int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode,
+			 int xfer_bulk,
+			 int num_bufs, int max_pkt_size, int packet_multiplier,
+			 int (*urb_data_copy)
+					(struct em28xx *dev, struct urb *urb));
 void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode);
 void em28xx_stop_urbs(struct em28xx *dev);
 int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev);
-- 
1.7.10.4

[PATCH v2 11/21] em28xx: clear USB halt/stall condition in em28xx_init_usb_xfer when using bulk transfers

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |   11 +++++++++++
 1 Datei geändert, 11 Zeilen hinzugefügt(+)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index d8a8e8b..8b8f783 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -1174,6 +1174,17 @@ int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode,
 			return rc;
 	}
 
+	if (xfer_bulk) {
+		rc = usb_clear_halt(dev->udev, usb_bufs->urb[0]->pipe);
+		if (rc < 0) {
+			em28xx_err("failed to clear USB bulk endpoint "
+				   "stall/halt condition (error=%i)\n",
+				   rc);
+			em28xx_uninit_usb_xfer(dev, mode);
+			return rc;
+		}
+	}
+
 	init_waitqueue_head(&dma_q->wq);
 	init_waitqueue_head(&vbi_dma_q->wq);
 
-- 
1.7.10.4

[PATCH v2 12/21] em28xx: remove double checks for urb->status == -ENOENT in urb_data_copy functions

[Frank Schäfer]

This check is already done in the URB handler
em28xx_irq_callback before calling these functions.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-dvb.c   |    5 +----
 drivers/media/usb/em28xx/em28xx-video.c |   10 ++--------
 2 Dateien geändert, 3 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index eeabc25..1ad4f10 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -134,11 +134,8 @@ static inline int em28xx_dvb_isoc_copy(struct em28xx *dev, struct urb *urb)
 	if ((dev->state & DEV_DISCONNECTED) || (dev->state & DEV_MISCONFIGURED))
 		return 0;
 
-	if (urb->status < 0) {
+	if (urb->status < 0)
 		print_err_status(dev, -1, urb->status);
-		if (urb->status == -ENOENT)
-			return 0;
-	}
 
 	for (i = 0; i < urb->number_of_packets; i++) {
 		int status = urb->iso_frame_desc[i].status;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 4024dfc..3518753 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -429,11 +429,8 @@ static inline int em28xx_isoc_copy(struct em28xx *dev, struct urb *urb)
 	if ((dev->state & DEV_DISCONNECTED) || (dev->state & DEV_MISCONFIGURED))
 		return 0;
 
-	if (urb->status < 0) {
+	if (urb->status < 0)
 		print_err_status(dev, -1, urb->status);
-		if (urb->status == -ENOENT)
-			return 0;
-	}
 
 	buf = dev->usb_ctl.vid_buf;
 	if (buf != NULL)
@@ -525,11 +522,8 @@ static inline int em28xx_isoc_copy_vbi(struct em28xx *dev, struct urb *urb)
 	if ((dev->state & DEV_DISCONNECTED) || (dev->state & DEV_MISCONFIGURED))
 		return 0;
 
-	if (urb->status < 0) {
+	if (urb->status < 0)
 		print_err_status(dev, -1, urb->status);
-		if (urb->status == -ENOENT)
-			return 0;
-	}
 
 	buf = dev->usb_ctl.vid_buf;
 	if (buf != NULL)
-- 
1.7.10.4

[PATCH v2 13/21] em28xx: rename function em28xx_isoc_copy and extend for USB bulk transfers

[Frank Schäfer]

The URB data processing for bulk transfers is very similar to what
is done with isoc transfers, so create a common function that works
with both transfer types based on the existing isoc function.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   66 +++++++++++++++++++------------
 1 Datei geändert, 41 Zeilen hinzugefügt(+), 25 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 3518753..63b0cc3 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -6,6 +6,7 @@
 		      Markus Rechberger <mrechberger@gmail.com>
 		      Mauro Carvalho Chehab <mchehab@infradead.org>
 		      Sascha Sommer <saschasommer@freenet.de>
+   Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
 
 	Some parts based on SN9C10x PC Camera Controllers GPL driver made
 		by Luca Risolia <luca.risolia@studio.unibo.it>
@@ -412,16 +413,14 @@ static inline void vbi_get_next_buf(struct em28xx_dmaqueue *dma_q,
 	return;
 }
 
-/*
- * Controls the isoc copy of each urb packet
- */
-static inline int em28xx_isoc_copy(struct em28xx *dev, struct urb *urb)
+/* Processes and copies the URB data content to a frame buffer queue */
+static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 {
 	struct em28xx_buffer    *buf;
 	struct em28xx_dmaqueue  *dma_q = &dev->vidq;
-	unsigned char *outp = NULL;
-	int i, len = 0, rc = 1;
-	unsigned char *p;
+	int xfer_bulk, num_packets, i, rc = 1;
+	unsigned int actual_length, len = 0;
+	unsigned char *p, *outp = NULL;
 
 	if (!dev)
 		return 0;
@@ -432,33 +431,47 @@ static inline int em28xx_isoc_copy(struct em28xx *dev, struct urb *urb)
 	if (urb->status < 0)
 		print_err_status(dev, -1, urb->status);
 
+	xfer_bulk = usb_pipebulk(urb->pipe);
+
 	buf = dev->usb_ctl.vid_buf;
 	if (buf != NULL)
 		outp = videobuf_to_vmalloc(&buf->vb);
 
-	for (i = 0; i < urb->number_of_packets; i++) {
-		int status = urb->iso_frame_desc[i].status;
+	if (xfer_bulk) /* bulk */
+		num_packets = 1;
+	else /* isoc */
+		num_packets = urb->number_of_packets;
+
+	for (i = 0; i < num_packets; i++) {
+		if (xfer_bulk) { /* bulk */
+			actual_length = urb->actual_length;
+
+			p = urb->transfer_buffer;
+		} else { /* isoc */
+			if (urb->iso_frame_desc[i].status < 0) {
+				print_err_status(dev, i,
+						 urb->iso_frame_desc[i].status);
+				if (urb->iso_frame_desc[i].status != -EPROTO)
+					continue;
+			}
 
-		if (status < 0) {
-			print_err_status(dev, i, status);
-			if (urb->iso_frame_desc[i].status != -EPROTO)
+			actual_length = urb->iso_frame_desc[i].actual_length;
+			if (actual_length > dev->max_pkt_size) {
+				em28xx_isocdbg("packet bigger than "
+					       "packet size");
 				continue;
-		}
-
-		len = urb->iso_frame_desc[i].actual_length - 4;
+			}
 
-		if (urb->iso_frame_desc[i].actual_length <= 0) {
-			/* em28xx_isocdbg("packet %d is empty",i); - spammy */
-			continue;
+			p = urb->transfer_buffer +
+			    urb->iso_frame_desc[i].offset;
 		}
-		if (urb->iso_frame_desc[i].actual_length >
-						dev->max_pkt_size) {
-			em28xx_isocdbg("packet bigger than packet size");
+
+		if (actual_length <= 0) {
+			/* NOTE: happens very often with isoc transfers */
+			/* em28xx_usbdbg("packet %d is empty",i); - spammy */
 			continue;
 		}
 
-		p = urb->transfer_buffer + urb->iso_frame_desc[i].offset;
-
 		/* FIXME: incomplete buffer checks where removed to make
 		   logic simpler. Impacts of those changes should be evaluated
 		 */
@@ -492,9 +505,12 @@ static inline int em28xx_isoc_copy(struct em28xx *dev, struct urb *urb)
 		}
 		if (buf != NULL) {
 			if (p[0] != 0x88 && p[0] != 0x22) {
+				/* NOTE: no intermediate data packet header
+				 * 88 88 88 88 when using bulk transfers */
 				em28xx_isocdbg("frame is not complete\n");
-				len += 4;
+				len = actual_length;
 			} else {
+				len = actual_length - 4;
 				p += 4;
 			}
 			em28xx_copy_video(dev, dma_q, buf, p, outp, len);
@@ -767,7 +783,7 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 						  EM28XX_NUM_BUFS,
 						  dev->max_pkt_size,
 						  EM28XX_NUM_ISOC_PACKETS,
-						  em28xx_isoc_copy);
+						  em28xx_urb_data_copy);
 		if (rc < 0)
 			goto fail;
 	}
-- 
1.7.10.4

[PATCH v2 14/21] em28xx: rename function em28xx_isoc_copy_vbi and extend for USB bulk transfers

[Frank Schäfer]

The URB data processing for bulk transfers is very similar to what
is done with isoc transfers, so create a common function that works
with both transfer types based on the existing isoc function.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   71 +++++++++++++++++++------------
 1 Datei geändert, 44 Zeilen hinzugefügt(+), 27 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 63b0cc3..d6de1cc 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -519,18 +519,16 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 	return rc;
 }
 
-/* Version of isoc handler that takes into account a mixture of video and
-   VBI data */
-static inline int em28xx_isoc_copy_vbi(struct em28xx *dev, struct urb *urb)
+/* Version of the urb data handler that takes into account a mixture of
+   video and VBI data */
+static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
 {
 	struct em28xx_buffer    *buf, *vbi_buf;
 	struct em28xx_dmaqueue  *dma_q = &dev->vidq;
 	struct em28xx_dmaqueue  *vbi_dma_q = &dev->vbiq;
-	unsigned char *outp = NULL;
-	unsigned char *vbioutp = NULL;
-	int i, len = 0, rc = 1;
-	unsigned char *p;
-	int vbi_size;
+	int xfer_bulk, vbi_size, num_packets, i, rc = 1;
+	unsigned int actual_length, len = 0;
+	unsigned char *p, *outp = NULL, *vbioutp = NULL;
 
 	if (!dev)
 		return 0;
@@ -541,6 +539,8 @@ static inline int em28xx_isoc_copy_vbi(struct em28xx *dev, struct urb *urb)
 	if (urb->status < 0)
 		print_err_status(dev, -1, urb->status);
 
+	xfer_bulk = usb_pipebulk(urb->pipe);
+
 	buf = dev->usb_ctl.vid_buf;
 	if (buf != NULL)
 		outp = videobuf_to_vmalloc(&buf->vb);
@@ -549,28 +549,41 @@ static inline int em28xx_isoc_copy_vbi(struct em28xx *dev, struct urb *urb)
 	if (vbi_buf != NULL)
 		vbioutp = videobuf_to_vmalloc(&vbi_buf->vb);
 
-	for (i = 0; i < urb->number_of_packets; i++) {
-		int status = urb->iso_frame_desc[i].status;
+	if (xfer_bulk) /* bulk */
+		num_packets = 1;
+	else /* isoc */
+		num_packets = urb->number_of_packets;
+
+	for (i = 0; i < num_packets; i++) {
+		if (xfer_bulk) { /* bulk */
+			actual_length = urb->actual_length;
+
+			p = urb->transfer_buffer;
+		} else { /* isoc */
+			if (urb->iso_frame_desc[i].status < 0) {
+				print_err_status(dev, i,
+						 urb->iso_frame_desc[i].status);
+				if (urb->iso_frame_desc[i].status != -EPROTO)
+					continue;
+			}
 
-		if (status < 0) {
-			print_err_status(dev, i, status);
-			if (urb->iso_frame_desc[i].status != -EPROTO)
+			actual_length = urb->iso_frame_desc[i].actual_length;
+			if (actual_length > dev->max_pkt_size) {
+				em28xx_isocdbg("packet bigger than "
+					       "packet size");
 				continue;
-		}
+			}
 
-		len = urb->iso_frame_desc[i].actual_length;
-		if (urb->iso_frame_desc[i].actual_length <= 0) {
-			/* em28xx_isocdbg("packet %d is empty",i); - spammy */
-			continue;
+			p = urb->transfer_buffer +
+			    urb->iso_frame_desc[i].offset;
 		}
-		if (urb->iso_frame_desc[i].actual_length >
-						dev->max_pkt_size) {
-			em28xx_isocdbg("packet bigger than packet size");
+
+		if (actual_length <= 0) {
+			/* NOTE: happens very often with isoc transfers */
+			/* em28xx_usbdbg("packet %d is empty",i); - spammy */
 			continue;
 		}
 
-		p = urb->transfer_buffer + urb->iso_frame_desc[i].offset;
-
 		/* capture type 0 = vbi start
 		   capture type 1 = video start
 		   capture type 2 = video in progress */
@@ -580,16 +593,20 @@ static inline int em28xx_isoc_copy_vbi(struct em28xx *dev, struct urb *urb)
 			em28xx_isocdbg("VBI START HEADER!!!\n");
 			dev->cur_field = p[2];
 			p += 4;
-			len -= 4;
+			len = actual_length - 4;
 		} else if (p[0] == 0x88 && p[1] == 0x88 &&
 			   p[2] == 0x88 && p[3] == 0x88) {
 			/* continuation */
 			p += 4;
-			len -= 4;
+			len = actual_length - 4;
 		} else if (p[0] == 0x22 && p[1] == 0x5a) {
 			/* start video */
 			p += 4;
-			len -= 4;
+			len = actual_length - 4;
+		} else {
+			/* NOTE: With bulk transfers, intermediate data packets
+			 * have no continuation header */
+			len = actual_length;
 		}
 
 		vbi_size = dev->vbi_width * dev->vbi_height;
@@ -777,7 +794,7 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 						  EM28XX_NUM_BUFS,
 						  dev->max_pkt_size,
 						  EM28XX_NUM_ISOC_PACKETS,
-						  em28xx_isoc_copy_vbi);
+						  em28xx_urb_data_copy_vbi);
 		else
 			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE, 0,
 						  EM28XX_NUM_BUFS,
-- 
1.7.10.4

[PATCH v2 15/21] em28xx: rename function em28xx_dvb_isoc_copy and extend for USB bulk transfers

[Frank Schäfer]

The URB data processing for DVB bulk transfers is very similar to
what is done with isoc transfers, so create a common function that
works with both transfer types based on the existing isoc function.

Tested with device Hauppauge HVR-930c.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-dvb.c |   44 +++++++++++++++++++++++----------
 1 Datei geändert, 31 Zeilen hinzugefügt(+), 13 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 1ad4f10..b46246a 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -10,6 +10,8 @@
 
  (c) 2008 Aidan Thornton <makosoft@googlemail.com>
 
+ (c) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
+
  Based on cx88-dvb, saa7134-dvb and videobuf-dvb originally written by:
 	(c) 2004, 2005 Chris Pascoe <c.pascoe@itee.uq.edu.au>
 	(c) 2004 Gerd Knorr <kraxel@bytesex.org> [SuSE Labs]
@@ -124,9 +126,9 @@ static inline void print_err_status(struct em28xx *dev,
 	}
 }
 
-static inline int em28xx_dvb_isoc_copy(struct em28xx *dev, struct urb *urb)
+static inline int em28xx_dvb_urb_data_copy(struct em28xx *dev, struct urb *urb)
 {
-	int i;
+	int xfer_bulk, num_packets, i;
 
 	if (!dev)
 		return 0;
@@ -137,18 +139,34 @@ static inline int em28xx_dvb_isoc_copy(struct em28xx *dev, struct urb *urb)
 	if (urb->status < 0)
 		print_err_status(dev, -1, urb->status);
 
-	for (i = 0; i < urb->number_of_packets; i++) {
-		int status = urb->iso_frame_desc[i].status;
+	xfer_bulk = usb_pipebulk(urb->pipe);
 
-		if (status < 0) {
-			print_err_status(dev, i, status);
-			if (urb->iso_frame_desc[i].status != -EPROTO)
-				continue;
-		}
+	if (xfer_bulk) /* bulk */
+		num_packets = 1;
+	else /* isoc */
+		num_packets = urb->number_of_packets;
 
-		dvb_dmx_swfilter(&dev->dvb->demux, urb->transfer_buffer +
-				 urb->iso_frame_desc[i].offset,
-				 urb->iso_frame_desc[i].actual_length);
+	for (i = 0; i < num_packets; i++) {
+		if (xfer_bulk) {
+			if (urb->status < 0) {
+				print_err_status(dev, i, urb->status);
+				if (urb->status != -EPROTO)
+					continue;
+			}
+			dvb_dmx_swfilter(&dev->dvb->demux, urb->transfer_buffer,
+					urb->actual_length);
+		} else {
+			if (urb->iso_frame_desc[i].status < 0) {
+				print_err_status(dev, i,
+						 urb->iso_frame_desc[i].status);
+				if (urb->iso_frame_desc[i].status != -EPROTO)
+					continue;
+			}
+			dvb_dmx_swfilter(&dev->dvb->demux,
+					 urb->transfer_buffer +
+					 urb->iso_frame_desc[i].offset,
+					 urb->iso_frame_desc[i].actual_length);
+		}
 	}
 
 	return 0;
@@ -177,7 +195,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 				    EM28XX_DVB_NUM_BUFS,
 				    max_dvb_packet_size,
 				    EM28XX_DVB_NUM_ISOC_PACKETS,
-				    em28xx_dvb_isoc_copy);
+				    em28xx_dvb_urb_data_copy);
 }
 
 static int em28xx_stop_streaming(struct em28xx_dvb *dvb)
-- 
1.7.10.4

[PATCH v2 16/21] em28xx: rename usb debugging module parameter and macro

[Frank Schäfer]

Rename module parameter isoc_debug to usb_debug and macro
em28xx_isocdbg to em28xx_usb dbg to reflect that they are
used for isoc and bulk USB transfers.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   58 +++++++++++++++----------------
 1 Datei geändert, 28 Zeilen hinzugefügt(+), 30 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index d6de1cc..f435206 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -58,13 +58,13 @@
 		printk(KERN_INFO "%s %s :"fmt, \
 			 dev->name, __func__ , ##arg); } while (0)
 
-static unsigned int isoc_debug;
-module_param(isoc_debug, int, 0644);
-MODULE_PARM_DESC(isoc_debug, "enable debug messages [isoc transfers]");
+static unsigned int usb_debug;
+module_param(usb_debug, int, 0644);
+MODULE_PARM_DESC(usb_debug, "enable debug messages [isoc transfers]");
 
-#define em28xx_isocdbg(fmt, arg...) \
+#define em28xx_usbdbg(fmt, arg...) \
 do {\
-	if (isoc_debug) { \
+	if (usb_debug) { \
 		printk(KERN_INFO "%s %s :"fmt, \
 			 dev->name, __func__ , ##arg); \
 	} \
@@ -161,7 +161,7 @@ static inline void buffer_filled(struct em28xx *dev,
 				  struct em28xx_buffer *buf)
 {
 	/* Advice that buffer was filled */
-	em28xx_isocdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
+	em28xx_usbdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
 	buf->vb.state = VIDEOBUF_DONE;
 	buf->vb.field_count++;
 	do_gettimeofday(&buf->vb.ts);
@@ -177,7 +177,7 @@ static inline void vbi_buffer_filled(struct em28xx *dev,
 				     struct em28xx_buffer *buf)
 {
 	/* Advice that buffer was filled */
-	em28xx_isocdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
+	em28xx_usbdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
 
 	buf->vb.state = VIDEOBUF_DONE;
 	buf->vb.field_count++;
@@ -226,9 +226,9 @@ static void em28xx_copy_video(struct em28xx *dev,
 	lencopy = lencopy > remain ? remain : lencopy;
 
 	if ((char *)startwrite + lencopy > (char *)outp + buf->vb.size) {
-		em28xx_isocdbg("Overflow of %zi bytes past buffer end (1)\n",
-			       ((char *)startwrite + lencopy) -
-			       ((char *)outp + buf->vb.size));
+		em28xx_usbdbg("Overflow of %zi bytes past buffer end (1)\n",
+			      ((char *)startwrite + lencopy) -
+			      ((char *)outp + buf->vb.size));
 		remain = (char *)outp + buf->vb.size - (char *)startwrite;
 		lencopy = remain;
 	}
@@ -251,7 +251,7 @@ static void em28xx_copy_video(struct em28xx *dev,
 
 		if ((char *)startwrite + lencopy > (char *)outp +
 		    buf->vb.size) {
-			em28xx_isocdbg("Overflow of %zi bytes past buffer end"
+			em28xx_usbdbg("Overflow of %zi bytes past buffer end"
 				       "(2)\n",
 				       ((char *)startwrite + lencopy) -
 				       ((char *)outp + buf->vb.size));
@@ -280,24 +280,24 @@ static void em28xx_copy_vbi(struct em28xx *dev,
 	int bytesperline;
 
 	if (dev == NULL) {
-		em28xx_isocdbg("dev is null\n");
+		em28xx_usbdbg("dev is null\n");
 		return;
 	}
 	bytesperline = dev->vbi_width;
 
 	if (dma_q == NULL) {
-		em28xx_isocdbg("dma_q is null\n");
+		em28xx_usbdbg("dma_q is null\n");
 		return;
 	}
 	if (buf == NULL) {
 		return;
 	}
 	if (p == NULL) {
-		em28xx_isocdbg("p is null\n");
+		em28xx_usbdbg("p is null\n");
 		return;
 	}
 	if (outp == NULL) {
-		em28xx_isocdbg("outp is null\n");
+		em28xx_usbdbg("outp is null\n");
 		return;
 	}
 
@@ -351,9 +351,9 @@ static inline void print_err_status(struct em28xx *dev,
 		break;
 	}
 	if (packet < 0) {
-		em28xx_isocdbg("URB status %d [%s].\n",	status, errmsg);
+		em28xx_usbdbg("URB status %d [%s].\n",	status, errmsg);
 	} else {
-		em28xx_isocdbg("URB packet %d, status %d [%s].\n",
+		em28xx_usbdbg("URB packet %d, status %d [%s].\n",
 			       packet, status, errmsg);
 	}
 }
@@ -368,7 +368,7 @@ static inline void get_next_buf(struct em28xx_dmaqueue *dma_q,
 	char *outp;
 
 	if (list_empty(&dma_q->active)) {
-		em28xx_isocdbg("No active queue to serve\n");
+		em28xx_usbdbg("No active queue to serve\n");
 		dev->usb_ctl.vid_buf = NULL;
 		*buf = NULL;
 		return;
@@ -396,7 +396,7 @@ static inline void vbi_get_next_buf(struct em28xx_dmaqueue *dma_q,
 	char *outp;
 
 	if (list_empty(&dma_q->active)) {
-		em28xx_isocdbg("No active queue to serve\n");
+		em28xx_usbdbg("No active queue to serve\n");
 		dev->usb_ctl.vbi_buf = NULL;
 		*buf = NULL;
 		return;
@@ -457,8 +457,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 
 			actual_length = urb->iso_frame_desc[i].actual_length;
 			if (actual_length > dev->max_pkt_size) {
-				em28xx_isocdbg("packet bigger than "
-					       "packet size");
+				em28xx_usbdbg("packet bigger than packet size");
 				continue;
 			}
 
@@ -476,12 +475,12 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 		   logic simpler. Impacts of those changes should be evaluated
 		 */
 		if (p[0] == 0x33 && p[1] == 0x95 && p[2] == 0x00) {
-			em28xx_isocdbg("VBI HEADER!!!\n");
+			em28xx_usbdbg("VBI HEADER!!!\n");
 			/* FIXME: Should add vbi copy */
 			continue;
 		}
 		if (p[0] == 0x22 && p[1] == 0x5a) {
-			em28xx_isocdbg("Video frame %d, length=%i, %s\n", p[2],
+			em28xx_usbdbg("Video frame %d, length=%i, %s\n", p[2],
 				       len, (p[2] & 1) ? "odd" : "even");
 
 			if (dev->progressive || !(p[2] & 1)) {
@@ -507,7 +506,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 			if (p[0] != 0x88 && p[0] != 0x22) {
 				/* NOTE: no intermediate data packet header
 				 * 88 88 88 88 when using bulk transfers */
-				em28xx_isocdbg("frame is not complete\n");
+				em28xx_usbdbg("frame is not complete\n");
 				len = actual_length;
 			} else {
 				len = actual_length - 4;
@@ -569,8 +568,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
 
 			actual_length = urb->iso_frame_desc[i].actual_length;
 			if (actual_length > dev->max_pkt_size) {
-				em28xx_isocdbg("packet bigger than "
-					       "packet size");
+				em28xx_usbdbg("packet bigger than packet size");
 				continue;
 			}
 
@@ -590,7 +588,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
 		if (p[0] == 0x33 && p[1] == 0x95) {
 			dev->capture_type = 0;
 			dev->vbi_read = 0;
-			em28xx_isocdbg("VBI START HEADER!!!\n");
+			em28xx_usbdbg("VBI START HEADER!!!\n");
 			dev->cur_field = p[2];
 			p += 4;
 			len = actual_length - 4;
@@ -615,7 +613,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
 			if (dev->vbi_read >= vbi_size) {
 				/* We've already read all the VBI data, so
 				   treat the rest as video */
-				em28xx_isocdbg("dev->vbi_read > vbi_size\n");
+				em28xx_usbdbg("dev->vbi_read > vbi_size\n");
 			} else if ((dev->vbi_read + len) < vbi_size) {
 				/* This entire frame is VBI data */
 				if (dev->vbi_read == 0 &&
@@ -687,7 +685,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
 				len -= 4;
 			}
 			if (len >= 4 && p[0] == 0x22 && p[1] == 0x5a) {
-				em28xx_isocdbg("Video frame %d, len=%i, %s\n",
+				em28xx_usbdbg("Video frame %d, len=%i, %s\n",
 					       p[2], len, (p[2] & 1) ?
 					       "odd" : "even");
 				p += 4;
@@ -837,7 +835,7 @@ static void buffer_release(struct videobuf_queue *vq,
 	struct em28xx_fh       *fh   = vq->priv_data;
 	struct em28xx          *dev  = (struct em28xx *)fh->dev;
 
-	em28xx_isocdbg("em28xx: called buffer_release\n");
+	em28xx_usbdbg("em28xx: called buffer_release\n");
 
 	free_buffer(vq, buf);
 }
-- 
1.7.10.4

Re: [PATCH v2 16/21] em28xx: rename usb debugging module parameter and macro

[Mauro Carvalho Chehab]

Em Thu,  8 Nov 2012 20:11:48 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Rename module parameter isoc_debug to usb_debug and macro
> em28xx_isocdbg to em28xx_usb dbg to reflect that they are
> used for isoc and bulk USB transfers.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-video.c |   58 +++++++++++++++----------------
>  1 Datei geändert, 28 Zeilen hinzugefügt(+), 30 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index d6de1cc..f435206 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -58,13 +58,13 @@
>  		printk(KERN_INFO "%s %s :"fmt, \
>  			 dev->name, __func__ , ##arg); } while (0)
>  
> -static unsigned int isoc_debug;
> -module_param(isoc_debug, int, 0644);
> -MODULE_PARM_DESC(isoc_debug, "enable debug messages [isoc transfers]");
> +static unsigned int usb_debug;
> +module_param(usb_debug, int, 0644);
> +MODULE_PARM_DESC(usb_debug, "enable debug messages [isoc transfers]");

NACK: usb_debug is too generic: it could refer to control URB's, stream
URB's, and other non-URB related USB debugging. Also, it can cause 
some harm for the ones using it.

As the rest of this series don't depend on this one, I'll just skip it.

IMHO, the better is to either live it as-is, to avoid breaking for
someone with "isoc_debug" parameter on their /etc/modprobe.d, or to
do a "deprecate" path:

	- adding a new one called "stream_debug" (or something like that);
	- keep the old one for a while, printing a warning message to
point that this got removed; 
	- after a few kernel cycles, remove the legacy one.

Even better: simply unify all debug params into a single one, where 
each bit means one type of debug, like what was done on other drivers.

>  
> -#define em28xx_isocdbg(fmt, arg...) \
> +#define em28xx_usbdbg(fmt, arg...) \
>  do {\
> -	if (isoc_debug) { \
> +	if (usb_debug) { \
>  		printk(KERN_INFO "%s %s :"fmt, \
>  			 dev->name, __func__ , ##arg); \
>  	} \
> @@ -161,7 +161,7 @@ static inline void buffer_filled(struct em28xx *dev,
>  				  struct em28xx_buffer *buf)
>  {
>  	/* Advice that buffer was filled */
> -	em28xx_isocdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
> +	em28xx_usbdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
>  	buf->vb.state = VIDEOBUF_DONE;
>  	buf->vb.field_count++;
>  	do_gettimeofday(&buf->vb.ts);
> @@ -177,7 +177,7 @@ static inline void vbi_buffer_filled(struct em28xx *dev,
>  				     struct em28xx_buffer *buf)
>  {
>  	/* Advice that buffer was filled */
> -	em28xx_isocdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
> +	em28xx_usbdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
>  
>  	buf->vb.state = VIDEOBUF_DONE;
>  	buf->vb.field_count++;
> @@ -226,9 +226,9 @@ static void em28xx_copy_video(struct em28xx *dev,
>  	lencopy = lencopy > remain ? remain : lencopy;
>  
>  	if ((char *)startwrite + lencopy > (char *)outp + buf->vb.size) {
> -		em28xx_isocdbg("Overflow of %zi bytes past buffer end (1)\n",
> -			       ((char *)startwrite + lencopy) -
> -			       ((char *)outp + buf->vb.size));
> +		em28xx_usbdbg("Overflow of %zi bytes past buffer end (1)\n",
> +			      ((char *)startwrite + lencopy) -
> +			      ((char *)outp + buf->vb.size));
>  		remain = (char *)outp + buf->vb.size - (char *)startwrite;
>  		lencopy = remain;
>  	}
> @@ -251,7 +251,7 @@ static void em28xx_copy_video(struct em28xx *dev,
>  
>  		if ((char *)startwrite + lencopy > (char *)outp +
>  		    buf->vb.size) {
> -			em28xx_isocdbg("Overflow of %zi bytes past buffer end"
> +			em28xx_usbdbg("Overflow of %zi bytes past buffer end"
>  				       "(2)\n",
>  				       ((char *)startwrite + lencopy) -
>  				       ((char *)outp + buf->vb.size));
> @@ -280,24 +280,24 @@ static void em28xx_copy_vbi(struct em28xx *dev,
>  	int bytesperline;
>  
>  	if (dev == NULL) {
> -		em28xx_isocdbg("dev is null\n");
> +		em28xx_usbdbg("dev is null\n");
>  		return;
>  	}
>  	bytesperline = dev->vbi_width;
>  
>  	if (dma_q == NULL) {
> -		em28xx_isocdbg("dma_q is null\n");
> +		em28xx_usbdbg("dma_q is null\n");
>  		return;
>  	}
>  	if (buf == NULL) {
>  		return;
>  	}
>  	if (p == NULL) {
> -		em28xx_isocdbg("p is null\n");
> +		em28xx_usbdbg("p is null\n");
>  		return;
>  	}
>  	if (outp == NULL) {
> -		em28xx_isocdbg("outp is null\n");
> +		em28xx_usbdbg("outp is null\n");
>  		return;
>  	}
>  
> @@ -351,9 +351,9 @@ static inline void print_err_status(struct em28xx *dev,
>  		break;
>  	}
>  	if (packet < 0) {
> -		em28xx_isocdbg("URB status %d [%s].\n",	status, errmsg);
> +		em28xx_usbdbg("URB status %d [%s].\n",	status, errmsg);
>  	} else {
> -		em28xx_isocdbg("URB packet %d, status %d [%s].\n",
> +		em28xx_usbdbg("URB packet %d, status %d [%s].\n",
>  			       packet, status, errmsg);
>  	}
>  }
> @@ -368,7 +368,7 @@ static inline void get_next_buf(struct em28xx_dmaqueue *dma_q,
>  	char *outp;
>  
>  	if (list_empty(&dma_q->active)) {
> -		em28xx_isocdbg("No active queue to serve\n");
> +		em28xx_usbdbg("No active queue to serve\n");
>  		dev->usb_ctl.vid_buf = NULL;
>  		*buf = NULL;
>  		return;
> @@ -396,7 +396,7 @@ static inline void vbi_get_next_buf(struct em28xx_dmaqueue *dma_q,
>  	char *outp;
>  
>  	if (list_empty(&dma_q->active)) {
> -		em28xx_isocdbg("No active queue to serve\n");
> +		em28xx_usbdbg("No active queue to serve\n");
>  		dev->usb_ctl.vbi_buf = NULL;
>  		*buf = NULL;
>  		return;
> @@ -457,8 +457,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
>  
>  			actual_length = urb->iso_frame_desc[i].actual_length;
>  			if (actual_length > dev->max_pkt_size) {
> -				em28xx_isocdbg("packet bigger than "
> -					       "packet size");
> +				em28xx_usbdbg("packet bigger than packet size");
>  				continue;
>  			}
>  
> @@ -476,12 +475,12 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
>  		   logic simpler. Impacts of those changes should be evaluated
>  		 */
>  		if (p[0] == 0x33 && p[1] == 0x95 && p[2] == 0x00) {
> -			em28xx_isocdbg("VBI HEADER!!!\n");
> +			em28xx_usbdbg("VBI HEADER!!!\n");
>  			/* FIXME: Should add vbi copy */
>  			continue;
>  		}
>  		if (p[0] == 0x22 && p[1] == 0x5a) {
> -			em28xx_isocdbg("Video frame %d, length=%i, %s\n", p[2],
> +			em28xx_usbdbg("Video frame %d, length=%i, %s\n", p[2],
>  				       len, (p[2] & 1) ? "odd" : "even");
>  
>  			if (dev->progressive || !(p[2] & 1)) {
> @@ -507,7 +506,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
>  			if (p[0] != 0x88 && p[0] != 0x22) {
>  				/* NOTE: no intermediate data packet header
>  				 * 88 88 88 88 when using bulk transfers */
> -				em28xx_isocdbg("frame is not complete\n");
> +				em28xx_usbdbg("frame is not complete\n");
>  				len = actual_length;
>  			} else {
>  				len = actual_length - 4;
> @@ -569,8 +568,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
>  
>  			actual_length = urb->iso_frame_desc[i].actual_length;
>  			if (actual_length > dev->max_pkt_size) {
> -				em28xx_isocdbg("packet bigger than "
> -					       "packet size");
> +				em28xx_usbdbg("packet bigger than packet size");
>  				continue;
>  			}
>  
> @@ -590,7 +588,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
>  		if (p[0] == 0x33 && p[1] == 0x95) {
>  			dev->capture_type = 0;
>  			dev->vbi_read = 0;
> -			em28xx_isocdbg("VBI START HEADER!!!\n");
> +			em28xx_usbdbg("VBI START HEADER!!!\n");
>  			dev->cur_field = p[2];
>  			p += 4;
>  			len = actual_length - 4;
> @@ -615,7 +613,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
>  			if (dev->vbi_read >= vbi_size) {
>  				/* We've already read all the VBI data, so
>  				   treat the rest as video */
> -				em28xx_isocdbg("dev->vbi_read > vbi_size\n");
> +				em28xx_usbdbg("dev->vbi_read > vbi_size\n");
>  			} else if ((dev->vbi_read + len) < vbi_size) {
>  				/* This entire frame is VBI data */
>  				if (dev->vbi_read == 0 &&
> @@ -687,7 +685,7 @@ static inline int em28xx_urb_data_copy_vbi(struct em28xx *dev, struct urb *urb)
>  				len -= 4;
>  			}
>  			if (len >= 4 && p[0] == 0x22 && p[1] == 0x5a) {
> -				em28xx_isocdbg("Video frame %d, len=%i, %s\n",
> +				em28xx_usbdbg("Video frame %d, len=%i, %s\n",
>  					       p[2], len, (p[2] & 1) ?
>  					       "odd" : "even");
>  				p += 4;
> @@ -837,7 +835,7 @@ static void buffer_release(struct videobuf_queue *vq,
>  	struct em28xx_fh       *fh   = vq->priv_data;
>  	struct em28xx          *dev  = (struct em28xx *)fh->dev;
>  
> -	em28xx_isocdbg("em28xx: called buffer_release\n");
> +	em28xx_usbdbg("em28xx: called buffer_release\n");
>  
>  	free_buffer(vq, buf);
>  }


-- 

Cheers,
Mauro

Re: [PATCH v2 16/21] em28xx: rename usb debugging module parameter and macro

[Frank Schäfer]

Am 22.12.2012 21:10, schrieb Mauro Carvalho Chehab:
> Em Thu,  8 Nov 2012 20:11:48 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Rename module parameter isoc_debug to usb_debug and macro
>> em28xx_isocdbg to em28xx_usb dbg to reflect that they are
>> used for isoc and bulk USB transfers.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-video.c |   58 +++++++++++++++----------------
>>  1 Datei geändert, 28 Zeilen hinzugefügt(+), 30 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>> index d6de1cc..f435206 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -58,13 +58,13 @@
>>  		printk(KERN_INFO "%s %s :"fmt, \
>>  			 dev->name, __func__ , ##arg); } while (0)
>>  
>> -static unsigned int isoc_debug;
>> -module_param(isoc_debug, int, 0644);
>> -MODULE_PARM_DESC(isoc_debug, "enable debug messages [isoc transfers]");
>> +static unsigned int usb_debug;
>> +module_param(usb_debug, int, 0644);
>> +MODULE_PARM_DESC(usb_debug, "enable debug messages [isoc transfers]");
> NACK: usb_debug is too generic: it could refer to control URB's, stream
> URB's, and other non-URB related USB debugging.

Depends on what you think should be the role of this debug parameter.

> Also, it can cause some harm for the ones using it.
>
> As the rest of this series don't depend on this one, I'll just skip it.
>
> IMHO, the better is to either live it as-is, to avoid breaking for
> someone with "isoc_debug" parameter on their /etc/modprobe.d, or to
> do a "deprecate" path:
>
> 	- adding a new one called "stream_debug" (or something like that);
> 	- keep the old one for a while, printing a warning message to
> point that this got removed; 
> 	- after a few kernel cycles, remove the legacy one.

So module parameters are part of the API ? Hmmm... that's new to me.

> Even better: simply unify all debug params into a single one, where 
> each bit means one type of debug, like what was done on other drivers.

Yeah, I agree, that would be the best solution.
The whole debugging code could need an overhault, but I really can't do
that all at once.

Regards,
Frank

Re: [PATCH v2 16/21] em28xx: rename usb debugging module parameter and macro

[Mauro Carvalho Chehab]

Em Sun, 23 Dec 2012 14:34:28 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 22.12.2012 21:10, schrieb Mauro Carvalho Chehab:
> > Em Thu,  8 Nov 2012 20:11:48 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Rename module parameter isoc_debug to usb_debug and macro
> >> em28xx_isocdbg to em28xx_usb dbg to reflect that they are
> >> used for isoc and bulk USB transfers.
> >>
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-video.c |   58 +++++++++++++++----------------
> >>  1 Datei geändert, 28 Zeilen hinzugefügt(+), 30 Zeilen entfernt(-)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> >> index d6de1cc..f435206 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-video.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> >> @@ -58,13 +58,13 @@
> >>  		printk(KERN_INFO "%s %s :"fmt, \
> >>  			 dev->name, __func__ , ##arg); } while (0)
> >>  
> >> -static unsigned int isoc_debug;
> >> -module_param(isoc_debug, int, 0644);
> >> -MODULE_PARM_DESC(isoc_debug, "enable debug messages [isoc transfers]");
> >> +static unsigned int usb_debug;
> >> +module_param(usb_debug, int, 0644);
> >> +MODULE_PARM_DESC(usb_debug, "enable debug messages [isoc transfers]");
> > NACK: usb_debug is too generic: it could refer to control URB's, stream
> > URB's, and other non-URB related USB debugging.
> 
> Depends on what you think should be the role of this debug parameter.

There is already one debug parameter, for example, that shows all control
URB traffic. There is another one for the I2C commands (also sent via USB).

So, the naming choice here is really unfortunate, IMHO.

> > Also, it can cause some harm for the ones using it.
> >
> > As the rest of this series don't depend on this one, I'll just skip it.
> >
> > IMHO, the better is to either live it as-is, to avoid breaking for
> > someone with "isoc_debug" parameter on their /etc/modprobe.d, or to
> > do a "deprecate" path:
> >
> > 	- adding a new one called "stream_debug" (or something like that);
> > 	- keep the old one for a while, printing a warning message to
> > point that this got removed; 
> > 	- after a few kernel cycles, remove the legacy one.
> 
> So module parameters are part of the API ? Hmmm... that's new to me.

We consider so, as modprobe refuses to load a module if a parameter vanishes.

> 
> > Even better: simply unify all debug params into a single one, where 
> > each bit means one type of debug, like what was done on other drivers.
> 
> Yeah, I agree, that would be the best solution.
> The whole debugging code could need an overhault, but I really can't do
> that all at once.

Yeah, changing it takes some time.

Regards,
Mauro

[PATCH v2 17/21] em28xx: rename some USB parameter fields in struct em28xx to clarify their role

[Frank Schäfer]

Also improve the comments.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |   20 +++++++++++---------
 drivers/media/usb/em28xx/em28xx-core.c  |    8 ++++----
 drivers/media/usb/em28xx/em28xx-dvb.c   |    4 ++--
 drivers/media/usb/em28xx/em28xx-video.c |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |   14 ++++++++------
 5 Dateien geändert, 26 Zeilen hinzugefügt(+), 22 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index bfce34d..873b52f 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3183,9 +3183,10 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	}
 
 	/* compute alternate max packet sizes */
-	dev->alt_max_pkt_size = kmalloc(sizeof(dev->alt_max_pkt_size[0]) *
+	dev->alt_max_pkt_size_isoc =
+				kmalloc(sizeof(dev->alt_max_pkt_size_isoc[0]) *
 					interface->num_altsetting, GFP_KERNEL);
-	if (dev->alt_max_pkt_size == NULL) {
+	if (dev->alt_max_pkt_size_isoc == NULL) {
 		em28xx_errdev("out of memory!\n");
 		kfree(dev);
 		retval = -ENOMEM;
@@ -3216,13 +3217,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 					break;
 				case EM28XX_EP_ANALOG:
 					has_video = true;
-					dev->alt_max_pkt_size[i] = size;
+					dev->alt_max_pkt_size_isoc[i] = size;
 					break;
 				case EM28XX_EP_DIGITAL:
 					has_dvb = true;
-					if (size > dev->dvb_max_pkt_size) {
-						dev->dvb_max_pkt_size = size;
-						dev->dvb_alt = i;
+					if (size > dev->dvb_max_pkt_size_isoc) {
+						dev->dvb_max_pkt_size_isoc =
+									  size;
+						dev->dvb_alt_isoc = i;
 					}
 					break;
 				}
@@ -3324,7 +3326,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		/* pre-allocate DVB isoc transfer buffers */
 		retval = em28xx_alloc_urbs(dev, EM28XX_DIGITAL_MODE, 0,
 					   EM28XX_DVB_NUM_BUFS,
-					   dev->dvb_max_pkt_size,
+					   dev->dvb_max_pkt_size_isoc,
 					   EM28XX_DVB_NUM_ISOC_PACKETS);
 		if (retval) {
 			goto unlock_and_free;
@@ -3344,7 +3346,7 @@ unlock_and_free:
 	mutex_unlock(&dev->lock);
 
 err_free:
-	kfree(dev->alt_max_pkt_size);
+	kfree(dev->alt_max_pkt_size_isoc);
 	kfree(dev);
 
 err:
@@ -3409,7 +3411,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 	em28xx_close_extension(dev);
 
 	if (!dev->users) {
-		kfree(dev->alt_max_pkt_size);
+		kfree(dev->alt_max_pkt_size_isoc);
 		kfree(dev);
 	}
 }
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 8b8f783..6b588e2 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -830,14 +830,14 @@ int em28xx_set_alternate(struct em28xx *dev)
 
 	for (i = 0; i < dev->num_alt; i++) {
 		/* stop when the selected alt setting offers enough bandwidth */
-		if (dev->alt_max_pkt_size[i] >= min_pkt_size) {
+		if (dev->alt_max_pkt_size_isoc[i] >= min_pkt_size) {
 			dev->alt = i;
 			break;
 		/* otherwise make sure that we end up with the maximum bandwidth
 		   because the min_pkt_size equation might be wrong...
 		*/
-		} else if (dev->alt_max_pkt_size[i] >
-			   dev->alt_max_pkt_size[dev->alt])
+		} else if (dev->alt_max_pkt_size_isoc[i] >
+			   dev->alt_max_pkt_size_isoc[dev->alt])
 			dev->alt = i;
 	}
 
@@ -845,7 +845,7 @@ set_alt:
 	if (dev->alt != prev_alt) {
 		em28xx_coredbg("minimum isoc packet size: %u (alt=%d)\n",
 				min_pkt_size, dev->alt);
-		dev->max_pkt_size = dev->alt_max_pkt_size[dev->alt];
+		dev->max_pkt_size = dev->alt_max_pkt_size_isoc[dev->alt];
 		em28xx_coredbg("setting alternate %d with wMaxPacketSize=%u\n",
 			       dev->alt, dev->max_pkt_size);
 		errCode = usb_set_interface(dev->udev, 0, dev->alt);
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index b46246a..3fc7e27 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -178,12 +178,12 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 	struct em28xx *dev = dvb->adapter.priv;
 	int max_dvb_packet_size;
 
-	usb_set_interface(dev->udev, 0, dev->dvb_alt);
+	usb_set_interface(dev->udev, 0, dev->dvb_alt_isoc);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
 
-	max_dvb_packet_size = dev->dvb_max_pkt_size;
+	max_dvb_packet_size = dev->dvb_max_pkt_size_isoc;
 	if (max_dvb_packet_size < 0)
 		return max_dvb_packet_size;
 	dprintk(1, "Using %d buffers each with %d x %d bytes\n",
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index f435206..8767c06 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2286,7 +2286,7 @@ static int em28xx_v4l2_close(struct file *filp)
 		   free the remaining resources */
 		if (dev->state & DEV_DISCONNECTED) {
 			em28xx_release_resources(dev);
-			kfree(dev->alt_max_pkt_size);
+			kfree(dev->alt_max_pkt_size_isoc);
 			mutex_unlock(&dev->lock);
 			kfree(dev);
 			kfree(fh);
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 950a717..6b8d3e6b 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -4,6 +4,7 @@
    Copyright (C) 2005 Markus Rechberger <mrechberger@gmail.com>
 		      Ludovico Cavedon <cavedon@sssup.it>
 		      Mauro Carvalho Chehab <mchehab@infradead.org>
+   Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
 
    Based on the em2800 driver from Sascha Sommer <saschasommer@freenet.de>
 
@@ -583,12 +584,13 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
-	int alt;		/* alternate */
-	int max_pkt_size;	/* max packet size of isoc transaction */
-	int num_alt;		/* Number of alternative settings */
-	unsigned int *alt_max_pkt_size;	/* array of wMaxPacketSize */
-	int dvb_alt;				/* alternate for DVB */
-	unsigned int dvb_max_pkt_size;		/* wMaxPacketSize for DVB */
+	int alt;		/* alternate setting */
+	int max_pkt_size;	/* max packet size of the selected ep at alt */
+	int num_alt;		/* number of alternative settings */
+	unsigned int *alt_max_pkt_size_isoc; /* array of isoc wMaxPacketSize */
+	int dvb_alt_isoc;	/* alternate setting for DVB isoc transfers */
+	unsigned int dvb_max_pkt_size_isoc;	/* isoc max packet size of the
+						   selected DVB ep at dvb_alt */
 	char urb_buf[URB_MAX_CTRL_SIZE];	/* urb control msg buffer */
 
 	/* helper funcs that call usb_control_msg */
-- 
1.7.10.4

[PATCH v2 18/21] em28xx: add fields for analog and DVB USB transfer type selection to struct em28xx

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx.h |    4 ++++
 1 Datei geändert, 4 Zeilen hinzugefügt(+)

diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 6b8d3e6b..f5be522 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -588,9 +588,13 @@ struct em28xx {
 	int max_pkt_size;	/* max packet size of the selected ep at alt */
 	int num_alt;		/* number of alternative settings */
 	unsigned int *alt_max_pkt_size_isoc; /* array of isoc wMaxPacketSize */
+	unsigned int analog_xfer_bulk:1;	/* use bulk instead of isoc
+						   transfers for analog      */
 	int dvb_alt_isoc;	/* alternate setting for DVB isoc transfers */
 	unsigned int dvb_max_pkt_size_isoc;	/* isoc max packet size of the
 						   selected DVB ep at dvb_alt */
+	unsigned int dvb_xfer_bulk:1;		/* use bulk instead of isoc
+						   transfers for DVB          */
 	char urb_buf[URB_MAX_CTRL_SIZE];	/* urb control msg buffer */
 
 	/* helper funcs that call usb_control_msg */
-- 
1.7.10.4

[PATCH v2 19/21] em28xx: set USB alternate settings for analog video bulk transfers properly

[Frank Schäfer]

Extend function em28xx_set_alternate:
- use alternate setting 0 for bulk transfers as default
- respect module parameter 'alt'=0 for bulk transfers
- set max_packet_size to 512 bytes for bulk transfers

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |   23 +++++++++++++++--------
 1 Datei geändert, 15 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 6b588e2..06d5734 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -805,21 +805,23 @@ int em28xx_resolution_set(struct em28xx *dev)
 	return em28xx_scaler_set(dev, dev->hscale, dev->vscale);
 }
 
+/* Set USB alternate setting for analog video */
 int em28xx_set_alternate(struct em28xx *dev)
 {
 	int errCode, prev_alt = dev->alt;
 	int i;
 	unsigned int min_pkt_size = dev->width * 2 + 4;
 
-	/*
-	 * alt = 0 is used only for control messages, so, only values
-	 * greater than 0 can be used for streaming.
-	 */
-	if (alt && alt < dev->num_alt) {
+	/* NOTE: for isoc transfers, only alt settings > 0 are allowed
+		 for bulk transfers, use alt=0 as default value */
+	dev->alt = 0;
+	if ((alt > 0) && (alt < dev->num_alt)) {
 		em28xx_coredbg("alternate forced to %d\n", dev->alt);
 		dev->alt = alt;
 		goto set_alt;
 	}
+	if (dev->analog_xfer_bulk)
+		goto set_alt;
 
 	/* When image size is bigger than a certain value,
 	   the frame size should be increased, otherwise, only
@@ -843,9 +845,14 @@ int em28xx_set_alternate(struct em28xx *dev)
 
 set_alt:
 	if (dev->alt != prev_alt) {
-		em28xx_coredbg("minimum isoc packet size: %u (alt=%d)\n",
-				min_pkt_size, dev->alt);
-		dev->max_pkt_size = dev->alt_max_pkt_size_isoc[dev->alt];
+		if (dev->analog_xfer_bulk) {
+			dev->max_pkt_size = 512; /* USB 2.0 spec */
+		} else { /* isoc */
+			em28xx_coredbg("minimum isoc packet size: "
+				       "%u (alt=%d)\n", min_pkt_size, dev->alt);
+			dev->max_pkt_size =
+					  dev->alt_max_pkt_size_isoc[dev->alt];
+		}
 		em28xx_coredbg("setting alternate %d with wMaxPacketSize=%u\n",
 			       dev->alt, dev->max_pkt_size);
 		errCode = usb_set_interface(dev->udev, 0, dev->alt);
-- 
1.7.10.4

[PATCH v2 20/21] em28xx: improve USB endpoint logic, also use bulk transfers

[Frank Schäfer]

The current enpoint logic ignores all bulk endpoints and uses
a fixed mapping between endpint addresses and the supported
data stream types (analog/audio/DVB):
  Ep 0x82, isoc	=> analog
  Ep 0x83, isoc	=> audio
  Ep 0x84, isoc	=> DVB

Now that the code can also do bulk transfers, the endpoint
logic has to be extended to also consider bulk endpoints.
The new logic preserves backwards compatibility and reflects
the endpoint configurations we have seen so far:
  Ep 0x82, isoc		=> analog
  Ep 0x82, bulk		=> analog
  Ep 0x83, isoc*	=> audio
  Ep 0x84, isoc		=> digital
  Ep 0x84, bulk		=> analog or digital**
 (*: audio should always be isoc)
 (**: analog, if ep 0x82 is isoc, otherwise digital)

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |   97 +++++++++++++++++++++++++------
 drivers/media/usb/em28xx/em28xx-core.c  |   32 ++++++++--
 drivers/media/usb/em28xx/em28xx-dvb.c   |   34 +++++++----
 drivers/media/usb/em28xx/em28xx-reg.h   |    4 +-
 drivers/media/usb/em28xx/em28xx-video.c |   10 ++--
 drivers/media/usb/em28xx/em28xx.h       |   12 ++++
 6 Dateien geändert, 149 Zeilen hinzugefügt(+), 40 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 873b52f..a9344f0 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -6,6 +6,7 @@
 		      Markus Rechberger <mrechberger@gmail.com>
 		      Mauro Carvalho Chehab <mchehab@infradead.org>
 		      Sascha Sommer <saschasommer@freenet.de>
+   Copyright (C) 2012 Frank Schäfer <fschaefer.oss@googlemail.com>
 
    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
@@ -3209,26 +3210,69 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 			if (udev->speed == USB_SPEED_HIGH)
 				size = size * hb_mult(sizedescr);
 
-			if (usb_endpoint_xfer_isoc(e) &&
-			    usb_endpoint_dir_in(e)) {
+			if (usb_endpoint_dir_in(e)) {
 				switch (e->bEndpointAddress) {
-				case EM28XX_EP_AUDIO:
-					has_audio = true;
-					break;
-				case EM28XX_EP_ANALOG:
+				case 0x82:
 					has_video = true;
-					dev->alt_max_pkt_size_isoc[i] = size;
+					if (usb_endpoint_xfer_isoc(e)) {
+						dev->analog_ep_isoc =
+							    e->bEndpointAddress;
+						dev->alt_max_pkt_size_isoc[i] = size;
+					} else if (usb_endpoint_xfer_bulk(e)) {
+						dev->analog_ep_bulk =
+							    e->bEndpointAddress;
+					}
+					break;
+				case 0x83:
+					if (usb_endpoint_xfer_isoc(e)) {
+						has_audio = true;
+					} else {
+						printk(KERN_INFO DRIVER_NAME
+						": error: skipping audio end"
+						"point 0x83, because it uses"
+						" bulk transfers !\n");
+					}
 					break;
-				case EM28XX_EP_DIGITAL:
-					has_dvb = true;
-					if (size > dev->dvb_max_pkt_size_isoc) {
-						dev->dvb_max_pkt_size_isoc =
-									  size;
-						dev->dvb_alt_isoc = i;
+				case 0x84:
+					if (has_video &&
+					    (usb_endpoint_xfer_bulk(e))) {
+						dev->analog_ep_bulk =
+							    e->bEndpointAddress;
+					} else {
+						has_dvb = true;
+						if (usb_endpoint_xfer_isoc(e)) {
+							dev->dvb_ep_isoc = e->bEndpointAddress;
+							if (size > dev->dvb_max_pkt_size_isoc) {
+								dev->dvb_max_pkt_size_isoc = size;
+								dev->dvb_alt_isoc = i;
+							}
+						} else {
+							dev->dvb_ep_bulk = e->bEndpointAddress;
+						}
 					}
 					break;
 				}
 			}
+			/* NOTE:
+			 * Old logic with support for isoc transfers only was:
+			 *  0x82	isoc		=> analog
+			 *  0x83	isoc		=> audio
+			 *  0x84	isoc		=> digital
+			 *
+			 * New logic with support for bulk transfers
+			 *  0x82	isoc		=> analog
+			 *  0x82	bulk		=> analog
+			 *  0x83	isoc*		=> audio
+			 *  0x84	isoc		=> digital
+			 *  0x84	bulk		=> analog or digital**
+			 * (*: audio should always be isoc)
+			 * (**: analog, if ep 0x82 is isoc, otherwise digital)
+			 *
+			 * The new logic preserves backwards compatibility and
+			 * reflects the endpoint configurations we have seen
+			 * so far. But there might be devices for which this
+			 * logic is not sufficient...
+			 */
 		}
 	}
 
@@ -3289,6 +3333,12 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		goto err_free;
 	}
 
+	/* Select USB transfer types to use */
+	if (has_video && !dev->analog_ep_isoc)
+		dev->analog_xfer_bulk = 1;
+	if (has_dvb && !dev->dvb_ep_isoc)
+		dev->dvb_xfer_bulk = 1;
+
 	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
 	dev->devno = nr;
 	dev->model = id->driver_info;
@@ -3323,12 +3373,23 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	}
 
 	if (has_dvb) {
-		/* pre-allocate DVB isoc transfer buffers */
-		retval = em28xx_alloc_urbs(dev, EM28XX_DIGITAL_MODE, 0,
-					   EM28XX_DVB_NUM_BUFS,
-					   dev->dvb_max_pkt_size_isoc,
-					   EM28XX_DVB_NUM_ISOC_PACKETS);
+		/* pre-allocate DVB usb transfer buffers */
+		if (dev->dvb_xfer_bulk) {
+			retval = em28xx_alloc_urbs(dev, EM28XX_DIGITAL_MODE,
+					    dev->dvb_xfer_bulk,
+					    EM28XX_DVB_NUM_BUFS,
+					    512,
+					    EM28XX_DVB_BULK_PACKET_MULTIPLIER);
+		} else {
+			retval = em28xx_alloc_urbs(dev, EM28XX_DIGITAL_MODE,
+					    dev->dvb_xfer_bulk,
+					    EM28XX_DVB_NUM_BUFS,
+					    dev->dvb_max_pkt_size_isoc,
+					    EM28XX_DVB_NUM_ISOC_PACKETS);
+		}
 		if (retval) {
+			printk(DRIVER_NAME ": Failed to pre-allocate USB"
+			       " transfer buffers for DVB.\n");
 			goto unlock_and_free;
 		}
 	}
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 06d5734..c78d38b 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -847,11 +847,13 @@ set_alt:
 	if (dev->alt != prev_alt) {
 		if (dev->analog_xfer_bulk) {
 			dev->max_pkt_size = 512; /* USB 2.0 spec */
+			dev->packet_multiplier = EM28XX_BULK_PACKET_MULTIPLIER;
 		} else { /* isoc */
 			em28xx_coredbg("minimum isoc packet size: "
 				       "%u (alt=%d)\n", min_pkt_size, dev->alt);
 			dev->max_pkt_size =
 					  dev->alt_max_pkt_size_isoc[dev->alt];
+			dev->packet_multiplier = EM28XX_NUM_ISOC_PACKETS;
 		}
 		em28xx_coredbg("setting alternate %d with wMaxPacketSize=%u\n",
 			       dev->alt, dev->max_pkt_size);
@@ -1054,10 +1056,28 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
 
 	em28xx_isocdbg("em28xx: called em28xx_alloc_isoc in mode %d\n", mode);
 
-	if (mode == EM28XX_DIGITAL_MODE)
+	/* Check mode and if we have an endpoint for the selected
+	   transfer type, select buffer				 */
+	if (mode == EM28XX_DIGITAL_MODE) {
+		if ((xfer_bulk && !dev->dvb_ep_bulk) ||
+		    (!xfer_bulk && !dev->dvb_ep_isoc)) {
+			em28xx_errdev("no endpoint for DVB mode and "
+				      "transfer type %d\n", xfer_bulk > 0);
+			return -EINVAL;
+		}
 		usb_bufs = &dev->usb_ctl.digital_bufs;
-	else
+	} else if (mode == EM28XX_ANALOG_MODE) {
+		if ((xfer_bulk && !dev->analog_ep_bulk) ||
+		    (!xfer_bulk && !dev->analog_ep_isoc)) {
+			em28xx_errdev("no endpoint for analog mode and "
+				      "transfer type %d\n", xfer_bulk > 0);
+			return -EINVAL;
+		}
 		usb_bufs = &dev->usb_ctl.analog_bufs;
+	} else {
+		em28xx_errdev("invalid mode selected\n");
+		return -EINVAL;
+	}
 
 	/* De-allocates all pending stuff */
 	em28xx_uninit_usb_xfer(dev, mode);
@@ -1113,8 +1133,8 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
 		if (xfer_bulk) { /* bulk */
 			pipe = usb_rcvbulkpipe(dev->udev,
 					       mode == EM28XX_ANALOG_MODE ?
-					       EM28XX_EP_ANALOG :
-					       EM28XX_EP_DIGITAL);
+					       dev->analog_ep_bulk :
+					       dev->dvb_ep_bulk);
 			usb_fill_bulk_urb(urb, dev->udev, pipe,
 					  usb_bufs->transfer_buffer[i], sb_size,
 					  em28xx_irq_callback, dev);
@@ -1122,8 +1142,8 @@ int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk,
 		} else { /* isoc */
 			pipe = usb_rcvisocpipe(dev->udev,
 					       mode == EM28XX_ANALOG_MODE ?
-					       EM28XX_EP_ANALOG :
-					       EM28XX_EP_DIGITAL);
+					       dev->analog_ep_isoc :
+					       dev->dvb_ep_isoc);
 			usb_fill_int_urb(urb, dev->udev, pipe,
 					 usb_bufs->transfer_buffer[i], sb_size,
 					 em28xx_irq_callback, dev, 1);
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 3fc7e27..8d44e40 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -176,25 +176,39 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 {
 	int rc;
 	struct em28xx *dev = dvb->adapter.priv;
-	int max_dvb_packet_size;
+	int dvb_max_packet_size, packet_multiplier, dvb_alt;
+
+	if (dev->dvb_xfer_bulk) {
+		if (!dev->dvb_ep_bulk)
+			return -ENODEV;
+		dvb_max_packet_size = 512; /* USB 2.0 spec */
+		packet_multiplier = EM28XX_DVB_BULK_PACKET_MULTIPLIER;
+		dvb_alt = 0;
+	} else { /* isoc */
+		if (!dev->dvb_ep_isoc)
+			return -ENODEV;
+		dvb_max_packet_size = dev->dvb_max_pkt_size_isoc;
+		if (dvb_max_packet_size < 0)
+			return dvb_max_packet_size;
+		packet_multiplier = EM28XX_DVB_NUM_ISOC_PACKETS;
+		dvb_alt = dev->dvb_alt_isoc;
+	}
 
-	usb_set_interface(dev->udev, 0, dev->dvb_alt_isoc);
+	usb_set_interface(dev->udev, 0, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
 
-	max_dvb_packet_size = dev->dvb_max_pkt_size_isoc;
-	if (max_dvb_packet_size < 0)
-		return max_dvb_packet_size;
 	dprintk(1, "Using %d buffers each with %d x %d bytes\n",
 		EM28XX_DVB_NUM_BUFS,
-		EM28XX_DVB_NUM_ISOC_PACKETS,
-		max_dvb_packet_size);
+		packet_multiplier,
+		dvb_max_packet_size);
 
-	return em28xx_init_usb_xfer(dev, EM28XX_DIGITAL_MODE, 0,
+	return em28xx_init_usb_xfer(dev, EM28XX_DIGITAL_MODE,
+				    dev->dvb_xfer_bulk,
 				    EM28XX_DVB_NUM_BUFS,
-				    max_dvb_packet_size,
-				    EM28XX_DVB_NUM_ISOC_PACKETS,
+				    dvb_max_packet_size,
+				    packet_multiplier,
 				    em28xx_dvb_urb_data_copy);
 }
 
diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
index 6ff3682..8cd3acf 100644
--- a/drivers/media/usb/em28xx/em28xx-reg.h
+++ b/drivers/media/usb/em28xx/em28xx-reg.h
@@ -13,9 +13,9 @@
 #define EM_GPO_3   (1 << 3)
 
 /* em28xx endpoints */
-#define EM28XX_EP_ANALOG	0x82
+/* 0x82:   (always ?) analog */
 #define EM28XX_EP_AUDIO		0x83
-#define EM28XX_EP_DIGITAL	0x84
+/* 0x84:   digital or analog */
 
 /* em2800 registers */
 #define EM2800_R08_AUDIOSRC 0x08
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 8767c06..4ec54fd 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -788,16 +788,18 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 
 	if (urb_init) {
 		if (em28xx_vbi_supported(dev) == 1)
-			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE, 0,
+			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE,
+						  dev->analog_xfer_bulk,
 						  EM28XX_NUM_BUFS,
 						  dev->max_pkt_size,
-						  EM28XX_NUM_ISOC_PACKETS,
+						  dev->packet_multiplier,
 						  em28xx_urb_data_copy_vbi);
 		else
-			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE, 0,
+			rc = em28xx_init_usb_xfer(dev, EM28XX_ANALOG_MODE,
+						  dev->analog_xfer_bulk,
 						  EM28XX_NUM_BUFS,
 						  dev->max_pkt_size,
-						  EM28XX_NUM_ISOC_PACKETS,
+						  dev->packet_multiplier,
 						  em28xx_urb_data_copy);
 		if (rc < 0)
 			goto fail;
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index f5be522..aa413bd 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -165,6 +165,12 @@
 #define EM28XX_NUM_ISOC_PACKETS 64
 #define EM28XX_DVB_NUM_ISOC_PACKETS 64
 
+/* bulk transfers: transfer buffer size = packet size * packet multiplier
+   USB 2.0 spec says bulk packet size is always 512 bytes
+ */
+#define EM28XX_BULK_PACKET_MULTIPLIER 384
+#define EM28XX_DVB_BULK_PACKET_MULTIPLIER 384
+
 #define EM28XX_INTERLACED_DEFAULT 1
 
 /*
@@ -584,8 +590,14 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	u8 analog_ep_isoc;	/* address of isoc endpoint for analog */
+	u8 analog_ep_bulk;	/* address of bulk endpoint for analog */
+	u8 dvb_ep_isoc;		/* address of isoc endpoint for DVB */
+	u8 dvb_ep_bulk;		/* address of bulk endpoint for DVC */
 	int alt;		/* alternate setting */
 	int max_pkt_size;	/* max packet size of the selected ep at alt */
+	int packet_multiplier;	/* multiplier for wMaxPacketSize, used for
+				   URB buffer size definition */
 	int num_alt;		/* number of alternative settings */
 	unsigned int *alt_max_pkt_size_isoc; /* array of isoc wMaxPacketSize */
 	unsigned int analog_xfer_bulk:1;	/* use bulk instead of isoc
-- 
1.7.10.4

[PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Frank Schäfer]

By default, isoc transfers are used if possible.
With the new module parameter, bulk can be selected as the
preferred USB transfer type.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |   11 +++++++++--
 1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index a9344f0..7f5b303 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -61,6 +61,11 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
 module_param_array(card,  int, NULL, 0444);
 MODULE_PARM_DESC(card,     "card type");
 
+static unsigned int prefer_bulk;
+module_param(prefer_bulk, int, 0644);
+MODULE_PARM_DESC(prefer_bulk, "prefer USB bulk transfers");
+
+
 /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
 static unsigned long em28xx_devused;
 
@@ -3334,9 +3339,11 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	}
 
 	/* Select USB transfer types to use */
-	if (has_video && !dev->analog_ep_isoc)
+	if (has_video &&
+	    (!dev->analog_ep_isoc || (prefer_bulk && dev->analog_ep_bulk)))
 		dev->analog_xfer_bulk = 1;
-	if (has_dvb && !dev->dvb_ep_isoc)
+	if (has_dvb &&
+	    (!dev->dvb_ep_isoc || (prefer_bulk && dev->dvb_ep_bulk)))
 		dev->dvb_xfer_bulk = 1;
 
 	snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
-- 
1.7.10.4

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Devin Heitmueller]

On Thu, Nov 8, 2012 at 1:11 PM, Frank Schäfer
<fschaefer.oss@googlemail.com> wrote:
> By default, isoc transfers are used if possible.
> With the new module parameter, bulk can be selected as the
> preferred USB transfer type.

Hi Frank,

Does your device actually expose both isoc and bulk endpoints?  If I
recall from the datasheet, whether isoc or bulk mode is provided is
not actually configurable from the driver.  The EEPROM dictates how
the endpoint map gets defined, and hence it's either one or the other.
 If that is indeed the case, then we don't need a modprobe option at
all (since would never actually be user configurable), and we should
just add a field to the board definition to indicate that bulk should
be used for that product.

Devin

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

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Frank Schäfer]

Am 08.11.2012 21:19, schrieb Devin Heitmueller:
> On Thu, Nov 8, 2012 at 1:11 PM, Frank Schäfer
> <fschaefer.oss@googlemail.com> wrote:
>> By default, isoc transfers are used if possible.
>> With the new module parameter, bulk can be selected as the
>> preferred USB transfer type.
> Hi Frank,
>
> Does your device actually expose both isoc and bulk endpoints?  If I
> recall from the datasheet, whether isoc or bulk mode is provided is
> not actually configurable from the driver.  The EEPROM dictates how
> the endpoint map gets defined, and hence it's either one or the other.
>  If that is indeed the case, then we don't need a modprobe option at
> all (since would never actually be user configurable), and we should
> just add a field to the board definition to indicate that bulk should
> be used for that product.
>
> Devin
>

Hi Devin,

at least the "Silvercrest Webcam 1.3mpix" (board 71) exposes both
endpoint types (0x82=isoc and 0x84=bulk).

Regards,
Frank

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Devin Heitmueller]

On Thu, Nov 8, 2012 at 1:37 PM, Frank Schäfer
<fschaefer.oss@googlemail.com> wrote:
> at least the "Silvercrest Webcam 1.3mpix" (board 71) exposes both
> endpoint types (0x82=isoc and 0x84=bulk).

Ah, interesting.  It might be worthwhile to log a warning in dmesg if
the user sets the modprobe option but the board doesn't actually
expose any bulk endpoints.  This might help avoid questions from users
(we already got one such question by somebody who believed enabling
this would put the device into bulk mode even though his hardware
didn't support it).

Devin

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

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Frank Schäfer]

Am 08.11.2012 21:46, schrieb Devin Heitmueller:
> On Thu, Nov 8, 2012 at 1:37 PM, Frank Schäfer
> <fschaefer.oss@googlemail.com> wrote:
>> at least the "Silvercrest Webcam 1.3mpix" (board 71) exposes both
>> endpoint types (0x82=isoc and 0x84=bulk).
> Ah, interesting.  It might be worthwhile to log a warning in dmesg if
> the user sets the modprobe option but the board doesn't actually
> expose any bulk endpoints.  This might help avoid questions from users
> (we already got one such question by somebody who believed enabling
> this would put the device into bulk mode even though his hardware
> didn't support it).
>
> Devin
>

Well, I deliberately called the module 'prefer_bulk' (and not
'use_bulk', 'force_bulk' ...) which should imply that nothing is guaranteed.
And selecting bulk transfers for a device which actually doesn not
provide bulk support doesn't make sense and is clearly the users fault.
Anway, I'm fine with adding a warning message and maybe I could extend
the module parameter description, too.

I'm going to wait for further feedback from Mauro before sending an
updated version of the patch (series).

Regards,
Frank

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Devin Heitmueller]

On Fri, Nov 9, 2012 at 11:00 AM, Frank Schäfer
<fschaefer.oss@googlemail.com> wrote:
> Well, I deliberately called the module 'prefer_bulk' (and not
> 'use_bulk', 'force_bulk' ...) which should imply that nothing is guaranteed.
> And selecting bulk transfers for a device which actually doesn not
> provide bulk support doesn't make sense and is clearly the users fault.
> Anway, I'm fine with adding a warning message and maybe I could extend
> the module parameter description, too.
>
> I'm going to wait for further feedback from Mauro before sending an
> updated version of the patch (series).

Yeah, none of this should hold up it being merged as-is.  A patch
adding the warning can be submitted after the fact.

Devin

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

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Mauro Carvalho Chehab]

Hi Frank,

Em Thu,  8 Nov 2012 20:11:53 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> By default, isoc transfers are used if possible.
> With the new module parameter, bulk can be selected as the
> preferred USB transfer type.

I did some tests yesterday with prefer_bulk. IMHO, webcams should
select bulk mode by default, as this allows more than one camera to
work at the same time (I tested yesterday with 3 Silvercrest ones on
my notebook). With ISOC transfers, the core won't let it to happen, as
a single camera reserves 51% of the max allowed isoc traffic.

> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c |   11 +++++++++--
>  1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index a9344f0..7f5b303 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -61,6 +61,11 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
>  module_param_array(card,  int, NULL, 0444);
>  MODULE_PARM_DESC(card,     "card type");
>  
> +static unsigned int prefer_bulk;
> +module_param(prefer_bulk, int, 0644);

This needs to be changed to 0444, as prefer_bulk doesn't allow changing
it dynamically, as the test is done during device probe, not at stream on.

Regards,
Mauro

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Frank Schäfer]

Am 23.12.2012 14:44, schrieb Mauro Carvalho Chehab:
> Hi Frank,
>
> Em Thu,  8 Nov 2012 20:11:53 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> By default, isoc transfers are used if possible.
>> With the new module parameter, bulk can be selected as the
>> preferred USB transfer type.
> I did some tests yesterday with prefer_bulk. IMHO, webcams should
> select bulk mode by default, as this allows more than one camera to
> work at the same time (I tested yesterday with 3 Silvercrest ones on
> my notebook). With ISOC transfers, the core won't let it to happen, as
> a single camera reserves 51% of the max allowed isoc traffic.

Ok. I just didn't want to change the current behavior because of
potential regressions.
Why not change it for all devices ? Frame data processing with bulk
transfers has a smaller overhead than with isoc (although not really
measurable ;) ).

I will send a patch after christmas.

>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-cards.c |   11 +++++++++--
>>  1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>> index a9344f0..7f5b303 100644
>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>> @@ -61,6 +61,11 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
>>  module_param_array(card,  int, NULL, 0444);
>>  MODULE_PARM_DESC(card,     "card type");
>>  
>> +static unsigned int prefer_bulk;
>> +module_param(prefer_bulk, int, 0644);
> This needs to be changed to 0444, as prefer_bulk doesn't allow changing
> it dynamically, as the test is done during device probe, not at stream on.

Good catch !
Can you fix it ? I'm a bit in hurry right now.
Otherwise I will try to send a patch tomorrow.

Merry Christmas !
Frank

>
> Regards,
> Mauro

Re: [PATCH v2 21/21] em28xx: add module parameter for selection of the preferred USB transfer type

[Mauro Carvalho Chehab]

Em Sun, 23 Dec 2012 15:01:26 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 23.12.2012 14:44, schrieb Mauro Carvalho Chehab:
> > Hi Frank,
> >
> > Em Thu,  8 Nov 2012 20:11:53 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> By default, isoc transfers are used if possible.
> >> With the new module parameter, bulk can be selected as the
> >> preferred USB transfer type.
> > I did some tests yesterday with prefer_bulk. IMHO, webcams should
> > select bulk mode by default, as this allows more than one camera to
> > work at the same time (I tested yesterday with 3 Silvercrest ones on
> > my notebook). With ISOC transfers, the core won't let it to happen, as
> > a single camera reserves 51% of the max allowed isoc traffic.
> 
> Ok. I just didn't want to change the current behavior because of
> potential regressions.
> Why not change it for all devices ? Frame data processing with bulk
> transfers has a smaller overhead than with isoc (although not really
> measurable ;) ).

It is better to keep it as-is for the other devices. There are simply too
much non-webcam devices for us to be sure that this will always work. 
As there are very few webcams supported, the risk of this change is low
if applied only to webcams.

> I will send a patch after christmas.
> 
> >
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-cards.c |   11 +++++++++--
> >>  1 Datei geändert, 9 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >> index a9344f0..7f5b303 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >> @@ -61,6 +61,11 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
> >>  module_param_array(card,  int, NULL, 0444);
> >>  MODULE_PARM_DESC(card,     "card type");
> >>  
> >> +static unsigned int prefer_bulk;
> >> +module_param(prefer_bulk, int, 0644);
> > This needs to be changed to 0444, as prefer_bulk doesn't allow changing
> > it dynamically, as the test is done during device probe, not at stream on.
> 
> Good catch !
> Can you fix it ? I'm a bit in hurry right now.
> Otherwise I will try to send a patch tomorrow.

Yeah, I can do it.

> Merry Christmas !
> Frank

Merry Christmas!

-- 

Cheers,
Mauro