[WIP PATCH] Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20.

Post by David Henningsson
+#define CTLS_OUTPUT(index, name) \
+ do { \
+ err = add_output_ctls(mixer, index, name, info); \
+ if (err< 0) \
+ return err; \
+ while (0)

AFAICS the last line should be:

} while (0)

Tobias

Daniel Mack

2014-10-08 19:04:06 UTC

Hi,

Post by David Henningsson
---
So, this is how Tobias' and Robin's patch look now. I've merged it all to one
big patch, both for my own simplicity and because I thought that made the most
sense. I've also fixed most checkpatch stuff (apart from long lines warnings).
It's not ready for merging yet, I assume. But it would be good with a hint w r t
what the big issues are with this patch as you see it. And then we could see if I
have some spare cycles to fix that up, or not.

Thanks for doing that!

I've done a first round of superficial review. In general, the #ifdefs
have to be cleaned up, and dead code (such that is inside of comments)
has to be removed or turned into something useful.

Also, some comments imply that not all code paths have been tested
really. It would be nice if that could be done. AFAIR there were some
volunteers that wanted to help with testing.

Some more comments inline.

Best regards,
Daniel

Post by David Henningsson
diff --git a/sound/usb/scarlettmixer.c b/sound/usb/scarlettmixer.c
new file mode 100644
index 0000000..873d5d6
--- /dev/null
+++ b/sound/usb/scarlettmixer.c

...

Post by David Henningsson
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/usb/audio-v2.h>
+
+#include <sound/core.h>
+#include <sound/control.h>
+#include <sound/tlv.h>
+
+#include "usbaudio.h"
+#include "mixer.h"
+#include "helper.h"
+#include "power.h"
+
+#include "scarlettmixer.h"
+
+/* #define WITH_METER */
+/* #define WITH_LOGSCALEMETER */

These should either be converted to module parameters, or removed
alltogether. Why are they configurable, anyway?

Post by David Henningsson
+#define LEVEL_BIAS 128 /* some gui mixers can't handle negative ctl values (alsamixergui, qasmixer, ...) */
+
+#ifndef LEVEL_BIAS
+ #define LEVEL_BIAS 0
+#endif

Same here.

Post by David Henningsson
+static void scarlett_mixer_elem_free(struct snd_kcontrol *kctl)
+{
+ kfree(kctl->private_data);
+ kctl->private_data = NULL;
+}
+
+/***************************** Low Level USB I/O *****************************/
+
+/* stripped down/adapted from get_ctl_value_v2 */
+static int get_ctl_urb2(struct snd_usb_audio *chip,
+ int bRequest, int wValue, int index,
+ unsigned char *buf, int size)
+{
+ int ret, idx = 0;
+
+ ret = snd_usb_autoresume(chip);
+ if (ret < 0 && ret != -ENODEV) {
+ ret = -EIO;
+ goto error;
+ }
+
+ down_read(&chip->shutdown_rwsem);
+ if (chip->shutdown) {
+ ret = -ENODEV;
+ } else {
+ idx = snd_usb_ctrl_intf(chip) | (index << 8);
+ ret = snd_usb_ctl_msg(chip->dev,
+ usb_rcvctrlpipe(chip->dev, 0),
+ bRequest,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ wValue, idx, buf, size);
+ }
+ up_read(&chip->shutdown_rwsem);
+ snd_usb_autosuspend(chip);
+
+ if (ret < 0) {
+ snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, size = %d\n",
+ bRequest, wValue, idx, size);
+ return ret;
+ }
+#if 0 /* rg debug XXX */
+ snd_printk(KERN_ERR "req ctl value: req = %#x, rtype = %#x, wValue = %#x, wIndex = %#x, size = %d\n",
+ bRequest, (USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN), wValue, idx, size);
+#endif

That can be moved to snd_printdd()

