[alsa-devel] [PATCH V4] ALSA: usb-audio: Scarlett Gen 2 mixer interface

[Takashi Iwai]

On Tue, 09 Jul 2019 18:24:30 +0200,
Geoffrey D. Bennett wrote:
> 
> On Tue, Jul 09, 2019 at 03:57:28PM +0200, Takashi Iwai wrote:
> [...]
> > > But I tested suspending while the delayed work
> > >   was waiting and it seemed like the delayed work was cancelled
> > >   anyway. Probably better to do a flush on suspend though;
> > 
> > The flush would block, hence it wastes lots of time at suspend.
> > Ideally the suspend should be performed immediately.
> > 
> > > should this
> > >   be done through a hook like:
> > > 
> > >   diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > >   index 2444737a14b2..1572a89267c6 100644
> > >   --- a/sound/usb/mixer.c
> > >   +++ b/sound/usb/mixer.c
> > >   @@ -3547,6 +3547,8 @@ static int snd_usb_mixer_activate(struct usb_mixer_interface *mixer)
> > >    int snd_usb_mixer_suspend(struct usb_mixer_interface *mixer)
> > >    {
> > >           snd_usb_mixer_inactivate(mixer);
> > >   +       if (mixer->private_suspend)
> > >   +               mixer->private_suspend(mixer);
> > >           return 0;
> > >    }
> > >    
> > >   diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
> > >   index fa6e216a06f0..d94c688c65f7 100644
> > >   --- a/sound/usb/mixer.h
> > >   +++ b/sound/usb/mixer.h
> > >   @@ -28,6 +28,7 @@ struct usb_mixer_interface {
> > >    
> > >           void *private_data;
> > >           void (*private_free)(struct usb_mixer_interface *private_data);
> > >   +       void (*private_suspend)(struct usb_mixer_interface *);
> > >    };
> > >    
> > >    #define MAX_CHANNELS   16      /* max logical channels */
> > > 
> > >   ...or just keep the existing behaviour where it seems to get
> > >   cancelled?
> > 
> > I'm fine to add the new suspend/resume callbacks, but maybe the
> > corresponding resume would be needed.
> 
> Did you mean "wouldn't be needed"?

No, it's needed because at resume you'd reschedule the work that was
canceled at suspend.  It's specific.

> [...]
> > > +static int scarlett_gen2_mixer_enable;
> > > +module_param(scarlett_gen2_mixer_enable, int, 0444);
> > > +MODULE_PARM_DESC(scarlett_gen2_mixer_enable,
> > > +		 "Focusrite Scarlett Gen 2 Mixer Driver Enable");
> > 
> > Do we need this?  If disabling the quirk is really required, it should
> > be implemented rather in a generic option, instead.
> 
> Actually it would be best to have it disabled by default as I have had
> two reports from people who tried this mixer driver and it broke audio
> for them.

Hm, and these have the same USB device ID?

> The failure looks like this... the first vendor-specific USB message
> sent to the interface to initialise it results in an error:
> 
> e2d87480 3146602314 S Ci:1:007:0 s a1 03 0000 0005 0010 16 <
> e2d87480 3152107418 C Ci:1:007:0 -2 0
> 
> My mixer driver gives up initialising, and subsequent USB messages
> fail:
> 
> e2d87480 3152110424 S Ci:1:007:0 s 80 06 0304 0409 00ff 255 <
> e2d87480 3152121815 C Ci:1:007:0 -71 0
> e2d87480 3152121955 S Ci:1:007:0 s 80 06 0304 0409 0002 2 <
> e2d87480 3152131554 C Ci:1:007:0 -71 0
> e2d87480 3152133420 S Ci:1:007:0 s 80 06 0305 0409 00ff 255 <
> e2d87480 3157224469 C Ci:1:007:0 -2 0
> 
> I don't see any way to distinguish between devices that do and don't
> work before sending the initialisation message. lsusb -v output is
> identical. I need someone to do a USB capture to see what the vendor
> driver does differently, but I haven't found someone who can do this
> yet.
> 
> I have 9 success reports so far and 2 failure reports. I hope that by
> getting this driver out to a wider audience I can get the data I need
> to make it work in all cases and then we can remove the
> scarlett_gen2_mixer_enable quirk and have the driver always enabled.
> 
> Thanks for your feedback again. The other things which I didn't
> comment on I agree with and will fix.

I guess you can use chip->setup value for the quirk-specific
configuration as a start.  It's a bit hackish but the parameter is
already there for long time, hence you don't need to add any extra
stuff for that.


thanks,

Takashi