[PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

This patch completes the support for Digidesign Mbox 1 sound card by 
adding capture.

The issue with this card was that it has two endpoints on the same
interface.  This patch creates a dual endpoint quirk.
The quirk interface needs a second audioformat struct for this to work
which I called ".data2".

Signed-off-by: Damien Zammit <damien@zamaudio.com>


[-- Attachment #2: 0001-ALSA-usb-audio-mbox1.patch --]
[-- Type: text/x-patch, Size: 5524 bytes --]

This patch completes the support for Digidesign Mbox 1 sound card by adding capture.

The issue with this card is that it has two endpoints on the same
interface.  This patch creates a dual endpoint quirk.
The quirk interface needs a second audioformat struct for this to work
which I called ".data2".

Signed-off-by: Damien Zammit <damien@zamaudio.com>
---
 sound/usb/quirks-table.h |   25 +++++++++++++++
 sound/usb/quirks.c       |   79 ++++++++++++++++++++++++++++++++++++++++++++++
 sound/usb/usbaudio.h     |    2 ++
 3 files changed, 106 insertions(+)

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index f652b10..0ea9f00 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2895,7 +2895,32 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 					.channels = 2,
 					.iface = 1,
 					.altsetting = 1,
+	                                .altset_idx = 1,
+					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
+					.endpoint = 0x02,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_44100 |
+						 SNDRV_PCM_RATE_48000,
+					.rate_min = 44100,
+					.rate_max = 48000,
+					.nr_rates = 2,
+					.rate_table = (unsigned int[]) {
+						44100, 48000
+					}
+				},
+				.data2 = &(const struct audioformat) {
+					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+					.channels = 2,
+					.iface = 1,
+					.altsetting = 1,
 					.altset_idx = 1,
+					.attributes = 0x00,
+					.endpoint = 0x81,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_44100 |
+						 SNDRV_PCM_RATE_48000,
 					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
 					.endpoint = 0x02,
 					.ep_attr = 0x01,
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 8973070..3ad6aa2 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -387,6 +387,84 @@ static int create_autodetect_quirks(struct snd_usb_audio *chip,
 }
 
 /*
+ * create 2 streams for an interface without proper descriptors but with dual endpoints
+ */
+static int create_fixed_dual_stream_quirk(struct snd_usb_audio *chip,
+				     struct usb_interface *iface,
+				     struct usb_driver *driver,
+				     const struct snd_usb_audio_quirk *quirk)
+{
+	struct audioformat *fp1;
+	struct audioformat *fp2;
+	struct usb_host_interface *alts;
+	int stream1, stream2, err;
+	unsigned *rate_table = NULL;
+
+	fp1 = kmemdup(quirk->data, sizeof(*fp1), GFP_KERNEL);
+	if (!fp1) {
+		snd_printk(KERN_ERR "cannot memdup 1\n");
+		return -ENOMEM;
+	}
+	fp2 = kmemdup(quirk->data2, sizeof(*fp2), GFP_KERNEL);
+	if (!fp2) {
+		snd_printk(KERN_ERR "cannot memdup 2\n");
+		return -ENOMEM;
+	}
+	if (fp1->nr_rates > MAX_NR_RATES) {
+		kfree(fp1);
+		kfree(fp2);
+		return -EINVAL;
+	}
+	if (fp1->nr_rates > 0) {
+		rate_table = kmemdup(fp1->rate_table,
+				     sizeof(int) * fp1->nr_rates, GFP_KERNEL);
+		if (!rate_table) {
+			kfree(fp1);
+			kfree(fp2);
+			return -ENOMEM;
+		}
+		fp1->rate_table = rate_table;
+		fp2->rate_table = rate_table;
+	}
+
+	stream1 = (fp1->endpoint & USB_DIR_IN)
+		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+	stream2 = (fp2->endpoint & USB_DIR_IN)
+		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+	err = snd_usb_add_audio_stream(chip, stream1, fp1);
+	if (err < 0) {
+		kfree(fp1);
+		kfree(fp2);
+		kfree(rate_table);
+		return err;
+	}
+	err = snd_usb_add_audio_stream(chip, stream2, fp2);
+	if (err < 0) {
+		kfree(fp1);
+		kfree(fp2);
+		kfree(rate_table);
+		return err;
+	}
+	if (fp1->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
+	    fp1->altset_idx >= iface->num_altsetting ||
+	    fp2->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
+	    fp2->altset_idx >= iface->num_altsetting
+	    ) {
+		kfree(fp1);
+		kfree(fp2);
+		kfree(rate_table);
+		return -EINVAL;
+	}
+	alts = &iface->altsetting[fp1->altset_idx];
+	fp1->datainterval = fp2->datainterval = snd_usb_parse_datainterval(chip, alts);
+	fp1->maxpacksize = fp2->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+	usb_set_interface(chip->dev, fp1->iface, 0);
+	snd_usb_init_pitch(chip, fp1->iface, alts, fp1);
+	snd_usb_init_sample_rate(chip, fp1->iface, alts, fp1, fp1->rate_max);
+	return 0;
+}
+
+/*
  * Create a stream for an Edirol UA-700/UA-25/UA-4FX interface.  
  * The only way to detect the sample rate is by looking at wMaxPacketSize.
  */
@@ -528,6 +606,7 @@ int snd_usb_create_quirk(struct snd_usb_audio *chip,
 		[QUIRK_MIDI_FTDI] = create_any_midi_quirk,
 		[QUIRK_AUDIO_STANDARD_INTERFACE] = create_standard_audio_quirk,
 		[QUIRK_AUDIO_FIXED_ENDPOINT] = create_fixed_stream_quirk,
+		[QUIRK_AUDIO_FIXED_DUAL_ENDPOINT] = create_fixed_dual_stream_quirk,
 		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk,
 		[QUIRK_AUDIO_ALIGN_TRANSFER] = create_align_transfer_quirk,
 		[QUIRK_AUDIO_STANDARD_MIXER] = create_standard_mixer_quirk,
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 5d2fe05..66fd867 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -86,6 +86,7 @@ enum quirk_type {
 	QUIRK_MIDI_FTDI,
 	QUIRK_AUDIO_STANDARD_INTERFACE,
 	QUIRK_AUDIO_FIXED_ENDPOINT,
+	QUIRK_AUDIO_FIXED_DUAL_ENDPOINT,
 	QUIRK_AUDIO_EDIROL_UAXX,
 	QUIRK_AUDIO_ALIGN_TRANSFER,
 	QUIRK_AUDIO_STANDARD_MIXER,
@@ -99,6 +100,7 @@ struct snd_usb_audio_quirk {
 	int16_t ifnum;
 	uint16_t type;
 	const void *data;
+	const void *data2;
 };
 
 #define combine_word(s)    ((*(s)) | ((unsigned int)(s)[1] << 8))
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

[-- Attachment #1: Type: text/plain, Size: 122 bytes --]

This second patch removes a few extra lines I added by mistake when I 
merged my private repo to the one for submission.


[-- Attachment #2: 0002-Removed-accidental-extra-lines.patch --]
[-- Type: text/x-patch, Size: 890 bytes --]

[PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

This second patch removes a few extra lines I added by mistake when I merged my private repo to the one for submission.

Signed-off-by: Damien Zammit <damien@zamaudio.com>
---
 sound/usb/quirks-table.h |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index 0ea9f00..f799359 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2921,11 +2921,6 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 					.maxpacksize = 0x130,
 					.rates = SNDRV_PCM_RATE_44100 |
 						 SNDRV_PCM_RATE_48000,
-					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
-					.endpoint = 0x02,
-					.ep_attr = 0x01,
-					.rates = SNDRV_PCM_RATE_44100 |
-						 SNDRV_PCM_RATE_48000,
 					.rate_min = 44100,
 					.rate_max = 48000,
 					.nr_rates = 2,
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Clemens Ladisch]

Damien Zammit wrote:
> This patch creates a dual endpoint quirk.
> The quirk interface needs a second audioformat struct for this to work
> which I called ".data2".

Couldn't you just let .data point to an array of two structs?


Regards,
Clemens

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

On 27/01/14 03:52, Clemens Ladisch wrote:
>> This patch creates a dual endpoint quirk.
>> >The quirk interface needs a second audioformat struct for this to work
>> >which I called ".data2".
> Couldn't you just let .data point to an array of two structs?

Thanks Clemens, I have created a new patch using this suggestion.

Damien

[-- Attachment #2: 0001-Cleaned-up-Digidesign-Mbox-1-patch.patch --]
[-- Type: text/x-patch, Size: 6038 bytes --]

This patch creates a dual endpoint quirk.
The quirk uses .data as an array of two structs.

Signed-off-by: Damien Zammit <damien@zamaudio.com>
---
 sound/usb/quirks-table.h |   57 ++++++++++++++++++++++-----------
 sound/usb/quirks.c       |   78 ++++++++++++++++++++++++++++++++++++++++++++++
 sound/usb/usbaudio.h     |    1 +
 3 files changed, 118 insertions(+), 18 deletions(-)

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index f652b10..bdbc931 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2889,23 +2889,45 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 			},
 			{
 				.ifnum = 1,
-				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
-				.data = &(const struct audioformat) {
-					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
-					.channels = 2,
-					.iface = 1,
-					.altsetting = 1,
-					.altset_idx = 1,
-					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
-					.endpoint = 0x02,
-					.ep_attr = 0x01,
-					.rates = SNDRV_PCM_RATE_44100 |
-						 SNDRV_PCM_RATE_48000,
-					.rate_min = 44100,
-					.rate_max = 48000,
-					.nr_rates = 2,
-					.rate_table = (unsigned int[]) {
-						44100, 48000
+				.type = QUIRK_AUDIO_FIXED_DUAL_ENDPOINT,
+				.data = (const struct audioformat[]) {
+					[0] = {
+						.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+						.channels = 2,
+						.iface = 1,
+						.altsetting = 1,
+						.altset_idx = 1,
+						.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
+						.endpoint = 0x02,
+						.ep_attr = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
+						.maxpacksize = 0x130,
+						.rates = SNDRV_PCM_RATE_44100 |
+							 SNDRV_PCM_RATE_48000,
+						.rate_min = 44100,
+						.rate_max = 48000,
+						.nr_rates = 2,
+						.rate_table = (unsigned int[]) {
+							44100, 48000
+						}
+					},
+					[1] = {
+						.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+						.channels = 2,
+						.iface = 1,
+						.altsetting = 1,
+						.altset_idx = 1,
+						.attributes = 0x00,
+						.endpoint = 0x81,
+						.ep_attr = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+						.maxpacksize = 0x130,
+						.rates = SNDRV_PCM_RATE_44100 |
+							 SNDRV_PCM_RATE_48000,
+						.rate_min = 44100,
+						.rate_max = 48000,
+						.nr_rates = 2,
+						.rate_table = (unsigned int[]) {
+							44100, 48000
+						}
 					}
 				}
 			},
@@ -2913,7 +2935,6 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 				.ifnum = -1
 			}
 		}
-
 	}
 },
 
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 8973070..df2de44 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -387,6 +387,83 @@ static int create_autodetect_quirks(struct snd_usb_audio *chip,
 }
 
 /*
+ * create 2 streams for an interface without proper descriptors but with dual endpoints
+ */
+static int create_fixed_dual_stream_quirk(struct snd_usb_audio *chip,
+				     struct usb_interface *iface,
+				     struct usb_driver *driver,
+				     const struct snd_usb_audio_quirk *quirk)
+{
+	struct audioformat *fp1, *fp2;
+	struct usb_host_interface *alts;
+	int stream1, stream2, err;
+	unsigned *rate_table = NULL;
+
+	fp1 = kmemdup((const struct audioformat*)quirk->data, sizeof(*fp1), GFP_KERNEL);
+	if (!fp1) {
+		snd_printk(KERN_ERR "cannot memdup 1\n");
+		return -ENOMEM;
+	}
+	fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
+	if (!fp2) {
+		snd_printk(KERN_ERR "cannot memdup 2\n");
+		return -ENOMEM;
+	}
+	if (fp1->nr_rates > MAX_NR_RATES) {
+		kfree(fp1);
+		kfree(fp2);
+		return -EINVAL;
+	}
+	if (fp1->nr_rates > 0) {
+		rate_table = kmemdup(fp1->rate_table,
+				     sizeof(int) * fp1->nr_rates, GFP_KERNEL);
+		if (!rate_table) {
+			kfree(fp1);
+			kfree(fp2);
+			return -ENOMEM;
+		}
+		fp1->rate_table = rate_table;
+		fp2->rate_table = rate_table;
+	}
+
+	stream1 = (fp1->endpoint & USB_DIR_IN)
+		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+	stream2 = (fp2->endpoint & USB_DIR_IN)
+		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+	err = snd_usb_add_audio_stream(chip, stream1, fp1);
+	if (err < 0) {
+		kfree(fp1);
+		kfree(fp2);
+		kfree(rate_table);
+		return err;
+	}
+	err = snd_usb_add_audio_stream(chip, stream2, fp2);
+	if (err < 0) {
+		kfree(fp1);
+		kfree(fp2);
+		kfree(rate_table);
+		return err;
+	}
+	if (fp1->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
+	    fp1->altset_idx >= iface->num_altsetting ||
+	    fp2->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
+	    fp2->altset_idx >= iface->num_altsetting
+	    ) {
+		kfree(fp1);
+		kfree(fp2);
+		kfree(rate_table);
+		return -EINVAL;
+	}
+	alts = &iface->altsetting[fp1->altset_idx];
+	fp1->datainterval = fp2->datainterval = snd_usb_parse_datainterval(chip, alts);
+	fp1->maxpacksize = fp2->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+	usb_set_interface(chip->dev, fp1->iface, 0);
+	snd_usb_init_pitch(chip, fp1->iface, alts, fp1);
+	snd_usb_init_sample_rate(chip, fp1->iface, alts, fp1, fp1->rate_max);
+	return 0;
+}
+
+/*
  * Create a stream for an Edirol UA-700/UA-25/UA-4FX interface.  
  * The only way to detect the sample rate is by looking at wMaxPacketSize.
  */
@@ -528,6 +605,7 @@ int snd_usb_create_quirk(struct snd_usb_audio *chip,
 		[QUIRK_MIDI_FTDI] = create_any_midi_quirk,
 		[QUIRK_AUDIO_STANDARD_INTERFACE] = create_standard_audio_quirk,
 		[QUIRK_AUDIO_FIXED_ENDPOINT] = create_fixed_stream_quirk,
+		[QUIRK_AUDIO_FIXED_DUAL_ENDPOINT] = create_fixed_dual_stream_quirk,
 		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk,
 		[QUIRK_AUDIO_ALIGN_TRANSFER] = create_align_transfer_quirk,
 		[QUIRK_AUDIO_STANDARD_MIXER] = create_standard_mixer_quirk,
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 5d2fe05..4025670 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -86,6 +86,7 @@ enum quirk_type {
 	QUIRK_MIDI_FTDI,
 	QUIRK_AUDIO_STANDARD_INTERFACE,
 	QUIRK_AUDIO_FIXED_ENDPOINT,
+	QUIRK_AUDIO_FIXED_DUAL_ENDPOINT,
 	QUIRK_AUDIO_EDIROL_UAXX,
 	QUIRK_AUDIO_ALIGN_TRANSFER,
 	QUIRK_AUDIO_STANDARD_MIXER,
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

Hi Clemens,

I wish to bring to your attention that the patch I submitted previously
(0001-Cleaned-up-Digidesign-Mbox-1-patch.patch) has been working for the
last two months on my device and other people have also reported
complete success with it. (see http://www.zamaudio.com/?p=953 for details).

We discovered and fixed an issue with outdated firmware causing sporadic
white noise on the device; Avid (the manufacturer) provided an update
which fixed this issue entirely (v0.22).  Unfortunately, the firmware
update can only be applied using a Mac computer since it is bundled in a
.dmg installer.

If users do not apply the firmware update, the device will still work
with linux, but every second or third time they open a playback stream
there is a loud fuzzing noise which could possibly damage external
equipment.  I suppose it is caused by a bug in the firmware.

Therefore, with the official updated firmware and my usb-audio quirk
provided in the previous post, I am pleased to say that Digidesign Mbox
1 (0dba:1000) works well in duplex mode, in both 44100 and 48000Hz modes
(SPDIF untested).

I am keen to get my patch into the kernel with your recommendations for
cleaning up the code.

Although I have not inspected the inside of the unit, in my opinion it
has higher quality converters and preamps than the second generation
models.  Also, even though it is powered by dirty USB power, it is
surprisingly quiet on the main outputs when silence is playing.

Regards,
Damien

On 27/01/14 14:02, Damien Zammit wrote:
> On 27/01/14 03:52, Clemens Ladisch wrote:
>>> This patch creates a dual endpoint quirk.
>>> >The quirk interface needs a second audioformat struct for this to work
>>> >which I called ".data2".
>> Couldn't you just let .data point to an array of two structs?
> 
> Thanks Clemens, I have created a new patch using this suggestion.
> 
> Damien

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Daniel Mack]

Hi Damien,

Thanks for your patch! See my two comments below.

On 01/27/2014 04:02 AM, Damien Zammit wrote:
> On 27/01/14 03:52, Clemens Ladisch wrote:
>>> >> This patch creates a dual endpoint quirk.
>>>> >> >The quirk interface needs a second audioformat struct for this to work
>>>> >> >which I called ".data2".
>> > Couldn't you just let .data point to an array of two structs?
> Thanks Clemens, I have created a new patch using this suggestion.

[...]

> +	fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
> +	if (!fp2) {
> +		snd_printk(KERN_ERR "cannot memdup 2\n");
> +		return -ENOMEM;
> +	}
> +	if (fp1->nr_rates > MAX_NR_RATES) {
> +		kfree(fp1);
> +		kfree(fp2);

Please do proper error unwinding here with jump labels rather than
open-coding the kfree() calls from multiple places.

Also, I wonder whether a more generic quirk type to set up a dynamic
number of fixed interfaces wouldn't be nicer. IOW, add a field to struct
snd_usb_audio_quirk to denote the number of array members in quirk->data
and call the quirk type QUIRK_AUDIO_FIXED_MULTI_ENDPOINT or something.
Then, rewrite the logic to iterate over the interfaces in a loop. That
might also make the code more readable.


Thanks,
Daniel

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Mark Hills]

On Mon, 3 Mar 2014, Daniel Mack wrote:

> Hi Damien,
> 
> Thanks for your patch! See my two comments below.
> 
> On 01/27/2014 04:02 AM, Damien Zammit wrote:
> > On 27/01/14 03:52, Clemens Ladisch wrote:
> >>> >> This patch creates a dual endpoint quirk.
> >>>> >> >The quirk interface needs a second audioformat struct for this to work
> >>>> >> >which I called ".data2".
> >> > Couldn't you just let .data point to an array of two structs?
> > Thanks Clemens, I have created a new patch using this suggestion.
> 
> [...]
> 
> > +	fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
> > +	if (!fp2) {
> > +		snd_printk(KERN_ERR "cannot memdup 2\n");
> > +		return -ENOMEM;
> > +	}
> > +	if (fp1->nr_rates > MAX_NR_RATES) {
> > +		kfree(fp1);
> > +		kfree(fp2);
> 
> Please do proper error unwinding here with jump labels rather than
> open-coding the kfree() calls from multiple places.
> 
> Also, I wonder whether a more generic quirk type to set up a dynamic
> number of fixed interfaces wouldn't be nicer. IOW, add a field to struct
> snd_usb_audio_quirk to denote the number of array members in quirk->data
> and call the quirk type QUIRK_AUDIO_FIXED_MULTI_ENDPOINT or something.
> Then, rewrite the logic to iterate over the interfaces in a loop. That
> might also make the code more readable.

I'm a bit late to this, but I think a more generic quirk is necessary.

The problem with the patch is that the new code still calls functions 
hard-wired to the first endpoint on the interface, eg. 
snd_usb_init_pitch(). Also parameters such as altsetting apply to the 
interface, not endpoint.

So whilst it may work in the given case, much of the quirk is ignored.

This is worth fixing though, as there is the same limitation with the 
Focusrite/Novation interfaces, and I suspect others too.

A cleaner implementation is probably an array of endpoints in the quirk, 
something like the patch below (untested)

I found the problem is functions using an endpoint index (eg. 0) whereas 
mainly the function uses the ID (eg. 0x81). What is the relationship 
between the two?

It's to replace calls such as:

  usb_sndctrlpipe(dev, 0)
  get_endpoint(alts, 0)

Or does some kind of spec define that these should be applied to the first 
endpoint and that all will be affected?

Thanks

-- 
Mark


---
 sound/usb/card.h         |  2 ++
 sound/usb/quirks-table.h |  5 ++++-
 sound/usb/quirks.c       | 58 ++++++++++++++++++++++++++++++++++++------------
 sound/usb/stream.c       |  5 +++--
 sound/usb/stream.h       |  1 +
 5 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 9867ab8..3398708 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -20,6 +20,8 @@ struct audioformat {
 	unsigned char attributes;	/* corresponding attributes of cs endpoint */
 	unsigned char endpoint;		/* endpoint */
 	unsigned char ep_attr;		/* endpoint attributes */
+	unsigned char nr_endpoints;     /* number of endpoint table entries */
+	unsigned char *endpoint_table;  /* endpoint table */
 	unsigned char datainterval;	/* log_2 of data packet interval */
 	unsigned char protocol;		/* UAC_VERSION_1/2 */
 	unsigned int maxpacksize;	/* max. packet size */
diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index f5f0595..ab64e50 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2807,7 +2807,10 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
-					.endpoint = 0x02,
+					.nr_endpoints = 2,
+					.endpoint_table = (unsigned char[]) {
+						0x02, 0x81
+					},
 					.ep_attr = 0x01,
 					.rates = SNDRV_PCM_RATE_44100 |
 						 SNDRV_PCM_RATE_48000,
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 0df9ede..01c77e2 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -120,12 +120,12 @@ static int create_standard_audio_quirk(struct snd_usb_audio *chip,
 }
 
 /*
- * create a stream for an endpoint/altsetting without proper descriptors
+ * create a stream using the given quirk and endpoint
  */
-static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
+static int create_stream_at_endpoint(struct snd_usb_audio *chip,
 				     struct usb_interface *iface,
-				     struct usb_driver *driver,
-				     const struct snd_usb_audio_quirk *quirk)
+				     unsigned char endpoint,
+				     const struct audioformat *quirk_data)
 {
 	struct audioformat *fp;
 	struct usb_host_interface *alts;
@@ -133,7 +133,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 	int stream, err;
 	unsigned *rate_table = NULL;
 
-	fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL);
+	fp = kmemdup(quirk_data, sizeof(*fp), GFP_KERNEL);
 	if (!fp) {
 		snd_printk(KERN_ERR "cannot memdup\n");
 		return -ENOMEM;
@@ -152,20 +152,14 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 		fp->rate_table = rate_table;
 	}
 
-	stream = (fp->endpoint & USB_DIR_IN)
+	stream = (endpoint & USB_DIR_IN)
 		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
-	err = snd_usb_add_audio_stream(chip, stream, fp);
+	err = snd_usb_add_audio_stream(chip, stream, endpoint, fp);
 	if (err < 0) {
 		kfree(fp);
 		kfree(rate_table);
 		return err;
 	}
-	if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
-	    fp->altset_idx >= iface->num_altsetting) {
-		kfree(fp);
-		kfree(rate_table);
-		return -EINVAL;
-	}
 	alts = &iface->altsetting[fp->altset_idx];
 	altsd = get_iface_desc(alts);
 	fp->protocol = altsd->bInterfaceProtocol;
@@ -174,9 +168,45 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 		fp->datainterval = snd_usb_parse_datainterval(chip, alts);
 	if (fp->maxpacksize == 0)
 		fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+
+	return 0;
+}
+
+/*
+ * create a stream for an endpoint/altsetting without proper descriptors
+ */
+static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
+				     struct usb_interface *iface,
+				     struct usb_driver *driver,
+				     struct snd_usb_audio_quirk *quirk)
+{
+	struct audioformat *fp = quirk->data;
+	struct usb_host_interface *alts;
+	int n, err;
+
+	if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
+	    fp->altset_idx >= iface->num_altsetting) {
+		return -EINVAL;
+	}
+
 	usb_set_interface(chip->dev, fp->iface, 0);
+
+	if (fp->nr_endpoints == 0) {
+		err = create_stream_at_endpoint(chip, iface, fp->endpoint, fp);
+		if (err < 0)
+			return err;
+	}
+	for (n = 0; n < fp->nr_endpoints; n++) {
+		err = create_stream_at_endpoint(chip, iface, fp->endpoint_table[n], fp);
+		if (err < 0)
+			return err;
+	}
+
+	/* FIXME: these functions fixed to the first endpoint */
+	alts = &iface->altsetting[fp->altset_idx];
 	snd_usb_init_pitch(chip, fp->iface, alts, fp);
 	snd_usb_init_sample_rate(chip, fp->iface, alts, fp, fp->rate_max);
+
 	return 0;
 }
 
@@ -471,7 +501,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
 
 	stream = (fp->endpoint & USB_DIR_IN)
 		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
-	err = snd_usb_add_audio_stream(chip, stream, fp);
+	err = snd_usb_add_audio_stream(chip, stream, fp->endpoint, fp);
 	if (err < 0) {
 		kfree(fp);
 		return err;
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 2fb71be..34947db 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -319,6 +319,7 @@ static struct snd_pcm_chmap_elem *convert_chmap(int channels, unsigned int bits,
  */
 int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 			     int stream,
+			     unsigned char endpoint,
 			     struct audioformat *fp)
 {
 	struct snd_usb_stream *as;
@@ -330,7 +331,7 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 		if (as->fmt_type != fp->fmt_type)
 			continue;
 		subs = &as->substream[stream];
-		if (subs->ep_num == fp->endpoint) {
+		if (subs->ep_num == endpoint) {
 			list_add_tail(&fp->list, &subs->fmt_list);
 			subs->num_formats++;
 			subs->formats |= fp->formats;
@@ -708,7 +709,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
 		fp->chmap = convert_chmap(fp->channels, chconfig, protocol);
 
 		snd_printdd(KERN_INFO "%d:%u:%d: add audio endpoint %#x\n", dev->devnum, iface_no, altno, fp->endpoint);
-		err = snd_usb_add_audio_stream(chip, stream, fp);
+		err = snd_usb_add_audio_stream(chip, stream, fp->endpoint, fp);
 		if (err < 0) {
 			kfree(fp->rate_table);
 			kfree(fp->chmap);
diff --git a/sound/usb/stream.h b/sound/usb/stream.h
index c97f679..4c1b834 100644
--- a/sound/usb/stream.h
+++ b/sound/usb/stream.h
@@ -6,6 +6,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip,
 
 int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
 			     int stream,
+			     unsigned char endpoint,
 			     struct audioformat *fp);
 
 #endif /* __USBAUDIO_STREAM_H */
-- 
1.8.4

Re: [alsa-devel] [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

On 29/03/14 16:01, Mark Hills wrote:
> I'm a bit late to this, but I think a more generic quirk is necessary.
Thanks Mark, I don't know much about other devices, but I have provided
some comments about my latest code:
http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140329/bab4ce5a/attachment-0001.bin

> Or does some kind of spec define that these should be applied to the first 
> endpoint and that all will be affected?
I used educated guesses to make the Mbox 1 functional:

I set the altsetting of the interface based on the value provided in the
first endpoint entry of the quirk, (since they must be all the same for
a shared interface):
+		alts = &iface->altsetting[fp[0]->altset_idx];

This works for any device where the interface is shared across multiple
endpoints, right?  Otherwise you wouldn't use this new quirk type.

After reading all the quirk data and adding streams one by one, I went
on to configure the interface based on the quirk data for the first entry:
+	usb_set_interface(chip->dev, fp[0]->iface, 0);
+	snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]);
+	snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0],
fp[0]->rate_max);

I assumed that the supported rates for all interfaces was the same and
that they could be read from the first endpoint entry in the quirk.
I know, most of the quirk data is ignored, but they share the same
interface so most of it is redundant anyway, isn't it?

I can't see what limitations this multi-endpoint quirk type has that
might need to be adjusted for other devices.  Can you provide an example
of a device that uses multiple endpoints within a single interface whose
supported rates differ between endpoints?  If you can, then I also think
we need a better model.  If you can't, is it because it is impossible?

Damien
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Mark Hills]

On Sat, 29 Mar 2014, Damien Zammit wrote:

> On 29/03/14 16:01, Mark Hills wrote:
> > I'm a bit late to this, but I think a more generic quirk is necessary.
> Thanks Mark, I don't know much about other devices, but I have provided
> some comments about my latest code:
> http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140329/bab4ce5a/attachment-0001.bin

Thanks. I took a look and I think I have a similar concern to Daniel for 
readability. The code should really be shared with the quirk on a single 
endpoint.

The duplicate code in the patch already has different handling of 
maxpacksize and datainterval -- is that intentional? It's apparent because 
the new code ignores the values given in the quirk.

> > Or does some kind of spec define that these should be applied to the first 
> > endpoint and that all will be affected?
> I used educated guesses to make the Mbox 1 functional:
> 
> I set the altsetting of the interface based on the value provided in the
> first endpoint entry of the quirk, (since they must be all the same for
> a shared interface):
> +		alts = &iface->altsetting[fp[0]->altset_idx];
> 
> This works for any device where the interface is shared across multiple
> endpoints, right?  Otherwise you wouldn't use this new quirk type.
> 
> After reading all the quirk data and adding streams one by one, I went
> on to configure the interface based on the quirk data for the first entry:
> +	usb_set_interface(chip->dev, fp[0]->iface, 0);
> +	snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]);
> +	snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0],
> fp[0]->rate_max);
> 
> I assumed that the supported rates for all interfaces was the same and
> that they could be read from the first endpoint entry in the quirk.
> I know, most of the quirk data is ignored, but they share the same
> interface so most of it is redundant anyway, isn't it?