Post by David Henningsson
+ return 0;
+}
+
+/* adopted from snd_usb_mixer_set_ctl_value */
+static int set_ctl_urb2(struct snd_usb_audio *chip,
+ int request, int wValue, int index,
+ unsigned char *buf, int val_len)
+{
+ int idx = 0, err, timeout = 10;
+
+ err = snd_usb_autoresume(chip);
+ if (err < 0 && err != -ENODEV)
+ return -EIO;
+ down_read(&chip->shutdown_rwsem);
+ while (timeout-- > 0) {
+ if (chip->shutdown)
+ break;
+ idx = snd_usb_ctrl_intf(chip) | (index << 8);
+ if (snd_usb_ctl_msg(chip->dev,
+ usb_sndctrlpipe(chip->dev, 0), request,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT,
+ wValue, idx, buf, val_len) >= 0) {
+ err = 0;
+ goto out;
+ }
+ }
+ snd_printdd(KERN_ERR "cannot set ctl value: req = %#x, wValue = %#x, wIndex = %#x, len = %d, data = %#x/%#x\n",
+ request, wValue, idx, val_len, buf[0], buf[1]);
+ err = -EINVAL;
+
+ up_read(&chip->shutdown_rwsem);
+ snd_usb_autosuspend(chip);
+#if 0 /* rg debug XXX */
+ snd_printk(KERN_ERR "set ctl value: req = %#x, wValue = %#x, wIndex = %#x, len = %d, data = %#x/%#x\n",
+ request, wValue, idx, val_len, buf[0], buf[1]);
+#endif

Same here.

...

Post by David Henningsson
+static int sig_to_db(const int sig16bit)
+{
+ int i;
+ const int dbtbl[148] = {
+ 13, 14, 15, 16, 16, 17, 18, 20, 21, 22, 23, 25, 26, 28, 29, 31, 33, 35,
+ 37, 39, 41, 44, 46, 49, 52, 55, 58, 62, 66, 69, 74, 78, 83, 87, 93, 98,
+ 104, 110, 117, 123, 131, 139, 147, 155, 165, 174, 185, 196, 207, 220,
+ 233, 246, 261, 276, 293, 310, 328, 348, 369, 390, 414, 438, 464, 491,
+ 521, 551, 584, 619, 655, 694, 735, 779, 825, 874, 926, 981, 1039, 1100,
+ 1165, 1234, 1308, 1385, 1467, 1554, 1646, 1744, 1847, 1957, 2072, 2195,
+ 2325, 2463, 2609, 2764, 2927, 3101, 3285, 3479, 3685, 3904, 4135, 4380,
+ 4640, 4915, 5206, 5514, 5841, 6187, 6554, 6942, 7353, 7789, 8250, 8739,
+ 9257, 9806, 10387, 11002, 11654, 12345, 13076, 13851, 14672, 15541,
+ 16462, 17437, 18471, 19565, 20724, 21952, 23253, 24631, 26090, 27636,
+ 29274, 31008, 32846, 34792, 36854, 39037, 41350, 43801, 46396, 49145,
+ 52057, 55142, 58409, 61870
+ };
+
+ if (sig16bit < 13) {
+ switch (sig16bit) {
+ case 0: return 0;
+ case 1: return 1; /* -96.0dB */
+ case 2: return 13; /* -90.5dB */
+ case 3: return 20; /* -87.0dB */
+ case 4: return 26; /* -84.0dB */
+ case 5: return 29; /* -82.5dB */
+ case 6: return 32; /* -81.0dB */
+ case 7: return 35; /* -79.5dB */
+ case 8: return 37; /* -78.5dB */
+ case 9: return 39; /* -77.5dB */
+ case 10: return 41; /* -76.5dB */
+ case 11: return 43; /* -75.5dB */
+ case 12: return 45; /* -74.5dB */
+ }
+ }

I'd say a lookup table is nicer here, but that's a nit.

Post by David Henningsson
+static int scarlett_ctl_enum_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
+{
+ struct scarlett_mixer_elem_info *elem = kctl->private_data;
+ int err, val;
+
+ err = get_ctl_value(elem, 0, &val);
+ if (err < 0)
+ return err;
+
+ /* snd_printk(KERN_WARNING "enum %s: %x %x\n", ucontrol->id.name, val, elem->opt->len); */

No dead code in general. Either remove it or use snd_printd[d].

Post by David Henningsson
+static int scarlett_ctl_enum_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
+{
+ struct scarlett_mixer_elem_info *elem = kctl->private_data;
+ int changed = 0;
+ int err, oval, val;
+
+ err = get_ctl_value(elem, 0, &oval);
+ if (err < 0)
+ return err;
+
+ val = ucontrol->value.integer.value[0];
+#if 0 /* TODO? */
+ if (val == -1) {
+ val = elem->enum->len + 1; /* only true for master, not for mixer [also master must be used] */
+ /* ... or? > 0x20, 18i8: 0x22 */
+ } else
+#endif

Could someone with access to such hardware sort that out, maybe? :)

Post by David Henningsson
+static int scarlett_ctl_save_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
+{
+ struct scarlett_mixer_elem_info *elem = kctl->private_data;
+ int err;
+
+ if (ucontrol->value.enumerated.item[0] > 0) {
+ char buf[1] = { 0xa5 };
+
+ err = set_ctl_urb2(elem->mixer->chip, UAC2_CS_MEM, 0x005a, 0x3c, buf, 1);
+ if (err < 0)
+ return err;
+
+ snd_printk(KERN_INFO "Scarlett: Saved settings to hardware.\n");
+ }
+
+ return 0; /* (?) */

What's the confusion here? :)

...

Post by David Henningsson
+#define INIT(value) \
+ do { \
+ err = init_ctl(elem, value); \
+ if (err < 0) \
+ return err; \
+ } while (0)
+
+#define CTL_SWITCH(cmd, off, no, count, name) \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl_switch, cmd, off, no, 2, count, name, NULL, &elem); \
+ if (err < 0) \
+ return err; \
+ } while (0)
+
+/* no multichannel enum, always count == 1 (at least for now) */
+#define CTL_ENUM(cmd, off, no, name, opt) \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl_enum, cmd, off, no, 2, 1, name, opt, &elem); \
+ if (err < 0) \
+ return err; \
+ } while (0)
+
+#define CTL_MIXER(cmd, off, no, count, name) \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl, cmd, off, no, 2, count, name, NULL, &elem); \
+ if (err < 0) \
+ return err; \
+ INIT(-32768); /* -128*256 */ \
+ } while (0)
+
+#define CTL_MASTER(cmd, off, no, count, name) \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl_master, cmd, off, no, 2, count, name, NULL, &elem); \
+ if (err < 0) \
+ return err; \
+ INIT(0); \
+ } while (0)
+
+#define CTL_PEAK(cmd, off, no, count, name) /* but UAC2_CS_MEM */ \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl_meter, cmd, off, no, 2, count, name, NULL, NULL); \
+ if (err < 0) \
+ return err; \
+ } while (0)
+
+static int add_output_ctls(struct usb_mixer_interface *mixer,
+ int index, const char *name,
+ const struct scarlett_device_info *info)
+{
+ int err;
+ char mx[45];
+ struct scarlett_mixer_elem_info *elem;
+
+ snprintf(mx, 45, "Master %d (%s) Playback Switch", index+1, name); /* mute */
+ CTL_SWITCH(0x0a, 0x01, 2*index+1, 2, mx);
+
+ snprintf(mx, 45, "Master %d (%s) Playback Volume", index+1, name);
+ CTL_MASTER(0x0a, 0x02, 2*index+1, 2, mx);
+
+ snprintf(mx, 45, "Master %dL (%s) Source Playback Enum", index+1, name);
+ CTL_ENUM(0x33, 0x00, 2*index, mx, &info->opt_master);
+ INIT(info->mix_start);
+
+ snprintf(mx, 45, "Master %dR (%s) Source Playback Enum", index+1, name);
+ CTL_ENUM(0x33, 0x00, 2*index+1, mx, &info->opt_master);
+ INIT(info->mix_start + 1);

Hmm, I don't really like the style of magically returning from macros.
That makes the control flow uneasy to read IMO. Couldn't something with
statically initialized structs be done here, and then keep the code that
does the work generic? I haven't tried to implement that, just an idea.

...

Post by David Henningsson
+/* untested... */
+static const struct scarlett_device_info s6i6_info = {

Would be nice to get some testers here.

Post by David Henningsson
+ .matrix_in = 18,
+ .matrix_out = 8,
+ .input_len = 6,
+ .output_len = 6,
+
+ .pcm_start = 0,
+ .analog_start = 12,
+ .spdif_start = 16,
+ .adat_start = 18,
+ .mix_start = 18,
+
+ .opt_master = {
+ .start = -1,
+ .len = 27,
+ .texts = s6i6_texts
+ },
+
+ .opt_matrix = {
+ .start = -1,
+ .len = 19,
+ .texts = s6i6_texts
+ },
+
+ .controls_fn = scarlet_s6i6_controls,
+ .matrix_mux_init = {
+ 12, 13, 14, 15, /* Analog -> 1..4 */
+ 16, 17, /* SPDIF -> 5,6 */
+ 0, 1, 2, 3, 4, 5, 6, 7, /* PCM[1..12] -> 7..18 */
+ 8, 9, 10, 11
+ }
+};
+
+/* and 2 loop channels: Mix1, Mix2 */
+static const char * const s8i6_texts[] = {
+ txtOff, /* 'off' == 0xff */
+ txtPcm1, txtPcm2, txtPcm3, txtPcm4,
+ txtPcm5, txtPcm6, txtPcm7, txtPcm8,
+ txtPcm9, txtPcm10, txtPcm11, txtPcm12,
+ txtAnlg1, txtAnlg2, txtAnlg3, txtAnlg4,
+ txtSpdif1, txtSpdif2,
+ txtMix1, txtMix2, txtMix3, txtMix4,
+ txtMix5, txtMix6
+};
+
+/* untested... */
+static const struct scarlett_device_info s8i6_info = {
+ .matrix_in = 18,
+ .matrix_out = 6,
+ .input_len = 8,
+ .output_len = 6,
+
+ .pcm_start = 0,
+ .analog_start = 12,
+ .spdif_start = 16,
+ .adat_start = 18,
+ .mix_start = 18,
+
+ .opt_master = {
+ .start = -1,
+ .len = 25,
+ .texts = s8i6_texts
+ },
+
+ .opt_matrix = {
+ .start = -1,
+ .len = 19,
+ .texts = s8i6_texts
+ },
+
+ .controls_fn = scarlet_s8i6_controls,
+ .matrix_mux_init = {
+ 12, 13, 14, 15, /* Analog -> 1..4 */
+ 16, 17, /* SPDIF -> 5,6 */
+ 0, 1, 2, 3, 4, 5, 6, 7, /* PCM[1..12] -> 7..18 */
+ 8, 9, 10, 11
+ }
+};
+
+static const char * const s18i6_texts[] = {
+ txtOff, /* 'off' == 0xff */
+ txtPcm1, txtPcm2, txtPcm3, txtPcm4,
+ txtPcm5, txtPcm6,
+ txtAnlg1, txtAnlg2, txtAnlg3, txtAnlg4,
+ txtAnlg5, txtAnlg6, txtAnlg7, txtAnlg8,
+ txtSpdif1, txtSpdif2,
+ txtAdat1, txtAdat2, txtAdat3, txtAdat4,
+ txtAdat5, txtAdat6, txtAdat7, txtAdat8,
+ txtMix1, txtMix2, txtMix3, txtMix4,
+ txtMix5, txtMix6
+};
+
+static const struct scarlett_device_info s18i6_info = {
+ .matrix_in = 18,
+ .matrix_out = 6,
+ .input_len = 18,
+ .output_len = 6,
+
+ .pcm_start = 0,
+ .analog_start = 6,
+ .spdif_start = 14,
+ .adat_start = 16,
+ .mix_start = 24,
+
+ .opt_master = {
+ .start = -1,
+ .len = 31,
+ .texts = s18i6_texts
+ },
+
+ .opt_matrix = {
+ .start = -1,
+ .len = 25,
+ .texts = s18i6_texts
+ },
+
+ .controls_fn = scarlet_s18i6_controls,
+ .matrix_mux_init = {
+ 6, 7, 8, 9, 10, 11, 12, 13, /* Analog -> 1..8 */
+ 16, 17, 18, 19, 20, 21, /* ADAT[1..6] -> 9..14 */
+ 14, 15, /* SPDIF -> 15,16 */
+ 0, 1 /* PCM[1,2] -> 17,18 */
+ }
+};
+
+static const char * const s18i8_texts[] = {
+ txtOff, /* 'off' == 0xff (original software: 0x22) */
+ txtPcm1, txtPcm2, txtPcm3, txtPcm4,
+ txtPcm5, txtPcm6, txtPcm7, txtPcm8,
+ txtAnlg1, txtAnlg2, txtAnlg3, txtAnlg4,
+ txtAnlg5, txtAnlg6, txtAnlg7, txtAnlg8,
+ txtSpdif1, txtSpdif2,
+ txtAdat1, txtAdat2, txtAdat3, txtAdat4,
+ txtAdat5, txtAdat6, txtAdat7, txtAdat8,
+ txtMix1, txtMix2, txtMix3, txtMix4,
+ txtMix5, txtMix6, txtMix7, txtMix8
+};
+
+static const struct scarlett_device_info s18i8_info = {
+ .matrix_in = 18,
+ .matrix_out = 8,
+ .input_len = 18,
+ .output_len = 8,
+
+ .pcm_start = 0,
+ .analog_start = 8,
+ .spdif_start = 16,
+ .adat_start = 18,
+ .mix_start = 26,
+
+ .opt_master = {
+ .start = -1,
+ .len = 35,
+ .texts = s18i8_texts
+ },

Here, and in some other occasions, ARRAY_SIZE() should be used.

Post by David Henningsson
+ .opt_matrix = {
+ .start = -1,
+ .len = 27,
+ .texts = s18i8_texts
+ },

Same here.

Post by David Henningsson
+static const struct scarlett_device_info s18i20_info = {
+ .matrix_in = 18,
+ .matrix_out = 8,
+ .input_len = 18,
+ .output_len = 20,
+
+ .pcm_start = 0,
+ .analog_start = 20,
+ .spdif_start = 28,
+ .adat_start = 30,
+ .mix_start = 38,
+
+ .opt_master = {
+ .start = -1,
+ .len = 47,
+ .texts = s18i20_texts
+ },

Dito

Post by David Henningsson
+
+ .opt_matrix = {
+ .start = -1,
+ .len = 39,
+ .texts = s18i20_texts
+ },

Dito

Post by David Henningsson
+
+ .controls_fn = scarlet_s18i20_controls,
+ .matrix_mux_init = {
+ 20, 21, 22, 23, 24, 25, 26, 27, /* Analog -> 1..8 */
+ 30, 31, 32, 33, 34, 35, /* ADAT[1..6] -> 9..14 */
+ 28, 29, /* SPDIF -> 15,16 */
+ 0, 1 /* PCM[1,2] -> 17,18 */
+ }
+};
+
+/*
+int scarlett_reset(struct usb_mixer_interface *mixer)
+{
+ TODO? save first-time init flag into device?
+
+ unmute [master +] mixes (switches are currently not initialized)
+ [set(get!) impedance: 0x01, 0x09, 1..2]
+ [set(get!) 0x01, 0x08, 3..4]
+ [set(get!) pad: 0x01, 0x0b, 1..4]
+
+ matrix inputs (currently in scarlett_mixer_controls)
+}
+*/

Should be removed, or get a real implementation.

Robin Gareus

2014-10-08 21:21:54 UTC

Thanks David for picking this up.

Post by David Henningsson
+
+/* #define WITH_METER */
+/* #define WITH_LOGSCALEMETER */

These should either be converted to module parameters, or removed
alltogether. Why are they configurable, anyway?

Mainly because I lost interest in Focusrite devices before finishing the
work. They were handy during initial development and the idea was to
just enable them by default at some point.

Last I checked they both worked fine for the 18i6, but it seems support
other Scarlett devices is missing and/or untested, so I guess they're
commented out for that reason.

Post by Daniel Mack
+#define CTL_SWITCH(cmd, off, no, count, name) \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl_switch, cmd, off, no, 2,

count, name, NULL, &elem); \

Post by Daniel Mack
+ if (err < 0) \
+ return err; \
+ } while (0)

