From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH RFC V2] ALSA: usb-audio: Scarlett Gen 2 mixer interface Date: Mon, 29 Apr 2019 16:49:26 +0200 Message-ID: References: <20190429135602.GA3872@b4.vu> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 9205DF80C0D for ; Mon, 29 Apr 2019 16:49:27 +0200 (CEST) In-Reply-To: <20190429135602.GA3872@b4.vu> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: "Geoffrey D. Bennett" Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org On Mon, 29 Apr 2019 15:56:02 +0200, Geoffrey D. Bennett wrote: > > Add mixer quirk for the Focusrite Scarlett 18i8 and 18i20 Gen 2 audio > interfaces. Although the interfaces are USB compliant, additional > input/output level controls and hardware routing/mixing functionality > are available using proprietary USB requests. > > Signed-off-by: Geoffrey D. Bennett > --- > Hi all, > > This patch adds the following controls for the Scarlett 18i8 and 18i20 > Gen 2: > - Master volume knob indicator (18i20 only) > - Volume controls for the analogue HW outputs > - HW/SW volume switches for the 10 analogue HW outputs (18i20 only) > - Output mux (where the sound for the HW outputs comes from; defaults > to PCM outputs) > - Capture mux (where the sound for PCM recording comes from; defaults > to HW inputs) > - Matrix mux (where the sound going into the mixer comes from; 18 > inputs default off) > - Mixer matrix (18 inputs * 10 outputs = 180 controls) > - Mute and dim indicators (18i20 only) > - Line Level/Instrument Level and Pad controls (18i8 only) > - Level meters > > Changes since v1: > - Add support for the Scarlett 18i8 Gen 2 > - Save configuration parameters to NVRAM > - Implemented feedback from Takashi's email 24/Apr/2019 > - Moved private field from struct snd_usb_audio to struct > usb_mixer_interface > - Added timer and buffer fields to struct usb_mixer_interface > - Other small code fixes/cleanups/improvements > > Outstanding issues/questions: > > - Adding fields to struct usb_mixer_interface seemed a better place > than struct snd_usb_audio as in version 1 of this patch (I think > these new fields are used in the same sort of context as the "Sound > Blaster remote control stuff" fields, and I am reusing rc_urb and > rc_setup_packet). I wasn't sure of the naming; should they be named > generically ("timer", "private") or specifically > ("scarlett2_buffer")? Should the Sound Blaster RC fields that I'm > reusing be renamed to have generic names? The best would be completely without it, but it's a better place, indeed. However, the addition should be only one pointer field private_data and one function pointer private_free. Put the all device specific stuff to private_data and release via the own private_free callback instead. > - There are software controllable pads on the 18i8 line-inputs. I > called the controls "Line In %d Pad Capture Switch". They work > correctly in alsamixer (shown in the capture view as e.g. "Line In 1 > Pad") but the "on" state is displayed as "L R CAPTURE" which looks > unusual. I could use an Enum with Off/On values, but I thought this > is what "Switch" is for (as per control-names.rst). Should I leave > this as-is, or is there a better name that will make this show up as > a regular on/off switch? > > amixer contents shows: > > numid=15,iface=MIXER,name='Line In 1 Pad Capture Switch' > ; type=BOOLEAN,access=rw------,values=1 > : values=off > > amixer scontents shows: > > Simple mixer control 'Line In 1 Pad',0 > Capabilities: cswitch cswitch-joined > Capture channels: Mono > Mono: Capture [off] It's possible to override per device-specific stuff via a shared-object plugin in alsa-lib, but this is far too complex. Rather leave it as is. alsamixer isn't the best tool for a complicated mixer UI, after all. About the other changes: a timer is no suitable choice for such a purpose. The easier option would be the delayed work. Then you can use GFP_KERNEL instead of GFP_ATOMIC. Also, the delayed work needs to be canceled or flushed at PM suspend callback as well as disconnecting. thanks, Takashi