If the data in the quirk is truly redundant, then really it should not be 
present, or have the structure to use it -- it is misleading like this.

The alternative is that the format is guaranteed to be identical between 
the endpoints on the same interface, which is what I attempted in my 
previous patch (however this assumption is incorrect, see below)
 
> I can't see what limitations this multi-endpoint quirk type has that
> might need to be adjusted for other devices.  Can you provide an example
> of a device that uses multiple endpoints within a single interface whose
> supported rates differ between endpoints? 

Yes, I dug out a Novation Twitch here. It uses different number of 
channels (and hence buffer sizes) on the record and playback endpoints. At 
very least other Focusrite devices will have this, too.

> If you can, then I also think we need a better model.  If you can't, is 
> it because it is impossible?
> 
> Damien
> 

Ok so I did further experiments.

One thing was to add more code to my patch that specifically sets 
sample_format etc. directly on the affected endpoint.

It seems the assumption that an enpoint has sole ownership of the 
interface is quite deeply spread in the code. For example, set_format() 
called as part of snd_usb_pcm_prepare().

In my case the result is blocking on one audio stream when the other is 
opened, I didn't look into detail whether this was the driver or hardware.

But I'm suprised your Mbox deals with the prepare step, perhaps it is 
suprisingly tolerant. Did you confirm that you could start and stop a 
recording during playback and that both streams were truly working?

