* [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.