Skip to contentSkip to conversation

em28xx: fix usb alternate setting for analog and digital video endpoints > 0

PATCH In review Frank Schäfer 2 participants 7 messages Jan 18, 2013 – Feb 6, 2013

Subsystems (2)
Media (V4L/DVB) EM28XX VIDEO4LINUX driver
lore
Share & tools
lore

Frank Schäfer

author
#0 lore raw

PATCH 1/1 em28xx: fix usb alternate setting for analog and digital video endpoints > 0

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +5 -5
index 2fdb66e..cdbfe0a 100644
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
 
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +1 -1
index ce4f252..210859a 100644
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */
Signature · 1 line
1.7.10.4
#1 lore raw

Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 20 lines

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +0 -0
index 2fdb66e..cdbfe0a 100644

drivers/media/usb/em28xx/em28xx-audio.c static int snd_em28xx_capture_open(struct snd_pcm_substream *substream) · line 287

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-audio.c +2 -2
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
 
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)

Please don't merge a non-fix change (variable rename) with a fix.

Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Quoted 39 lines
drivers/media/usb/em28xx/em28xx-audio.c +3 -3
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index ce4f252..210859a 100644

drivers/media/usb/em28xx/em28xx-core.c set_alt: · line 868

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -1
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);

This hunk doesn't apply upstream:

patching file drivers/media/usb/em28xx/em28xx-core.c
Hunk #1 FAILED at 862.
1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

Quoted 34 lines
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */
Signature · 2 lines
Cheers,
Mauro
author
#2 lore raw

Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:

Quoted 25 lines

Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 22 lines

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +0 -0
index 2fdb66e..cdbfe0a 100644
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
Quoted 6 lines

drivers/media/usb/em28xx/em28xx-audio.c static int snd_em28xx_capture_open(struct snd_pcm_substream *substream) · line 287

Quoted 5 lines
drivers/media/usb/em28xx/em28xx-audio.c +2 -2
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)

Please don't merge a non-fix change (variable rename) with a fix.

Ok, sorry, it seems to be trivial...

Quoted 1 line

Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Did you read the complete patch ? ;)
Or do you really want the video interface number to be called audio_ifnum ?

Quoted 49 lines
Quoted 39 lines
drivers/media/usb/em28xx/em28xx-audio.c +3 -3
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index ce4f252..210859a 100644

drivers/media/usb/em28xx/em28xx-core.c set_alt: · line 868

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -1
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);

This hunk doesn't apply upstream:

Quoted 3 lines

patching file drivers/media/usb/em28xx/em28xx-core.c
Hunk #1 FAILED at 862.
1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

It applies after

http://patchwork.linuxtv.org/patch/16197/

has been applied.

Regards,
Frank

Quoted 36 lines
Quoted 35 lines
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */
#3 lore raw

Em Tue, 05 Feb 2013 22:37:50 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 33 lines

Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:

Quoted 25 lines

Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 22 lines

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +0 -0
index 2fdb66e..cdbfe0a 100644
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
Quoted 6 lines

drivers/media/usb/em28xx/em28xx-audio.c static int snd_em28xx_capture_open(struct snd_pcm_substream *substream) · line 287

Quoted 5 lines
drivers/media/usb/em28xx/em28xx-audio.c +2 -2
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)

Please don't merge a non-fix change (variable rename) with a fix.

Quoted 6 lines

Ok, sorry, it seems to be trivial...

Quoted 1 line

Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Did you read the complete patch ? ;)
Or do you really want the video interface number to be called audio_ifnum ?

There are two types of em28xx audio vendor class. In one type, the audio IF is the same as the video one, but on the other one, it is different. That's why audio_ifnum were added in the first place.

See this commit:

commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6

Author: Mauro Carvalho Chehab <[email protected]>
Date:   Fri Jun 17 15:15:12 2011 -0300

[media] em28xx: Add support for devices with a separate audio interface

Some devices use a separate interface for the vendor audio class.