curious, why "do { } while (0)" and not just "{ }" ?

Post by Daniel Mack
Hmm, I don't really like the style of magically returning from macros

Yes, same here. I'm initially responsible for one such macro and wanted
to get rid of it. I'm sorry if that lead Tobias to adding more in that
style.

Cheers!
robin

Tobias Hoffmann

2014-10-08 22:06:09 UTC

Post by Robin Gareus
Thanks David for picking this up.

Post by David Henningsson
+
+/* #define WITH_METER */
+/* #define WITH_LOGSCALEMETER */

These should either be converted to module parameters, or removed
alltogether. Why are they configurable, anyway?

Mainly because I lost interest in Focusrite devices before finishing the
work. They were handy during initial development and the idea was to
just enable them by default at some point.
Last I checked they both worked fine for the 18i6, but it seems support
other Scarlett devices is missing and/or untested, so I guess they're
commented out for that reason.

AFAIR in the latest version something crashes when opening such
"channels" with (I think) alsamixer.
I didn't even try to debug that, because no Mixer-GUI could actually
display "realtime" metering data by periodically polling channel values,
IIRC. The even bigger problem is, that the Scarlett HW returns all
metering data at once (for a given metering point), e.g. all 18 inputs
together, in contrast to the mixer-channels, where each value can be set
separately.
Naively mapped, one would get three snd_kcontrols (metering points:
input, mix, pcm) with snd_ctl_elem_info::count = 18, 6, 18 (e.g.).
Alternatively, the driver would have to read all values, cache them
internally and expose them as separate snd_kcontrols. In either case, a
mixer GUI probably still has to have special knowledge about the card to
display the values in a helpful way.

