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@xxxxx>
> ---
> (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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
