[PATCH 0/2] Digidesign Mbox 1 duplex mode and mixer

[PATCH 0/2] Digidesign Mbox 1 duplex mode and mixer

[Damien Zammit]

Takashi asked me to rewrite the patches and send them without
attachments.  I hope this email series works and they can be
reviewed.

These two patches provide the following for Digidesign Mbox 1:
 o Mixer control for clock source selection: Internal/SPDIF
 o Duplex mode (playback and capture)

Damien Zammit (2):
  snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock source
  snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

 sound/usb/card.h         |   5 ++
 sound/usb/mixer_maps.c   |   9 +++
 sound/usb/mixer_quirks.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++
 sound/usb/quirks-table.h |  73 ++++++++++++-------
 sound/usb/quirks.c       |  30 ++++++++
 sound/usb/usbaudio.h     |   1 +
 6 files changed, 271 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH 1/2] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock source

[Damien Zammit]

Signed-off-by: Damien Zammit <damien@zamaudio.com>
---
 sound/usb/mixer_maps.c   |   9 +++
 sound/usb/mixer_quirks.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 186 insertions(+)

diff --git a/sound/usb/mixer_maps.c b/sound/usb/mixer_maps.c
index d1d72ff..9ee3fc2 100644
--- a/sound/usb/mixer_maps.c
+++ b/sound/usb/mixer_maps.c
@@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = {
 	{ 0 } /* terminator */
 };
 
+static struct usbmix_name_map mbox1_map[] = {
+	{ 1, "Control" },
+	{ 0 } /* terminator */
+};
+
 static struct usbmix_selector_map c400_selectors[] = {
 	{
 		.id = 0x80,
@@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = {
 		.map = aureon_51_2_map,
 	},
 	{
+		.id = USB_ID(0x0dba, 0x1000),
+		.map = mbox1_map,
+	},
+	{
 		.id = USB_ID(0x13e5, 0x0001),
 		.map = scratch_live_map,
 		.ignore_ctl_error = 1,
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 3980bf5..716c32c 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -565,6 +565,176 @@ static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer)
 	return 0;
 }
 
+/* Digidesign Mbox 1 clock source switch (internal/spdif) */
+
+struct snd_mbox1_switch_priv_val {
+	struct usb_mixer_interface *mixer;
+	int cached_value;
+	int is_cached;
+};
+
+static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_mbox1_switch_priv_val *pval;
+	unsigned char value;
+
+	value = 0x00;
+
+	pval = (struct snd_mbox1_switch_priv_val *)
+		kctl->private_value;
+
+	if (pval->is_cached) {
+		ucontrol->value.enumerated.item[0] = pval->cached_value;
+		return 0;
+	}
+
+	ucontrol->value.enumerated.item[0] = value;
+	pval->cached_value = value;
+	pval->is_cached = 1;
+
+	return 0;
+}
+
+static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_usb_audio *chip;
+	struct snd_mbox1_switch_priv_val *pval;
+
+	struct usb_mixer_interface *mixer;
+	int changed, cur_val, err, new_val;
+	unsigned char value[2];
+	unsigned char buff[3];
+
+	changed = 0;
+	value[0] = 0x00;
+	value[1] = 0x00;
+
+	pval = (struct snd_mbox1_switch_priv_val *)
+		kctl->private_value;
+	cur_val = pval->cached_value;
+	new_val = ucontrol->value.enumerated.item[0];
+
+	mixer = (struct usb_mixer_interface *) pval->mixer;
+	if (snd_BUG_ON(!mixer))
+		return -EINVAL;
+
+	chip = (struct snd_usb_audio *) mixer->chip;
+	if (snd_BUG_ON(!chip))
+		return -EINVAL;
+
+	if (!pval->is_cached) {
+		cur_val = value[0];
+		pval->cached_value = cur_val;
+		pval->is_cached = 1;
+	}
+	/* update value if needed */
+	if (cur_val != new_val) {
+		value[0] = new_val;
+		value[1] = 0;
+		down_read(&mixer->chip->shutdown_rwsem);
+		if (mixer->chip->shutdown) {
+			err = -ENODEV;
+		} else {
+			err = snd_usb_ctl_msg(mixer->chip->dev,
+				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1);
+			if (err < 0)
+				return err;
+			err = snd_usb_ctl_msg(mixer->chip->dev,
+				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
+			if (err < 0)
+				return err;
+			if (new_val == 0) {
+				buff[0] = 0x80;
+				buff[1] = 0xbb;
+				buff[2] = 0x00;
+			} else {
+				buff[0] = buff[1] = buff[2] = 0;
+			}
+			err = snd_usb_ctl_msg(mixer->chip->dev,
+				usb_sndctrlpipe(mixer->chip->dev, 0), 0x1,
+				USB_TYPE_CLASS |
+				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
+			if (err < 0)
+				return err;
+			err = snd_usb_ctl_msg(mixer->chip->dev,
+				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
+			if (err < 0)
+				return err;
+			err = snd_usb_ctl_msg(mixer->chip->dev,
+				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3);
+			if (err < 0)
+				return err;
+		}
+		up_read(&mixer->chip->shutdown_rwsem);
+		if (err < 0)
+			return err;
+
+		pval->cached_value = new_val;
+		pval->is_cached = 1;
+		changed = 1;
+	}
+
+	return changed;
+}
+
+static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol,
+				 struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const texts[2] = {
+		"Internal",
+		"S/PDIF"
+	};
+
+	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
+}
+
+static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer)
+{
+	static struct snd_kcontrol_new template = {
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "Clock Source",
+		.index = 0,
+		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+		.info = snd_mbox1_switch_info,
+		.get = snd_mbox1_switch_get,
+		.put = snd_mbox1_switch_put
+	};
+
+	struct snd_kcontrol *kctl;
+	struct snd_mbox1_switch_priv_val *pval;
+
+	pval = kzalloc(sizeof(*pval), GFP_KERNEL);
+	if (!pval)
+		return -ENOMEM;
+
+	pval->cached_value = 0;
+	pval->is_cached = 0;
+	pval->mixer = mixer;
+
+	template.private_value = (unsigned long) pval;
+	kctl = snd_ctl_new1(&template, mixer->chip);
+	if (!kctl) {
+		kfree(pval);
+		return -ENOMEM;
+	}
+
+	return snd_ctl_add(mixer->chip->card, kctl);
+}
+
 /* Native Instruments device quirks */
 
 #define _MAKE_NI_CONTROL(bRequest,wIndex) ((bRequest) << 16 | (wIndex))
