[alsa-devel] sound: usb-audio: full capture/playback/spdif support for Digidesign Mbox 2

[Takashi Iwai]

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 at 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