* [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
@ 2013-01-18 17:25 Frank Schäfer
2013-02-05 20:57 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 7+ messages in thread
From: Frank Schäfer @ 2013-01-18 17:25 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, Frank Schäfer
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 <fschaefer.oss@googlemail.com>
---
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(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 2fdb66e..cdbfe0a 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -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) */
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 0a5aa62..553db17 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -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++) {
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index ce4f252..210859a 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -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);
diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index a81ec2e..dbeed6c 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -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;
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 5f0b2c5..0dc5b73 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -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 */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
2013-01-18 17:25 [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0 Frank Schäfer
@ 2013-02-05 20:57 ` Mauro Carvalho Chehab
2013-02-05 21:37 ` Frank Schäfer
0 siblings, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-02-05 20:57 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Fri, 18 Jan 2013 18:25:48 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> 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 <fschaefer.oss@googlemail.com>
> ---
> 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(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> index 2fdb66e..cdbfe0a 100644
> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> @@ -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.
> 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) */
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 0a5aa62..553db17 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -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++) {
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index ce4f252..210859a 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -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
> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> index a81ec2e..dbeed6c 100644
> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> @@ -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;
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 5f0b2c5..0dc5b73 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -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 */
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
2013-02-05 20:57 ` Mauro Carvalho Chehab
@ 2013-02-05 21:37 ` Frank Schäfer
2013-02-05 22:06 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 7+ messages in thread
From: Frank Schäfer @ 2013-02-05 21:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:
> Em Fri, 18 Jan 2013 18:25:48 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> 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 <fschaefer.oss@googlemail.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>> index 2fdb66e..cdbfe0a 100644
>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>> @@ -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.
Ok, sorry, it seems to be trivial...
> 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 ?
>> 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) */
>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>> index 0a5aa62..553db17 100644
>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>> @@ -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++) {
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>> index ce4f252..210859a 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -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
It applies after
http://patchwork.linuxtv.org/patch/16197/
has been applied.
Regards,
Frank
>
>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>> index a81ec2e..dbeed6c 100644
>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>> @@ -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;
>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>> index 5f0b2c5..0dc5b73 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -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 */
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
2013-02-05 21:37 ` Frank Schäfer
@ 2013-02-05 22:06 ` Mauro Carvalho Chehab
2013-02-06 15:31 ` Frank Schäfer
0 siblings, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-02-05 22:06 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Tue, 05 Feb 2013 22:37:50 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:
> > Em Fri, 18 Jan 2013 18:25:48 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> 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 <fschaefer.oss@googlemail.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> >> index 2fdb66e..cdbfe0a 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> >> @@ -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.
>
> Ok, sorry, it seems to be trivial...
>
> > 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 <mchehab@redhat.com>
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 <mchehab@redhat.com>
>
> >> 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) */
> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >> index 0a5aa62..553db17 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >> @@ -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++) {
> >> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> >> index ce4f252..210859a 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-core.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> >> @@ -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
>
> It applies after
>
> http://patchwork.linuxtv.org/patch/16197/
>
> has been applied.
>
> Regards,
> Frank
>
> >
> >> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> >> index a81ec2e..dbeed6c 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> >> @@ -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;
> >> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> >> index 5f0b2c5..0dc5b73 100644
> >> --- a/drivers/media/usb/em28xx/em28xx.h
> >> +++ b/drivers/media/usb/em28xx/em28xx.h
> >> @@ -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 */
> >
>
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
2013-02-05 22:06 ` Mauro Carvalho Chehab
@ 2013-02-06 15:31 ` Frank Schäfer
2013-02-06 16:03 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 7+ messages in thread
From: Frank Schäfer @ 2013-02-06 15:31 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List
Am 05.02.2013 23:06, schrieb Mauro Carvalho Chehab:
> Em Tue, 05 Feb 2013 22:37:50 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:
>>> Em Fri, 18 Jan 2013 18:25:48 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> 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 <fschaefer.oss@googlemail.com>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>>> index 2fdb66e..cdbfe0a 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>>> @@ -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.
>> Ok, sorry, it seems to be trivial...
>>
>>> 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
> That's why audio_ifnum were added in the first place.
>
> See this commit:
>
> commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6
> Author: Mauro Carvalho Chehab <mchehab@redhat.com>
> 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 <mchehab@redhat.com>
>
>>>> 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) */
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>>> index 0a5aa62..553db17 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>>> @@ -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++) {
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>>>> index ce4f252..210859a 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>>>> @@ -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
>> It applies after
>>
>> http://patchwork.linuxtv.org/patch/16197/
>>
>> has been applied.
>>
>> Regards,
>> Frank
>>
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>> index a81ec2e..dbeed6c 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>> @@ -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;
>>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>>> index 5f0b2c5..0dc5b73 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>>> @@ -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 */
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
2013-02-06 15:31 ` Frank Schäfer
@ 2013-02-06 16:03 ` Mauro Carvalho Chehab
2013-02-06 17:40 ` Frank Schäfer
0 siblings, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-02-06 16:03 UTC (permalink / raw)
To: Frank Schäfer; +Cc: Linux Media Mailing List
Em Wed, 06 Feb 2013 16:31:41 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 05.02.2013 23:06, schrieb Mauro Carvalho Chehab:
> > Em Tue, 05 Feb 2013 22:37:50 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:
> >>> Em Fri, 18 Jan 2013 18:25:48 +0100
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> 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 <fschaefer.oss@googlemail.com>
> >>>> ---
> >>>> 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(-)
> >>>>
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> >>>> index 2fdb66e..cdbfe0a 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> >>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> >>>> @@ -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.
> >> Ok, sorry, it seems to be trivial...
> >>
> >>> 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.
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.
>
> Regards,
> Frank
>
> > That's why audio_ifnum were added in the first place.
> >
> > See this commit:
> >
> > commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6
> > Author: Mauro Carvalho Chehab <mchehab@redhat.com>
> > 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 <mchehab@redhat.com>
> >
> >>>> 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) */
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >>>> index 0a5aa62..553db17 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >>>> @@ -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++) {
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> >>>> index ce4f252..210859a 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx-core.c
> >>>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> >>>> @@ -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
> >> It applies after
> >>
> >> http://patchwork.linuxtv.org/patch/16197/
> >>
> >> has been applied.
> >>
> >> Regards,
> >> Frank
> >>
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> >>>> index a81ec2e..dbeed6c 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> >>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> >>>> @@ -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;
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> >>>> index 5f0b2c5..0dc5b73 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx.h
> >>>> +++ b/drivers/media/usb/em28xx/em28xx.h
> >>>> @@ -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 */
>
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0
2013-02-06 16:03 ` Mauro Carvalho Chehab
@ 2013-02-06 17:40 ` Frank Schäfer
0 siblings, 0 replies; 7+ messages in thread
From: Frank Schäfer @ 2013-02-06 17:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List
Am 06.02.2013 17:03, schrieb Mauro Carvalho Chehab:
> Em Wed, 06 Feb 2013 16:31:41 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 05.02.2013 23:06, schrieb Mauro Carvalho Chehab:
>>> Em Tue, 05 Feb 2013 22:37:50 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:
>>>>> Em Fri, 18 Jan 2013 18:25:48 +0100
>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>
>>>>>> 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 <fschaefer.oss@googlemail.com>
>>>>>> ---
>>>>>> 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(-)
>>>>>>
>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>>> index 2fdb66e..cdbfe0a 100644
>>>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>>> @@ -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.
>>>> Ok, sorry, it seems to be trivial...
>>>>
>>>>> 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.
> 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.
Sure, but the device struct pointer "dev" passed to the module is different.
> 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
>
>> Regards,
>> Frank
>>
>>> That's why audio_ifnum were added in the first place.
>>>
>>> See this commit:
>>>
>>> commit 4f83e7b3ef938eb9a01eadf81a0f3b2c67d3afb6
>>> Author: Mauro Carvalho Chehab <mchehab@redhat.com>
>>> 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 <mchehab@redhat.com>
>>>
>>>>>> 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) */
>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>>>>> index 0a5aa62..553db17 100644
>>>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>>>>> @@ -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++) {
>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>>>>>> index ce4f252..210859a 100644
>>>>>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>>>>>> @@ -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
>>>> It applies after
>>>>
>>>> http://patchwork.linuxtv.org/patch/16197/
>>>>
>>>> has been applied.
>>>>
>>>> Regards,
>>>> Frank
>>>>
>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>>> index a81ec2e..dbeed6c 100644
>>>>>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>>>>>> @@ -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;
>>>>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>>>>> index 5f0b2c5..0dc5b73 100644
>>>>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>>>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>>>>> @@ -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 */
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-02-06 17:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-18 17:25 [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0 Frank Schäfer
2013-02-05 20:57 ` Mauro Carvalho Chehab
2013-02-05 21:37 ` Frank Schäfer
2013-02-05 22:06 ` Mauro Carvalho Chehab
2013-02-06 15:31 ` Frank Schäfer
2013-02-06 16:03 ` Mauro Carvalho Chehab
2013-02-06 17:40 ` Frank Schäfer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.