Skip to contentSkip to conversation

ALSA: Add driver for big Scarlett 4th Gen interfaces

PATCH Superseded Geoffrey D. Bennett 2 participants 6 messages Jan 16, 2025

Subsystems (2)
Sound Focusrite control protocol/SCARLETT2 mixer drivers (Scarlett Gen 2+, Clarett, and Vocaster)

v7 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Geoffrey D. Bennett

author maintainer
#0 lore raw

v7 cover ALSA: Add driver for big Scarlett 4th Gen interfaces

Hi Takashi,

Thanks again for your feedback. You are right on all counts and I believe that I have updated all as you suggested.

FYI, here's my thinking for this #define's name:

#define SNDRV_CTL_TLVT_LABELS 0x110 /* channel labels */

I started with the driver-specific and vague-purpose name SNDRV_CTL_TLVT_FCP_METER_METADATA but reconsidered after your suggestion to use an ioctl() to set it. With the ioctl() FCP_IOCTL_SET_METER_LABELS, the TLV type should be specific ("labels" not "metadata"), and I think it doesn't even need to be driver-specific. There are presumably other cases where a control element has many channels and it could be useful to have a TLV type to use to name them.

Regards,
Geoffrey.

---
Changes in v7:
- Add #define SNDRV_CTL_TLVT_LABELS for channel labels
- Add ioctl for setting meter labels and remove TLV write support
- Simplify reinit handling
- Remove more unnecessary casts
- Add #define FCP_SEGMENT_APP_GOLD to remove hardcoded value

---
Changes in v6:
- Move re-initialisation out of fcp_usb() so it's clear there's no infinite recursion
- Update theory of operation and clarify the use of meter TLV metadata
- Rename meter_labels to meter_metadata for clarity
- Remove unnecessary casts in fcp_ioctl_cmd()

---
Changes in v5:
- Remove version/union complexity from init ioctl
- Add documentation to clarify ioctl usage and big picture

---
Changes in v4:
- Use variable-length data arrays in ioctl structs instead of pointers
- Add CAP_SYS_RAWIO requirement to hwdep interface
- Add validation of flash commands to prevent accidental bricking due to erasing/writing the App_Gold segment
- Refactor URB cleanup

---
Changes in v3:
- Update ioctl structs and add ioctl_compat op to work with 32-bit userspace on 64-bit kernels
- Update driver to do all init steps so it can re-init after suspend/resume
- Add version field to init ioctl for future compatibility
- Improve error messages when unexpected response data is received

---
Changes in v2 as per Takashi's feedback:
- Use fixed-size data arrays instead of pointers in ioctl structs
- Define notify struct outside of struct fcp_dev
- Use u8/u16 types without __ prefix
- Use cleanup.h for code simplification
- Add init flag to ensure FCP_IOCTL_INIT is called before FCP_IOCTL_CMD and FCP_IOCTL_SET_METER_MAP
- Do not destroy/recreate the meter control (the number of channels is now fixed when it is created)

Geoffrey D. Bennett (2):
ALSA: FCP: Add Focusrite Control Protocol driver
ALSA: scarlett2: Add device_setup option to use FCP driver

 MAINTAINERS                 |   10 +-
 include/uapi/sound/fcp.h    |  120 ++++
 include/uapi/sound/tlv.h    |    2 +
 sound/usb/Makefile          |    1 +
 sound/usb/fcp.c             | 1121 +++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h             |    7 +
 sound/usb/mixer_quirks.c    |    7 +
 sound/usb/mixer_scarlett2.c |    8 +
 8 files changed, 1272 insertions(+), 4 deletions(-)
 create mode 100644 include/uapi/sound/fcp.h
 create mode 100644 sound/usb/fcp.c
 create mode 100644 sound/usb/fcp.h
Signature · 1 line
2.45.0
author maintainer
#1 lore raw

PATCH v7 1/2 ALSA: FCP: Add Focusrite Control Protocol driver

Add a new kernel driver for the Focusrite Control Protocol (FCP), which is used by Focusrite Scarlett 2nd Gen, 3rd Gen, 4th Gen, Clarett USB, Clarett+, and Vocaster series audio interfaces. This driver provides a user-space control interface via ALSA's hwdep subsystem.

Unlike the existing Scarlett2 driver which implements all ALSA controls in kernel space, this new FCP driver takes a different approach by providing a minimal kernel interface that allows a user-space driver to send FCP commands and receive notifications. The only control implemented in kernel space is the Level Meter, since it requires frequent polling of volatile data.

While this driver supports all interfaces that the Scarlett2 driver works with, it is initially enabled only for 4th Gen 16i16, 18i16, and 18i20 interfaces that are not supported by the Scarlett2 driver.

