[alsa-devel] [PATCH v2] ALSA: usb-audio: Add support for Presonus Studio 1810c

[Takashi Iwai]

On Sat, 11 Jan 2020 18:40:56 +0100,
mickflemm at gmail.com wrote:
> 
> This patch adds support for Presonus Studio 1810c, a usb interface
> that's UAC2 compliant with a few quirks and a few extra hw-specific
> controls. I've tested all 3 altsettings and the added switch
> controls and they work as expected.
> 
> More infos on the card:
> https://www.presonus.com/products/Studio-1810c
> 
> Note that this work is based on packet inspection with
> usbmon. I just wanted to get this card to work for using
> it on our open-source radio station:
> https://github.com/UoC-Radio
> 
> v2 address issues reported by Takashi:
> * Properly get/set enum type controls
> * Prevent race condition on switch_get/set
> * Various control naming changes
> * Various coding style fixes
> 
> Signed-off-by: Nick Kossifidis <mickflemm at gmail.com>

Thanks, the patch looks almost good for merge, just a couple of
nitpicks:

> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -262,6 +262,23 @@ static int parse_uac2_sample_rate_range(struct snd_usb_audio *chip,
>  		}
>  
>  		for (rate = min; rate <= max; rate += res) {
> +
> +			/*
> +			 * Presonus Studio 1810c anounces invalid
> +			 * sampling rates for its streams.
> +			 */
> +			if (chip->usb_id == USB_ID(0x0194f, 0x010c) &&
> +			((rate > 48000 && fp->altsetting == 1) ||
> +			 ((rate < 88200 || rate > 96000)
> +			  && fp->altsetting == 2) ||
> +			 ((rate < 176400 || rate > 192000)
> +			  && fp->altsetting == 3))) {

I still find it too hard to read.  Maybe better to factor out this
check into a function.  Also it's clearer in a form of something like:

static bool sc1810c_valid_sample_rate()
{
	switch (fp->altsetting) {
	case 1:
		return rate <= 48000;
	case 2:
		return rate >= 88200 && rate <= 96000;
	case 3:
		return rate >= 176400 && rate <= 192000;
	default:
		return true;
	}
}

> --- /dev/null
> +++ b/sound/usb/mixer_s1810c.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Presonus Studio 1810c driver for ALSA
> + * Copyright (C) 2019 Nick Kossifidis <mickflemm at gmail.com>
> + *
> + * Based on reverse engineering of the communication protocol
> + * between the windows driver / Univeral Control (UC) program
> + * and the device, through usbmon.
> + *
> + * For now this bypasses the mixer, with all channels split,
> + * so that the software can mix with greater flexibility.
> + * It also adds controls for the 4 buttons on the front of
> + * the device.
> + */
> +
> +#include <linux/usb.h>
> +#include <linux/usb/audio-v2.h>
> +#include <linux/slab.h>
> +#include <sound/core.h>
> +#include <sound/control.h>
> +
> +#include "usbaudio.h"
> +#include "mixer.h"
> +#include "mixer_quirks.h"
> +#include "helper.h"

Include mixer_s1810c.h here, too, for the function declaration.

> +static const struct snd_kcontrol_new snd_s1810c_line_sw = {
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +	.name = "Line 1/2 Source Type Switch",

This one is with an enum, so better to avoid "Switch" suffix.
The user-space expects a "Switch" suffix is for a boolean blindly.
Either omit the switch suffix, or use "Route" suffix, in any.

Also, when I apply with git-am, the author is taken from From: line,
which doesn't contain your full name .  The best would to submit the
patch with git-send-email, then it'll add a proper From: line if
needed.


thanks,

Takashi