Specifically as snd_usb_pcm_close() shuts down the whole interface. Is 
your Mbox continuing smoothly when this happens on one of the streams?

Thanks

-- 
Mark

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

On 30/03/14 09:57, Mark Hills wrote:
> Thanks. I took a look and I think I have a similar concern to Daniel for 
> readability. The code should really be shared with the quirk on a single 
> endpoint.
I guess a significant rewrite will be required.

> The duplicate code in the patch already has different handling of 
> maxpacksize and datainterval -- is that intentional? It's apparent because 
> the new code ignores the values given in the quirk.
Not intentional, maxpacksize and datainterval happened to be identical
for both endpoints on the Mbox.

> If the data in the quirk is truly redundant, then really it should not be 
> present, or have the structure to use it -- it is misleading like this.
Yeah, I agree.  I have an idea.  The following is an excerpt from lsusb
for the Mbox:

    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0
      bInterfaceProtocol      0
      iInterface              0
      Endpoint Descriptor:
        bLength                 9
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            5
          Transfer Type            Isochronous
          Synch Type               Asynchronous
          Usage Type               Data
        wMaxPacketSize     0x0130  1x 304 bytes
        bInterval               1
        bRefresh                0
        bSynchAddress           0
      Endpoint Descriptor:
        bLength                 9
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            5
          Transfer Type            Isochronous
          Synch Type               Asynchronous
          Usage Type               Data
        wMaxPacketSize     0x0130  1x 304 bytes
        bInterval               1
        bRefresh                0
        bSynchAddress           0

