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

[Damien Zammit]

> On Wed, Dec 1, 2010 at 2:26 AM, Clemens Ladisch <clemens at ladisch.de> wrote:
>> Damien Zammit wrote:
>>> Unfortunately I am sending this email from gmail, so I'm not sure
>>> about line formatting, yikes.
>>
>> Long lines are wrapped.

Not much i can do about that, i dont have a plain text email client.

>>
>>> +             /*
>>> +              * We have to make sure that the USB core looks
>>> +              * again at interface 6 by calling usb_set_interface() on it.
>>> +              */
>>> +             usb_set_interface(umidi->dev, 6, 0);
>>
>> Why?  Is this another duplicate endpoint number?

The windows driver did this, and it doesn't work unless this code is here.

>>
>>> +             memcpy(&endpoints[0], quirk->data,
>>> +                     sizeof(struct snd_usb_midi_endpoint_info));
>>
>> I think you should be able to call snd_usbmidi_detect_per_port_endpoints
>> here.

Can you explain this a bit further, i'm not sure what this means.


>>
>>> +/* DIGIDESIGN MBOX 2 */
>>> +{
>>> +       /* Damien Zammit <damien.zammit at gmail.com> */
>>
>> This usually goes into the commit log.

I removed this from my patch

>>
>>> +       USB_DEVICE(0x0dba, 0x3000),
>>
>> The entries are supposed to be ordered by their IDs.
>>

I put this in order

>>> +                         * but now we will try sending
>>> capture/playback enable magic
>>> +                         *
>>> +                         * 80 bb 00 = 24bit mode - S24_3BE
>>> +                         * 44 ac 00 = 16bit mode?
>>
>> Why the question mark?

These are actually old comments which are irrelevant, i have removed this.

>>
>>> +                        //enablemagic[0]=0x44;
>>> +                        //enablemagic[1]=0xac;
>>> +                        //enablemagic[2]=0x00;
>>
>> Why is this commented out?
>>

Again, i was testing different sample rates, this is unnecessary and
has been removed.

>>> +#define MBOX2_SET_48K_SPDIF            0
>>> +#define MBOX2_SET_44K_SPDIF            1
>>> +#define MBOX2_SET_48K_ANALOG           2
>>> +#define MBOX2_SET_44K_ANALOG           3
>>
>> In theory, this should be some control instead of a module parameter.

I have removed support for spdif mode because the rest of the driver
doesnt support sending the full spdif frames anyway.

>>
>>> +static int mbox2_skip_setting_quirk(struct snd_usb_audio *chip,
>>> +                                        int iface, int altno)
>>
>> This function isn't consistently indented with tabs.
>>

I fixed the tab indentation


Clemens wrote last year:
>> +mbox2_reboot:
>> +                snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),
>> +                        0x85, 0xc0, 0x0001, 0x0000, &bootresponse, 0x0012, 1000);
>> +
>> +                if (bootresponse == MBOX2_BOOT_LOADING) {
>> +                        snd_printdd("device not ready, resending boot sequence...\n");
>> +                        goto mbox2_reboot;
>> +                }

>This could be made a loop.  And it could run infinitely long; maybe you
>should add a timeout.  (How long does it usually need?)

Can you help me write a loop with a timeout that is RT safe? I don't
know how to "sleep" for say 300ms in a safe way for all kernel
versions.  The device responds before 5 seconds from zero power to
bootup.

I am eager to get support into the kernel for this device because it
is very popular among audio enthusiasts.
SPDIF isnt all that important i think, but I made duplex mode work a
few weeks ago and could just enable that by default without the
device_setup parameter if that is preferred by the kernel developers.

Damien