@@ -1605,6 +1775,13 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 					      snd_audigy2nx_proc_read);
 		break;
 
+	/* Digidesign Mbox 1 */
+	case USB_ID(0x0dba, 0x1000):
+		err = snd_mbox1_create_sync_switch(mixer);
+		if (err < 0)
+			break;
+		break;
+
 	/* EMU0204 */
 	case USB_ID(0x041e, 0x3f19):
 		err = snd_emu0204_controls_create(mixer);
-- 
1.9.1

[PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Damien Zammit]

This patch provides duplex support for the Digidesign Mbox 1 sound
card and has been a work in progress for about a year.
Users have confirmed on my website that previous versions of this patch
have worked on the hardware and I have been testing extensively.

It also enables the mixer control for providing clock source
selector based on the previous patch.

The sample rate has been hardcoded to 48kHz because it works better with
the S/PDIF sync mode when the sample rate is locked.  This is the
highest rate that the device supports and no loss of functionality
is observed by restricting the sample rate.

Signed-off-by: Damien Zammit <damien@zamaudio.com>
---
 sound/usb/card.h         |  5 ++++
 sound/usb/quirks-table.h | 73 ++++++++++++++++++++++++++++++++----------------
 sound/usb/quirks.c       | 30 ++++++++++++++++++++
 sound/usb/usbaudio.h     |  1 +
 4 files changed, 85 insertions(+), 24 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 97acb90..a8fe22f 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -33,6 +33,11 @@ struct audioformat {
 	bool dsd_bitrev;		/* reverse the bits of each DSD sample */
 };
 
+struct audioformats {
+	unsigned int n_formats;
+	const struct audioformat *format;
+};
+
 struct snd_usb_substream;
 struct snd_usb_endpoint;
 
diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index c657752..0b5f4fb 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2937,43 +2937,68 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 	/* Thanks to Clemens Ladisch <clemens@ladisch.de> */
 	USB_DEVICE(0x0dba, 0x1000),
 	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
-		.vendor_name = "Digidesign",
-		.product_name = "MBox",
-		.ifnum = QUIRK_ANY_INTERFACE,
-		.type = QUIRK_COMPOSITE,
-		.data = (const struct snd_usb_audio_quirk[]){
-			{
-				.ifnum = 0,
-				.type = QUIRK_IGNORE_INTERFACE,
-			},
-			{
-				.ifnum = 1,
-				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
-				.data = &(const struct audioformat) {
+	.vendor_name = "Digidesign",
+	.product_name = "MBox",
+	.ifnum = QUIRK_ANY_INTERFACE,
+	.type = QUIRK_COMPOSITE,
+	.data = (const struct snd_usb_audio_quirk[]) {
+		{
+			.ifnum = 0,
+			.type = QUIRK_AUDIO_STANDARD_MIXER,
+		},
+		{
+			.ifnum = 1,
+			.type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINT,
+			.data = &(const struct audioformats)
+			{
+				.n_formats = 2,
+				.format = (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,
+					.attributes = 0x4,
 					.endpoint = 0x02,
-					.ep_attr = 0x01,
-					.rates = SNDRV_PCM_RATE_44100 |
-						 SNDRV_PCM_RATE_48000,
-					.rate_min = 44100,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC |
+						USB_ENDPOINT_SYNC_SYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_48000,
+					.rate_min = 48000,
 					.rate_max = 48000,
-					.nr_rates = 2,
+					.nr_rates = 1,
 					.rate_table = (unsigned int[]) {
-						44100, 48000
+						48000
+					}
+					},
+					{
+					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+					.channels = 2,
+					.iface = 1,
+					.altsetting = 1,
+					.altset_idx = 1,
+					.attributes = 0x4,
+					.endpoint = 0x81,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC |
+						USB_ENDPOINT_SYNC_ASYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_48000,
+					.rate_min = 48000,
+					.rate_max = 48000,
+					.nr_rates = 1,
+					.rate_table = (unsigned int[]) {
+						48000
+					}
 					}
 				}
-			},
-			{
-				.ifnum = -1
 			}
+		},
+		{
+			.ifnum = -1
 		}
-
 	}
+}
 },
 
 /* DIGIDESIGN MBOX 2 */
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index d2aa45a..90227cd 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -180,6 +180,34 @@ 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 audioformats *fp;
+	int i;
+
+	fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL);
+	if (!fp) {
+		usb_audio_err(chip, "cannot memdup\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < fp->n_formats; i++) {
+		struct snd_usb_audio_quirk nquirk = {
+			.ifnum = quirk->ifnum,
+			.type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINT,
+			.data = (const struct audioformat *) &fp->format[i]
+		};
+		create_fixed_stream_quirk(chip, iface, driver, &nquirk);
+	}
+	return 0;
+}
+
 static int create_auto_pcm_quirk(struct snd_usb_audio *chip,
 				 struct usb_interface *iface,
 				 struct usb_driver *driver)
@@ -528,6 +556,8 @@ 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 91d0380..31d8ff0 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -96,6 +96,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,
-- 
1.9.1

Re: [PATCH 1/2] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock source

[Takashi Iwai]

