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

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 25 Dec 2024 22:23:06 +0100,
Geoffrey D. Bennett wrote:
> 
> Hi Takashi,
> 
> Merry Christmas!
> 
> This patch series introduces a new Focusrite Control Protocol (FCP)
> driver that provides equivalent functionality to the existing
> Scarlett2 driver, but splits the implementation between a minimal
> kernel driver and a user-space component. My initial user-space
> implementation supports the new Scarlett 4th Gen 16i16, 18i16, and
> 18i20 interfaces, but the new FCP kernel driver can be used with any
> device supported by the Scarlett2 driver (as an opt-in).
> 
> With these new interfaces, I started out by trying to extend the
> existing Scarlett2 driver but found that that wasn't practical. The
> control locations (struct scarlett2_config offset field) have
> previously varied between firmware versions (which is why I added the
> "Minimum Firmware Version" requirement for previous 4th Gen and
> Vocaster devices), and the situation was worse with the new 4th Gen
> devices, with every new firmware version moving the controls.
> 
> While the device provides a map indicating control locations,
> implementing the required JSON parsing in the kernel isn't feasible,
> necessitating a user-space implementation.
> 
> The new approach in the FCP driver significantly improves both
> maintainability and paves the way for future enhancements:
> 
> Firstly, the kernel component of the FCP driver is <10% the size of
> the Scarlett2 driver, with the obvious maintainability advantages;
> cloc reports:
> 
> File                            blank     comment        code
> sound/usb/mixer_scarlett2.c      1765         852        7152
> sound/usb/fcp.c                   174          64         645
> 
> Secondly, changes that aren't feasible to implement in the kernel due
> to backward compatibility constraints will be able to be implemented
> in user-space, with end-users easily able to choose an older version
> of the controls if needed. For example:
> 
> 1) The largest interface has >500 ALSA controls just for the matrix
> mixer (one control for each entry in the N×M matrix). This would be
> better done as N controls with M values each, especially since the
> device supports writing a row (all input gains for one mixer output)
> at a time.
> 
> 2) Similarly, the mux table entries have a control per entry, but
> they're all written at once, so it would be better to have a single
> ALSA control for the whole mux table.
> 
> 3) The interfaces have 3 independent mux tables (one per sample rate
> 48kHz, 96kHz, 192kHz), but supporting them isn't feasible in Scarlett2
> due to backward compatibility constraints.
> 
> 4) Users of the Scarlett2 driver frequently request higher-level
> controls like stereo-linking, pan, mute, and solo. These would be very
> challenging to implement in the kernel while maintaining backward
> compatibility to the existing controls, but again, it's easy to have
> configurable controls in user-space.
> 
> I did consider the alternative of a small user-space helper that just
> reads the map (this is the direction I was heading with my previous
> commit 9930c26 ALSA: scarlett2: Add support for device map retrieval)
> and uploads the offsets to the driver, but this would add even more
> complexity to the driver while gaining none of the possible benefits
> of a user-space implementation. Since user-space implementation is
> unavoidable, moving as much functionality there as possible provides
> the cleanest and most maintainable solution.
> 
> Hence, I present for your consideration the following patches:
> 
> 1) The new FCP driver implementation, handling only the essential
> kernel-space components (base protocol and level meter)
> 
> 2) An option enabling Scarlett2-supported devices to use the FCP
> driver, providing an opt-in migration path forward
> 
> The accompanying user-space implementation is available at:
> https://github.com/geoffreybennett/fcp-support

Thanks for the patches.  The argument about kcontrol sizes sounds
convincing to have a more flexible implementation with hwdep.
As far as the backward compatibility is kept, I find it's OK.

But, the implementations need more consideration.
I'll comment on each patch.


thanks,

Takashi