[PATCH v5 0/2] ALSA: Add driver for big Scarlett 4th Gen interfaces

["Geoffrey D. Bennett" <g@xxxxx>]

Hi Takashi,

Thank you again for your feedback.

On Fri, Jan 10, 2025 at 06:03:02PM +0100, Takashi Iwai wrote:
> On Sat, 04 Jan 2025 18:12:54 +0100,
> Geoffrey D. Bennett wrote:
[...]
> > +union fcp_init {
> > +>--__u32 version;
> > +>--struct fcp_init_v1 v1;
> > +};
> > +
> > +#define FCP_IOCTL_INIT _IOWR('S', 0x64, union fcp_init)
>-
> I don't think this version and union are needed.
> If you need a different way of initialization, you can provide simply
> a new ioctl, instead of keeping the same ioctl and handles something
> conditionally in a complex way.

Noted and fixed.

> Also, in which situation should the init ioctl be performed?
> And in which situation it shouldn't (must not) be performed?
> Currently the driver seems allowing to run it at any time.  e.g. what
> happens if the init ioctl is called while you're playing or recording
> streams?  The same question is applied to the fcp_cmd, too.

The init must be performed at least once before any commands will
work, but can be safely performed again at any time, including during
playback/recording. Based on my testing, it seems that the control
interface is completely separate from the audio/isochronous transfers.
I've updated the comments to explicitly note that the init ioctl can
be called at any time.

fcp_cmd is expected to be called at any time as well.

> > +>--if (!capable(CAP_SYS_RAWIO))
> > +>-->-------return -EPERM;
>-
> So now you limit the hwdep operation essentially only to root user.
> Is it intended, right?  That is, the hwdep should be accessed only at
> probe time for configuring the stuff (preferably some service or udev
> stuff)?

Correct. During development it was convenient to not have that
restriction, but as you point out there are some risks that way, and
the user-space side of the driver is intended to be automatically
started by udev/systemd, which can run it with CAP_SYS_RAWIO.

FYI, the complete end-to-end setup from kernel driver to user-space
driver (fcp-server from the fcp-support project) (automatically loaded
by udev/systemd) to user-space control app (alsamixer, amixer, or
alsa-scarlett-gui) is working well for me and a few other testers.

The ALSA controls created by the user-space driver are nearly
indistinguishable from those created by the existing scarlett2 driver.
All the expected features (mixing, routing, gain/volume, levels,
firmware updates, etc.) are working well. The current public code for
the user-space driver matches v1 of the kernel side that I previously
submitted. Once we get the ioctls finalised then I'll push the
corresponding user-space side.

> Maybe it'd be helpful to give some "big picture" to indicate how
> things should work.  Also, the more proper documentation should be
> provided for the hwdep interface, too.  The kernel doc in comments
> should be fine, too.

Noted and fixed.

> Last but not least, I don't think we need to split to two patches.
> The second patch merely adds the setup hook, and this can be folded
> into the main patch.

I'm not sure I agree with combining the patches. Here's my reasoning:

Patch 1 adds the new FCP driver which supports the new 4th Gen
16i16/18i16/18i20 interfaces. This patch is complete on its own and
these interfaces don't require patch 2.

Patch 2 is optional and adds the ability (but not the default) to use
the new FCP driver with existing hardware (that is already supported
by the scarlett2 driver). This capability is added through a module
parameter in the scarlett2 driver and is logically separate from the
FCP driver.

But I'm okay to combine them if you still think that would be better.

Regards,
Geoffrey.

---
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    |  107 ++++
 sound/usb/Makefile          |    1 +
 sound/usb/fcp.c             | 1044 +++++++++++++++++++++++++++++++++++
 sound/usb/fcp.h             |    7 +
 sound/usb/mixer_quirks.c    |    7 +
 sound/usb/mixer_scarlett2.c |    8 +
 7 files changed, 1180 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

-- 
2.45.0