At Tue,  4 Nov 2014 11:06:36 +1100,
Damien Zammit wrote:
> 
> Signed-off-by: Damien Zammit <damien@zamaudio.com>
> ---
>  sound/usb/mixer_maps.c   |   9 +++
>  sound/usb/mixer_quirks.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 186 insertions(+)
> 
> diff --git a/sound/usb/mixer_maps.c b/sound/usb/mixer_maps.c
> index d1d72ff..9ee3fc2 100644
> --- a/sound/usb/mixer_maps.c
> +++ b/sound/usb/mixer_maps.c
> @@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = {
>  	{ 0 } /* terminator */
>  };
>  
> +static struct usbmix_name_map mbox1_map[] = {
> +	{ 1, "Control" },

"Control" sounds too ambiguous.

> +	{ 0 } /* terminator */
> +};
> +
>  static struct usbmix_selector_map c400_selectors[] = {
>  	{
>  		.id = 0x80,
> @@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = {
>  		.map = aureon_51_2_map,
>  	},
>  	{
> +		.id = USB_ID(0x0dba, 0x1000),
> +		.map = mbox1_map,
> +	},
> +	{
>  		.id = USB_ID(0x13e5, 0x0001),
>  		.map = scratch_live_map,
>  		.ignore_ctl_error = 1,
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index 3980bf5..716c32c 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -565,6 +565,176 @@ static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer)
>  	return 0;
>  }
>  
> +/* Digidesign Mbox 1 clock source switch (internal/spdif) */
> +
> +struct snd_mbox1_switch_priv_val {
> +	struct usb_mixer_interface *mixer;
> +	int cached_value;
> +	int is_cached;

Use bool instead of int.  Also true/false for 1/0.

> +};
> +
> +static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_mbox1_switch_priv_val *pval;
> +	unsigned char value;
> +
> +	value = 0x00;
> +
> +	pval = (struct snd_mbox1_switch_priv_val *)
> +		kctl->private_value;
> +
> +	if (pval->is_cached) {
> +		ucontrol->value.enumerated.item[0] = pval->cached_value;
> +		return 0;
> +	}
> +
> +	ucontrol->value.enumerated.item[0] = value;
> +	pval->cached_value = value;
> +	pval->is_cached = 1;
> +
> +	return 0;
> +}
> +
> +static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_audio *chip;
> +	struct snd_mbox1_switch_priv_val *pval;
> +
> +	struct usb_mixer_interface *mixer;
> +	int changed, cur_val, err, new_val;
> +	unsigned char value[2];
> +	unsigned char buff[3];
> +
> +	changed = 0;
> +	value[0] = 0x00;
> +	value[1] = 0x00;
> +
> +	pval = (struct snd_mbox1_switch_priv_val *)
> +		kctl->private_value;
> +	cur_val = pval->cached_value;
> +	new_val = ucontrol->value.enumerated.item[0];
> +
> +	mixer = (struct usb_mixer_interface *) pval->mixer;

Superfluous cast.

> +	if (snd_BUG_ON(!mixer))
> +		return -EINVAL;
> +
> +	chip = (struct snd_usb_audio *) mixer->chip;

Ditto.

> +	if (snd_BUG_ON(!chip))
> +		return -EINVAL;
> +
> +	if (!pval->is_cached) {
> +		cur_val = value[0];
> +		pval->cached_value = cur_val;
> +		pval->is_cached = 1;
> +	}
> +	/* update value if needed */
> +	if (cur_val != new_val) {
> +		value[0] = new_val;
> +		value[1] = 0;
> +		down_read(&mixer->chip->shutdown_rwsem);

You can replace mixer->chip with chip (in below too).

> +		if (mixer->chip->shutdown) {
> +			err = -ENODEV;
> +		} else {
> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1);
> +			if (err < 0)
> +				return err;
> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
> +			if (err < 0)
> +				return err;

What do these two messages and why needed?

> +			if (new_val == 0) {
> +				buff[0] = 0x80;
> +				buff[1] = 0xbb;
> +				buff[2] = 0x00;
> +			} else {
> +				buff[0] = buff[1] = buff[2] = 0;
> +			}
> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> +				usb_sndctrlpipe(mixer->chip->dev, 0), 0x1,
> +				USB_TYPE_CLASS |
> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
> +			if (err < 0)
> +				return err;
> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
> +			if (err < 0)
> +				return err;
> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3);
> +			if (err < 0)
> +				return err;

Ditto.

> +		}
> +		up_read(&mixer->chip->shutdown_rwsem);
> +		if (err < 0)
> +			return err;
> +
> +		pval->cached_value = new_val;
> +		pval->is_cached = 1;
> +		changed = 1;
> +	}
> +
> +	return changed;
> +}
> +
> +static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol,
> +				 struct snd_ctl_elem_info *uinfo)
> +{
> +	static const char *const texts[2] = {
> +		"Internal",
> +		"S/PDIF"
> +	};
> +
> +	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
> +}
> +
> +static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer)
> +{
> +	static struct snd_kcontrol_new template = {
> +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +		.name = "Clock Source",
> +		.index = 0,
> +		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
> +		.info = snd_mbox1_switch_info,
> +		.get = snd_mbox1_switch_get,
> +		.put = snd_mbox1_switch_put
> +	};
> +
> +	struct snd_kcontrol *kctl;
> +	struct snd_mbox1_switch_priv_val *pval;
> +
> +	pval = kzalloc(sizeof(*pval), GFP_KERNEL);
> +	if (!pval)
> +		return -ENOMEM;
> +
> +	pval->cached_value = 0;
> +	pval->is_cached = 0;
> +	pval->mixer = mixer;
> +
> +	template.private_value = (unsigned long) pval;

This is basically racy (although it wouldn't happen practically
much) since static variable isn't local, thus two concurrent accesses
overwrite with each other.  Set kct->private_value instead of the
template below.

> +	kctl = snd_ctl_new1(&template, mixer->chip);
> +	if (!kctl) {
> +		kfree(pval);
> +		return -ENOMEM;
> +	}

You need to set private_free.  Otherwise pval will be leaked.


thanks,

Takashi


> +
> +	return snd_ctl_add(mixer->chip->card, kctl);
> +}
> +
>  /* Native Instruments device quirks */
>  
>  #define _MAKE_NI_CONTROL(bRequest,wIndex) ((bRequest) << 16 | (wIndex))
> @@ -1605,6 +1775,13 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
>  					      snd_audigy2nx_proc_read);
>  		break;
>  
> +	/* Digidesign Mbox 1 */
> +	case USB_ID(0x0dba, 0x1000):
> +		err = snd_mbox1_create_sync_switch(mixer);
> +		if (err < 0)
> +			break;
> +		break;
> +
>  	/* EMU0204 */
>  	case USB_ID(0x041e, 0x3f19):
>  		err = snd_emu0204_controls_create(mixer);
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Takashi Iwai]