Looking at the current struct for a snd_usb_audio_quirk:

struct snd_usb_audio_quirk {
        const char *vendor_name;
        const char *product_name;
        int16_t ifnum;
        uint16_t type;
        const void *data;
}

It is quite versatile, we shouldn't need to alter the quirk struct, just
the data it holds.  Is an array of audioformats for this kind of
interface the ideal choice?

Also, maybe we can read bNumEndpoints to determine if it is a
multi-endpoint interface and act accordingly, then we don't need an
extra element in snd_usb_audio_quirk.

How is a shared interface supposed to be initialised when it has
different properties for each endpoint?

> Yes, I dug out a Novation Twitch here. It uses different number of 
> channels (and hence buffer sizes) on the record and playback endpoints. At 
> very least other Focusrite devices will have this, too.
Interesting.

> It seems the assumption that an enpoint has sole ownership of the 
> interface is quite deeply spread in the code. For example, set_format() 
> called as part of snd_usb_pcm_prepare().
I am still trying to wrap my head around this one, but it sounds like
the major stumbling block with the current code?

> But I'm suprised your Mbox deals with the prepare step, perhaps it is 
> suprisingly tolerant. Did you confirm that you could start and stop a 
> recording during playback and that both streams were truly working?
Yes, I can start and stop recording while headphones are blasting, and
my microphone works.