Signed-off-by Geoffrey D. Bennett <[email protected]>
---
 MAINTAINERS              |   10 +-
 include/uapi/sound/fcp.h |  120 ++++
 include/uapi/sound/tlv.h |    2 +
 sound/usb/Makefile       |    1 +
 sound/usb/fcp.c          | 1121 ++++++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h          |    7 +
 sound/usb/mixer_quirks.c |    7 +
 7 files changed, 1264 insertions(+), 4 deletions(-)
 create mode 100644 include/uapi/sound/fcp.h
 create mode 100644 sound/usb/fcp.c
 create mode 100644 sound/usb/fcp.h
MAINTAINERS +6 -4
index 6bb4ec0c162a..2fd0945cb738 100644
@@ -8868,14 +8868,16 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/input/joystick/fsia6b.c
 
-FOCUSRITE SCARLETT2 MIXER DRIVER (Scarlett Gen 2+ and Clarett)
+FOCUSRITE CONTROL PROTOCOL/SCARLETT2 MIXER DRIVERS (Scarlett Gen 2+, Clarett, and Vocaster)
 M:	Geoffrey D. Bennett <[email protected]>
 L:	[email protected]
 S:	Maintained
-W:	https://github.com/geoffreybennett/scarlett-gen2
-B:	https://github.com/geoffreybennett/scarlett-gen2/issues
-T:	git https://github.com/geoffreybennett/scarlett-gen2.git
+W:	https://github.com/geoffreybennett/linux-fcp
+B:	https://github.com/geoffreybennett/linux-fcp/issues
+T:	git https://github.com/geoffreybennett/linux-fcp.git
+F:	include/uapi/sound/fcp.h
 F:	include/uapi/sound/scarlett2.h
+F:	sound/usb/fcp.c
 F:	sound/usb/mixer_scarlett2.c
 
 FORCEDETH GIGABIT ETHERNET DRIVER
include/uapi/sound/fcp.h new file +120 -0

120 lines not on this page · raw message

include/uapi/sound/tlv.h +2 -0
index b99a2414b53d..b6e260e35529 100644
@@ -18,6 +18,8 @@
 #define SNDRV_CTL_TLVT_CHMAP_VAR	0x102	/* channels freely swappable */
 #define SNDRV_CTL_TLVT_CHMAP_PAIRED	0x103	/* pair-wise swappable */
 
+#define SNDRV_CTL_TLVT_LABELS		0x110	/* channel labels */
+
 /*
  * TLV structure is right behind the struct snd_ctl_tlv:
  *   unsigned int type  	- see SNDRV_CTL_TLVT_*
sound/usb/Makefile +1 -0
index 0532499dbc6d..30bd5348477b 100644
@@ -6,6 +6,7 @@
 snd-usb-audio-y := 	card.o \
 			clock.o \
 			endpoint.o \
+			fcp.o \
 			format.o \
 			helper.o \
 			implicit.o \
sound/usb/fcp.c new file +1121 -0

1,121 lines not on this page · raw message

sound/usb/fcp.h new file +7 -0
new file mode 100644
index 000000000000..d58cc7db75b1
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __USBAUDIO_FCP_H
+#define __USBAUDIO_FCP_H
+
+int snd_fcp_init(struct usb_mixer_interface *mixer);
+
+#endif /* __USBAUDIO_FCP_H */
sound/usb/mixer_quirks.c +7 -0
index a95ebcf4e46e..150fbb5ac7ed 100644
@@ -38,6 +38,7 @@
 #include "mixer_us16x08.h"
 #include "mixer_s1810c.h"
 #include "helper.h"