Signed-off-by Mauro Carvalho Chehab <[email protected]>
Quoted 100 lines
Quoted 50 lines
Quoted 40 lines
drivers/media/usb/em28xx/em28xx-audio.c +3 -3
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index ce4f252..210859a 100644

drivers/media/usb/em28xx/em28xx-core.c set_alt: · line 868

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -1
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);

This hunk doesn't apply upstream:

Quoted 3 lines

patching file drivers/media/usb/em28xx/em28xx-core.c
Hunk #1 FAILED at 862.
1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

It applies after

http://patchwork.linuxtv.org/patch/16197/

has been applied.

Regards,
Frank

Quoted 36 lines
Quoted 35 lines
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */
Signature · 2 lines
Cheers,
Mauro
author
#4 lore raw

Am 05.02.2013 23:06, schrieb Mauro Carvalho Chehab:

Quoted 35 lines

Em Tue, 05 Feb 2013 22:37:50 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 32 lines

Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:

Quoted 31 lines

Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 20 lines

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +0 -0
index 2fdb66e..cdbfe0a 100644

drivers/media/usb/em28xx/em28xx-audio.c static int snd_em28xx_capture_open(struct snd_pcm_substream *substream) · line 287

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-audio.c +2 -2
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
 
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)

Please don't merge a non-fix change (variable rename) with a fix.

Quoted 7 lines
Quoted 5 lines

Ok, sorry, it seems to be trivial...

Quoted 1 line

Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Did you read the complete patch ? ;)
Or do you really want the video interface number to be called audio_ifnum ?

There are two types of em28xx audio vendor class. In one type, the audio IF is the same as the video one, but on the other one, it is different.

Sure, but if I'm not misunderstanding the code, we have two device instances with separate device structs when audio is on a separate interface.
Hence we don't need two fields for the interface number in the struct and that's why renamed it.

Regards,
Frank

Quoted 109 lines

That's why audio_ifnum were added in the first place.

See this commit:

commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6

Author: Mauro Carvalho Chehab <[email protected]>
Date:   Fri Jun 17 15:15:12 2011 -0300

[media] em28xx: Add support for devices with a separate audio interface

Some devices use a separate interface for the vendor audio class.

Signed-off-by Mauro Carvalho Chehab <[email protected]>
Quoted 95 lines
Quoted 49 lines
Quoted 39 lines
drivers/media/usb/em28xx/em28xx-audio.c +3 -3
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index ce4f252..210859a 100644

drivers/media/usb/em28xx/em28xx-core.c set_alt: · line 868

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -1
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);

This hunk doesn't apply upstream:

Quoted 3 lines

patching file drivers/media/usb/em28xx/em28xx-core.c
Hunk #1 FAILED at 862.
1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

It applies after

http://patchwork.linuxtv.org/patch/16197/

has been applied.

Regards,
Frank

Quoted 34 lines
Quoted 34 lines
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */
#5 lore raw

Em Wed, 06 Feb 2013 16:31:41 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 44 lines

Am 05.02.2013 23:06, schrieb Mauro Carvalho Chehab:

Quoted 35 lines

Em Tue, 05 Feb 2013 22:37:50 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 32 lines

Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:

Quoted 31 lines

Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 20 lines

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +0 -0
index 2fdb66e..cdbfe0a 100644

drivers/media/usb/em28xx/em28xx-audio.c static int snd_em28xx_capture_open(struct snd_pcm_substream *substream) · line 287

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-audio.c +2 -2
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
 
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)

Please don't merge a non-fix change (variable rename) with a fix.

Quoted 7 lines
Quoted 5 lines

Ok, sorry, it seems to be trivial...

Quoted 1 line

Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Did you read the complete patch ? ;)
Or do you really want the video interface number to be called audio_ifnum ?

There are two types of em28xx audio vendor class. In one type, the audio IF is the same as the video one, but on the other one, it is different.

Quoted 6 lines

Sure, but if I'm not misunderstanding the code, we have two device instances with separate device structs when audio is on a separate interface.