> Specifically as snd_usb_pcm_close() shuts down the whole interface. Is 
> your Mbox continuing smoothly when this happens on one of the streams?
I use JACK so I'm not sure if the streams are ever closed until I
shutdown the server.

Damien

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Daniel Mack]

Hi Damien,

On 03/30/2014 05:21 AM, Damien Zammit wrote:
> On 30/03/14 09:57, Mark Hills wrote:
>> Thanks. I took a look and I think I have a similar concern to Daniel for 
>> readability. The code should really be shared with the quirk on a single 
>> endpoint.
> I guess a significant rewrite will be required.

Yeah, now that you changed the code to handle an arbitrary amount of
endpoints, you could as well change all user of
QUIRK_AUDIO_FIXED_ENDPOINT over to the new one, and initialize .epmulti
to 1. That should already work, right? (Though, I have to mention that
I'm unhappy with the name of that variable :)).

>> The duplicate code in the patch already has different handling of 
>> maxpacksize and datainterval -- is that intentional? It's apparent because 
>> the new code ignores the values given in the quirk.

That souldn't be the case then, of course.

> Not intentional, maxpacksize and datainterval happened to be identical
> for both endpoints on the Mbox.

So it's easy to change.

> Looking at the current struct for a snd_usb_audio_quirk:
> 
> struct snd_usb_audio_quirk {
>         const char *vendor_name;
>         const char *product_name;
>         int16_t ifnum;
>         uint16_t type;
>         const void *data;
> }
> 
> It is quite versatile, we shouldn't need to alter the quirk struct, just
> the data it holds.

A const void* can hold any kind of struct, and you need to cast it back
to your struct eventually anyway. Thinking about it again, I don't like
the idea of an extra member in struct snd_usb_audio_quirk either. What
would be nicer is to introduce something like this:

struct audioformats {
	unsigned int n_formats;
	const struct audioformat *format;
};

... and then define the quirk like this:

	.type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINTS,
	.data = (const struct audioformats) {
		.n_formats = 2,
		.format = (const struct audioformat[]) {
			[0] = {
				.formats = SNDRV_PCM_FMTBIT_S24_3BE,
				...
			},
			[1] = {
				.formats = SNDRV_PCM_FMTBIT_S24_3BE,
				...
			},
		},
	}

IOW, encapsulate struct audioformat once more, so that the counter
variable is specific to this kind of quirk.

Also, you can write the actual quirk handler so that it loops over the
array entries, so you can re-use the code for both
QUIRK_AUDIO_FIXED_MULTI_ENDPOINTS and QUIRK_AUDIO_FIXED_ENDPOINT.

Do you follow? :)