At Tue,  4 Nov 2014 11:06:37 +1100,
Damien Zammit wrote:
> 
> This patch provides duplex support for the Digidesign Mbox 1 sound
> card and has been a work in progress for about a year.
> Users have confirmed on my website that previous versions of this patch
> have worked on the hardware and I have been testing extensively.
> 
> It also enables the mixer control for providing clock source
> selector based on the previous patch.
> 
> The sample rate has been hardcoded to 48kHz because it works better with
> the S/PDIF sync mode when the sample rate is locked.  This is the
> highest rate that the device supports and no loss of functionality
> is observed by restricting the sample rate.

Hmm, can we achieve this without introducing the new audioformats
thing, e.g. with COMPOSITE, instead?


thanks,

Takashi

> 
> Signed-off-by: Damien Zammit <damien@zamaudio.com>
> ---
>  sound/usb/card.h         |  5 ++++
>  sound/usb/quirks-table.h | 73 ++++++++++++++++++++++++++++++++----------------
>  sound/usb/quirks.c       | 30 ++++++++++++++++++++
>  sound/usb/usbaudio.h     |  1 +
>  4 files changed, 85 insertions(+), 24 deletions(-)
> 
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 97acb90..a8fe22f 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -33,6 +33,11 @@ struct audioformat {
>  	bool dsd_bitrev;		/* reverse the bits of each DSD sample */
>  };
>  
> +struct audioformats {
> +	unsigned int n_formats;
> +	const struct audioformat *format;
> +};
> +
>  struct snd_usb_substream;
>  struct snd_usb_endpoint;
>  
> diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
> index c657752..0b5f4fb 100644
> --- a/sound/usb/quirks-table.h
> +++ b/sound/usb/quirks-table.h
> @@ -2937,43 +2937,68 @@ YAMAHA_DEVICE(0x7010, "UB99"),
>  	/* Thanks to Clemens Ladisch <clemens@ladisch.de> */
>  	USB_DEVICE(0x0dba, 0x1000),
>  	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
> -		.vendor_name = "Digidesign",
> -		.product_name = "MBox",
> -		.ifnum = QUIRK_ANY_INTERFACE,
> -		.type = QUIRK_COMPOSITE,
> -		.data = (const struct snd_usb_audio_quirk[]){
> -			{
> -				.ifnum = 0,
> -				.type = QUIRK_IGNORE_INTERFACE,
> -			},
> -			{
> -				.ifnum = 1,
> -				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
> -				.data = &(const struct audioformat) {
> +	.vendor_name = "Digidesign",
> +	.product_name = "MBox",
> +	.ifnum = QUIRK_ANY_INTERFACE,
> +	.type = QUIRK_COMPOSITE,
> +	.data = (const struct snd_usb_audio_quirk[]) {
> +		{
> +			.ifnum = 0,
> +			.type = QUIRK_AUDIO_STANDARD_MIXER,
> +		},
> +		{
> +			.ifnum = 1,
> +			.type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINT,
> +			.data = &(const struct audioformats)
> +			{
> +				.n_formats = 2,
> +				.format = (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,
> +					.attributes = 0x4,
>  					.endpoint = 0x02,
> -					.ep_attr = 0x01,
> -					.rates = SNDRV_PCM_RATE_44100 |
> -						 SNDRV_PCM_RATE_48000,
> -					.rate_min = 44100,
> +					.ep_attr = USB_ENDPOINT_XFER_ISOC |
> +						USB_ENDPOINT_SYNC_SYNC,
> +					.maxpacksize = 0x130,
> +					.rates = SNDRV_PCM_RATE_48000,
> +					.rate_min = 48000,
>  					.rate_max = 48000,
> -					.nr_rates = 2,
> +					.nr_rates = 1,
>  					.rate_table = (unsigned int[]) {
> -						44100, 48000
> +						48000
> +					}
> +					},
> +					{
> +					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
> +					.channels = 2,
> +					.iface = 1,
> +					.altsetting = 1,
> +					.altset_idx = 1,
> +					.attributes = 0x4,
> +					.endpoint = 0x81,
> +					.ep_attr = USB_ENDPOINT_XFER_ISOC |
> +						USB_ENDPOINT_SYNC_ASYNC,
> +					.maxpacksize = 0x130,
> +					.rates = SNDRV_PCM_RATE_48000,
> +					.rate_min = 48000,
> +					.rate_max = 48000,
> +					.nr_rates = 1,
> +					.rate_table = (unsigned int[]) {
> +						48000
> +					}
>  					}
>  				}
> -			},
> -			{
> -				.ifnum = -1
>  			}
> +		},
> +		{
> +			.ifnum = -1
>  		}
> -
>  	}
> +}
>  },
>  
>  /* DIGIDESIGN MBOX 2 */
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index d2aa45a..90227cd 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -180,6 +180,34 @@ 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 audioformats *fp;
> +	int i;
> +
> +	fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL);
> +	if (!fp) {
> +		usb_audio_err(chip, "cannot memdup\n");
> +		return -ENOMEM;
> +	}
> +	for (i = 0; i < fp->n_formats; i++) {
> +		struct snd_usb_audio_quirk nquirk = {
> +			.ifnum = quirk->ifnum,
> +			.type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINT,
> +			.data = (const struct audioformat *) &fp->format[i]
> +		};
> +		create_fixed_stream_quirk(chip, iface, driver, &nquirk);
> +	}
> +	return 0;
> +}
> +
>  static int create_auto_pcm_quirk(struct snd_usb_audio *chip,
>  				 struct usb_interface *iface,
>  				 struct usb_driver *driver)
> @@ -528,6 +556,8 @@ 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 91d0380..31d8ff0 100644
> --- a/sound/usb/usbaudio.h
> +++ b/sound/usb/usbaudio.h
> @@ -96,6 +96,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,
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 1/2] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock source