Other drivers seem to use the hwdep-API for things like metering.
I do not understand the hwdep-API enough to implement it as such.
And as trrichad wrote a GUI [1], that can be used to display the hwdep
data correctly.

The one reason why the metering code is still in the driver (and
disabled), is because it basically documents how to read the metering
values...

Post by Robin Gareus

Post by Daniel Mack
+#define CTL_SWITCH(cmd, off, no, count, name) \
+ do { \
+ err = add_new_ctl(mixer,&usb_scarlett_ctl_switch, cmd, off, no, 2,

count, name, NULL,&elem); \

Post by Daniel Mack
+ if (err< 0) \
+ return err; \
+ } while (0)

curious, why "do { } while (0)" and not just "{ }" ?

I didn't even have { }, it was just
err = ...;
if (err<0) return err;

I would guess checkpatch suggested to add "do {} while(0)" for style
reasons?

Post by Robin Gareus

Post by Daniel Mack
Hmm, I don't really like the style of magically returning from macros

Yes, same here. I'm initially responsible for one such macro and wanted
to get rid of it. I'm sorry if that lead Tobias to adding more in that
style.

No, it's not your fault. I tried other ways, but the code only became
less readable:
- each operation can return an error, which has to be checked. Adding
the error checks everywhere (i.e. inlining the macros) just clutters the
code.
- CTL_ENUM, INIT and CTL_MIXER are called in a loop, based on data in
the static sXiY_info device info structs. And you don't want arrays with