Hence we don't need two fields for the interface number in the struct and that's why renamed it.

No. The way the extension mechanism was written, the same data instance is used by all modules. This is what is there at em28xx core:

void em28xx_init_extension(struct em28xx *dev)

{
	const struct em28xx_ops *ops = NULL;

	mutex_lock(&em28xx_devlist_mutex);
	list_add_tail(&dev->devlist, &em28xx_devlist);
	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
		if (ops->init)
			ops->init(dev);
	}
	mutex_unlock(&em28xx_devlist_mutex);
}

The ops->init() receives the data from the main driver. It doesn't matter if the USB interface is the same or not.

If the changes at the probing code changed that, then it broke audio support for several devices.

Quoted 114 lines

Regards,
Frank

Quoted 109 lines

That's why audio_ifnum were added in the first place.

See this commit:

commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6

Author: Mauro Carvalho Chehab <[email protected]>
Date:   Fri Jun 17 15:15:12 2011 -0300

[media] em28xx: Add support for devices with a separate audio interface

Some devices use a separate interface for the vendor audio class.

Signed-off-by Mauro Carvalho Chehab <[email protected]>
Quoted 95 lines
Quoted 49 lines
Quoted 39 lines
drivers/media/usb/em28xx/em28xx-audio.c +3 -3
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index ce4f252..210859a 100644

drivers/media/usb/em28xx/em28xx-core.c set_alt: · line 868

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -1
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);

This hunk doesn't apply upstream:

Quoted 3 lines

patching file drivers/media/usb/em28xx/em28xx-core.c
Hunk #1 FAILED at 862.
1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

It applies after

http://patchwork.linuxtv.org/patch/16197/

has been applied.

Regards,
Frank

Quoted 34 lines
Quoted 34 lines
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */
Signature · 2 lines
Cheers,
Mauro
author
#6 lore raw

Am 06.02.2013 17:03, schrieb Mauro Carvalho Chehab:

Quoted 68 lines

Em Wed, 06 Feb 2013 16:31:41 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 41 lines

Am 05.02.2013 23:06, schrieb Mauro Carvalho Chehab:

Quoted 40 lines

Em Tue, 05 Feb 2013 22:37:50 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 37 lines

Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:

Quoted 25 lines

Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 22 lines

While the current code handles sound interfaces with a number > 0 correctly, it assumes that the interface number for analog + digital video is always 0 when changing the alternate setting.

(NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
 drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
 drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
 drivers/media/usb/em28xx/em28xx.h       |    3 +--

5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-audio.c +0 -0
index 2fdb66e..cdbfe0a 100644
@@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
 	}
Quoted 6 lines

drivers/media/usb/em28xx/em28xx-audio.c static int snd_em28xx_capture_open(struct snd_pcm_substream *substream) · line 287

Quoted 5 lines
drivers/media/usb/em28xx/em28xx-audio.c +2 -2
 	runtime->hw = snd_em28xx_hw_capture;
-	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
-		if (dev->audio_ifnum)
+	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
+		if (dev->ifnum)

Please don't merge a non-fix change (variable rename) with a fix.

Ok, sorry, it seems to be trivial...

Quoted 1 line

Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Did you read the complete patch ? ;)
Or do you really want the video interface number to be called audio_ifnum ?

Quoted 8 lines
Quoted 2 lines

There are two types of em28xx audio vendor class. In one type, the audio IF is the same as the video one, but on the other one, it is different.

Sure, but if I'm not misunderstanding the code, we have two device instances with separate device structs when audio is on a separate interface.

Hence we don't need two fields for the interface number in the struct and that's why renamed it.

No. The way the extension mechanism was written, the same data instance is used by all modules. This is what is there at em28xx core:

void em28xx_init_extension(struct em28xx *dev)

{
	const struct em28xx_ops *ops = NULL;

	mutex_lock(&em28xx_devlist_mutex);
	list_add_tail(&dev->devlist, &em28xx_devlist);
	list_for_each_entry(ops, &em28xx_extension_devlist, next) {
		if (ops->init)
			ops->init(dev);
	}
	mutex_unlock(&em28xx_devlist_mutex);
}
Quoted 2 lines