>  Is an array of audioformats for this kind of
> interface the ideal choice?

Not sure whether I understand what you mean. Would be best to share a
patch that implements your idea.

> Also, maybe we can read bNumEndpoints to determine if it is a
> multi-endpoint interface and act accordingly, then we don't need an
> extra element in snd_usb_audio_quirk.

I think adding an array type is the best way to handle it.


Thanks,
Daniel

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

On 01/04/14 04:47, Daniel Mack wrote:
> Yeah, now that you changed the code to handle an arbitrary amount of
> endpoints, you could as well change all user of
> QUIRK_AUDIO_FIXED_ENDPOINT over to the new one, and initialize .epmulti
> to 1. That should already work, right? (Though, I have to mention that
> I'm unhappy with the name of that variable :)).
Given Mark's concerns, I'm not sure this will work for devices which
share an interface with multiple endpoints where the endpoints have
different properties.  I think my code only works when the multiple
endpoints have the same properties apart from the endpoint #.

For example, the existing code which brings up a fixed interface assumes
that there is only one endpoint and refers to it via an index which is
usually hardcoded to 0.  So most of the quirk would be ignored as only
the data for the first mentioned endpoint would be used for certain
functions.
For example, functions such as get_endpoint(alts, 0) etc.  This is the
major stumbling block that Mark is referring to I think.  What are your
thoughts on this one Daniel?  Have you seen Mark's untested code?  It's
part of this thread and seems to address this problem.

> A const void* can hold any kind of struct, and you need to cast it back
> to your struct eventually anyway. Thinking about it again, I don't like
> the idea of an extra member in struct snd_usb_audio_quirk either. What
> would be nicer is to introduce something like this:
> 
> struct audioformats {
> 	unsigned int n_formats;
> 	const struct audioformat *format;
> };
Yes, this looks good, I can use this idea.

> Also, you can write the actual quirk handler so that it loops over the
> array entries, so you can re-use the code for both
> QUIRK_AUDIO_FIXED_MULTI_ENDPOINTS and QUIRK_AUDIO_FIXED_ENDPOINT.
> 
> Do you follow? :)
Yes, it makes sense.

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