Post by Robin Gareus
100 entries *for each supported Scarlett type*.

- also add_output_ctls() calculates some of the values from the
function arguments.
- In many places the same macro (e.g. CTL_SWITCH) isn't actually
repeated that often, but different CTL_* are used.

Still, some macros can probably be inlined, and esp. the device-specific
scarlett_sXiY_controls init functions now turn out to be similar enough
that a static struct entries per device and a generic init function
based on these entris seem to make sense.

Tobias

[1] https://github.com/trrichard/ScarlettMixer

Takashi Iwai

2014-10-09 08:39:06 UTC

At Wed, 08 Oct 2014 23:21:54 +0200,

Post by David Henningsson
+#define CTL_SWITCH(cmd, off, no, count, name) \
+ do { \
+ err = add_new_ctl(mixer, &usb_scarlett_ctl_switch, cmd, off, no, 2,

count, name, NULL, &elem); \

Post by David Henningsson
+ if (err < 0) \
+ return err; \
+ } while (0)

curious, why "do { } while (0)" and not just "{ }" ?

It's a standard trick to make the macro working with if/else in kernel
codes.

Suppose a macro like

#define FOO() { .... }

and use it like

if (something)
FOO();
else
bar();

then you'll get a build error.
But it can be avoided by surrounding do and while (0).

#define FOO() do { .... } while (0)

Takashi

Tobias Hoffmann

2014-10-08 22:49:57 UTC

Post by David Henningsson
+
+/* #define WITH_METER */
+/* #define WITH_LOGSCALEMETER */
These should either be converted to module parameters, or removed
alltogether. Why are they configurable, anyway?

As I've said in my earlier mail, code is current not working: Metering
should be removed from this patch (or rewritten to use hwdep-API).

Post by David Henningsson
+#define LEVEL_BIAS 128 /* some gui mixers can't handle negative ctl values (alsamixergui, qasmixer, ...) */
+
+#ifndef LEVEL_BIAS
+ #define LEVEL_BIAS 0
+#endif

Same here.

As LEVEL_BIAS = 0 definitely causes problems with some mixer GUIs, the
#ifndef ... #endif should be removed completely.

Post by David Henningsson
+#if 0 /* rg debug XXX */
+ snd_printk(KERN_ERR "req ctl value: req = %#x, rtype = %#x, wValue = %#x, wIndex = %#x, size = %d\n",
+ bRequest, (USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN), wValue, idx, size);
+#endif

That can be moved to snd_printdd()

These are left overs from Robins code, even I didn't need them. It's
probably safe to just remove all the
#if 0 /* rg debug XXX */ ... #endif
from the patch.

