Re: [PATCH v2 1/1] ALSA: usb: Add support for Reloop Jockey 3 DJ controllers

[Takashi Iwai <tiwai@suse.de>]

On Wed, 17 Jun 2026 22:39:37 +0200,
Frank van de Pol wrote:
> 
> Introduce a dedicated repository subdirectory and driver for the Reloop
> Jockey 3 Master Edition and Reloop Jockey 3 Remix USB DJ controllers.
> 
> Because these devices utilize a non-standard, proprietary Ploytec USB
> framing protocol instead of standard USB Audio Class (UAC) mechanisms,
> they require specialized handling outside of standard quirks.
> 
> This driver provides:
>  - 24-bit multi-channel (6x4) audio capture and playback.
>  - Support for 44.1, 48, 88.2, and 96 kHz sample rates.
>  - ALSA RawMIDI input and output mapping for the integrated control surface.
> 
> The custom Ploytec encapsulation and streaming format was successfully
> reverse-engineered via USB protocol analysis and is isolated cleanly into
> a local hardware-independent codec abstraction.
> 
> Signed-off-by: Frank van de Pol <fvdpol@gmail.com>
> 
> Updates based on the feedback from code review
> 
> address issues from code review, further optimization

Just a few nitpicking below, in addition to Sashiko reviews.

> --- /dev/null
> +++ b/sound/usb/jockey3/Kconfig
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config SND_USB_JOCKEY3
> +	tristate "Reloop Jockey 3 support"
> +	default m

Drop the "default m".

> +static void jockey3_playback_callback(struct urb *urb)
> +{
....
> +	if (urb->status) {
> +		if (urb->status == -ENOENT || urb->status == -ECONNRESET ||
> +		    urb->status == -ESHUTDOWN)
> +			return;
> +
> +		/* Fatal error: stop resubmitting to prevent interrupt storm */
> +		dev_err(&chip->intf0->dev, "Playback URB fatal error: %d\n", urb->status);
> +		set_bit(JOCKEY3_FLAG_DISCONNECTED, &chip->flags);
> +		return;
> +	}

This code pattern appears repeatedly, so it can be factored out as a
common helper.

> +	if (unlikely(test_bit(JOCKEY3_FLAG_DISCONNECTED, &chip->flags)))
> +		return;

Also this check appears very frequently, and can be a form that is
easier for readers, too.

> +static int jockey3_pcm_open(struct snd_pcm_substream *substream)
> +{
....
> +	runtime->hw.rate_min = 44100;
> +	runtime->hw.rate_max = 96000;
> +	runtime->hw.buffer_bytes_max = 1024 * 1024;
> +	runtime->hw.period_bytes_min = 64;
> +	runtime->hw.period_bytes_max = 512 * 1024;
> +	runtime->hw.periods_min = 2;
> +	runtime->hw.periods_max = 1024;

Maybe worth to mention that those are some sane software limitation
you chose, not about the hardware limit.

> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		runtime->hw.channels_min = 4;
> +		runtime->hw.channels_max = 4;
> +		chip->playback_substream = substream;
> +	} else {
> +		runtime->hw.channels_min = 6;
> +		runtime->hw.channels_max = 6;
> +		chip->capture_substream = substream;
> +	}

The substream assignment should be done better with the spinlock, and
set at last, for avoiding the potential race?  Otherwise...

> +	scoped_guard(mutex, &chip->rate_mutex) {
> +		if (chip->active_streams > 0) {
> +			/* Force the new stream to match the existing hardware rate */
> +			ret = snd_pcm_hw_constraint_single(runtime,
> +							   SNDRV_PCM_HW_PARAM_RATE,
> +							   chip->current_rate);
> +			if (ret < 0)
> +				return ret;

... here the substream is still set and left.  It might be no serious
problem, but still better to be cleaned up.

> +static int jockey3_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct jockey3_chip *chip = snd_pcm_substream_chip(substream);
> +
> +	dev_dbg(&chip->intf0->dev, "PCM trigger stream %d, cmd %d\n", substream->stream, cmd);
> +
> +	if (test_bit(JOCKEY3_FLAG_DISCONNECTED, &chip->flags))
> +		return -ENODEV;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		guard(spinlock_irqsave)(&chip->playback_lock);
> +		if (cmd == SNDRV_PCM_TRIGGER_START)
> +			chip->stream_running = true;
> +		else if (cmd == SNDRV_PCM_TRIGGER_STOP)
> +			chip->stream_running = false;
> +	} else {
> +		guard(spinlock_irqsave)(&chip->capture_lock);
> +		if (cmd == SNDRV_PCM_TRIGGER_START)
> +			chip->capture_running = true;
> +		else if (cmd == SNDRV_PCM_TRIGGER_STOP)
> +			chip->capture_running = false;
> +	}

Better to implement SNDRV_PCM_TRIGGER_SUSPEND, too.  It can be just as
same as the stop operation.  Then the suspend/resume would be
supported, at least.


thanks,

Takashi