On 04/03/14 08:01, Daniel Mack wrote:
> Hi Damien,
> 
> Thanks for your patch! See my two comments below.
> 
> On 01/27/2014 04:02 AM, Damien Zammit wrote:
>> On 27/01/14 03:52, Clemens Ladisch wrote:
>>>>>> This patch creates a dual endpoint quirk.
>>>>>>>> The quirk interface needs a second audioformat struct for this to work
>>>>>>>> which I called ".data2".
>>>> Couldn't you just let .data point to an array of two structs?
>> Thanks Clemens, I have created a new patch using this suggestion.
> 
> [...]
> 
>> +	fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
>> +	if (!fp2) {
>> +		snd_printk(KERN_ERR "cannot memdup 2\n");
>> +		return -ENOMEM;
>> +	}
>> +	if (fp1->nr_rates > MAX_NR_RATES) {
>> +		kfree(fp1);
>> +		kfree(fp2);
> 
> Please do proper error unwinding here with jump labels rather than
> open-coding the kfree() calls from multiple places.
> 
> Also, I wonder whether a more generic quirk type to set up a dynamic
> number of fixed interfaces wouldn't be nicer. IOW, add a field to struct
> snd_usb_audio_quirk to denote the number of array members in quirk->data
> and call the quirk type QUIRK_AUDIO_FIXED_MULTI_ENDPOINT or something.
> Then, rewrite the logic to iterate over the interfaces in a loop. That
> might also make the code more readable.

Hi Daniel, Clemens,
I have addressed the above issues, please find attached my new patch.
I did a proper git log message that describes my changes.  I am keen to
get this reviewed and hopefully accepted soon.
I did a clean clone of Takashi's 'sound' (for-next) and applied my
changes on top of that.

Damien


[-- Attachment #2: 0001-PATCH-ALSA-usb-audio-Capture-and-duplex-support-for-.patch --]
[-- Type: text/x-patch, Size: 6876 bytes --]

>From c20460de3a3d5635451f030a966cb7666866a7a1 Mon Sep 17 00:00:00 2001
From: Damien Zammit <damien@zamaudio.com>
Date: Sat, 29 Mar 2014 16:01:47 +1100
Subject: [PATCH] [PATCH] ALSA: usb-audio - Capture and duplex support for
 Digidesign Mbox 1 sound card

A few revisions of this patch have been made.  This patch provides support for usb cards
which have their streams on multiple endpoints within the same interface.
In particular, it provides duplex mode support for Digidesign Mbox 1.

I followed ALSA dev team's suggestion of making it work for N endpoints
rather than hardcoding it to two, and improved error handling has been provided.
An extra parameter has been added as requested to the usbaudio quirk struct 'epmulti'
to give the number of endpoints within a multi interface.

Signed-off-by: Damien Zammit <damien@zamaudio.com>
---
 sound/usb/quirks-table.h |   58 +++++++++++++++++++++++-----------
 sound/usb/quirks.c       |   79 ++++++++++++++++++++++++++++++++++++++++++++++
 sound/usb/usbaudio.h     |    2 ++
 3 files changed, 121 insertions(+), 18 deletions(-)

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index f652b10..e41b8a2 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2889,23 +2889,46 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 			},
 			{
 				.ifnum = 1,
-				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
-				.data = &(const struct audioformat) {
-					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
-					.channels = 2,
-					.iface = 1,
-					.altsetting = 1,
-					.altset_idx = 1,
-					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
-					.endpoint = 0x02,
-					.ep_attr = 0x01,
-					.rates = SNDRV_PCM_RATE_44100 |
-						 SNDRV_PCM_RATE_48000,
-					.rate_min = 44100,
-					.rate_max = 48000,
-					.nr_rates = 2,
-					.rate_table = (unsigned int[]) {
-						44100, 48000
+				.type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINT,
+				.epmulti = 2,
+				.data = (const struct audioformat[]) {
+					[0] = {
+						.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+						.channels = 2,
+						.iface = 1,
+						.altsetting = 1,
+						.altset_idx = 1,
+						.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
+						.endpoint = 0x02,
+						.ep_attr = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_SYNC,
+						.maxpacksize = 0x130,
+						.rates = SNDRV_PCM_RATE_44100 |
+							 SNDRV_PCM_RATE_48000,
+						.rate_min = 44100,
+						.rate_max = 48000,
+						.nr_rates = 2,
+						.rate_table = (unsigned int[]) {
+							44100, 48000
+						}
+					},
+					[1] = {
+						.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+						.channels = 2,
+						.iface = 1,
+						.altsetting = 1,
+						.altset_idx = 1,
+						.attributes = 0x00,
+						.endpoint = 0x81,
+						.ep_attr = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+						.maxpacksize = 0x130,
+						.rates = SNDRV_PCM_RATE_44100 |
+							 SNDRV_PCM_RATE_48000,
+						.rate_min = 44100,
+						.rate_max = 48000,
+						.nr_rates = 2,
+						.rate_table = (unsigned int[]) {
+							44100, 48000
+						}
 					}
 				}
 			},
@@ -2913,7 +2936,6 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 				.ifnum = -1
 			}
 		}
-
 	}
 },
 
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 7c57f22..affbced 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -180,6 +180,84 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 	return 0;
 }
 
+/*
+ * create N streams for an interface without proper descriptors but with multiple endpoints
+ */
+static int create_fixed_multi_stream_quirk(struct snd_usb_audio *chip,
+				     struct usb_interface *iface,
+				     struct usb_driver *driver,
+				     const struct snd_usb_audio_quirk *quirk)
+{
+	struct audioformat **fp;
+	struct usb_host_interface *alts = NULL;
+	int *stream;
+	int err, i;
+	unsigned *rate_table = NULL;
+	int rates_found;
+	rates_found = 0;
+
+	stream = (int*) kmalloc(sizeof(int) * quirk->epmulti, GFP_KERNEL);
+	if (!stream)
+		goto err_mem;
+	fp = (struct audioformat**) kmalloc(sizeof((const struct audioformat*)quirk->data) * quirk->epmulti, GFP_KERNEL);
+	if (!(fp))
+		goto err_mem;
+
+	for (i = 0; i < quirk->epmulti; ++i) {
+		fp[i] = kmemdup((const struct audioformat*)(quirk->data + i*sizeof(const struct audioformat)), sizeof(*fp[i]), GFP_KERNEL);
+		if (!(fp[i])) {
+			usb_audio_err(chip, "cannot memdup %d\n", i);
+			goto err_mem;
+		}
+		if (fp[i]->nr_rates > MAX_NR_RATES) {
+			err = -EINVAL;
+			goto err_rates;
+		}
+		if (fp[i]->nr_rates > 0 && !rates_found) {
+			rate_table = kmemdup(fp[i]->rate_table,
+				     sizeof(int) * fp[i]->nr_rates, GFP_KERNEL);
+			rates_found = 1;
+		}
+		if (!rate_table) {
+			err = -ENOMEM;
+			goto err_rates;
+		}
+		fp[i]->rate_table = rate_table;
+
+		stream[i] = (fp[i]->endpoint & USB_DIR_IN)
+			? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+
+		err = snd_usb_add_audio_stream(chip, stream[i], fp[i]);
+		if (err < 0)
+			goto err_ratetable;
+
+		if (fp[i]->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
+		    fp[i]->altset_idx >= iface->num_altsetting) {
+			err = -EINVAL;
+			goto err_ratetable;
+		}
+		alts = &iface->altsetting[fp[0]->altset_idx];
+		fp[i]->datainterval = snd_usb_parse_datainterval(chip, alts);
+		fp[i]->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+	}
+	usb_set_interface(chip->dev, fp[0]->iface, 0);
+	snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]);
+	snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0], fp[0]->rate_max);
+	return 0;
+
+err_mem:
+	return -ENOMEM;
+err_ratetable:
+	kfree(rate_table);
+err_rates:
+	for (i = 0; i < quirk->epmulti; ++i)
+		if(fp[i])
+			kfree(fp[i]);
+	kfree(fp);
+	kfree(stream);
+	return err;
+}
+
 static int create_auto_pcm_quirk(struct snd_usb_audio *chip,
 				 struct usb_interface *iface,
 				 struct usb_driver *driver)
