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

[Damien Zammit]

Hi Takashi,

Please find my responses below, and my updated patch (hopefully) in the right
format in a second post to come.

> 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.
>
I have followed these suggestions, thanks.

> > +		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?
Yes, unfortunately if the device is plugged in AFTER the module is loaded,
it takes up to 3.5 seconds to initialise and LEDs to power up, once it
receives the
'magic' boot setup packet, and before it will reply MBOX2_BOOT_READY.
I have moved the wait timeout to be at the end of each loop
so that it powers more quickly if already plugged in BEFORE the module
is loaded.

> > +			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?
>
I am copying the magic packet directly from the windows driver.  In
all cases which I tested, the response was 1 byte, even though the
size was specified as 0x12.
If the packet is modified to use a size of 0x01 byte, the device will
not initialise.
I suppose I could allocate 0x12 bytes for the response and only read
the first one?

> snd_printdd() should be avoided for error messages.  It's only for
> verbose debugging messages.
>
>
Thanks, I have changed this to snd_printk.

> > +			/* 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.
>
It works, because it sets the mode of the device correctly.
The only thing I am unsure about here is the "temp" variable, I
over-allocated it to
12 bytes because it never issues a response but I thought there might be cases
where it might send one, I have changed this down to 3 bytes long as
the size describes.

> > +	snd_printk("Invalid firmware size: %d\n",fwsize);
>
> Add KERN_ERR prefix.
Done.

> > +	if (altno != 3) return 1;
>
> Return a negative error code.
Done.

Please see my next post for the actual patch with these changes.

Regards,
Damien