[Damien Zammit]

Hi Takashi,

On 07/11/14 01:12, Takashi Iwai wrote:
> At Tue,  4 Nov 2014 11:06:36 +1100,
> Damien Zammit wrote:
>> +		down_read(&mixer->chip->shutdown_rwsem);
I dont know what down_read and up_read does.

>> +		if (mixer->chip->shutdown) {
>> +			err = -ENODEV;
>> +		} else {
>> +			err = snd_usb_ctl_msg(mixer->chip->dev,
>> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
>> +				USB_DIR_IN |
>> +				USB_TYPE_CLASS |
>> +				USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1);
>> +			if (err < 0)
>> +				return err;
>> +			err = snd_usb_ctl_msg(mixer->chip->dev,
>> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
>> +				USB_DIR_IN |
>> +				USB_TYPE_CLASS |
>> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
>> +			if (err < 0)
>> +				return err;
> 
> What do these two messages and why needed?

These two messages are the first half of the SPDIF mode selection.
The device does not respond to standard mixer commands.
I snooped the bus in Windows and replayed the packets when selecting
SPDIF mode.  Basically it appears that this command sets the mode of the
device to receive the SPDIF clock source setting.

>> +			if (new_val == 0) {
>> +				buff[0] = 0x80;
>> +				buff[1] = 0xbb;
>> +				buff[2] = 0x00;
>> +			} else {
>> +				buff[0] = buff[1] = buff[2] = 0;
>> +			}
If the mixer control is cleared to internal clock mode, the device
expects the next command to have the sample rate fed in.  Otherwise if
the mixer control is set to spdif mode, the buffer is fed 3 zero bytes
instead.  It does not work as expected if the sample rate is allowed to
be changed by the user, therefore I locked the sample rate to 48kHz.

>> +			err = snd_usb_ctl_msg(mixer->chip->dev,
>> +				usb_sndctrlpipe(mixer->chip->dev, 0), 0x1,
>> +				USB_TYPE_CLASS |
>> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
>> +			if (err < 0)
>> +				return err;
>> +			err = snd_usb_ctl_msg(mixer->chip->dev,
>> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
>> +				USB_DIR_IN |
>> +				USB_TYPE_CLASS |
>> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
>> +			if (err < 0)
>> +				return err;
>> +			err = snd_usb_ctl_msg(mixer->chip->dev,
>> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
>> +				USB_DIR_IN |
>> +				USB_TYPE_CLASS |
>> +				USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3);
>> +			if (err < 0)
>> +				return err;
This is where the magic happens...

>> +		}
>> +		up_read(&mixer->chip->shutdown_rwsem);
No idea what up_read is for.

Thanks for the review, and I will post a new patch soon.
How do I tell git send-email to reply to this thread?

Thanks,

Damien

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Damien Zammit]

Hi Takashi,

On 07/11/14 01:15, Takashi Iwai wrote:
> Hmm, can we achieve this without introducing the new audioformats
> thing, e.g. with COMPOSITE, instead?

I don't think COMPOSITE will work because the same interface has
multiple endpoints.  My code provides a framework for other devices with
the same issue.  I was told previously that this was the problem with
getting my previous attempt into the kernel.  Or is this doable without
adding a struct?

Damien

Re: [PATCH 1/2] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock source

[Takashi Iwai]

At Sun, 09 Nov 2014 00:30:48 +1100,
Damien Zammit wrote:
> 
> Hi Takashi,
> 
> On 07/11/14 01:12, Takashi Iwai wrote:
> > At Tue,  4 Nov 2014 11:06:36 +1100,
> > Damien Zammit wrote:
> >> +		down_read(&mixer->chip->shutdown_rwsem);
> I dont know what down_read and up_read does.

This is to protect against the disconnect and autopm.
If you touch the USB hardware, do lock with this.


> >> +		if (mixer->chip->shutdown) {
> >> +			err = -ENODEV;
> >> +		} else {
> >> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> >> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> >> +				USB_DIR_IN |
> >> +				USB_TYPE_CLASS |
> >> +				USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1);
> >> +			if (err < 0)
> >> +				return err;
> >> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> >> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> >> +				USB_DIR_IN |
> >> +				USB_TYPE_CLASS |
> >> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
> >> +			if (err < 0)
> >> +				return err;
> > 
> > What do these two messages and why needed?
> 
> These two messages are the first half of the SPDIF mode selection.
> The device does not respond to standard mixer commands.
> I snooped the bus in Windows and replayed the packets when selecting
> SPDIF mode.  Basically it appears that this command sets the mode of the
> device to receive the SPDIF clock source setting.

Then add the comment explaining that.  There is absolutely no reason
not to add any comment for such a black magic.

> >> +			if (new_val == 0) {
> >> +				buff[0] = 0x80;
> >> +				buff[1] = 0xbb;
> >> +				buff[2] = 0x00;
> >> +			} else {
> >> +				buff[0] = buff[1] = buff[2] = 0;
> >> +			}
> If the mixer control is cleared to internal clock mode, the device
> expects the next command to have the sample rate fed in.  Otherwise if
> the mixer control is set to spdif mode, the buffer is fed 3 zero bytes
> instead.  It does not work as expected if the sample rate is allowed to
> be changed by the user, therefore I locked the sample rate to 48kHz.

Ditto.  Give more comments.

> >> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> >> +				usb_sndctrlpipe(mixer->chip->dev, 0), 0x1,
> >> +				USB_TYPE_CLASS |
> >> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
> >> +			if (err < 0)
> >> +				return err;
> >> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> >> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> >> +				USB_DIR_IN |
> >> +				USB_TYPE_CLASS |
> >> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
> >> +			if (err < 0)
> >> +				return err;
> >> +			err = snd_usb_ctl_msg(mixer->chip->dev,
> >> +				usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,
> >> +				USB_DIR_IN |
> >> +				USB_TYPE_CLASS |
> >> +				USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3);
> >> +			if (err < 0)
> >> +				return err;
> This is where the magic happens...

And yet give more comments.


> >> +		up_read(&mixer->chip->shutdown_rwsem);
> No idea what up_read is for.
> 
> Thanks for the review, and I will post a new patch soon.
> How do I tell git send-email to reply to this thread?

For the revised patch series, you shouldn't connect to the old
thread.  Hanging too deeply make often rather difficult to read.


Takashi

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Takashi Iwai]

At Sun, 09 Nov 2014 00:35:29 +1100,
Damien Zammit wrote:
> 
> Hi Takashi,
> 
> On 07/11/14 01:15, Takashi Iwai wrote:
> > Hmm, can we achieve this without introducing the new audioformats
> > thing, e.g. with COMPOSITE, instead?
> 
> I don't think COMPOSITE will work because the same interface has
> multiple endpoints.  My code provides a framework for other devices with
> the same issue.  I was told previously that this was the problem with
> getting my previous attempt into the kernel.  Or is this doable without
> adding a struct?

I think the only point is the check in create_composite_quirk(), where
it marks the iface as claimed and skips the next entry that has been
already claimed.  However, the current code looks inconsistent -- it
allows multiple entries only if the iface matches with the current
one.  Fixing it like below would make things working.

It's a quick idea, so a bit more reviews would be needed, though.
Clemens, what do you think?

BTW, I won't take a look at you v2 series.  If the change above really
works, I'll brush up the patch below properly, so that you can include
in your new patch series.

Last but not least, please add maintainers to Cc when you post patches
for reviewing.


thanks,

Takashi

---
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
 		err = snd_usb_create_quirk(chip, iface, driver, quirk);
 		if (err < 0)
 			return err;
-		if (quirk->ifnum != probed_ifnum)
+	}
+
+	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
+		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
+		if (!iface)
+			continue;
+		if (quirk->ifnum != probed_ifnum &&
+		    !usb_interface_claimed(iface))
 			usb_driver_claim_interface(driver, iface, (void *)-1L);
 	}
