Re: [RFC PATCH v2 1/4] ALSA: usb: add RME Babyface Pro FS driver (proprietary mode) ― core + PCM

[Takashi Iwai <tiwai@suse.de>]

On Tue, 01 Sep 2026 11:06:32 +0200,
Ismaïl Bahloul wrote:
> 
> Add the core RME Babyface Pro FS driver (proprietary mode, VID
> 0x2a39 PID 0x3fc0): USB vendor-request protocol + cold init,
> interrupt-URB PCM streaming on interface 5, mixer-state persistence
> across re-probes/resume, and the card lifecycle (probe/disconnect/PM/
> module entry).
> 
> The control surface (mixer, front panel, DSP EQ) is stubbed here so
> the module links; the follow-up patches in this series implement each
> part.
> 
> Signed-off-by: Ismaïl Bahloul <i.bahloul01@gmail.com>
> Assisted-by: DeepSeek V4 Flash
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202609010710.w6NVelt4-lkp@intel.com/

You don't have to put Reported-by / Closes tags here.

And, above all, please try to give some "big picture" of the design of
the driver.  It's not clear what the stream_work does and how the
stream numbers are managed and how they influence on what.


> diff --git a/sound/usb/babyfacepro/Makefile b/sound/usb/babyfacepro/Makefile
> new file mode 100644
> index 000000000..40badfd14
> --- /dev/null
> +++ b/sound/usb/babyfacepro/Makefile
> @@ -0,0 +1,2 @@
> +snd-usb-babyface-pro-y := babyfacepro.o babyfacepro-ctl.o
> +obj-$(CONFIG_SND_USB_BABYFACE_PRO) += snd-usb-babyface-pro.o
> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c

SPDX tag is missing in Makefile?


> --- /dev/null
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c
(snip)
> +/* Crosspoint-map output order vs the master-map order — HARDWARE-
> + * VERIFIED 2026-08-24: the block that feeds the Phones is the FIRST
> + * crosspoint block (0x34), while the Phones master is the SECOND
> + * (0x03E2/0x0006).  The crosspoint map lists the Phones first (the
> + * monitor output); the master map lists AN1/2 first.  Control index =
> + * the canonical order (AN1/2=0, PH3/4=1, ...) so the crosspoint and
> + * master controls line up; this table maps to the register block.
> + */

Avoid non-ASCII letters as much as possible.
LLM tends to put the too fancy letters and comment styles, e.g....

