| Message ID | 5e02cab4.1c69fb81.83bec.f334@mx.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: usb-audio: Add support for Presonus Studio 1810c | expand |
Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on sound/for-next]
[cannot apply to v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/mickflemm-gmail-com/ALSA-usb-audio-Add-support-for-Presonus-Studio-1810c/20191226-163321
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
sound/usb/mixer_s1810c.c: In function 'snd_sc1810_mixer_state_free':
>> sound/usb/mixer_s1810c.c:507:2: error: implicit declaration of function 'vfree'; did you mean 'kfree'? [-Werror=implicit-function-declaration]
vfree(private);
^~~~~
kfree
sound/usb/mixer_s1810c.c: In function 'snd_sc1810_init_mixer':
>> sound/usb/mixer_s1810c.c:536:12: error: implicit declaration of function 'vzalloc'; did you mean 'kzalloc'? [-Werror=implicit-function-declaration]
private = vzalloc(sizeof(struct s1810_mixer_state));
^~~~~~~
kzalloc
>> sound/usb/mixer_s1810c.c:536:10: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
private = vzalloc(sizeof(struct s1810_mixer_state));
^
cc1: some warnings being treated as errors
vim +507 sound/usb/mixer_s1810c.c
503
504 static void snd_sc1810_mixer_state_free(struct usb_mixer_interface *mixer)
505 {
506 struct s1810_mixer_state *private = mixer->private_data;
> 507 vfree(private);
508 mixer->private_data = NULL;
509 }
510
511 /* Entry point, called from mixer_quirks.c */
512 int snd_sc1810_init_mixer(struct usb_mixer_interface *mixer)
513 {
514 struct s1810_mixer_state *private = NULL;
515 struct snd_usb_audio *chip = mixer->chip;
516 struct usb_device *dev = chip->dev;
517 int ret = 0;
518
519 /* Run this only once */
520 if (!list_empty(&chip->mixer_list))
521 return 0;
522
523 dev_info(&dev->dev,
524 "Presonus Studio 1810c, device_setup: %u\n", chip->setup);
525 if (chip->setup == 1)
526 dev_info(&dev->dev, "(8out/18in @ 48KHz)\n");
527 else if (chip->setup == 2)
528 dev_info(&dev->dev, "(6out/8in @ 192KHz)\n");
529 else
530 dev_info(&dev->dev, "(8out/14in @ 96KHz)\n");
531
532 ret = snd_s1810c_init_mixer_maps(chip);
533 if (ret < 0)
534 return ret;
535
> 536 private = vzalloc(sizeof(struct s1810_mixer_state));
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
On Wed, 25 Dec 2019 03:34:24 +0100, mickflemm@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 > > Signed-off-by: Nick Kossifidis <mickflemm@gmail.com> Thanks for the patch. In addition to the issues kbuild bot suggested, below are my quick review comments: > diff --git a/sound/usb/format.c b/sound/usb/format.c > index d79db7130..edf3f2a55 100644 > --- 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))) { > + if (res) > + continue; > + else > + break; > + } It's hard to imagine what result this would lead to, because the conditions are so complex. Maybe better to prepare a fixed table instead? Or, can we simply take a fixed quirk? > diff --git a/sound/usb/mixer_s1810c.c b/sound/usb/mixer_s1810c.c > new file mode 100644 > index 000000000..ff86e4aab > --- /dev/null > +++ b/sound/usb/mixer_s1810c.c > @@ -0,0 +1,565 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Presonus Studio 1810c driver for ALSA > + * Copyright (C) 2019 Nick Kossifidis <mickflemm@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. > + */ Usually place a blank line here. > +#include <linux/usb.h> > +#include <linux/usb/audio-v2.h> > +#include <sound/control.h> > +#include <linux/slab.h> The common practice is to group linux/*.h first, followed by sound/*.h. And <sound/core.h> is almost always needed before other sound/*.h. > +struct s1810c_ctl_packet { > + uint32_t a; > + uint32_t b; > + uint32_t fixed1; > + uint32_t fixed2; > + uint32_t c; > + uint32_t d; > + uint32_t e; Use u32, u16 or such types instead of uint*_t. It's the standard type for kernel codes. Also applied in other places in the patch, too. > +static int > +snd_sc1810c_get_status_field(struct usb_device *dev, > + uint32_t *field, int field_idx, uint16_t *seqnum) > +{ > + struct s1810c_state_packet pkt_out = { 0 }; > + struct s1810c_state_packet pkt_in = { 0 }; > + int ret = 0; > + > + pkt_out.fields[SC1810C_STATE_F1_IDX] = SC1810C_SET_STATE_F1; > + pkt_out.fields[SC1810C_STATE_F2_IDX] = SC1810C_SET_STATE_F2; > + ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), > + SC1810C_SET_STATE_REQ, > + SC1810C_SET_STATE_REQTYPE, > + (*seqnum), 0, &pkt_out, sizeof(pkt_out)); Avoid unnecessary parentheses around *seqnum. Ditto for other possible places. > +static int > +snd_s1810c_switch_set(struct snd_kcontrol *kctl, > + struct snd_ctl_elem_value *ctl_elem) > +{ > + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl); > + struct usb_mixer_interface *mixer = list->mixer; > + uint32_t curval = 0; > + uint32_t newval = (uint32_t) ctl_elem->value.integer.value[0]; > + int ret = 0; > + > + ret = snd_s1810c_get_switch_state(mixer, kctl, &curval); > + if (ret < 0) > + return 0; > + > + if (curval == newval) > + return 0; > + > + kctl->private_value &= ~(0x1 << 16); > + kctl->private_value |= (unsigned int)(newval & 0x1) << 16; > + ret = snd_s1810c_set_switch_state(mixer, kctl); Hm, this can be racy. The control get/put isn't protected, so you might get the inconsistency here when multiple kctls are accessed concurrently. > +static int > +snd_s1810c_line_sw_info(struct snd_kcontrol *kctl, > + struct snd_ctl_elem_info *uinfo) > +{ > + static const char *const texts[2] = { "Preamp on (Mic/Inst)", > + "Preamp off (Line in)" > + }; Better to put "Preamp on..." item in the next line. > +static struct snd_kcontrol_new snd_s1810c_line_sw = { This (and other following definitions) can be const? > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > + .name = "Line 1/2 source type", The control name should consist of words starting with a capital letter, so it should be "Line 1/2 Source Type". Also, usually a control name needs a standard suffix. Though, this has a special return enum type, so it can be OK as is. However... > + .info = snd_s1810c_line_sw_info, > + .get = snd_s1810c_switch_get, > + .put = snd_s1810c_switch_set, ... this shows that the combination is invalid. The enum type doesn't get/put the values in integer fields. It's incompatible. > + .private_value = (SC1810C_STATE_LINE_SW | SC1810C_CTL_LINE_SW << 8) > +}; > + > +static struct snd_kcontrol_new snd_s1810c_mute_sw = { > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > + .name = "Mute Main Out", ... and this one, for example, should deserve for "Switch" suffix, as it's a standard boolean switch (which use integer fields unlike enum). > +/* Entry point, called from mixer_quirks.c */ > +int snd_sc1810_init_mixer(struct usb_mixer_interface *mixer) > +{ > + struct s1810_mixer_state *private = NULL; > + struct snd_usb_audio *chip = mixer->chip; > + struct usb_device *dev = chip->dev; > + int ret = 0; > + > + /* Run this only once */ > + if (!list_empty(&chip->mixer_list)) > + return 0; > + > + dev_info(&dev->dev, > + "Presonus Studio 1810c, device_setup: %u\n", chip->setup); > + if (chip->setup == 1) > + dev_info(&dev->dev, "(8out/18in @ 48KHz)\n"); > + else if (chip->setup == 2) > + dev_info(&dev->dev, "(6out/8in @ 192KHz)\n"); > + else > + dev_info(&dev->dev, "(8out/14in @ 96KHz)\n"); > + > + ret = snd_s1810c_init_mixer_maps(chip); > + if (ret < 0) > + return ret; > + > + private = vzalloc(sizeof(struct s1810_mixer_state)); I don't think vmalloc is needed here, as the object is so small. Use kzalloc() and kfree() instead (unless I overlook something). > + if (!private) { > + dev_err(&dev->dev, "could not allocate mixer state\n"); The error message is usually superfluous, as kernel spews the warning in anyway at each allocation error. thanks, Takashi
Hello Takashi, happy new year and thanks for the feedback, On Fri, Dec 27, 2019 at 10:48 AM Takashi Iwai <tiwai@suse.de> wrote: > > diff --git a/sound/usb/format.c b/sound/usb/format.c > > index d79db7130..edf3f2a55 100644 > > --- 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))) { > > + if (res) > > + continue; > > + else > > + break; > > + } > > It's hard to imagine what result this would lead to, because the > conditions are so complex. > Maybe better to prepare a fixed table instead? Or, can we simply take > a fixed quirk? > I thought of using a fixed table but on the other hand the card does support reporting rates via the UAC2-compliant command and I noticed there are similar quirks on parse_audio_format_rates_v1. Maybe have a new quirk function that we can call from within the for loop for both UAC1/2 to filter misreported rates in one place ? I think it'll be cleaner that way. > > +static int > > +snd_s1810c_switch_set(struct snd_kcontrol *kctl, > > + struct snd_ctl_elem_value *ctl_elem) > > +{ > > + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl); > > + struct usb_mixer_interface *mixer = list->mixer; > > + uint32_t curval = 0; > > + uint32_t newval = (uint32_t) ctl_elem->value.integer.value[0]; > > + int ret = 0; > > + > > + ret = snd_s1810c_get_switch_state(mixer, kctl, &curval); > > + if (ret < 0) > > + return 0; > > + > > + if (curval == newval) > > + return 0; > > + > > + kctl->private_value &= ~(0x1 << 16); > > + kctl->private_value |= (unsigned int)(newval & 0x1) << 16; > > + ret = snd_s1810c_set_switch_state(mixer, kctl); > > Hm, this can be racy. The control get/put isn't protected, so you > might get the inconsistency here when multiple kctls are accessed > concurrently. > There is a lock on get/set_switch_state to serialize access to the card, it should take care of that. > > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > > + .name = "Line 1/2 source type", > > The control name should consist of words starting with a capital > letter, so it should be "Line 1/2 Source Type". > Also, usually a control name needs a standard suffix. Though, this > has a special return enum type, so it can be OK as is. > > However... > > > + .info = snd_s1810c_line_sw_info, > > + .get = snd_s1810c_switch_get, > > + .put = snd_s1810c_switch_set, > > ... this shows that the combination is invalid. The enum type doesn't > get/put the values in integer fields. It's incompatible. > I didn't get that one, isn't enum type an integer ? All controls added here are on/off switches and use the same get/set functions, I just wanted to provide some more info for some of them so I added custom .info functions. > > + .private_value = (SC1810C_STATE_LINE_SW | SC1810C_CTL_LINE_SW << 8) > > +}; > > + > > +static struct snd_kcontrol_new snd_s1810c_mute_sw = { > > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > > + .name = "Mute Main Out", > > ... and this one, for example, should deserve for "Switch" suffix, as > it's a standard boolean switch (which use integer fields unlike > enum). > Isn't "Switch" superfluous ? I don't see anything similar on the mixer controls of my other sound cards, e.g. it says "Mic Boost" not "Switch Mic Boost on". Also I still don't get the enum part, how can this be a standard boolean switch and the others not ? You mean because it uses snd_ctl_boolean_mono_info ? > > +/* Entry point, called from mixer_quirks.c */ > > +int snd_sc1810_init_mixer(struct usb_mixer_interface *mixer) > > +{ > > + struct s1810_mixer_state *private = NULL; > > + struct snd_usb_audio *chip = mixer->chip; > > + struct usb_device *dev = chip->dev; > > + int ret = 0; > > + > > + /* Run this only once */ > > + if (!list_empty(&chip->mixer_list)) > > + return 0; > > + > > + dev_info(&dev->dev, > > + "Presonus Studio 1810c, device_setup: %u\n", chip->setup); > > + if (chip->setup == 1) > > + dev_info(&dev->dev, "(8out/18in @ 48KHz)\n"); > > + else if (chip->setup == 2) > > + dev_info(&dev->dev, "(6out/8in @ 192KHz)\n"); > > + else > > + dev_info(&dev->dev, "(8out/14in @ 96KHz)\n"); > > + > > + ret = snd_s1810c_init_mixer_maps(chip); > > + if (ret < 0) > > + return ret; > > + > > + private = vzalloc(sizeof(struct s1810_mixer_state)); > > I don't think vmalloc is needed here, as the object is so small. > Use kzalloc() and kfree() instead (unless I overlook something). > Good point, I'll switch to kzalloc/kfree, this will also make the bot happy.
On Mon, 06 Jan 2020 02:00:39 +0100, Nick Kossifidis wrote: > > Hello Takashi, happy new year and thanks for the feedback, > > > On Fri, Dec 27, 2019 at 10:48 AM Takashi Iwai <tiwai@suse.de> wrote: > > > diff --git a/sound/usb/format.c b/sound/usb/format.c > > > index d79db7130..edf3f2a55 100644 > > > --- 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))) { > > > + if (res) > > > + continue; > > > + else > > > + break; > > > + } > > > > It's hard to imagine what result this would lead to, because the > > conditions are so complex. > > Maybe better to prepare a fixed table instead? Or, can we simply take > > a fixed quirk? > > > > I thought of using a fixed table but on the other hand the card does > support reporting rates via the UAC2-compliant command and I noticed > there are similar quirks on parse_audio_format_rates_v1. Maybe have a > new quirk function that we can call from within the for loop for both > UAC1/2 to filter misreported rates in one place ? I think it'll be > cleaner that way. Let's try in some different ways and compare how they look like. If the current code is the best in the end, we should give some more comments about the resultant rate tables. > > > > +static int > > > +snd_s1810c_switch_set(struct snd_kcontrol *kctl, > > > + struct snd_ctl_elem_value *ctl_elem) > > > +{ > > > + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl); > > > + struct usb_mixer_interface *mixer = list->mixer; > > > + uint32_t curval = 0; > > > + uint32_t newval = (uint32_t) ctl_elem->value.integer.value[0]; > > > + int ret = 0; > > > + > > > + ret = snd_s1810c_get_switch_state(mixer, kctl, &curval); > > > + if (ret < 0) > > > + return 0; > > > + > > > + if (curval == newval) > > > + return 0; > > > + > > > + kctl->private_value &= ~(0x1 << 16); > > > + kctl->private_value |= (unsigned int)(newval & 0x1) << 16; > > > + ret = snd_s1810c_set_switch_state(mixer, kctl); > > > > Hm, this can be racy. The control get/put isn't protected, so you > > might get the inconsistency here when multiple kctls are accessed > > concurrently. > > > > There is a lock on get/set_switch_state to serialize access to the > card, it should take care of that. The lock doesn't cover the two lines above fiddling with kctl->private_value. That's the racy point. > > > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > > > + .name = "Line 1/2 source type", > > > > The control name should consist of words starting with a capital > > letter, so it should be "Line 1/2 Source Type". > > Also, usually a control name needs a standard suffix. Though, this > > has a special return enum type, so it can be OK as is. > > > > However... > > > > > + .info = snd_s1810c_line_sw_info, > > > + .get = snd_s1810c_switch_get, > > > + .put = snd_s1810c_switch_set, > > > > ... this shows that the combination is invalid. The enum type doesn't > > get/put the values in integer fields. It's incompatible. > > > > I didn't get that one, isn't enum type an integer ? No, it takes a different type, value.enumerated.item[]. It's an int array, while value.integer.value[] (which is used for SNDRV_CTL_ELEM_TYPE_BOOLEAN and SNDRV_CTL_ELEM_TYPE_INTEGER) is a long array, hence the size differs on 64bit arch. > All controls added > here are on/off switches and use the same get/set functions, I just > wanted to provide some more info for some of them so I added custom > .info functions. > > > > + .private_value = (SC1810C_STATE_LINE_SW | SC1810C_CTL_LINE_SW << 8) > > > +}; > > > + > > > +static struct snd_kcontrol_new snd_s1810c_mute_sw = { > > > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > > > + .name = "Mute Main Out", > > > > ... and this one, for example, should deserve for "Switch" suffix, as > > it's a standard boolean switch (which use integer fields unlike > > enum). > > > > Isn't "Switch" superfluous ? I don't see anything similar on the mixer > controls of my other sound cards, e.g. it says "Mic Boost" not "Switch > Mic Boost on". Also I still don't get the enum part, how can this be a > standard boolean switch and the others not ? You mean because it uses > snd_ctl_boolean_mono_info ? There is a standard control name definition, see Documentation/sound/designs/control-names.rst. "Mic Boost" is one of standard names, hence it's OK. Although many drivers already don't follow that rule, you'll still see some conceptual idea there. thanks, Takashi
diff --git a/sound/usb/Makefile b/sound/usb/Makefile index 78edd7d2f..56031026b 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -13,6 +13,7 @@ snd-usb-audio-objs := card.o \ mixer_scarlett.o \ mixer_scarlett_gen2.o \ mixer_us16x08.o \ + mixer_s1810c.o \ pcm.o \ power.o \ proc.o \ diff --git a/sound/usb/format.c b/sound/usb/format.c index d79db7130..edf3f2a55 100644 --- 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))) { + if (res) + continue; + else + break; + } + if (fp->rate_table) fp->rate_table[nr_rates] = rate; if (!fp->rate_min || rate < fp->rate_min) diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index 39e27ae6c..71e705fab 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -34,6 +34,7 @@ #include "mixer_scarlett.h" #include "mixer_scarlett_gen2.h" #include "mixer_us16x08.h" +#include "mixer_s1810c.h" #include "helper.h" struct std_mono_table { @@ -2277,6 +2278,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer) case USB_ID(0x2a39, 0x3fd4): /* RME */ err = snd_rme_controls_create(mixer); break; + + case USB_ID(0x0194f, 0x010c): /* Presonus Studio 1810c */ + err = snd_sc1810_init_mixer(mixer); + break; } return err; diff --git a/sound/usb/mixer_s1810c.c b/sound/usb/mixer_s1810c.c new file mode 100644 index 000000000..ff86e4aab --- /dev/null +++ b/sound/usb/mixer_s1810c.c @@ -0,0 +1,565 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Presonus Studio 1810c driver for ALSA + * Copyright (C) 2019 Nick Kossifidis <mickflemm@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 <sound/control.h> +#include <linux/slab.h> + +#include "usbaudio.h" +#include "mixer.h" +#include "mixer_quirks.h" +#include "helper.h" + +#define SC1810C_CMD_REQ 160 +#define SC1810C_CMD_REQTYPE \ + (USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT) +#define SC1810C_CMD_F1 0x50617269 +#define SC1810C_CMD_F2 0x14 + +/* + * DISCLAIMER: These are just guesses based on the + * dumps I got. + * + * It seems like a selects between + * device (0), mixer (0x64) and output (0x65) + * + * For mixer (0x64): + * * b selects an input channel (see below). + * * c selects an output channel pair (see below). + * * d selects left (0) or right (1) of that pair. + * * e 0-> disconnect, 0x01000000-> connect, + * 0x0109-> used for stereo-linking channels, + * e is also used for setting volume levels + * in which case b is also set so I guess + * this way it is possible to set the volume + * level from the specified input to the + * specified output. + * + * IN Channels: + * 0 - 7 Mic/Inst/Line (Analog inputs) + * 8 - 9 S/PDIF + * 10 - 17 ADAT + * 18 - 35 DAW (Inputs from the host) + * + * OUT Channels (pairs): + * 0 -> Main out + * 1 -> Line1/2 + * 2 -> Line3/4 + * 3 -> S/PDIF + * 4 -> ADAT? + * + * For device (0): + * * b and c are not used, at least not on the + * dumps I got. + * * d sets the control id to be modified + * (see below). + * * e sets the setting for that control. + * (so for the switches I was interested + * in it's 0/1) + * + * For output (0x65): + * * b is the output channel (see above). + * * c is zero. + * * e I guess the same as with mixer except 0x0109 + * which I didn't see in my dumps. + * + * The two fixed fields have the same values for + * mixer and output but a different set for device. + */ +struct s1810c_ctl_packet { + uint32_t a; + uint32_t b; + uint32_t fixed1; + uint32_t fixed2; + uint32_t c; + uint32_t d; + uint32_t e; +}; + +#define SC1810C_CTL_LINE_SW 0 +#define SC1810C_CTL_MUTE_SW 1 +#define SC1810C_CTL_AB_SW 3 +#define SC1810C_CTL_48V_SW 4 + +#define SC1810C_SET_STATE_REQ 161 +#define SC1810C_SET_STATE_REQTYPE SC1810C_CMD_REQTYPE +#define SC1810C_SET_STATE_F1 0x64656D73 +#define SC1810C_SET_STATE_F2 0xF4 + +#define SC1810C_GET_STATE_REQ 162 +#define SC1810C_GET_STATE_REQTYPE \ + (USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN) +#define SC1810C_GET_STATE_F1 SC1810C_SET_STATE_F1 +#define SC1810C_GET_STATE_F2 SC1810C_SET_STATE_F2 + +#define SC1810C_STATE_F1_IDX 2 +#define SC1810C_STATE_F2_IDX 3 + +/* + * This packet includes mixer volumes and + * various other fields, it's an extended + * version of ctl_packet, with a and b + * being zero and different f1/f2. + */ +struct s1810c_state_packet { + uint32_t fields[63]; +}; + +#define SC1810C_STATE_48V_SW 58 +#define SC1810C_STATE_LINE_SW 59 +#define SC1810C_STATE_MUTE_SW 60 +#define SC1810C_STATE_AB_SW 62 + +struct s1810_mixer_state { + uint16_t seqnum; + struct mutex usb_mutex; +}; + +static int +snd_s1810c_send_ctl_packet(struct usb_device *dev, uint32_t a, + uint32_t b, uint32_t c, uint32_t d, uint32_t e) +{ + struct s1810c_ctl_packet pkt = { 0 }; + int ret = 0; + + pkt.fixed1 = SC1810C_CMD_F1; + pkt.fixed2 = SC1810C_CMD_F2; + + pkt.a = a; + pkt.b = b; + pkt.c = c; + pkt.d = d; + /* + * Value for settings 0/1 for this + * output channel is always 0 (probably because + * there is no ADAT output on 1810c) + */ + pkt.e = (c == 4) ? 0 : e; + + ret = snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), + SC1810C_CMD_REQ, + SC1810C_CMD_REQTYPE, 0, 0, &pkt, sizeof(pkt)); + if (ret < 0) { + dev_warn(&dev->dev, "could not send ctl packet\n"); + return ret; + } + return 0; +} + +/* + * When opening Universal Control the program periodicaly + * sends and receives state packets for syncinc state between + * the device and the host. + * + * Note that if we send only the request to get data back we'll + * get an error, we need to first send an empty state packet and + * then ask to receive a filled. Their seqnumbers must also match. + */ +static int +snd_sc1810c_get_status_field(struct usb_device *dev, + uint32_t *field, int field_idx, uint16_t *seqnum) +{ + struct s1810c_state_packet pkt_out = { 0 }; + struct s1810c_state_packet pkt_in = { 0 }; + int ret = 0; + + pkt_out.fields[SC1810C_STATE_F1_IDX] = SC1810C_SET_STATE_F1; + pkt_out.fields[SC1810C_STATE_F2_IDX] = SC1810C_SET_STATE_F2; + ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), + SC1810C_SET_STATE_REQ, + SC1810C_SET_STATE_REQTYPE, + (*seqnum), 0, &pkt_out, sizeof(pkt_out)); + if (ret < 0) { + dev_warn(&dev->dev, "could not send state packet (%d)\n", ret); + return ret; + } + + ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), + SC1810C_GET_STATE_REQ, + SC1810C_GET_STATE_REQTYPE, + (*seqnum), 0, &pkt_in, sizeof(pkt_in)); + if (ret < 0) { + dev_warn(&dev->dev, "could not get state field %u (%d)\n", + field_idx, ret); + return ret; + } + + (*field) = pkt_in.fields[field_idx]; + (*seqnum)++; + return 0; +} + +/* + * This is what I got when bypassing the mixer with + * all channels split. I'm not 100% sure of what's going + * on, I could probably clean this up based on my observations + * but I prefer to keep the same behavior as the windows driver. + */ +static int snd_s1810c_init_mixer_maps(struct snd_usb_audio *chip) +{ + uint32_t a, b, c, e, n, off; + struct usb_device *dev = chip->dev; + + /* Set initial volume levels ? */ + a = 0x64; + e = 0xbc; + for (n = 0; n < 2; n++) { + off = n * 18; + for (b = off, c = 0; b < 18 + off; b++) { + /* This channel to all outputs ? */ + for (c = 0; c <= 8; c++) { + snd_s1810c_send_ctl_packet(dev, a, b, c, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, c, 1, e); + } + /* This channel to main output (again) */ + snd_s1810c_send_ctl_packet(dev, a, b, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, 0, 1, e); + } + /* + * I noticed on UC that DAW channels have different + * initial volumes (they can go both above and below + * 0db), so this makes sense. + */ + e = 0xb53bf0; + } + + /* Connect analog outputs ? */ + a = 0x65; + e = 0x01000000; + for (b = 1; b < 3; b++) { + snd_s1810c_send_ctl_packet(dev, a, b, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, 0, 1, e); + } + snd_s1810c_send_ctl_packet(dev, a, 0, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, 0, 0, 1, e); + + /* Set initial volume levels for S/PDIF mappings ? */ + a = 0x64; + e = 0xbc; + c = 3; + for (n = 0; n < 2; n++) { + off = n * 18; + for (b = off; b < 18 + off; b++) { + snd_s1810c_send_ctl_packet(dev, a, b, c, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, c, 1, e); + } + e = 0xb53bf0; + } + + /* Connect S/PDIF output ? */ + a = 0x65; + e = 0x01000000; + snd_s1810c_send_ctl_packet(dev, a, 3, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, 3, 0, 1, e); + + /* Connect all outputs (again) ? */ + a = 0x65; + e = 0x01000000; + for (b = 0; b < 4; b++) { + snd_s1810c_send_ctl_packet(dev, a, b, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, 0, 1, e); + } + + /* Basic routing to get sound out of the device */ + a = 0x64; + e = 0x01000000; + for (c = 0; c < 4; c++) { + for (b = 0; b < 36; b++) { + if ((c == 0 && b == 18) || /* DAW1/2 -> Main */ + (c == 1 && b == 20) || /* DAW3/4 -> Line3/4 */ + (c == 2 && b == 22) || /* DAW4/5 -> Line5/4 */ + (c == 3 && b == 24)) { /* DAW5/6 -> S/PDIF */ + /* Left */ + snd_s1810c_send_ctl_packet(dev, a, b, c, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, c, 1, 0); + b++; + /* Right */ + snd_s1810c_send_ctl_packet(dev, a, b, c, 0, 0); + snd_s1810c_send_ctl_packet(dev, a, b, c, 1, e); + } else { + /* Leave the rest disconnected */ + snd_s1810c_send_ctl_packet(dev, a, b, c, 0, 0); + snd_s1810c_send_ctl_packet(dev, a, b, c, 1, 0); + } + } + } + + /* Set initial volume levels for S/PDIF (again) ? */ + a = 0x64; + e = 0xbc; + c = 3; + for (n = 0; n < 2; n++) { + off = n * 18; + for (b = off; b < 18 + off; b++) { + snd_s1810c_send_ctl_packet(dev, a, b, c, 0, e); + snd_s1810c_send_ctl_packet(dev, a, b, c, 1, e); + } + e = 0xb53bf0; + } + + /* Connect S/PDIF outputs (again) ? */ + a = 0x65; + e = 0x01000000; + snd_s1810c_send_ctl_packet(dev, a, 3, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, 3, 0, 1, e); + + /* Again ? */ + snd_s1810c_send_ctl_packet(dev, a, 3, 0, 0, e); + snd_s1810c_send_ctl_packet(dev, a, 3, 0, 1, e); + + return 0; +} + +/* + * Sync state with the device and retrieve the requested field, + * whose index is specified in (kctl->private_value & 0xFF), + * from the received fields array. + */ +static int +snd_s1810c_get_switch_state(struct usb_mixer_interface *mixer, + struct snd_kcontrol *kctl, uint32_t *state) +{ + struct snd_usb_audio *chip = mixer->chip; + struct s1810_mixer_state *private = mixer->private_data; + uint32_t field = 0; + uint32_t ctl_idx = (uint32_t) (kctl->private_value & 0xFF); + int ret = 0; + + mutex_lock(&private->usb_mutex); + ret = snd_sc1810c_get_status_field(chip->dev, &field, + ctl_idx, &private->seqnum); + if (ret < 0) + goto unlock; + + *state = field; + unlock: + mutex_unlock(&private->usb_mutex); + return ret ? ret : 0; +} + +/* + * Send a control packet to the device for the control id + * specified in (kctl->private_value >> 8) with value + * specified in (kctl->private_value >> 16). + */ +static int +snd_s1810c_set_switch_state(struct usb_mixer_interface *mixer, + struct snd_kcontrol *kctl) +{ + struct snd_usb_audio *chip = mixer->chip; + struct s1810_mixer_state *private = mixer->private_data; + uint32_t pval = (uint32_t) kctl->private_value; + uint32_t ctl_id = (pval >> 8) & 0xFF; + uint32_t ctl_val = (pval >> 16) & 0x1; + int ret = 0; + + mutex_lock(&private->usb_mutex); + ret = snd_s1810c_send_ctl_packet(chip->dev, 0, 0, 0, ctl_id, ctl_val); + mutex_unlock(&private->usb_mutex); + return ret; +} + +/* Generic get/set/init functions for switch controls */ + +static int +snd_s1810c_switch_get(struct snd_kcontrol *kctl, + struct snd_ctl_elem_value *ctl_elem) +{ + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl); + struct usb_mixer_interface *mixer = list->mixer; + uint32_t state = 0; + int ret = 0; + + ret = snd_s1810c_get_switch_state(mixer, kctl, &state); + if (ret < 0) + return ret; + + ctl_elem->value.integer.value[0] = (long)state; + + return 0; +} + +static int +snd_s1810c_switch_set(struct snd_kcontrol *kctl, + struct snd_ctl_elem_value *ctl_elem) +{ + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl); + struct usb_mixer_interface *mixer = list->mixer; + uint32_t curval = 0; + uint32_t newval = (uint32_t) ctl_elem->value.integer.value[0]; + int ret = 0; + + ret = snd_s1810c_get_switch_state(mixer, kctl, &curval); + if (ret < 0) + return 0; + + if (curval == newval) + return 0; + + kctl->private_value &= ~(0x1 << 16); + kctl->private_value |= (unsigned int)(newval & 0x1) << 16; + ret = snd_s1810c_set_switch_state(mixer, kctl); + if (ret < 0) + return 0; + + return 1; +} + +static int +snd_s1810c_switch_init(struct usb_mixer_interface *mixer, + const struct snd_kcontrol_new *new_kctl) +{ + struct snd_kcontrol *kctl; + struct usb_mixer_elem_info *elem; + + elem = kzalloc(sizeof(struct usb_mixer_elem_info), GFP_KERNEL); + if (!elem) + return -ENOMEM; + + elem->head.mixer = mixer; + elem->control = 0; + elem->head.id = 0; + elem->channels = 1; + + kctl = snd_ctl_new1(new_kctl, elem); + if (!kctl) { + kfree(elem); + return -ENOMEM; + } + kctl->private_free = snd_usb_mixer_elem_free; + + return snd_usb_mixer_add_control(&elem->head, kctl); +} + +static int +snd_s1810c_line_sw_info(struct snd_kcontrol *kctl, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const texts[2] = { "Preamp on (Mic/Inst)", + "Preamp off (Line in)" + }; + + return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts); +} + +static struct snd_kcontrol_new snd_s1810c_line_sw = { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Line 1/2 source type", + .info = snd_s1810c_line_sw_info, + .get = snd_s1810c_switch_get, + .put = snd_s1810c_switch_set, + .private_value = (SC1810C_STATE_LINE_SW | SC1810C_CTL_LINE_SW << 8) +}; + +static struct snd_kcontrol_new snd_s1810c_mute_sw = { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Mute Main Out", + .info = snd_ctl_boolean_mono_info, + .get = snd_s1810c_switch_get, + .put = snd_s1810c_switch_set, + .private_value = (SC1810C_STATE_MUTE_SW | SC1810C_CTL_MUTE_SW << 8) +}; + +static struct snd_kcontrol_new snd_s1810c_48v_sw = { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "48V Phantom power on Mic inputs", + .info = snd_ctl_boolean_mono_info, + .get = snd_s1810c_switch_get, + .put = snd_s1810c_switch_set, + .private_value = (SC1810C_STATE_48V_SW | SC1810C_CTL_48V_SW << 8) +}; + +static int +snd_s1810c_ab_sw_info(struct snd_kcontrol *kctl, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const texts[2] = { "1/2", + "3/4" + }; + + return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts); +} + +static struct snd_kcontrol_new snd_s1810c_ab_sw = { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Headphone 1 source selector", + .info = snd_s1810c_ab_sw_info, + .get = snd_s1810c_switch_get, + .put = snd_s1810c_switch_set, + .private_value = (SC1810C_STATE_AB_SW | SC1810C_CTL_AB_SW << 8) +}; + +static void snd_sc1810_mixer_state_free(struct usb_mixer_interface *mixer) +{ + struct s1810_mixer_state *private = mixer->private_data; + vfree(private); + mixer->private_data = NULL; +} + +/* Entry point, called from mixer_quirks.c */ +int snd_sc1810_init_mixer(struct usb_mixer_interface *mixer) +{ + struct s1810_mixer_state *private = NULL; + struct snd_usb_audio *chip = mixer->chip; + struct usb_device *dev = chip->dev; + int ret = 0; + + /* Run this only once */ + if (!list_empty(&chip->mixer_list)) + return 0; + + dev_info(&dev->dev, + "Presonus Studio 1810c, device_setup: %u\n", chip->setup); + if (chip->setup == 1) + dev_info(&dev->dev, "(8out/18in @ 48KHz)\n"); + else if (chip->setup == 2) + dev_info(&dev->dev, "(6out/8in @ 192KHz)\n"); + else + dev_info(&dev->dev, "(8out/14in @ 96KHz)\n"); + + ret = snd_s1810c_init_mixer_maps(chip); + if (ret < 0) + return ret; + + private = vzalloc(sizeof(struct s1810_mixer_state)); + if (!private) { + dev_err(&dev->dev, "could not allocate mixer state\n"); + return -ENOMEM; + } + + mutex_init(&private->usb_mutex); + + mixer->private_data = private; + mixer->private_free = snd_sc1810_mixer_state_free; + + private->seqnum = 1; + + ret = snd_s1810c_switch_init(mixer, &snd_s1810c_line_sw); + if (ret < 0) + return ret; + + ret = snd_s1810c_switch_init(mixer, &snd_s1810c_mute_sw); + if (ret < 0) + return ret; + + ret = snd_s1810c_switch_init(mixer, &snd_s1810c_48v_sw); + if (ret < 0) + return ret; + + ret = snd_s1810c_switch_init(mixer, &snd_s1810c_ab_sw); + if (ret < 0) + return ret; + return ret; +} diff --git a/sound/usb/mixer_s1810c.h b/sound/usb/mixer_s1810c.h new file mode 100644 index 000000000..2a9ae0922 --- /dev/null +++ b/sound/usb/mixer_s1810c.h @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Presonus Studio 1810c driver for ALSA + * Copyright (C) 2019 Nick Kossifidis <mickflemm@gmail.com> + */ +int snd_sc1810_init_mixer(struct usb_mixer_interface *mixer); diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 349e1e529..9a0ceaaf9 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -1216,6 +1216,36 @@ static int fasttrackpro_skip_setting_quirk(struct snd_usb_audio *chip, return 0; /* keep this altsetting */ } +static int s1810c_skip_setting_quirk(struct snd_usb_audio *chip, + int iface, int altno) +{ + /* + * Playback: + * 1: 6 Analog + 2 S/PDIF + * 2: 6 Analog + 2 S/PDIF + * 3: 6 Analog + * + * Capture: + * 1: 8 Analog + 2 S/PDIF + 8 ADAT + * 2: 8 Analog + 2 S/PDIF + 4 ADAT + * 3: 8 Analog + */ + + /* + * I'll leave 2 as the default one and + * use device_setup to switch to the + * other two. + */ + if (chip->setup == 1 && altno != 1) + return 1; + else if (chip->setup == 2 && altno != 3) + return 1; + else if (altno != 2) + return 1; + + return 0; +} + int snd_usb_apply_interface_quirk(struct snd_usb_audio *chip, int iface, int altno) @@ -1229,6 +1259,10 @@ int snd_usb_apply_interface_quirk(struct snd_usb_audio *chip, /* fasttrackpro usb: skip altsets incompatible with device_setup */ if (chip->usb_id == USB_ID(0x0763, 0x2012)) return fasttrackpro_skip_setting_quirk(chip, iface, altno); + /* presonus studio 1810c: skip altsets incompatible with device_setup */ + if (chip->usb_id == USB_ID(0x0194f, 0x010c)) + return s1810c_skip_setting_quirk(chip, iface, altno); + return 0; }
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 Signed-off-by: Nick Kossifidis <mickflemm@gmail.com> --- sound/usb/Makefile | 1 + sound/usb/format.c | 17 ++ sound/usb/mixer_quirks.c | 5 + sound/usb/mixer_s1810c.c | 565 +++++++++++++++++++++++++++++++++++++++ sound/usb/mixer_s1810c.h | 6 + sound/usb/quirks.c | 34 +++ 6 files changed, 628 insertions(+) create mode 100644 sound/usb/mixer_s1810c.c create mode 100644 sound/usb/mixer_s1810c.h