At Fri, 14 Dec 2012 01:20:59 +1100, Damien Zammit wrote:
Support for Digidesign Mbox 2 USB sound card:
This patch is the result of a lot of trial and error, since there are no specs available for the device.
Full duplex support is provided, i.e. playback and recording in stereo. The format is hardcoded at 48000Hz @ 24 bit, which is the maximum that the device supports. Also, MIDI in and MIDI out both work.
Users will notice that the S/PDIF light also flashes when playback or recording is active. I believe this means that S/PDIF input/output is simultaneously activated with the analogue i/o during use. But this particular functionality remains untested.
Note that this particular version of the patch is so far untested on the physical hardware because I have not compiled a full kernel with the changes. However, extensive testing has been done by many users of the hardware who believe other versions of my patch have worked since circa 2009.
Signed-off-by: Damien Zammit damien@zamaudio.com
Thanks for the quick update.
One more hint: put "[PATCH]" prefix to the subject so that people notice it's to review or merge more clearly.
Some nitpicking about codes below:
diff --git a/sound/usb/midi.c b/sound/usb/midi.c index 34b9bb7..45532dd 100644 --- a/sound/usb/midi.c +++ b/sound/usb/midi.c @@ -2181,6 +2181,11 @@ int snd_usbmidi_create(struct snd_card *card, umidi->usb_protocol_ops = &snd_usbmidi_novation_ops; err = snd_usbmidi_detect_per_port_endpoints(umidi, endpoints); break;
- case QUIRK_MIDI_MBOX2:
/* Digidesign Mbox 2 uses MIDIMAN MIDI protocol */umidi->usb_protocol_ops = &snd_usbmidi_midiman_ops;err = snd_usbmidi_detect_per_port_endpoints(umidi, endpoints);break;
If the quirk handling is as same as QUIRK_MIDI_MIDIMAN, can't we just reuse it instead of adding yet a new type?
+int snd_usb_mbox2_boot_quirk(struct usb_device *dev) +{
- struct usb_host_config *config = dev->actconfig;
- int err;
- u8 enablemagic[3];
- u8 bootresponse;
- u8 temp[12];
- int fwsize;
- int count;
- fwsize = le16_to_cpu(get_cfg_desc(config)->wTotalLength);
- if (fwsize == MBOX2_FIRMWARE_SIZE) {
Here you can reduce the indentation depth by simply returning the error like
if (fwsize != MBOX_FIRMWARE_SIZE) { snd_printk(KERN_ERR "usb-audio: Invalid firmware size: %d\n", fwsize); return -ENODEV; }
.... // the reset
(And, as you see, always put KERN_* prefix to *_printk(). Not necessarily for snd_printd() and snd_printdd(), if it's a debug message. Also better to put some suffix to the message, too.)
snd_printk("Sending Digidesign Mbox 2 boot sequence...\n");count = 0;bootresponse = MBOX2_BOOT_LOADING;while ((bootresponse == MBOX2_BOOT_LOADING) && (count < 10)) {mdelay(500); /* 0.5 second delay */
Use msleep() for such a long delay.
snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),0x85, 0xc0, 0x0001, 0x0000, &bootresponse, 0x0012);if (bootresponse == MBOX2_BOOT_READY)break;snd_printk("device not ready, resending boot sequence...\n");count++;}if (bootresponse == MBOX2_BOOT_READY) {
The same as firwamre size check. Handle the short error case and abort here. This reduces the indent level and improves the readability much better -- you don't have to consider a too long branching in your head while reading the code.
snd_printk("device initialised!\n");
Isn't it a debug message?
err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,&dev->descriptor, sizeof(dev->descriptor));config = dev->actconfig;if (err < 0)snd_printdd("error usb_get_descriptor: %d\n", err);err = usb_reset_configuration(dev);if (err < 0)snd_printdd("error usb_reset_configuration: %d\n", err);snd_printdd("mbox2_boot: new boot length = %d\n",le16_to_cpu(get_cfg_desc(config)->wTotalLength));/* We want 48 kHz mode for now */enablemagic[0] = 0x80;enablemagic[1] = 0xbb;enablemagic[2] = 0x00;/* Send the magic! */snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),0x01, 0x22, 0x0100, 0x0085, &temp, 0x0003);snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0),0x81, 0xa2, 0x0100, 0x0085, &enablemagic, 0x0003);snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0),0x81, 0xa2, 0x0100, 0x0086, &enablemagic, 0x0003);snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0),0x81, 0xa2, 0x0100, 0x0003, &enablemagic, 0x0003);
The magic code is same as used in mbox2_skip_setting_quirk(). Move the code to a function and just call it from both places instead of keeping two open codes.
thanks,
Takashi