Post by David Henningsson
+static int scarlett_ctl_enum_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
+{
+ struct scarlett_mixer_elem_info *elem = kctl->private_data;
+ int changed = 0;
+ int err, oval, val;
+
+ err = get_ctl_value(elem, 0,&oval);
+ if (err< 0)
+ return err;
+
+ val = ucontrol->value.integer.value[0];
+#if 0 /* TODO? */
+ if (val == -1) {
+ val = elem->enum->len + 1; /* only true for master, not for mixer [also master must be used] */
+ /* ... or?> 0x20, 18i8: 0x22 */
+ } else
+#endif

Could someone with access to such hardware sort that out, maybe? :)

The current code works as-is, so #if 0 ... #endif can be removed.

Post by David Henningsson
+static int scarlett_ctl_save_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
+{
+ struct scarlett_mixer_elem_info *elem = kctl->private_data;
+ int err;
+
+ if (ucontrol->value.enumerated.item[0]> 0) {
+ char buf[1] = { 0xa5 };
+
+ err = set_ctl_urb2(elem->mixer->chip, UAC2_CS_MEM, 0x005a, 0x3c, buf, 1);
+ if (err< 0)
+ return err;
+
+ snd_printk(KERN_INFO "Scarlett: Saved settings to hardware.\n");
+ }
+
+ return 0; /* (?) */

What's the confusion here? :)

The "Save to HW" enum uses a special kcontrol .get/.put combo:
.get always returns 0, and .put issues a special command to the hardware.

I not 100% sure what the "best" return value of the .put function would
be in that case.
AFAIUI, .put functions have to return whether the value has changed...
and here:
- something happend (supporting return 1;)
- but .get still returns 0 (supporting return 0;)

Post by David Henningsson
+static struct snd_kcontrol_new usb_scarlett_ctl_save = {
+ .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+ .name = "",
+ .info = scarlett_ctl_enum_info,
+ .get = scarlett_ctl_save_get,
+ .get = scarlett_ctl_save_put,
+};

BUG! .get is set twice instead of .get and .put...

Testing is definitely needed for that change.

Post by David Henningsson
+/* untested... */
+static const struct scarlett_device_info s6i6_info = {

Would be nice to get some testers here.

I did heard of one person who used it (successfully) on s6i6.
That's not to say that we should not test the "final" patch on each device.

Post by David Henningsson
+
+/* untested... */
+static const struct scarlett_device_info s8i6_info = {

I can't remember any users with s8i6 right now...

I have a s18i8.

s18i6 is what the original code did, but I have not heard of tests with
the new code.

A few s18i20 users reported "no problems".

Post by David Henningsson
+
+static const char * const s18i8_texts[] = {
+ txtOff, /* 'off' == 0xff (original software: 0x22) */
+ txtPcm1, txtPcm2, txtPcm3, txtPcm4,
+ txtPcm5, txtPcm6, txtPcm7, txtPcm8,
+ txtAnlg1, txtAnlg2, txtAnlg3, txtAnlg4,
+ txtAnlg5, txtAnlg6, txtAnlg7, txtAnlg8,
+ txtSpdif1, txtSpdif2,
+ txtAdat1, txtAdat2, txtAdat3, txtAdat4,
+ txtAdat5, txtAdat6, txtAdat7, txtAdat8,
+ txtMix1, txtMix2, txtMix3, txtMix4,
+ txtMix5, txtMix6, txtMix7, txtMix8
+};
+
+static const struct scarlett_device_info s18i8_info = {
+ .matrix_in = 18,
+ .matrix_out = 8,
+ .input_len = 18,
+ .output_len = 8,
+
+ .pcm_start = 0,
+ .analog_start = 8,
+ .spdif_start = 16,
+ .adat_start = 18,
+ .mix_start = 26,
+
+ .opt_master = {
+ .start = -1,
+ .len = 35,
+ .texts = s18i8_texts
+ },
Here, and in some other occasions, ARRAY_SIZE() should be used.

Post by David Henningsson
+ .opt_matrix = {
+ .start = -1,
+ .len = 27,
+ .texts = s18i8_texts
+ },

Same here.

ARRAY_SIZE(s18i8_text) == 35, so .len = (ARRAY_SIZE()-8) ?
I not sure that's helpful...
The better way to think about it is:
.opt_matrix.len = .mix_start + 1,
.opt_master.len = .opt_matrix.len + .matrix_out // == ARRAY_SIZE()

Post by David Henningsson
+/*
+int scarlett_reset(struct usb_mixer_interface *mixer)
+{
+ TODO? save first-time init flag into device?
+
+ unmute [master +] mixes (switches are currently not initialized)
+ [set(get!) impedance: 0x01, 0x09, 1..2]
+ [set(get!) 0x01, 0x08, 3..4]
+ [set(get!) pad: 0x01, 0x0b, 1..4]
+
+ matrix inputs (currently in scarlett_mixer_controls)
+}
+*/
Should be removed, or get a real implementation.

Remove. Just some random ideas.

Tobias

Daniel Mack

2014-10-09 08:31:15 UTC

Hi Tobias,

As I've said in my earlier mail, code is current not working: Metering
should be removed from this patch (or rewritten to use hwdep-API).

Post by David Henningsson
+#define LEVEL_BIAS 128 /* some gui mixers can't handle negative ctl values (alsamixergui, qasmixer, ...) */
+
+#ifndef LEVEL_BIAS
+ #define LEVEL_BIAS 0
+#endif

Same here.

As LEVEL_BIAS = 0 definitely causes problems with some mixer GUIs, the
#ifndef ... #endif should be removed completely.

Alright. David, will you be working on new version of the patch?

That can be moved to snd_printdd()

These are left overs from Robins code, even I didn't need them. It's
probably safe to just remove all the
#if 0 /* rg debug XXX */ ... #endif
from the patch.

Sure it's safe, as dead code doesn't do anything :) The question is
whether the line produces any useful information for people having
trouble with their device. If so, we can just convert it to snd_printdd().

Post by David Henningsson
+static int scarlett_ctl_save_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
+{
+ struct scarlett_mixer_elem_info *elem = kctl->private_data;
+ int err;
+
+ if (ucontrol->value.enumerated.item[0]> 0) {
+ char buf[1] = { 0xa5 };
+
+ err = set_ctl_urb2(elem->mixer->chip, UAC2_CS_MEM, 0x005a, 0x3c, buf, 1);
+ if (err< 0)
+ return err;
+
+ snd_printk(KERN_INFO "Scarlett: Saved settings to hardware.\n");

Btw - this should become a debug print as well.

Post by David Henningsson
+ }
+
+ return 0; /* (?) */

What's the confusion here? :)

.get always returns 0, and .put issues a special command to the hardware.
I not 100% sure what the "best" return value of the .put function would
be in that case.
AFAIUI, .put functions have to return whether the value has changed...
- something happend (supporting return 1;)
- but .get still returns 0 (supporting return 0;)

Ah, I see. Yes, I'd consider that a misuse of the ALSA control API, as
you're not dealing with an actual control here. I think that should
become a hwdep interface then.

BUG! .get is set twice instead of .get and .put...

Right, so the code in .put was never actually used, right? Did the
compiler not warn about that?

Post by Tobias Hoffmann
Testing is definitely needed for that change.

Post by David Henningsson
+/* untested... */
+static const struct scarlett_device_info s6i6_info = {

Would be nice to get some testers here.

I did heard of one person who used it (successfully) on s6i6.
That's not to say that we should not test the "final" patch on each device.

Yes, that would be good.

Post by David Henningsson
Here, and in some other occasions, ARRAY_SIZE() should be used.

Post by David Henningsson
+ .opt_matrix = {
+ .start = -1,
+ .len = 27,
+ .texts = s18i8_texts
+ },

Same here.

ARRAY_SIZE(s18i8_text) == 35, so .len = (ARRAY_SIZE()-8) ?
I not sure that's helpful...
.opt_matrix.len = .mix_start + 1,
.opt_master.len = .opt_matrix.len + .matrix_out // == ARRAY_SIZE()

Hmm, yes. I was just thinking about something that makes the values look
a bit less magical.

Daniel

Tobias Hoffmann

2014-10-09 10:13:33 UTC

That can be moved to snd_printdd()

These are left overs from Robins code, even I didn't need them. It's
probably safe to just remove all the
#if 0 /* rg debug XXX */ ... #endif
from the patch.

Well, yes. I was looking for a short word for "no one will miss it",
when I wrote "safe"...

Post by Tobias Hoffmann
.get always returns 0, and .put issues a special command to the hardware.
I not 100% sure what the "best" return value of the .put function would
be in that case.
AFAIUI, .put functions have to return whether the value has changed...
- something happend (supporting return 1;)
- but .get still returns 0 (supporting return 0;)

Ah, I see. Yes, I'd consider that a misuse of the ALSA control API, as
you're not dealing with an actual control here. I think that should
become a hwdep interface then.

I think I've seen the same idea ("abuse") in another driver(?).

Post by Daniel Mack
+static struct snd_kcontrol_new usb_scarlett_ctl_save = {
+ .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+ .name = "",
+ .info = scarlett_ctl_enum_info,
+ .get = scarlett_ctl_save_get,
+ .get = scarlett_ctl_save_put,
+};

BUG! .get is set twice instead of .get and .put...

Right, so the code in .put was never actually used, right? Did the
compiler not warn about that?

No, it does not warn...
I do have some memories of testing that code (i.e. the printk was
called), and of experimenting with .get / .put for that particular
control, esp. concerning alsactl store/restore.

I will do a full compile / test cycle, when David has incorporated all
the comments into a new patch.

Post by Daniel Mack
ARRAY_SIZE(s18i8_text) == 35, so .len = (ARRAY_SIZE()-8) ?
I not sure that's helpful...
.opt_matrix.len = .mix_start + 1,
.opt_master.len = .opt_matrix.len + .matrix_out // == ARRAY_SIZE()
Hmm, yes. I was just thinking about something that makes the values look
a bit less magical.

Maybe a comment at the first such definition helps.

Tobias

David Henningsson

2014-10-13 13:30:29 UTC

Post by Daniel Mack
Hi Tobias,

As I've said in my earlier mail, code is current not working: Metering
should be removed from this patch (or rewritten to use hwdep-API).

Post by David Henningsson
+#define LEVEL_BIAS 128 /* some gui mixers can't handle negative ctl values (alsamixergui, qasmixer, ...) */
+
+#ifndef LEVEL_BIAS
+ #define LEVEL_BIAS 0
+#endif

Same here.

As LEVEL_BIAS = 0 definitely causes problems with some mixer GUIs, the
#ifndef ... #endif should be removed completely.

Alright. David, will you be working on new version of the patch?

Yes, time permitting.

Thanks for the comments - so far they have mostly been about polishing
and cleanup, and that's not too difficult, if that's all that's needed.
But at this point, it would be nice if the maintainer(s) took a
birds-eye view of this driver and looked at the over all structure. I e,
so I don't spend time polishing things, that would anyway need a big
rewrite before they are accepted.

// David

Takashi Iwai

2014-10-09 08:35:56 UTC

At Wed, 08 Oct 2014 21:04:06 +0200,

Post by David Henningsson
+ if (ret < 0) {
+ snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, size = %d\n",
+ bRequest, wValue, idx, size);
+ return ret;
+ }
+#if 0 /* rg debug XXX */
+ snd_printk(KERN_ERR "req ctl value: req = %#x, rtype = %#x, wValue = %#x, wIndex = %#x, size = %d\n",
+ bRequest, (USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN), wValue, idx, size);
+#endif

That can be moved to snd_printdd()

We've been moving from snd_print*() to dev_*() recently, so try to use
dev_dbg() here. Also, for other use of printk (or snd_printk() etc),
try to convert to dev_info(), dev_err(), etc, appropriately.

thanks,

Takashi

Tobias Hoffmann

2014-10-13 10:58:21 UTC

Post by David Henningsson
+ /* initialize sampling rate to 48000 */
+ err = set_ctl_urb2(mixer->chip, UAC2_CS_CUR, 0x0100, 0x29, "\x80\xbb\x00\x00", 4);
+ if (err< 0)
+ return err;
+

There have been two reports of users that had to replace the
set_ctl_urb2 line with:

char buffer[4] = { '\x80', '\xbb', '\x00', '\x00' };
err = set_ctl_urb2(mixer->chip, UAC2_CS_CUR, 0x0100, 0x29, buffer, 4);

otherwise they would get a kernel oops, e.g.:

BUG: unable to handle kernel paging request at ffffffffa0b43440
IP: [<ffffffff8136f3c2>] memcpy+0x12/0x110
...
Call Trace:
[<ffffffffa0b311d2>] ? snd_usb_ctl_msg+0xc2/0x160 [snd_usb_audio]
[<ffffffffa0b3b7aa>] set_ctl_urb2+0xaa/0x100 [snd_usb_audio]
[<ffffffffa0b3c9fa>] scarlett_mixer_controls+0x28a/0x4c0
[snd_usb_audio]
...

I'm not sure, why this happen, and why only for very few users.
Maybe it has to do with a particular compiler, or some kernel config
setting that places the bytes "somewhere else" when they are passed as
string?

Tobias

Clemens Ladisch

2014-10-13 11:38:55 UTC

+ err = set_ctl_urb2(mixer->chip, UAC2_CS_CUR, 0x0100, 0x29, "\x80\xbb\x00\x00", 4);

char buffer[4] = { '\x80', '\xbb', '\x00', '\x00' };
err = set_ctl_urb2(mixer->chip, UAC2_CS_CUR, 0x0100, 0x29, buffer, 4);
BUG: unable to handle kernel paging request at ffffffffa0b43440
IP: [<ffffffff8136f3c2>] memcpy+0x12/0x110
...
[<ffffffffa0b311d2>] ? snd_usb_ctl_msg+0xc2/0x160 [snd_usb_audio]
[<ffffffffa0b3b7aa>] set_ctl_urb2+0xaa/0x100 [snd_usb_audio]
[<ffffffffa0b3c9fa>] scarlett_mixer_controls+0x28a/0x4c0 [snd_usb_audio]

Strange.

Post by Tobias Hoffmann
Maybe it has to do with a particular compiler, or some kernel config
setting that places the bytes "somewhere else" when they are passed as
string?

Buffers on the stack or in modules cannot be used for DMA on some
architectures, but that is why snd_usb_ctl_msg() creates a copy of the
data.

This crash is inside memcpy(), where this memory type cannot make any
difference.

Please show the entire oops.

Regards,
Clemens

Tobias Hoffmann

2014-10-13 12:02:12 UTC

Post by Clemens Ladisch

+ err = set_ctl_urb2(mixer->chip, UAC2_CS_CUR, 0x0100, 0x29, "\x80\xbb\x00\x00", 4);

char buffer[4] = { '\x80', '\xbb', '\x00', '\x00' };
err = set_ctl_urb2(mixer->chip, UAC2_CS_CUR, 0x0100, 0x29, buffer, 4);

[...]
This crash is inside memcpy(), where this memory type cannot make any
difference.
Please show the entire oops.

The first reporter (18i8) only sent me a patch with exactly that change,

Post by Clemens Ladisch
It also oopses my kernel because of some constness issues.

Here are two reports (6i6) with oops trace:
https://github.com/smilingthax/alsa-driver_scarlett/issues/7

https://github.com/smilingthax/alsa-driver_scarlett/issues/2#issuecomment-54364354

Tobias

Clemens Ladisch

2014-10-13 12:09:42 UTC

Post by Tobias Hoffmann
https://github.com/smilingthax/alsa-driver_scarlett/issues/7
https://github.com/smilingthax/alsa-driver_scarlett/issues/2#issuecomment-54364354

Both kernels are tainted.
Anyway, in both cases, the simplest explanation would be that the driver
was not compiled for the correct kernel version/configuration.

Regards,
Clemens

Tobias Hoffmann

2014-10-13 12:21:58 UTC