+#include "fcp.h"
 
 struct std_mono_table {
 	unsigned int unitid, control, cmask;

@@ -4033,6 +4034,12 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_scarlett2_init(mixer);
 		break;
 
+	case USB_ID(0x1235, 0x821b): /* Focusrite Scarlett 16i16 4th Gen */
+	case USB_ID(0x1235, 0x821c): /* Focusrite Scarlett 18i16 4th Gen */
+	case USB_ID(0x1235, 0x821d): /* Focusrite Scarlett 18i20 4th Gen */
+		err = snd_fcp_init(mixer);
+		break;
+
 	case USB_ID(0x041e, 0x323b): /* Creative Sound Blaster E1 */
 		err = snd_soundblaster_e1_switch_create(mixer);
 		break;
Signature · 1 line
2.45.0
#2 lore raw

Re: [PATCH v7 1/2] ALSA: FCP: Add Focusrite Control Protocol driver

include/uapi/sound/tlv.h line 21

Quoted 9 lines · Geoffrey D. Bennett
include/uapi/sound/tlv.h +1 -0
index b99a2414b53d..b6e260e35529 100644
@@ -18,6 +18,8 @@
 #define SNDRV_CTL_TLVT_CHMAP_VAR	0x102	/* channels freely swappable */
 #define SNDRV_CTL_TLVT_CHMAP_PAIRED	0x103	/* pair-wise swappable */
 
+#define SNDRV_CTL_TLVT_LABELS		0x110	/* channel labels */

IMHO, the data definition looks too ambiguous here to be put as a public API. It can be also a more specific name such as *_CHANNEL_LABELS, too.

Quoted 18 lines
include/uapi/sound/tlv.h +18 -0
+static int fcp_meter_ctl_get(struct snd_kcontrol *kctl,
+			     struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_info *elem = kctl->private_data;
+	struct usb_mixer_interface *mixer = elem->head.mixer;
+	struct fcp_data *private = mixer->private_data;
+	int num_meter_slots, resp_size;
+	u32 *resp __free(kfree) = NULL;
+	int i, err = 0;
+
+	struct {
+		__le16 pad;
+		__le16 num_meters;
+		__le32 magic;
+	} __packed req;
+
+	guard(mutex)(&private->mutex);
+

include/uapi/sound/tlv.h

Quoted 10 lines
include/uapi/sound/tlv.h +10 -0
+	err = fcp_reinit(mixer);
+	if (err < 0)
+		return err;
+
+	num_meter_slots = private->num_meter_slots;
+	resp_size = num_meter_slots * sizeof(u32);
+
+	resp = kmalloc(resp_size, GFP_KERNEL);
+	if (!resp)
+		return -ENOMEM;

It depends on how often this kcontrol gets read, but if it's frequently read, it might make sense to allocate this buffer beforehand and keep using it instead of allocating / freeing at each call. The call is mutual, hence a common buffer is fine.

Quoted 7 lines
include/uapi/sound/tlv.h +7 -0
+static int fcp_meter_tlv_callback(struct snd_kcontrol *kctl,
+				  int op_flag, unsigned int size,
+				  unsigned int __user *tlv)
+{
+	struct usb_mixer_elem_info *elem = kctl->private_data;
+	struct usb_mixer_interface *mixer = elem->head.mixer;
+	struct fcp_data *private = mixer->private_data;

include/uapi/sound/tlv.h

Quoted 10 lines
include/uapi/sound/tlv.h +10 -0
+
+	if (op_flag == SNDRV_CTL_TLV_OP_READ) {
+		if (private->meter_labels_tlv_size == 0)
+			return 0;
+
+		if (size > private->meter_labels_tlv_size)
+			size = private->meter_labels_tlv_size;
+
+		if (copy_to_user(tlv, private->meter_labels_tlv, size))
+			return -EFAULT;

The private->meter_label* stuff can be changed dynamically via ioctl, hence this call should have the data protection, too.
I didn't check fully, but private->mutex can be taken in this code path, I suppose.

Quoted 26 lines
include/uapi/sound/tlv.h +26 -0
+/* FCP initialisation */
+static int fcp_ioctl_init(struct usb_mixer_interface *mixer,
+			  struct fcp_init __user *arg)
+{
+	struct fcp_init init;
+	struct usb_device *dev = mixer->chip->dev;
+	struct fcp_data *private = mixer->private_data;
+	void *resp __free(kfree) = NULL;
+	void *step2_resp;
+	int err, buf_size;
+
+	if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0)))
+		return -EINVAL;
+
+	/* Get initialisation parameters */
+	if (copy_from_user(&init, arg, sizeof(init)))
+		return -EFAULT;
+
+	/* Validate the response sizes */
+	if (init.step0_resp_size < 1 ||
+	    init.step0_resp_size > 255 ||
+	    init.step2_resp_size < 1 ||
+	    init.step2_resp_size > 255)
+		return -EINVAL;
+
+	private->step0_resp_size = init.step0_resp_size;

include/uapi/sound/tlv.h

Quoted 10 lines
include/uapi/sound/tlv.h +10 -0
+	private->step2_resp_size = init.step2_resp_size;
+	private->init1_opcode = init.init1_opcode;
+	private->init2_opcode = init.init2_opcode;
+
+	/* Allocate response buffer */
+	buf_size = private->step0_resp_size + private->step2_resp_size;
+
+	resp = kmalloc(buf_size, GFP_KERNEL);
+	if (!resp)
+		return -ENOMEM;

I'd set private->step* and co after allocation of the buffer. When they are set beforehand, even if the buffer allocation failed, those values are still set.

Quoted 4 lines
include/uapi/sound/tlv.h +4 -0
+/* Set the Level Meter labels */
+static int fcp_ioctl_set_meter_labels(struct usb_mixer_interface *mixer,
+				      struct fcp_meter_labels __user *arg)
+{

(snip)

include/uapi/sound/tlv.h

Quoted 3 lines
include/uapi/sound/tlv.h +3 -0
+	/* Calculate padded data size */
+	data_size = ((labels.labels_size + sizeof(unsigned int) - 1) &
+		     ~(sizeof(unsigned int) - 1));

ALIGN() macro can be used nicely here.

thanks,

Takashi

author maintainer
#3 lore raw

Re: [PATCH v7 1/2] ALSA: FCP: Add Focusrite Control Protocol driver

Hi Takashi,

A couple of questions for clarification:

Quoted 2 lines · Takashi Iwai

On Thu, 16 Jan 2025 10:03:35 +0100,
Geoffrey D. Bennett wrote:

[...]

Quoted 5 lines
Quoted 1 line

+#define SNDRV_CTL_TLVT_LABELS 0x110 /* channel labels */

IMHO, the data definition looks too ambiguous here to be put as a public API. It can be also a more specific name such as *_CHANNEL_LABELS, too.

Do you mean e.g. SNDRV_CTL_TLVT_FCP_CHANNEL_LABELS ?

But still define in include/uapi/sound/tlv.h?

And 0x110 is okay to use?

Quoted 1 line

[...]

Other comments noted and will update accordingly in v8.

Thanks,
Geoffrey.

#4 lore raw

Re: [PATCH v7 1/2] ALSA: FCP: Add Focusrite Control Protocol driver

Quoted 15 lines · Geoffrey D. Bennett

Hi Takashi,

A couple of questions for clarification:

Quoted 2 lines · Takashi Iwai

On Thu, 16 Jan 2025 10:03:35 +0100,
Geoffrey D. Bennett wrote:

[...]

Quoted 5 lines
Quoted 1 line

+#define SNDRV_CTL_TLVT_LABELS 0x110 /* channel labels */

IMHO, the data definition looks too ambiguous here to be put as a public API. It can be also a more specific name such as *_CHANNEL_LABELS, too.

Quoted 5 lines

Do you mean e.g. SNDRV_CTL_TLVT_FCP_CHANNEL_LABELS ?

But still define in include/uapi/sound/tlv.h?

And 0x110 is okay to use?

All yes.

thanks,

Takashi

author maintainer
#5 lore raw

PATCH v7 2/2 ALSA: scarlett2: Add device_setup option to use FCP driver

In reply to Geoffrey D. Bennett

Add a new device_setup option (SCARLETT2_USE_FCP_DRIVER = 0x08) that allows users to opt in to using the new FCP driver instead of the existing scarlett2 driver for their device. This provides a way to test the new FCP driver on existing supported hardware while keeping the Scarlett2 driver as the default.

When the SCARLETT2_USE_FCP_DRIVER bit is set in device_setup, the scarlett2 driver initialisation will hand off to the FCP driver instead of proceeding with its own initialisation. The FCP driver then provides access to the device via its hwdep interface.

Signed-off-by Geoffrey D. Bennett <[email protected]>
---
 sound/usb/mixer_scarlett2.c | 8 ++++++++
 1 file changed, 8 insertions(+)
sound/usb/mixer_scarlett2.c +8 -0
index 7f595c1752a5..288d22e6a0b2 100644
@@ -166,6 +166,7 @@
 #include "helper.h"
 
 #include "mixer_scarlett2.h"
+#include "fcp.h"
 
 /* device_setup value to allow turning MSD mode back on */
 #define SCARLETT2_MSD_ENABLE 0x02

@@ -173,6 +174,9 @@
 /* device_setup value to disable this mixer driver */
 #define SCARLETT2_DISABLE 0x04
 
+/* device_setup value to use the FCP driver instead */
+#define SCARLETT2_USE_FCP_DRIVER 0x08
+
 /* some gui mixers can't handle negative ctl values */
 #define SCARLETT2_VOLUME_BIAS 127
 

@@ -9702,6 +9706,10 @@ int snd_scarlett2_init(struct usb_mixer_interface *mixer)
 	if (!mixer->protocol)
 		return 0;
 
+	/* check if the user wants to use the FCP driver instead */
+	if (chip->setup & SCARLETT2_USE_FCP_DRIVER)
+		return snd_fcp_init(mixer);
+
 	/* find entry in scarlett2_devices */
 	entry = get_scarlett2_device_entry(mixer);
 	if (!entry) {
Signature · 1 line
2.45.0