[alsa-devel] [PATCH 0/2] Digidesign Mbox 1 duplex mode and mixer
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(-)
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);
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
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
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
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,
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,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
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
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; }
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;
}
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;
}
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
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
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;
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
participants (3)
-
Clemens Ladisch -
Damien Zammit -
Takashi Iwai