Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Damien Zammit <damien.zammit@gmail.com>]

On 30/03/14 09:57, Mark Hills wrote:
> Thanks. I took a look and I think I have a similar concern to Daniel for 
> readability. The code should really be shared with the quirk on a single 
> endpoint.
I guess a significant rewrite will be required.

> The duplicate code in the patch already has different handling of 
> maxpacksize and datainterval -- is that intentional? It's apparent because 
> the new code ignores the values given in the quirk.
Not intentional, maxpacksize and datainterval happened to be identical
for both endpoints on the Mbox.

> If the data in the quirk is truly redundant, then really it should not be 
> present, or have the structure to use it -- it is misleading like this.
Yeah, I agree.  I have an idea.  The following is an excerpt from lsusb
for the Mbox:

    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0
      bInterfaceProtocol      0
      iInterface              0
      Endpoint Descriptor:
        bLength                 9
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            5
          Transfer Type            Isochronous
          Synch Type               Asynchronous
          Usage Type               Data
        wMaxPacketSize     0x0130  1x 304 bytes
        bInterval               1
        bRefresh                0
        bSynchAddress           0
      Endpoint Descriptor:
        bLength                 9
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            5
          Transfer Type            Isochronous
          Synch Type               Asynchronous
          Usage Type               Data
        wMaxPacketSize     0x0130  1x 304 bytes
        bInterval               1
        bRefresh                0
        bSynchAddress           0

Looking at the current struct for a snd_usb_audio_quirk:

struct snd_usb_audio_quirk {
        const char *vendor_name;
        const char *product_name;
        int16_t ifnum;
        uint16_t type;
        const void *data;
}

It is quite versatile, we shouldn't need to alter the quirk struct, just
the data it holds.  Is an array of audioformats for this kind of
interface the ideal choice?

Also, maybe we can read bNumEndpoints to determine if it is a
multi-endpoint interface and act accordingly, then we don't need an
extra element in snd_usb_audio_quirk.

How is a shared interface supposed to be initialised when it has
different properties for each endpoint?

> Yes, I dug out a Novation Twitch here. It uses different number of 
> channels (and hence buffer sizes) on the record and playback endpoints. At 
> very least other Focusrite devices will have this, too.
Interesting.

> It seems the assumption that an enpoint has sole ownership of the 
> interface is quite deeply spread in the code. For example, set_format() 
> called as part of snd_usb_pcm_prepare().
I am still trying to wrap my head around this one, but it sounds like
the major stumbling block with the current code?

> But I'm suprised your Mbox deals with the prepare step, perhaps it is 
> suprisingly tolerant. Did you confirm that you could start and stop a 
> recording during playback and that both streams were truly working?
Yes, I can start and stop recording while headphones are blasting, and
my microphone works.

> Specifically as snd_usb_pcm_close() shuts down the whole interface. Is 
> your Mbox continuing smoothly when this happens on one of the streams?
I use JACK so I'm not sure if the streams are ever closed until I
shutdown the server.

Damien