> +const u8 bf_xpoint_block[6] = { 1, 0, 2, 3, 4, 5 };
> +
> +/* ── control-surface stubs ──────────────────────────────────────

.... like the above.  Avoid the unneeded separator like this.

> --- /dev/null
> +++ b/sound/usb/babyfacepro/babyfacepro.c
(snip)
> +int bf_vendor_write(struct snd_usb_babyface *chip, u8 req, u16 val, u16 idx)
> +{
> +	return usb_control_msg_send(chip->dev, 0, req,
> +				    USB_DIR_OUT | USB_TYPE_VENDOR |
> +				    USB_RECIP_DEVICE,
> +				    val, idx, NULL, 0, 1000, GFP_KERNEL);
> +}
> +
> +int bf_vendor_read(struct snd_usb_babyface *chip, u8 req, u16 idx, u8 *buf)
> +{
> +	return usb_control_msg_recv(chip->dev, 0, req,
> +				    USB_DIR_IN | USB_TYPE_VENDOR |
> +				    USB_RECIP_DEVICE,
> +				    0, idx, buf, 4, 1000, GFP_KERNEL);
> +}

Avoid magic 1000.  It's a timeout for 1000ms, so define it.

> +/* The 0x16 cold-init clear covers only 0x00-0x3D — the "cross"
> + * registers of a block (L-reg odd / R-reg even of the stereo
> + * sources) survive from the previous session and would sum L+R into
> + * BOTH channels of the output (mono).  Zero them explicitly: 10 odd
> + * L-registers (5,7,…23) + 10 even R-registers (4,6,…22).
> + */
> +int bf_crosspoint_clear_cross(struct snd_usb_babyface *chip,
> +			      unsigned int blk)
> +{
> +	int ret, k;
> +	u16 flag;
> +
> +	for (k = 5; k < 24; k += 2) {

Those L and R registers should be defined properly, instead of
hard-coded magic numbers in the loop condition.

> +		flag = bf_flag_cycle[chip->flag_cnt];
> +		chip->flag_cnt = (chip->flag_cnt + 1) & 3;
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, 0x0000,
> +				      (BF_REG_CROSS_BASE_L +
> +				       BF_REG_CROSS_STRIDE * blk + k) | flag);

This seems to be a quite often seen pattern.  Maybe it should be
better in a helper, e.g. bf_vendor_write_cycle()?

int bf_vendor_write_cycle(struct snd_usb_babyface *chip, u8 req, u16 val, u16 idx)
{
	u16 flag;

	flag = bf_flag_cycle[chip->flag_cnt];
	chip->flag_cnt = (chip->flag_cnt + 1) & 3;
	return bf_vendor_write(chip, req, val, idx);
}

> +/* ── mixer-state persistence across interface re-probes ────────
> + * A userspace client can claim the proprietary interface via usbfs
> + * (USBDEVFS_DISCONNECT_CLAIM — seen with PipeWire grabbing the
> + * device when a stream targets the sink, and with the TuxMix
> + * user-space daemon's libusb).  That detaches us and the card
> + * disappears for the duration; on release the interface re-probes.
> + * The device keeps its registers across the detach, but our cold
> + * init clears them — so save the mixer state at disconnect and
> + * restore it at the next probe.
> + */

Hmm, this could be done by alsactl restore, in general, too?

Though, the state save/restore could be used for the runtime PM, too,
so it can be useful.  But it's something to be considered later.

> +/* ── stream (interrupt URBs, caiaq-style) ──────────────── */
> +
> +static bool babyface_capture_copy(struct snd_usb_babyface *chip,
> +				  struct snd_pcm_substream *subs,
> +				  const u8 *data, unsigned int frames)
> +{
> +	struct snd_pcm_runtime *rt = subs->runtime;
> +	unsigned int buf_frames = rt->buffer_size;
> +	unsigned int words = chip->frame_bytes / 4;
> +	unsigned int chans = rt->channels;
> +	unsigned int pos, f, i;
> +	unsigned long new_period;
> +	bool crossed = false;
> +	u8 *dst;
> +
> +	spin_lock(&chip->lock);
> +	pos = chip->hw_ptr[SNDRV_PCM_STREAM_CAPTURE] % buf_frames;
> +	for (f = 0; f < frames; f++) {
> +		const __le32 *w = (const __le32 *)(data + f * chip->frame_bytes);
> +
> +		dst = rt->dma_area + frames_to_bytes(rt, pos);
> +		for (i = 0; i < chans; i++) {
> +			/* Channel map: app ch0-3 = device words 0-3 (AN1-4);
> +			 * app ch4-9 = words 6-11 (ADAT/SPDIF); app ch10/11 =
> +			 * words 12/13 = a FIXED-GAIN playback tap (observed
> +			 * 2026-08-25: the playback echoes there at ~−27 dB,
> +			 * independent of the output masters — NOT the output
> +			 * bus; the ADAT/SPDIF range is words 6-11 only).  The
> +			 * device words 4/5 are a fixed marker, not audio —
> +			 * skipped.  At 96/192 kHz the frame has fewer words;
> +			 * missing ones read as zero.
> +			 */
> +			static const u8 map[12] = { 0, 1, 2, 3, 6, 7, 8, 9,
> +						   10, 11, 12, 13 };
> +			u8 wi = i < 12 ? map[i] : 0xff;
> +			s32 s = 0;
> +
> +			if (wi < words) {
> +				/* 24-bit sample in bytes 1-3; arithmetic shift
> +				 * sign-extends from bit 23.  S24_LE container.
> +				 */
> +				s = (s32)le32_to_cpu(w[wi]) >> 8;
> +			}
> +			put_unaligned_le32((u32)s, dst + i * 4);

Why do you have to convert to S24_LE at all?
You can use S32_LE with msbits.  That's a far more standard format,
and even easier for user-space.
That is, just copy the data as-is.

> +static bool babyface_playback_copy(struct snd_usb_babyface *chip,
> +				   struct snd_pcm_substream *subs,
> +				   u8 *data, unsigned int frames)
> +{
> +	struct snd_pcm_runtime *rt = subs->runtime;
> +	unsigned int buf_frames = rt->buffer_size;
> +	unsigned int words = chip->frame_bytes / 4;
> +	unsigned int chans = rt->channels;
> +	unsigned int pos, f, i;
> +	unsigned long new_period;
> +	bool crossed = false;
> +	const u8 *src;
> +
> +	spin_lock(&chip->lock);
> +	/* Clamp to what the app has actually written: the in-flight URBs
> +	 * (nurbs × frames_per_urb) can exceed the app ring, and without
> +	 * this the driver advances hw_ptr past appl_ptr — the ALSA core
> +	 * then flags a spurious XRUN on the next app interaction even
> +	 * though the app refills on schedule (seen at period 16-128 /
> +	 * 96-192 kHz with nurbs=16).  The device just repeats the last
> +	 * frames (stale audio) instead of corrupting the stream state.
> +	 * NB: subtract the unbounded counters directly — modulo arithmetic
> +	 * is ambiguous at exact buffer multiples (appl=512, hw=0 → both
> +	 * wrap to 0).
> +	 */
> +	{
> +		snd_pcm_sframes_t data =
> +			(snd_pcm_sframes_t)(rt->control->appl_ptr -
> +					    chip->hw_ptr[SNDRV_PCM_STREAM_PLAYBACK]);
> +		if (data < 0)
> +			data = 0;
> +		if (data > (snd_pcm_sframes_t)buf_frames)
> +			data = (snd_pcm_sframes_t)buf_frames;
> +		if ((unsigned int)data < frames)
> +			frames = (unsigned int)data;
> +	}

Hmm, this sounds weird.  We can tolerate the XRUN check, but this
doesn't look good.

I stop at this point for now.


thanks,

Takashi