+
 	return 0;
 }
 

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Clemens Ladisch]

Takashi Iwai wrote:
> Damien Zammit wrote:
>> On 07/11/14 01:15, Takashi Iwai wrote:
>>> Hmm, can we achieve this without introducing the new audioformats
>>> thing, e.g. with COMPOSITE, instead?
>>
>> I don't think COMPOSITE will work because the same interface has
>> multiple endpoints.  My code provides a framework for other devices with
>> the same issue.  I was told previously that this was the problem with
>> getting my previous attempt into the kernel.  Or is this doable without
>> adding a struct?
>
> I think the only point is the check in create_composite_quirk(), where
> it marks the iface as claimed and skips the next entry that has been
> already claimed.  However, the current code looks inconsistent -- it
> allows multiple entries only if the iface matches with the current
> one.  Fixing it like below would make things working.
>
> It's a quick idea, so a bit more reviews would be needed, though.
> Clemens, what do you think?

Reviewed-by: Clemens Ladisch <clemens@ladisch.de>

> ---
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
>  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
>  		if (err < 0)
>  			return err;
> -		if (quirk->ifnum != probed_ifnum)
> +	}
> +
> +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
> +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
> +		if (!iface)
> +			continue;
> +		if (quirk->ifnum != probed_ifnum &&
> +		    !usb_interface_claimed(iface))
>  			usb_driver_claim_interface(driver, iface, (void *)-1L);
>  	}
> +
>  	return 0;
>  }
>

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Takashi Iwai]

At Sun, 09 Nov 2014 15:04:42 +0100,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Damien Zammit wrote:
> >> On 07/11/14 01:15, Takashi Iwai wrote:
> >>> Hmm, can we achieve this without introducing the new audioformats
> >>> thing, e.g. with COMPOSITE, instead?
> >>
> >> I don't think COMPOSITE will work because the same interface has
> >> multiple endpoints.  My code provides a framework for other devices with
> >> the same issue.  I was told previously that this was the problem with
> >> getting my previous attempt into the kernel.  Or is this doable without
> >> adding a struct?
> >
> > I think the only point is the check in create_composite_quirk(), where
> > it marks the iface as claimed and skips the next entry that has been
> > already claimed.  However, the current code looks inconsistent -- it
> > allows multiple entries only if the iface matches with the current
> > one.  Fixing it like below would make things working.
> >
> > It's a quick idea, so a bit more reviews would be needed, though.
> > Clemens, what do you think?
> 
> Reviewed-by: Clemens Ladisch <clemens@ladisch.de>

Thanks.  I now queued the proper patch to for-next branch and pushed
out.

Damien, could you rebase your patches to for-next branch of sound git
tree, and use this new feature?


Takashi

> 
> > ---
> > --- a/sound/usb/quirks.c
> > +++ b/sound/usb/quirks.c
> > @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
> >  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
> >  		if (err < 0)
> >  			return err;
> > -		if (quirk->ifnum != probed_ifnum)
> > +	}
> > +
> > +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
> > +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
> > +		if (!iface)
> > +			continue;
> > +		if (quirk->ifnum != probed_ifnum &&
> > +		    !usb_interface_claimed(iface))
> >  			usb_driver_claim_interface(driver, iface, (void *)-1L);
> >  	}
> > +
> >  	return 0;
> >  }
> >
> 

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Damien Zammit]

On 10/11/14 04:27, Takashi Iwai wrote:
> At Sun, 09 Nov 2014 15:04:42 +0100,
> Clemens Ladisch wrote:
>>
>> Takashi Iwai wrote:
>>> I think the only point is the check in create_composite_quirk(), where
>>> it marks the iface as claimed and skips the next entry that has been
>>> already claimed.  However, the current code looks inconsistent -- it
>>> allows multiple entries only if the iface matches with the current
>>> one.  Fixing it like below would make things working.
>>>
>>> It's a quick idea, so a bit more reviews would be needed, though.
>>> Clemens, what do you think?
>>
>> Reviewed-by: Clemens Ladisch <clemens@ladisch.de>