@@ -528,6 +606,7 @@ int snd_usb_create_quirk(struct snd_usb_audio *chip,
 		[QUIRK_MIDI_FTDI] = create_any_midi_quirk,
 		[QUIRK_AUDIO_STANDARD_INTERFACE] = create_standard_audio_quirk,
 		[QUIRK_AUDIO_FIXED_ENDPOINT] = create_fixed_stream_quirk,
+		[QUIRK_AUDIO_FIXED_MULTI_ENDPOINT] = create_fixed_multi_stream_quirk,
 		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk,
 		[QUIRK_AUDIO_ALIGN_TRANSFER] = create_align_transfer_quirk,
 		[QUIRK_AUDIO_STANDARD_MIXER] = create_standard_mixer_quirk,
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 25c4c7e..a516ca3 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -95,6 +95,7 @@ enum quirk_type {
 	QUIRK_MIDI_FTDI,
 	QUIRK_AUDIO_STANDARD_INTERFACE,
 	QUIRK_AUDIO_FIXED_ENDPOINT,
+	QUIRK_AUDIO_FIXED_MULTI_ENDPOINT,
 	QUIRK_AUDIO_EDIROL_UAXX,
 	QUIRK_AUDIO_ALIGN_TRANSFER,
 	QUIRK_AUDIO_STANDARD_MIXER,
@@ -108,6 +109,7 @@ struct snd_usb_audio_quirk {
 	int16_t ifnum;
 	uint16_t type;
 	const void *data;
+	uint16_t epmulti;
 };
 
 #define combine_word(s)    ((*(s)) | ((unsigned int)(s)[1] << 8))
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Daniel Mack]

Hi Damien,

On 03/29/2014 06:35 AM, Damien Zammit wrote:
> A few revisions of this patch have been made.  This patch provides support for usb cards
> which have their streams on multiple endpoints within the same interface.
> In particular, it provides duplex mode support for Digidesign Mbox 1.
> 
> I followed ALSA dev team's suggestion of making it work for N endpoints
> rather than hardcoding it to two, and improved error handling has been provided.
> An extra parameter has been added as requested to the usbaudio quirk struct 'epmulti'
> to give the number of endpoints within a multi interface.
> 

A few style nits below, which you can fix for the next version. Most
important, the error unwinder should be more in-line with other code in
the kernel.

> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index 7c57f22..affbced 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -180,6 +180,84 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
>  	return 0;
>  }
>  
> +/*
> + * create N streams for an interface without proper descriptors but with multiple endpoints
> + */
> +static int create_fixed_multi_stream_quirk(struct snd_usb_audio *chip,
> +				     struct usb_interface *iface,
> +				     struct usb_driver *driver,
> +				     const struct snd_usb_audio_quirk *quirk)
> +{
> +	struct audioformat **fp;
> +	struct usb_host_interface *alts = NULL;
> +	int *stream;
> +	int err, i;
> +	unsigned *rate_table = NULL;
> +	int rates_found;
> +	rates_found = 0;
> +
> +	stream = (int*) kmalloc(sizeof(int) * quirk->epmulti, GFP_KERNEL);

No need to cast the result of k[mzc]alloc. Same for other locations below.

> +	if (!stream)
> +		goto err_mem;

err = -ENOMEM;
goto exit_err;

> +	fp = (struct audioformat**) kmalloc(sizeof((const struct audioformat*)quirk->data) * quirk->epmulti, GFP_KERNEL);
> +	if (!(fp))

Extra parenthesis not needed.

> +		goto err_mem;
> +
> +	for (i = 0; i < quirk->epmulti; ++i) {

i++ is prefered if the operation order doesn't matter. It's not
important but a little more common in the kernel.

> +		fp[i] = kmemdup((const struct audioformat*)(quirk->data + i*sizeof(const struct audioformat)), sizeof(*fp[i]), GFP_KERNEL);

You need to do something about these overlong lines, as they're also
quite unparsable for humans. One solution is to pre-calculate the size
into a temporary variable.

> +		if (!(fp[i])) {

Extra parenthesis not needed.

> +			usb_audio_err(chip, "cannot memdup %d\n", i);

err = -ENOMEM;
goto exit_err;

> +			goto err_mem;
> +		}
> +		if (fp[i]->nr_rates > MAX_NR_RATES) {
> +			err = -EINVAL;
> +			goto err_rates;
> +		}
> +		if (fp[i]->nr_rates > 0 && !rates_found) {
> +			rate_table = kmemdup(fp[i]->rate_table,
> +				     sizeof(int) * fp[i]->nr_rates, GFP_KERNEL);
> +			rates_found = 1;

Not sure if I overlook anything, but isn't the extra variable
rates_found redundant here, as can derive the same information by
checking for (rate_table != NULL)?

> +		}
> +		if (!rate_table) {
> +			err = -ENOMEM;
> +			goto err_rates;
> +		}

This check makes more sense directly underneath the kmemdup().

> +		fp[i]->rate_table = rate_table;
> +
> +		stream[i] = (fp[i]->endpoint & USB_DIR_IN)
> +			? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
> +
> +		err = snd_usb_add_audio_stream(chip, stream[i], fp[i]);
> +		if (err < 0)
> +			goto err_ratetable;
> +
> +		if (fp[i]->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
> +		    fp[i]->altset_idx >= iface->num_altsetting) {
> +			err = -EINVAL;
> +			goto err_ratetable;
> +		}
> +		alts = &iface->altsetting[fp[0]->altset_idx];
> +		fp[i]->datainterval = snd_usb_parse_datainterval(chip, alts);
> +		fp[i]->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
> +	}
> +	usb_set_interface(chip->dev, fp[0]->iface, 0);
> +	snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]);
> +	snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0], fp[0]->rate_max);
> +	return 0;
> +
> +err_mem:
> +	return -ENOMEM;

You can drop this ...

> +err_ratetable:
> +	kfree(rate_table);
> +err_rates:
> +	for (i = 0; i < quirk->epmulti; ++i)
> +		if(fp[i])
> +			kfree(fp[i]);
> +	kfree(fp);
> +	kfree(stream);

... and place another label here:

exit_err:
> +	return err;
> +}
> +


Thanks,
Daniel