* [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format()
@ 2013-08-03 8:50 Eldad Zack
2013-08-03 8:50 ` [PATCH v2 1/9] ALSA: usb-audio: remove disabled debug code in set_format Eldad Zack
` (7 more replies)
0 siblings, 8 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Hi Takashi,
This is a low-priority series (applies against current for-next).
This series refactors set_format() and moves the sync endpoint
initilization code into separate function. The quirks for
implicit feedback are moved to a separate function as well.
The intended result is code that is easier to read without
function changes.
Changes from v2:
* Cleaned up the patchset, indentations and whitespace fixes.
* Patch #2: removed early returns when subs->sync_endpoint is already set.
This change now follows original semantics.
* Patch #9: I first corrected it thanks to Clemens Ladisch's feedback, but
then I wasn't sure if it improves anything, so I dropped it entirely.
* Dropped patch #10. Instead, added patch (now #9) to WARN_ON and return
NULL in snd_usb_add_endpoint. Otherwise, the check should be done at each
call site, and it "shouldn't happen" anyway.
Cheers,
Eldad
v1:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-July/064268.html
Eldad Zack (9):
ALSA: usb-audio: remove disabled debug code in set_format
ALSA: usb-audio: remove assignment from if condition
ALSA: usb-audio: separate sync endpoint setting from set_format
ALSA: usb-audio: move implicit fb quirks to separate function
ALSA: usb-audio: reverse condition logic in set_sync_endpoint
ALSA: usb-audio: do not initialize and check implicit_fb
ALSA: usb-audio: remove is_playback from implicit feedback quirks
ALSA: usb-audio: remove implicit_fb from quirk
ALSA: usb-audio: WARN_ON when alts is passed as NULL
sound/usb/endpoint.c | 3 +
sound/usb/pcm.c | 243 +++++++++++++++++++++++++++++----------------------
2 files changed, 141 insertions(+), 105 deletions(-)
--
1.8.1.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/9] ALSA: usb-audio: remove disabled debug code in set_format
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-03 8:50 ` [PATCH v2 2/9] ALSA: usb-audio: remove assignment from if condition Eldad Zack
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Code block does not compile when enabled.
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
sound/usb/pcm.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 15b151e..3d3e8d1 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -486,15 +486,6 @@ add_sync_ep:
snd_usb_set_format_quirk(subs, fmt);
-#if 0
- printk(KERN_DEBUG
- "setting done: format = %d, rate = %d..%d, channels = %d\n",
- fmt->format, fmt->rate_min, fmt->rate_max, fmt->channels);
- printk(KERN_DEBUG
- " datapipe = 0x%0x, syncpipe = 0x%0x\n",
- subs->datapipe, subs->syncpipe);
-#endif
-
return 0;
}
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/9] ALSA: usb-audio: remove assignment from if condition
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
2013-08-03 8:50 ` [PATCH v2 1/9] ALSA: usb-audio: remove disabled debug code in set_format Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-03 8:50 ` [PATCH v2 3/9] ALSA: usb-audio: separate sync endpoint setting from set_format Eldad Zack
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Following general kernel style.
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
sound/usb/pcm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 3d3e8d1..be5c7c2 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -479,7 +479,8 @@ add_sync_ep:
subs->data_endpoint->sync_master = subs->sync_endpoint;
}
- if ((err = snd_usb_init_pitch(subs->stream->chip, fmt->iface, alts, fmt)) < 0)
+ err = snd_usb_init_pitch(subs->stream->chip, fmt->iface, alts, fmt);
+ if (err < 0)
return err;
subs->cur_audiofmt = fmt;
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/9] ALSA: usb-audio: separate sync endpoint setting from set_format
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
2013-08-03 8:50 ` [PATCH v2 1/9] ALSA: usb-audio: remove disabled debug code in set_format Eldad Zack
2013-08-03 8:50 ` [PATCH v2 2/9] ALSA: usb-audio: remove assignment from if condition Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-03 8:50 ` [PATCH v2 4/9] ALSA: usb-audio: move implicit fb quirks to separate function Eldad Zack
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Setting the sync endpoint currently takes up about half of set_format().
Move it to a dedicated function.
No functional change.
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
sound/usb/pcm.c | 128 ++++++++++++++++++++++++++++++++------------------------
1 file changed, 73 insertions(+), 55 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index be5c7c2..e24ce7d 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -327,64 +327,17 @@ static int search_roland_implicit_fb(struct usb_device *dev, int ifnum,
return 0;
}
-/*
- * find a matching format and set up the interface
- */
-static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
+
+static int set_sync_endpoint(struct snd_usb_substream *subs,
+ struct audioformat *fmt,
+ struct usb_device *dev,
+ struct usb_host_interface *alts,
+ struct usb_interface_descriptor *altsd)
{
- struct usb_device *dev = subs->dev;
- struct usb_host_interface *alts;
- struct usb_interface_descriptor *altsd;
struct usb_interface *iface;
- unsigned int ep, attr;
int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
- int err, implicit_fb = 0;
-
- iface = usb_ifnum_to_if(dev, fmt->iface);
- if (WARN_ON(!iface))
- return -EINVAL;
- alts = &iface->altsetting[fmt->altset_idx];
- altsd = get_iface_desc(alts);
- if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting))
- return -EINVAL;
-
- if (fmt == subs->cur_audiofmt)
- return 0;
-
- /* close the old interface */
- if (subs->interface >= 0 && subs->interface != fmt->iface) {
- err = usb_set_interface(subs->dev, subs->interface, 0);
- if (err < 0) {
- snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n",
- dev->devnum, fmt->iface, fmt->altsetting, err);
- return -EIO;
- }
- subs->interface = -1;
- subs->altset_idx = 0;
- }
-
- /* set interface */
- if (subs->interface != fmt->iface ||
- subs->altset_idx != fmt->altset_idx) {
- err = usb_set_interface(dev, fmt->iface, fmt->altsetting);
- if (err < 0) {
- snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n",
- dev->devnum, fmt->iface, fmt->altsetting, err);
- return -EIO;
- }
- snd_printdd(KERN_INFO "setting usb interface %d:%d\n",
- fmt->iface, fmt->altsetting);
- subs->interface = fmt->iface;
- subs->altset_idx = fmt->altset_idx;
-
- snd_usb_set_interface_quirk(dev);
- }
-
- subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
- alts, fmt->endpoint, subs->direction,
- SND_USB_ENDPOINT_TYPE_DATA);
- if (!subs->data_endpoint)
- return -EINVAL;
+ unsigned int ep, attr;
+ int implicit_fb = 0;
/* we need a sync pipe in async OUT or adaptive IN mode */
/* check the number of EP, since some devices have broken
@@ -479,6 +432,71 @@ add_sync_ep:
subs->data_endpoint->sync_master = subs->sync_endpoint;
}
+ return 0;
+}
+
+/*
+ * find a matching format and set up the interface
+ */
+static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
+{
+ struct usb_device *dev = subs->dev;
+ struct usb_host_interface *alts;
+ struct usb_interface_descriptor *altsd;
+ struct usb_interface *iface;
+ int err;
+
+ iface = usb_ifnum_to_if(dev, fmt->iface);
+ if (WARN_ON(!iface))
+ return -EINVAL;
+ alts = &iface->altsetting[fmt->altset_idx];
+ altsd = get_iface_desc(alts);
+ if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting))
+ return -EINVAL;
+
+ if (fmt == subs->cur_audiofmt)
+ return 0;
+
+ /* close the old interface */
+ if (subs->interface >= 0 && subs->interface != fmt->iface) {
+ err = usb_set_interface(subs->dev, subs->interface, 0);
+ if (err < 0) {
+ snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n",
+ dev->devnum, fmt->iface, fmt->altsetting, err);
+ return -EIO;
+ }
+ subs->interface = -1;
+ subs->altset_idx = 0;
+ }
+
+ /* set interface */
+ if (subs->interface != fmt->iface ||
+ subs->altset_idx != fmt->altset_idx) {
+ err = usb_set_interface(dev, fmt->iface, fmt->altsetting);
+ if (err < 0) {
+ snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n",
+ dev->devnum, fmt->iface, fmt->altsetting, err);
+ return -EIO;
+ }
+ snd_printdd(KERN_INFO "setting usb interface %d:%d\n",
+ fmt->iface, fmt->altsetting);
+ subs->interface = fmt->iface;
+ subs->altset_idx = fmt->altset_idx;
+
+ snd_usb_set_interface_quirk(dev);
+ }
+
+ subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+ alts, fmt->endpoint, subs->direction,
+ SND_USB_ENDPOINT_TYPE_DATA);
+
+ if (!subs->data_endpoint)
+ return -EINVAL;
+
+ err = set_sync_endpoint(subs, fmt, dev, alts, altsd);
+ if (err < 0)
+ return err;
+
err = snd_usb_init_pitch(subs->stream->chip, fmt->iface, alts, fmt);
if (err < 0)
return err;
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/9] ALSA: usb-audio: move implicit fb quirks to separate function
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
` (2 preceding siblings ...)
2013-08-03 8:50 ` [PATCH v2 3/9] ALSA: usb-audio: separate sync endpoint setting from set_format Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-03 8:50 ` [PATCH v2 5/9] ALSA: usb-audio: reverse condition logic in set_sync_endpoint Eldad Zack
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Separate setting implicit feedback quirks from setting
a sync endpoint (which may also be explicit feedback or async).
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
v2: removed check to return early if subs->sync_endpoint is
already set, as this is a functional change (although it is
initialized to NULL and set to NULL on disconnect).
---
sound/usb/pcm.c | 60 ++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 15 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index e24ce7d..0016f28 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -327,24 +327,16 @@ static int search_roland_implicit_fb(struct usb_device *dev, int ifnum,
return 0;
}
-
-static int set_sync_endpoint(struct snd_usb_substream *subs,
- struct audioformat *fmt,
- struct usb_device *dev,
- struct usb_host_interface *alts,
- struct usb_interface_descriptor *altsd)
+static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
+ struct usb_device *dev,
+ struct usb_interface_descriptor *altsd,
+ unsigned int attr)
{
+ struct usb_host_interface *alts;
struct usb_interface *iface;
int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
- unsigned int ep, attr;
int implicit_fb = 0;
-
- /* we need a sync pipe in async OUT or adaptive IN mode */
- /* check the number of EP, since some devices have broken
- * descriptors which fool us. if it has only one EP,
- * assume it as adaptive-out or sync-in.
- */
- attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
+ unsigned int ep;
switch (subs->stream->chip->usb_id) {
case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
@@ -388,6 +380,45 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
goto add_sync_ep;
}
+ /* No quirk */
+ return 0;
+
+add_sync_ep:
+ subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+ alts, ep, !subs->direction,
+ implicit_fb ?
+ SND_USB_ENDPOINT_TYPE_DATA :
+ SND_USB_ENDPOINT_TYPE_SYNC);
+ if (!subs->sync_endpoint)
+ return -EINVAL;
+
+ subs->data_endpoint->sync_master = subs->sync_endpoint;
+
+ return 0;
+}
+
+static int set_sync_endpoint(struct snd_usb_substream *subs,
+ struct audioformat *fmt,
+ struct usb_device *dev,
+ struct usb_host_interface *alts,
+ struct usb_interface_descriptor *altsd)
+{
+ int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
+ unsigned int ep, attr;
+ int implicit_fb = 0;
+ int err;
+
+ /* we need a sync pipe in async OUT or adaptive IN mode */
+ /* check the number of EP, since some devices have broken
+ * descriptors which fool us. if it has only one EP,
+ * assume it as adaptive-out or sync-in.
+ */
+ attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
+
+ err = set_sync_ep_implicit_fb_quirk(subs, dev, altsd, attr);
+ if (err < 0)
+ return err;
+
if (((is_playback && attr == USB_ENDPOINT_SYNC_ASYNC) ||
(!is_playback && attr == USB_ENDPOINT_SYNC_ADAPTIVE)) &&
altsd->bNumEndpoints >= 2) {
@@ -420,7 +451,6 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
implicit_fb = (get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_USAGE_MASK)
== USB_ENDPOINT_USAGE_IMPLICIT_FB;
-add_sync_ep:
subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
alts, ep, !subs->direction,
implicit_fb ?
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 5/9] ALSA: usb-audio: reverse condition logic in set_sync_endpoint
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
` (3 preceding siblings ...)
2013-08-03 8:50 ` [PATCH v2 4/9] ALSA: usb-audio: move implicit fb quirks to separate function Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-03 8:50 ` [PATCH v2 6/9] ALSA: usb-audio: do not initialize and check implicit_fb Eldad Zack
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Reverse logic on the conditions required to qualify for a sync endpoint
and remove one level of indendation.
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
v2: identation and whitespace corrections
---
sound/usb/pcm.c | 81 ++++++++++++++++++++++++++++++---------------------------
1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 0016f28..c31dbdc 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -419,49 +419,52 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
if (err < 0)
return err;
- if (((is_playback && attr == USB_ENDPOINT_SYNC_ASYNC) ||
- (!is_playback && attr == USB_ENDPOINT_SYNC_ADAPTIVE)) &&
- altsd->bNumEndpoints >= 2) {
- /* check sync-pipe endpoint */
- /* ... and check descriptor size before accessing bSynchAddress
- because there is a version of the SB Audigy 2 NX firmware lacking
- the audio fields in the endpoint descriptors */
- if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
- (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
- get_endpoint(alts, 1)->bSynchAddress != 0 &&
- !implicit_fb)) {
- snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. bmAttributes %02x, bLength %d, bSynchAddress %02x\n",
- dev->devnum, fmt->iface, fmt->altsetting,
- get_endpoint(alts, 1)->bmAttributes,
- get_endpoint(alts, 1)->bLength,
- get_endpoint(alts, 1)->bSynchAddress);
- return -EINVAL;
- }
- ep = get_endpoint(alts, 1)->bEndpointAddress;
- if (!implicit_fb &&
- get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
- (( is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress | USB_DIR_IN)) ||
- (!is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress & ~USB_DIR_IN)))) {
- snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
- dev->devnum, fmt->iface, fmt->altsetting,
- is_playback, ep, get_endpoint(alts, 0)->bSynchAddress);
- return -EINVAL;
- }
-
- implicit_fb = (get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_USAGE_MASK)
- == USB_ENDPOINT_USAGE_IMPLICIT_FB;
+ if (altsd->bNumEndpoints < 2)
+ return 0;
- subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
- alts, ep, !subs->direction,
- implicit_fb ?
- SND_USB_ENDPOINT_TYPE_DATA :
- SND_USB_ENDPOINT_TYPE_SYNC);
- if (!subs->sync_endpoint)
- return -EINVAL;
+ if ((is_playback && attr != USB_ENDPOINT_SYNC_ASYNC) ||
+ (!is_playback && attr != USB_ENDPOINT_SYNC_ADAPTIVE))
+ return 0;
- subs->data_endpoint->sync_master = subs->sync_endpoint;
+ /* check sync-pipe endpoint */
+ /* ... and check descriptor size before accessing bSynchAddress
+ because there is a version of the SB Audigy 2 NX firmware lacking
+ the audio fields in the endpoint descriptors */
+ if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
+ (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
+ get_endpoint(alts, 1)->bSynchAddress != 0 &&
+ !implicit_fb)) {
+ snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. bmAttributes %02x, bLength %d, bSynchAddress %02x\n",
+ dev->devnum, fmt->iface, fmt->altsetting,
+ get_endpoint(alts, 1)->bmAttributes,
+ get_endpoint(alts, 1)->bLength,
+ get_endpoint(alts, 1)->bSynchAddress);
+ return -EINVAL;
+ }
+ ep = get_endpoint(alts, 1)->bEndpointAddress;
+ if (!implicit_fb &&
+ get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
+ ((is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress | USB_DIR_IN)) ||
+ (!is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress & ~USB_DIR_IN)))) {
+ snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
+ dev->devnum, fmt->iface, fmt->altsetting,
+ is_playback, ep, get_endpoint(alts, 0)->bSynchAddress);
+ return -EINVAL;
}
+ implicit_fb = (get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_USAGE_MASK)
+ == USB_ENDPOINT_USAGE_IMPLICIT_FB;
+
+ subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+ alts, ep, !subs->direction,
+ implicit_fb ?
+ SND_USB_ENDPOINT_TYPE_DATA :
+ SND_USB_ENDPOINT_TYPE_SYNC);
+ if (!subs->sync_endpoint)
+ return -EINVAL;
+
+ subs->data_endpoint->sync_master = subs->sync_endpoint;
+
return 0;
}
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 6/9] ALSA: usb-audio: do not initialize and check implicit_fb
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
` (4 preceding siblings ...)
2013-08-03 8:50 ` [PATCH v2 5/9] ALSA: usb-audio: reverse condition logic in set_sync_endpoint Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-03 8:50 ` [PATCH v2 7/9] ALSA: usb-audio: remove is_playback from implicit feedback quirks Eldad Zack
2013-08-06 8:56 ` [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Takashi Iwai
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
Since implicit_fb is not changed, !implicit_fb will always
be true - it is set only after these checks.
Similarly, there's also no need to set it at the top of the function.
Change the type of implicit_fb to bool (more appropriate).
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
v2: implicit_fb type set to bool
---
sound/usb/pcm.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c31dbdc..bb2e0f5 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -405,7 +405,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
{
int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
unsigned int ep, attr;
- int implicit_fb = 0;
+ bool implicit_fb;
int err;
/* we need a sync pipe in async OUT or adaptive IN mode */
@@ -432,8 +432,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
the audio fields in the endpoint descriptors */
if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
(get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
- get_endpoint(alts, 1)->bSynchAddress != 0 &&
- !implicit_fb)) {
+ get_endpoint(alts, 1)->bSynchAddress != 0)) {
snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. bmAttributes %02x, bLength %d, bSynchAddress %02x\n",
dev->devnum, fmt->iface, fmt->altsetting,
get_endpoint(alts, 1)->bmAttributes,
@@ -442,8 +441,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
return -EINVAL;
}
ep = get_endpoint(alts, 1)->bEndpointAddress;
- if (!implicit_fb &&
- get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
+ if (get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
((is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress | USB_DIR_IN)) ||
(!is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress & ~USB_DIR_IN)))) {
snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 7/9] ALSA: usb-audio: remove is_playback from implicit feedback quirks
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
` (5 preceding siblings ...)
2013-08-03 8:50 ` [PATCH v2 6/9] ALSA: usb-audio: do not initialize and check implicit_fb Eldad Zack
@ 2013-08-03 8:50 ` Eldad Zack
2013-08-06 8:56 ` [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Takashi Iwai
7 siblings, 0 replies; 9+ messages in thread
From: Eldad Zack @ 2013-08-03 8:50 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Clemens Ladisch, Eldad Zack, Daniel Mack
An implicit feedback endpoint can only be a capture source. The
consumer (sink) of the implicit feedback endpoint is therefore limited
to playback EPs.
Check if the target endpoint is a playback first and remove redundant
checks.
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
sound/usb/pcm.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index bb2e0f5..af30e08 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -334,41 +334,39 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
{
struct usb_host_interface *alts;
struct usb_interface *iface;
- int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
int implicit_fb = 0;
unsigned int ep;
+ /* Implicit feedback sync EPs consumers are always playback EPs */
+ if (subs->direction != SNDRV_PCM_STREAM_PLAYBACK)
+ return 0;
+
switch (subs->stream->chip->usb_id) {
case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
case USB_ID(0x0763, 0x2031): /* M-Audio Fast Track C600 */
- if (is_playback) {
- implicit_fb = 1;
- ep = 0x81;
- iface = usb_ifnum_to_if(dev, 3);
+ implicit_fb = 1;
+ ep = 0x81;
+ iface = usb_ifnum_to_if(dev, 3);
- if (!iface || iface->num_altsetting == 0)
- return -EINVAL;
+ if (!iface || iface->num_altsetting == 0)
+ return -EINVAL;
- alts = &iface->altsetting[1];
- goto add_sync_ep;
- }
+ alts = &iface->altsetting[1];
+ goto add_sync_ep;
break;
case USB_ID(0x0763, 0x2080): /* M-Audio FastTrack Ultra */
case USB_ID(0x0763, 0x2081):
- if (is_playback) {
- implicit_fb = 1;
- ep = 0x81;
- iface = usb_ifnum_to_if(dev, 2);
+ implicit_fb = 1;
+ ep = 0x81;
+ iface = usb_ifnum_to_if(dev, 2);
- if (!iface || iface->num_altsetting == 0)
- return -EINVAL;
+ if (!iface || iface->num_altsetting == 0)
+ return -EINVAL;
- alts = &iface->altsetting[1];
- goto add_sync_ep;
- }
+ alts = &iface->altsetting[1];
+ goto add_sync_ep;
}
- if (is_playback &&
- attr == USB_ENDPOINT_SYNC_ASYNC &&
+ if (attr == USB_ENDPOINT_SYNC_ASYNC &&
altsd->bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
altsd->bInterfaceProtocol == 2 &&
altsd->bNumEndpoints == 1 &&
--
1.8.1.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format()
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
` (6 preceding siblings ...)
2013-08-03 8:50 ` [PATCH v2 7/9] ALSA: usb-audio: remove is_playback from implicit feedback quirks Eldad Zack
@ 2013-08-06 8:56 ` Takashi Iwai
7 siblings, 0 replies; 9+ messages in thread
From: Takashi Iwai @ 2013-08-06 8:56 UTC (permalink / raw)
To: Eldad Zack; +Cc: alsa-devel, Clemens Ladisch, Daniel Mack
At Sat, 3 Aug 2013 10:50:14 +0200,
Eldad Zack wrote:
>
> Hi Takashi,
>
> This is a low-priority series (applies against current for-next).
> This series refactors set_format() and moves the sync endpoint
> initilization code into separate function. The quirks for
> implicit feedback are moved to a separate function as well.
>
> The intended result is code that is easier to read without
> function changes.
>
> Changes from v2:
> * Cleaned up the patchset, indentations and whitespace fixes.
> * Patch #2: removed early returns when subs->sync_endpoint is already set.
> This change now follows original semantics.
> * Patch #9: I first corrected it thanks to Clemens Ladisch's feedback, but
> then I wasn't sure if it improves anything, so I dropped it entirely.
> * Dropped patch #10. Instead, added patch (now #9) to WARN_ON and return
> NULL in snd_usb_add_endpoint. Otherwise, the check should be done at each
> call site, and it "shouldn't happen" anyway.
Thanks, now I applied all patches to for-next branch.
Takashi
>
> Cheers,
> Eldad
>
> v1:
> http://mailman.alsa-project.org/pipermail/alsa-devel/2013-July/064268.html
>
> Eldad Zack (9):
> ALSA: usb-audio: remove disabled debug code in set_format
> ALSA: usb-audio: remove assignment from if condition
> ALSA: usb-audio: separate sync endpoint setting from set_format
> ALSA: usb-audio: move implicit fb quirks to separate function
> ALSA: usb-audio: reverse condition logic in set_sync_endpoint
> ALSA: usb-audio: do not initialize and check implicit_fb
> ALSA: usb-audio: remove is_playback from implicit feedback quirks
> ALSA: usb-audio: remove implicit_fb from quirk
> ALSA: usb-audio: WARN_ON when alts is passed as NULL
>
> sound/usb/endpoint.c | 3 +
> sound/usb/pcm.c | 243 +++++++++++++++++++++++++++++----------------------
> 2 files changed, 141 insertions(+), 105 deletions(-)
>
> --
> 1.8.1.5
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-06 8:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-03 8:50 [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Eldad Zack
2013-08-03 8:50 ` [PATCH v2 1/9] ALSA: usb-audio: remove disabled debug code in set_format Eldad Zack
2013-08-03 8:50 ` [PATCH v2 2/9] ALSA: usb-audio: remove assignment from if condition Eldad Zack
2013-08-03 8:50 ` [PATCH v2 3/9] ALSA: usb-audio: separate sync endpoint setting from set_format Eldad Zack
2013-08-03 8:50 ` [PATCH v2 4/9] ALSA: usb-audio: move implicit fb quirks to separate function Eldad Zack
2013-08-03 8:50 ` [PATCH v2 5/9] ALSA: usb-audio: reverse condition logic in set_sync_endpoint Eldad Zack
2013-08-03 8:50 ` [PATCH v2 6/9] ALSA: usb-audio: do not initialize and check implicit_fb Eldad Zack
2013-08-03 8:50 ` [PATCH v2 7/9] ALSA: usb-audio: remove is_playback from implicit feedback quirks Eldad Zack
2013-08-06 8:56 ` [PATCH v2 0/9] ALSA: usb-audio: Refactor set_format() Takashi Iwai
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.