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

[Takashi Iwai]

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 at 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