Re: [alsa-devel] [PATCH RFC RESEND] ALSA: usb-audio: Scarlett mixer interface for 18i20 Gen 2

On Wed, 24 Apr 2019 01:58:42 +0200, Geoffrey D. Bennett wrote:

Add mixer quirk for the Focusrite Scarlett 18i20 Gen 2 audio interface. Although the interface is USB compliant, additional hardware mixing, routing, and metering functionality is available using proprietary USB requests.

Signed-off-by: Geoffrey D. Bennett g@b4.vu

(ping resend (no changes) as no feedback received for 3 weeks)

Oh sorry, I must have overlooked the post.

Hi all,

This patch adds the following controls for the Scarlett 18i20 Gen 2:

• Master volume knob indicator
• Volume controls for the 10 analogue HW outputs
• HW/SW volume switches for the 10 analogue HW outputs
• Output mux (where the sound for the HW outputs comes from; defaults to PCM outputs 1-20)
• Capture mux (where the sound for PCM recording comes from; defaults to HW inputs 1-18)
• 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
• Level meters

This is my first attempt at writing any kernel device driver code, so apologies in advance if I missed something. There's a couple of things I'm particularly unsure about:

• I added a private field to struct snd_usb_audio for storing the private mixer data. This seems wrong, but I didn't know where else I could/should put that data.

Hm, this is a thing I'd like to avoid as much as possible. Currently, each usb_mixer_elem_info may have a private pointer, and the original scarlett quirk uses it for storing its own "type" information. This could be used for storing the data instead?

• I've #define'd the vendor-specific interface, endpoint, max_data, and interval values. Is it okay to leave these hard-coded (they match the values provided by lsusb -v; I presume they never change?), or should they be looked up at runtime instead? (and if at runtime, how?)

In a case of USB-audio quirk entry, it's not too bad to hard-code the values in the entry table, indeed.

• I'm not sure if those snd_printk()s for unexpected responses from the hardware should be left in there? Removed? Reworded?

First off, avoid snd_printk() but use usb_audio_err() or dev_err() directly. The error should be fine to be there unless you can trigger the error too frequently from user-space intentionally.

Comments and feedback would be very welcome.

Below are some more comments:

+/* map from (dB + 80) * 2 to mixer value

• • for dB in 0 .. 172: int(8192 * pow(10, ((dB - 160) / 2 / 20)))
• */

+static const __u16 scarlett2_mixer_values[173] = {

The "__" prefix can be dropped here. In general, __u16 or friends are used for uapi definitions, and for the explicit type in the code, use u16, etc, instead.

+struct __attribute__((__packed__)) scarlett2_usb_volume_status {

We have __packed definition for __attribute__((__packed__). And put it rather after the closing brace, e.g.

struct scarlett2_usb_volume_status { .... } __packed;

+/* Send a proprietary format request to the Scarlett interface. */ +static int scarlett2_usb(

• struct usb_mixer_interface *mixer, __u32 cmd,
• void *req_data, __u16 req_size, void *resp_data, __u16 resp_size)

+{

• struct scarlett2_chip_data *private = mixer->chip->private;
• /* proprietary request/response format */
• struct scarlett2_usb_packet {

• __u32 cmd;
  

• __u16 size;
  

• __u16 seq;
  

• __u32 error;
  

• __u32 pad;
  

• __u8 data[];
  

• };

Better to define outside the function.

• __u16 req_buf_size =

• sizeof(struct scarlett2_usb_packet) + req_size;
  

• __u16 resp_buf_size =

• sizeof(struct scarlett2_usb_packet) + resp_size;
  

• struct scarlett2_usb_packet *req, *resp;
• int seq, err;
• __u32 resp_cmd;
• __u32 resp_seq;
• __u16 size;
• mutex_lock(&private->usb_mutex);
• /* build request message and send it */
• req = kmalloc(req_buf_size, GFP_KERNEL);
• if (!req) {

• mutex_unlock(&private->usb_mutex);
  

• return -ENOMEM;
  

The standard idiom is to move the whole mutex_unlock() calls into the end of the function and use goto here.

if (!req) { err = -ENOMEM; goto unlock; } .... unlock: mutex_lock(&private->usb_mutex); return err;

+int snd_scarlett_gen2_controls_create(struct usb_mixer_interface *mixer) +{

....

• /* Initialise private data, routing, sequence number */
• if ((err = scarlett2_init_private(mixer, info)) < 0)

• return err;
  

Try to run checkpatch.pl once and fix the errors reported there.

thanks,

Takashi