[alsa-devel] [PATCH] sound: usb-audio: Digidesign Mbox 2 Duplex mode support FIXED

[Takashi Iwai]

At Thu,  6 Jan 2011 21:27:28 +1100,
damien.zammit at gmail.com wrote:
> 
> From: Damien Zammit <damien.zammit at gmail.com>
> 
> I fixed the infinite loop problem, and the tab indentation *properly* this
> time.
> 
> Damien Zammit (1):
>   Support for Digidesign Mbox 2 in duplex analogue mode.
>     Also provided a wait timeout for bootup of the device of 5 seconds, 
>     there are no more infinite loops in this code.

Could you use git-format-patch or such so that I can apply the patch
directly via git-am?  Also, your patch is missing your sign-off.

Some review comments below.  I don't mention about the coding-style
issues here.  In general, try scripts/checkpatch.pl and fix warnings
suggested there before submission.

> @@ -594,3 +606,117 @@ void snd_usb_set_format_quirk(struct snd_usb_substream *subs,
>  	}
>  }
>  
> +#define MBOX2_FIRMWARE_SIZE    646
> +#define MBOX2_BOOT_LOADING     0x01 /* Hard coded into the device */
> +#define MBOX2_BOOT_READY       0x02 /* Hard coded into the device */
> +
> +static int snd_usb_mbox2_boot_quirk(struct usb_device *dev)
> +{
> +	struct usb_host_config *config = dev->actconfig;
> +	int err;
> +	wait_queue_head_t mbox2_wait_queue;
> +	u8 enablemagic[3];
> +	u8 bootresponse;
> +	u8 temp[12];
> +	int fwsize;
> +	int count;
> +
> +	init_waitqueue_head (&mbox2_wait_queue);
> +
> +	if ((fwsize = le16_to_cpu(get_cfg_desc(config)->wTotalLength)) == MBOX2_FIRMWARE_SIZE) {
> +		snd_printk("Sending Digidesign Mbox 2 boot sequence...\n");
> +
> +		/* From USB Snoop,
> +		 *
> +		 * SetupPacket = RT RQ VHVL INDX SIZE
> +		 * RT = 0xRT Request Type
> +		 * RQ = 0xRQ Request
> +		 * VHVL = 0xVLVH Value in reverse byte order
> +		 * INDX = 0xDXIN Index in reverse byte order
> +		 * SIZE = 0xZESI Size in reverse byte order
> +		 *
> +		 * Magic boot code setup packet: c0 85 01 00 00 00 12 00
> +		 *           RQ    RT    VLVH    DXIN                 ZESI
> +		 * becomes 0x85, 0xc0, 0x0001, 0x0000, &RETURNDATA, 0x0012, TIMEOUT
> +		 * for snd_usb_ctl_msg()
> +		 */
> +
> +		count = 0;
> +		bootresponse = MBOX2_BOOT_LOADING;
> +		while ((bootresponse == MBOX2_BOOT_LOADING) && (count<5)) {
> +			wait_event_timeout(mbox2_wait_queue, bootresponse == MBOX2_BOOT_READY, msecs_to_jiffies(1000)); /* 1 second delay */

Do we really need this asynchronous wait-event?

> +			snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),
> +				0x85, 0xc0, 0x0001, 0x0000, &bootresponse, 0x0012, 1000);

No need for error check?
The size is 0x12 although bootresponse is one byte?

> +			if (bootresponse == MBOX2_BOOT_READY)
> +				break;
> +
> +			snd_printk("device not ready, resending boot sequence...\n");
> +			count++;
> +		}	
> +
> +		if (bootresponse == MBOX2_BOOT_READY) {
> +			snd_printk("device initialised!\n");
> +
> +			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);

snd_printdd() should be avoided for error messages.  It's only for
verbose debugging messages.


> +			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, 1000);
> +			snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0),
> +				0x81, 0xa2, 0x0100, 0x0085, &enablemagic, 0x0003, 1000);
> +			snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0),
> +				0x81, 0xa2, 0x0100, 0x0086, &enablemagic, 0x0003, 1000);
> +			snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0),
> +				0x81, 0xa2, 0x0100, 0x0003, &enablemagic, 0x0003, 1000);

Hm, so these send the pointer to an array (i.e. char **)?
Somehow I doubt whether the data handling is correct or not.

> +			snd_printk(KERN_INFO "Digidesign Mbox 2: 24 Bit 48kHz Analogue");
> +
> +			return 0; /* Succesful boot */
> +		}
> +
> +		snd_printk("Unknown bootresponse, or timed out, ignoring device: %d\n",bootresponse);
> +		return -ENODEV;
> +	}
> +	snd_printk("Invalid firmware size: %d\n",fwsize);

Add KERN_ERR prefix.

> +	return -ENODEV;
> +}
> +
> +static int mbox2_skip_setting_quirk(struct snd_usb_audio *chip,
> +					int iface, int altno)
> +{
> +	u8 srate[3];
> +	u8 temp[12];
> +
> +	/* Reset ifaces 2,4,5 to 0 altsetting. */
> +	usb_set_interface(chip->dev, iface, 0);
> +
> +	//setup 48k
> +	srate[0]=0x80;
> +	srate[1]=0xbb;
> +	srate[2]=0x00;
> +
> +	/* Send the magic! */
> +	snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
> +		0x01, 0x22, 0x0100, 0x0085, &temp, 0x0003, 1000);
> +	snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +		0x81, 0xa2, 0x0100, 0x0085, &srate, 0x0003, 1000);
> +	snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +		0x81, 0xa2, 0x0100, 0x0086, &srate, 0x0003, 1000);
> +	snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +		0x81, 0xa2, 0x0100, 0x0003, &srate, 0x0003, 1000);
> +
> +	if (altno != 3) return 1;

Return a negative error code.


thanks,

Takashi