The ops->init() receives the data from the main driver. It doesn't matter if the USB interface is the same or not.

Sure, but the device struct pointer "dev" passed to the module is different.

Quoted 2 lines

If the changes at the probing code changed that, then it broke audio support for several devices.

If I'm not misunderstanding the code completely, em28xx_usb_probe() is called for each USB interface.
If an analog video, audio or DVB endpoint ist detected, it creates a new device instance (which has it's own device struct).

I don't know if this has ever been working differently, but at least it didn't change recently. I also think this is how it should be.

Unfortunately I don't have one of these device for testing, so I don't know if audio is currently broken for them.
What I can say for sure is, that devices with the isoc video/DVB endpoints at an interface > 0 are currently broken, which is what I'm trying to fix with this patch.

Regards,
Frank

Quoted 113 lines
Quoted 113 lines

Regards,
Frank

Quoted 109 lines

That's why audio_ifnum were added in the first place.

See this commit:

commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6

Author: Mauro Carvalho Chehab <[email protected]>
Date:   Fri Jun 17 15:15:12 2011 -0300

[media] em28xx: Add support for devices with a separate audio interface

Some devices use a separate interface for the vendor audio class.

Signed-off-by Mauro Carvalho Chehab <[email protected]>
Quoted 95 lines
Quoted 49 lines
Quoted 39 lines
drivers/media/usb/em28xx/em28xx-audio.c +3 -3
 			dev->alt = 1;
 		else
 			dev->alt = 7;
 
 		dprintk("changing alternate number on interface %d to %d\n",
-			dev->audio_ifnum, dev->alt);
-		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
+			dev->ifnum, dev->alt);
+		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 
 		/* Sets volume, mute, etc */
 		dev->mute = 0;

@@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
 	static int          devnr;
 	int                 err;
 
-	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
+	if (!dev->has_alsa_audio) {
 		/* This device does not support the extension (in this case
 		   the device is expecting the snd-usb-audio module or
 		   doesn't have analog audio support at all) */
drivers/media/usb/em28xx/em28xx-cards.c +1 -1
index 0a5aa62..553db17 100644
@@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 	dev->alt   = -1;
 	dev->is_audio_only = has_audio && !(has_video || has_dvb);
 	dev->has_alsa_audio = has_audio;
-	dev->audio_ifnum = ifnum;
+	dev->ifnum = ifnum;
 
 	/* Checks if audio is provided by some interface */
 	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index ce4f252..210859a 100644

drivers/media/usb/em28xx/em28xx-core.c set_alt: · line 868

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -1
@@ -862,7 +862,7 @@ set_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);
+	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
 	if (errCode < 0) {
 		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
 				dev->alt, errCode);

This hunk doesn't apply upstream:

Quoted 3 lines

patching file drivers/media/usb/em28xx/em28xx-core.c
Hunk #1 FAILED at 862.
1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

It applies after

http://patchwork.linuxtv.org/patch/16197/

has been applied.

Regards,
Frank

Quoted 34 lines
Quoted 34 lines
drivers/media/usb/em28xx/em28xx-dvb.c +1 -1
index a81ec2e..dbeed6c 100644
@@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 		dvb_alt = dev->dvb_alt_isoc;
 	}
 
-	usb_set_interface(dev->udev, 0, dvb_alt);
+	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
 	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
 	if (rc < 0)
 		return rc;
drivers/media/usb/em28xx/em28xx.h +1 -2
index 5f0b2c5..0dc5b73 100644
@@ -487,8 +487,6 @@ struct em28xx {
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-	int audio_ifnum;
-
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* provides ac97 mute and volume overrides */

@@ -597,6 +595,7 @@ struct em28xx {
 
 	/* usb transfer */
 	struct usb_device *udev;	/* the usb device */
+	int ifnum;			/* usb interface number */
 	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 */