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

[Frank van de Pol]

On Thu, Jun 18, 2026 at 05:28:50PM +0200, Takashi Iwai wrote:
> 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@xxxxxxxxx>
> > 
> > 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.

:-) Thank you, it is these kind of nitpicking that help improve the code
quality and maintainability, so more than happy to look into these.

> 
> > --- /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".

Done
 
> > +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.

Agreed, we have similar error handler logic for the playback, capture, and
midi-in callbacks; good opportunity for a common helper function.

> 
> > +	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.
> 

Good point; I moved the test_bit(.....) logic into a small helper functions
jockey3_is_disconnected() and jockey3_is_stopping().  Resulting code is much
easier on the eye:

	if (unlikely(jockey3_is_disconnected(chip)))
		return;

> > +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.

Ok, will look into these.  Getting the concurrency handling correct is quite
hard, since many subtile ways to mess it up.  Thank you for the guidance.
 
> > +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.

Hope to flesh out the v3 of this new driver this weekend.

Thanks,
Frank