>>> ---
>>> --- a/sound/usb/quirks.c
>>> +++ b/sound/usb/quirks.c
>>> @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
>>>  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
>>>  		if (err < 0)
>>>  			return err;
>>> -		if (quirk->ifnum != probed_ifnum)
>>> +	}
>>> +
>>> +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
>>> +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
>>> +		if (!iface)
>>> +			continue;
>>> +		if (quirk->ifnum != probed_ifnum &&
>>> +		    !usb_interface_claimed(iface))
>>>  			usb_driver_claim_interface(driver, iface, (void *)-1L);
>>>  	}
>>> +
>>>  	return 0;
>>>  }
>>>
>>

When I tried the patch on the hardware with my quirk, I got a kernel oops.

[   62.317456] BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
[   62.317461] IP: [<ffffffffa04a5f34>] create_composite_quirk+0x84/0xf0
[snd_usb_audio]
[   62.317469] PGD 0
[   62.317471] Oops: 0000 [#1] PREEMPT SMP
[   62.317474] Modules linked in: snd_usb_audio(OE+) snd_usbmidi_lib(OE)
snd_hwdep(O) snd_pcm(O) snd_seq_midi(O) snd_seq_midi_event(O)
snd_rawmidi(O) snd_seq(O) snd_seq_device(O) snd_timer(O) snd(O)
soundcore(O) binfmt_misc dvb_pll mt352 cx88_dvb videobuf_dvb
cx88_vp3054_i2c dvb_core rc_dntv_live_dvb_t kvm_amd kvm cx8800 cx8802
crct10dif_pclmul cx88xx crc32_pclmul ghash_clmulni_intel aesni_intel
btcx_risc tveeprom aes_x86_64 videobuf_dma_sg lrw rc_core gf128mul
videobuf_core dm_multipath glue_helper v4l2_common scsi_dh ablk_helper
videodev cryptd shpchp serio_raw i2c_algo_bit k10temp i2c_piix4 mac_hid
parport_pc ppdev lp parport btrfs(E) raid10(E) raid456(E)
async_raid6_recov(E) async_memcpy(E) async_pq(E) async_xor(E)
async_tx(E) xor(E) raid6_pq(E) raid0(E) multipath(E) linear(E)
dm_mirror(E) dm_region_hash(E) dm_log(E) raid1(E) hid_generic(E)
usbhid(E) hid(E) psmouse(E) firewire_ohci(E) firewire_core(E)
crc_itu_t(E) r8169(E) mii(E) sdhci_pci(E) sdhci(E) ahci(E) libahci(E)
[   62.317514] CPU: 1 PID: 1549 Comm: insmod Tainted: G           OE
3.18.0-rc2+ #3
[   62.317516] Hardware name: ASUS F2A85-M, BIOS 4.0-5272-g07d881a-dirty
01/16/2014
[   62.317518] task: ffff880036638000 ti: ffff880036004000 task.ti:
ffff880036004000
[   62.317519] RIP: 0010:[<ffffffffa04a5f34>]  [<ffffffffa04a5f34>]
create_composite_quirk+0x84/0xf0 [snd_usb_audio]
[   62.317526] RSP: 0018:ffff880036007b08  EFLAGS: 00010286
[   62.317528] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
0000000000000001
[   62.317529] RDX: 0000000000000008 RSI: 00000000ffffffff RDI:
ffff8804049b6400
[   62.317530] RBP: ffff880036007b28 R08: 0000000000000008 R09:
ffff88040e801b00
[   62.317531] R10: ffffffffa04221e0 R11: ffff880404f33890 R12:
0000000000000000
[   62.317532] R13: ffffffffa04b2f60 R14: ffff880036556d00 R15:
0000000000000000
[   62.317534] FS:  00007fa6ac676740(0000) GS:ffff88041ec80000(0000)
knlGS:0000000000000000
[   62.317535] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   62.317536] CR2: 0000000000000010 CR3: 00000000bad77000 CR4:
00000000000407e0
[   62.317538] Stack:
[   62.317539]  ffff880404f33800 0000000000000000 ffff880036556d00
ffffffffa04a8660
[   62.317541]  ffff880036007b38 ffffffffa04a5e7a ffff880036007bb8
ffffffffa0498888
[   62.317544]  ffff880404510cc8 0000000000000000 ffff880404f33890
ffff8804049b5c00
[   62.317546] Call Trace:
[   62.317554]  [<ffffffffa04a5e7a>] snd_usb_create_quirk+0x1a/0x50
[snd_usb_audio]
[   62.317560]  [<ffffffffa0498888>] usb_audio_probe+0x108/0x8d0
[snd_usb_audio]
[   62.317566]  [<ffffffff8158268f>] usb_probe_interface+0x1df/0x330
[   62.317569]  [<ffffffff814c4aad>] driver_probe_device+0x12d/0x3e0
[   62.317572]  [<ffffffff814c4e3b>] __driver_attach+0x9b/0xa0
[   62.317574]  [<ffffffff814c4da0>] ? __device_attach+0x40/0x40
[   62.317576]  [<ffffffff814c29d3>] bus_for_each_dev+0x63/0xa0
[   62.317578]  [<ffffffff814c448e>] driver_attach+0x1e/0x20
[   62.317580]  [<ffffffff814c4090>] bus_add_driver+0x180/0x240
[   62.317583]  [<ffffffff814c56a4>] driver_register+0x64/0xf0
[   62.317585]  [<ffffffff81580cf2>] usb_register_driver+0x82/0x160
[   62.317589]  [<ffffffffa04c1000>] ? 0xffffffffa04c1000
[   62.317594]  [<ffffffffa04c101e>] usb_audio_driver_init+0x1e/0x1000
[snd_usb_audio]
[   62.317597]  [<ffffffff8100212c>] do_one_initcall+0xbc/0x1f0
[   62.317601]  [<ffffffff811aa53c>] ? __vunmap+0x9c/0x110
[   62.317604]  [<ffffffff810f3252>] load_module+0x1d92/0x2820
[   62.317607]  [<ffffffff810ef0d0>] ? store_uevent+0x40/0x40
[   62.317610]  [<ffffffff810f3e56>] SyS_finit_module+0x86/0xb0
[   62.317613]  [<ffffffff81772aed>] system_call_fastpath+0x16/0x1b
[   62.317615] Code: 00 00 00 75 d2 48 89 d9 4c 89 ea 48 89 c6 4c 89 f7
e8 41 ff ff ff 85 c0 78 6f 48 83 c3 20 0f bf 73 10 66 85 f6 79 bd 48 8b
5b 18 <0f> bf 73 10 66 85 f6 79 10 eb 51 90 48 83 c3 20 0f bf 73 10 66
[   62.317638] RIP  [<ffffffffa04a5f34>]
create_composite_quirk+0x84/0xf0 [snd_usb_audio]
[   62.317645]  RSP <ffff880036007b08>
[   62.317646] CR2: 0000000000000010
[   62.317648] ---[ end trace d7b86fed44940082 ]---


My patch looks like this:

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index c657752..773859c 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2944,27 +2944,53 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 		.data = (const struct snd_usb_audio_quirk[]){
 			{
 				.ifnum = 0,
-				.type = QUIRK_IGNORE_INTERFACE,
+				.type = QUIRK_AUDIO_STANDARD_MIXER,
 			},
 			{
 				.ifnum = 1,
 				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
-				.data = &(const struct audioformat) {
-					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+				.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,
+					.attributes = 0x4,
 					.endpoint = 0x02,
-					.ep_attr = 0x01,
-					.rates = SNDRV_PCM_RATE_44100 |
-						 SNDRV_PCM_RATE_48000,
-					.rate_min = 44100,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC |
+						USB_ENDPOINT_SYNC_SYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_48000,
+					.rate_min = 48000,
 					.rate_max = 48000,
-					.nr_rates = 2,
+					.nr_rates = 1,
 					.rate_table = (unsigned int[]) {
-						44100, 48000
+						48000
+					}
+				}
+			},
+			{
+				.ifnum = 2,
+				.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 = 0x4,
+					.endpoint = 0x81,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC |
+						USB_ENDPOINT_SYNC_ASYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_48000,
+					.rate_min = 48000,
+					.rate_max = 48000,
+					.nr_rates = 1,
+					.rate_table = (unsigned int[]) {
+						48000
 					}
 				}
 			},


I think it is something to do with the usb_ifnum_to_if() function not
liking to be called when the interface is not claimed, or something like
that?  Or did I miss something silly?

Damien

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Damien Zammit]

On 10/11/14 10:45, Damien Zammit wrote:
Or did I miss something silly?

Yes, I think I accidentally removed my USB core drivers when I ran make
modules from sound/ and installed them over the top of my dpkg'ed kernel.

Will confirm this when I complete a full module compile.

Damien

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Takashi Iwai]

At Mon, 10 Nov 2014 10:45:19 +1100,
Damien Zammit wrote:
> 
> On 10/11/14 04:27, Takashi Iwai wrote:
> > At Sun, 09 Nov 2014 15:04:42 +0100,
> > Clemens Ladisch wrote:
> >>
> >> Takashi Iwai wrote:
> >>> I think the only point is the check in create_composite_quirk(), where
> >>> it marks the iface as claimed and skips the next entry that has been
> >>> already claimed.  However, the current code looks inconsistent -- it
> >>> allows multiple entries only if the iface matches with the current
> >>> one.  Fixing it like below would make things working.
> >>>
> >>> It's a quick idea, so a bit more reviews would be needed, though.
> >>> Clemens, what do you think?
> >>
> >> Reviewed-by: Clemens Ladisch <clemens@ladisch.de>
> 
> 
> >>> ---
> >>> --- a/sound/usb/quirks.c
> >>> +++ b/sound/usb/quirks.c
> >>> @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
> >>>  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
> >>>  		if (err < 0)
> >>>  			return err;
> >>> -		if (quirk->ifnum != probed_ifnum)
> >>> +	}
> >>> +
> >>> +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
> >>> +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
> >>> +		if (!iface)
> >>> +			continue;
> >>> +		if (quirk->ifnum != probed_ifnum &&
> >>> +		    !usb_interface_claimed(iface))
> >>>  			usb_driver_claim_interface(driver, iface, (void *)-1L);
> >>>  	}
> >>> +
> >>>  	return 0;
> >>>  }
> >>>
> >>
> 
> When I tried the patch on the hardware with my quirk, I got a kernel oops.

My bad, take the additional fix patch below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Fix Oops by composite quirk enhancement

The quirk argument itself was used as iterator, so it cannot be taken
back to the original value, obviously.

Fixes: d4b8fc66f770 ('ALSA: usb-audio: Allow multiple entries for the same iface in composite quirk')
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/quirks.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index e9ff3a6c60e4..809d7fab4633 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -43,12 +43,13 @@
 static int create_composite_quirk(struct snd_usb_audio *chip,
 				  struct usb_interface *iface,
 				  struct usb_driver *driver,
-				  const struct snd_usb_audio_quirk *quirk)
+				  const struct snd_usb_audio_quirk *quirk_comp)
 {
 	int probed_ifnum = get_iface_desc(iface->altsetting)->bInterfaceNumber;
+	const struct snd_usb_audio_quirk *quirk;
 	int err;
 
-	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
+	for (quirk = quirk_comp->data; quirk->ifnum >= 0; ++quirk) {
 		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
 		if (!iface)
 			continue;
@@ -60,7 +61,7 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
 			return err;
 	}
 
-	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
+	for (quirk = quirk_comp->data; quirk->ifnum >= 0; ++quirk) {
 		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
 		if (!iface)
 			continue;
-- 
2.1.3

Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Damien Zammit]

On 10/11/14 17:47, Takashi Iwai wrote:
> My bad, take the additional fix patch below.
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: usb-audio: Fix Oops by composite quirk enhancement
> 
> The quirk argument itself was used as iterator, so it cannot be taken
> back to the original value, obviously.
> 
> Fixes: d4b8fc66f770 ('ALSA: usb-audio: Allow multiple entries for the same iface in composite quirk')
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/quirks.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

I confirm that this fixes the kernel oops, and the logic works.  See
next patch series for working patch!

Damien