[PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Olivia Mackintosh]

This allows for N different devices to use the pioneer mixer quirk for
setting capture/record type and recording level. The impementation has
not changed much with the exception of an additional mask on
private_value to allow storing of a device index:
	DEVICE MASK	0xff000000
	GROUP_MASK	0x00ff0000
	VALUE_MASK	0x0000ffff

There is perhaps room for removing the duplication in the lookup tables
(name, wValue, wIndex) and deriving these. See the code header comment
to understand how this can be achieved.

Feedback is very much appreciated as I'm not the most proficient C
programmer but am learning as I go.

Signed-off-by: Olivia Mackintosh <livvy@base.nu>
---
 sound/usb/mixer_quirks.c | 195 +++++++++++++++++++++++++++++----------
 1 file changed, 148 insertions(+), 47 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index abad1d61a536..8518691509d9 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -2603,28 +2603,68 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
 }
 
 /*
- * Pioneer DJ DJM-250MK2 and maybe other DJM models
+ * Pioneer DJ DJM Mixers
  *
- * For playback, no duplicate mapping should be set.
- * There are three mixer stereo channels (CH1, CH2, AUX)
- * and three stereo sources (Playback 1-2, Playback 3-4, Playback 5-6).
- * Each channel should be mapped just once to one source.
- * If mapped multiple times, only one source will play on given channel
- * (sources are not mixed together).
+ * These devices generally have options for soft-switching the playback and
+ * capture sources in addition to the recording level. Although different
+ * devices have different configurations, there seems to be canonical values
+ * for specific capture/playback types:
  *
- * For recording, duplicate mapping is OK. We will get the same signal multiple times.
+ * 	Capture 			| Playback
+ * 	================================|===========
+ * 	Control Tone Line	0x00	| Ch1	0x00
+ * 	Control Tone CD/Line	0x01	| Ch2	0x01
+ * 	Control Tone Phono	0x03	| Aux	0x04
+ * 	Post Fader		0x06	|
+ * 	Cross Fader A		0x07	|
+ * 	Cross Fader B		0x08	|
+ * 	Mic			0x09	|
+ * 	Aux			0x0d	|
+ * 	Rec_Out			0x0a	|
+ * 	None			0x0f	|
+ * 	Ch1 Post Fader		0x11	|
+ * 	Ch2 Post Fader		0x12	|
  *
- * Channels 7-8 are in both directions fixed to FX SEND / FX RETURN.
+ * The wValue for configuring playback/capture type also contains the channel
+ * the input type should apply to:
+ * 	CHANNEL NUMBER	0xff00
+ * 	INPUT TYPE	0x00ff
+ * e.g. Channel 2 set to 'Control Tone Phono' will have wValue 0x0203
+ *
+ * The wIndex values are below:
+ * 	CAPTURE		0x8002
+ * 	CAPTURE LEVEL	0x8003
+ * 	PLAYBACK	0x8016
  *
- * See also notes in the quirks-table.h file.
  */
 
+#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
+	.name = _name, \
+	.options = snd_pioneer_djm_options_##suffix, \
+	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
+	.default_value = _default_value }
+
+struct snd_pioneer_djm_device {
+	char *name;
+	const struct snd_pioneer_djm_option_group *controls;
+	const size_t ncontrols;
+};
+
 struct snd_pioneer_djm_option {
 	const u16 wIndex;
 	const u16 wValue;
 	const char *name;
 };
 
+struct snd_pioneer_djm_option_group {
+	const char *name;
+	const struct snd_pioneer_djm_option *options;
+	const size_t count;
+	const u16 default_value;
+};
+
+/* Common Options
+ */
 static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level[] = {
 	{ .name =  "-5 dB",                  .wValue = 0x0300, .wIndex = 0x8003 },
 	{ .name = "-10 dB",                  .wValue = 0x0200, .wIndex = 0x8003 },
@@ -2632,6 +2672,8 @@ static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level
 	{ .name = "-19 dB",                  .wValue = 0x0000, .wIndex = 0x8003 }
 };
 
+/* DJM250MK2 Options
+ */
 static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch12[] = {
 	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
 	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
@@ -2682,20 +2724,8 @@ static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_56[]
 	{ .name =  "AUX",                    .wValue = 0x0304, .wIndex = 0x8016 }
 };
 
-struct snd_pioneer_djm_option_group {
-	const char *name;
-	const struct snd_pioneer_djm_option *options;
-	const size_t count;
-	const u16 default_value;
-};
-
-#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
-	.name = _name, \
-	.options = snd_pioneer_djm_options_##suffix, \
-	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
-	.default_value = _default_value }
 
-static const struct snd_pioneer_djm_option_group snd_pioneer_djm_option_groups[] = {
+static const struct snd_pioneer_djm_option_group snd_pioneer_djm250mk2_option_groups[] = {
 	snd_pioneer_djm_option_group_item("Master Capture Level Capture Switch", capture_level, 0),
 	snd_pioneer_djm_option_group_item("Capture 1-2 Capture Switch",          capture_ch12,  2),
 	snd_pioneer_djm_option_group_item("Capture 3-4 Capture Switch",          capture_ch34,  2),
@@ -2705,22 +2735,77 @@ static const struct snd_pioneer_djm_option_group snd_pioneer_djm_option_groups[]
 	snd_pioneer_djm_option_group_item("Playback 5-6 Playback Switch",        playback_56,   2)
 };
 
+
+/* DJM750 */
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch1[] = {
+	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
+	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
+	{ .name =  "Post CH1 Fader",         .wValue = 0x0106, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0107, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0108, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch2[] = {
+	{ .name =  "CH2 Control Tone LINE",  .wValue = 0x0200, .wIndex = 0x8002 },
+	{ .name =  "CH2 Control Tone CDLINE",.wValue = 0x0201, .wIndex = 0x8002 },
+	{ .name =  "Post CH2 Fader",         .wValue = 0x0206, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0207, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0208, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch3[] = {
+	{ .name =  "CH3 Control Tone LINE",  .wValue = 0x0300, .wIndex = 0x8002 },
+	{ .name =  "CH3 Control Tone CDLINE",.wValue = 0x0301, .wIndex = 0x8002 },
+	{ .name =  "Post CH3 Fader",         .wValue = 0x0306, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0307, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0308, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch4[] = {
+	{ .name =  "CH4 Control Tone LINE",  .wValue = 0x0400, .wIndex = 0x8002 },
+	{ .name =  "CH4 Control Tone PHONO", .wValue = 0x0403, .wIndex = 0x8002 },
+	{ .name =  "Post CH4 Fader",         .wValue = 0x0406, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0407, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0408, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option_group snd_pioneer_djm750_option_groups[] = {
+	snd_pioneer_djm_option_group_item("Capture Level", capture_level, 0),
+	snd_pioneer_djm_option_group_item("CH1 Input", djm750_capture_ch1, 2),
+	snd_pioneer_djm_option_group_item("CH2 Input", djm750_capture_ch2, 2),
+	snd_pioneer_djm_option_group_item("CH3 Input", djm750_capture_ch3, 0),
+	snd_pioneer_djm_option_group_item("CH4 Input", djm750_capture_ch4, 0),
+};
+
+static const struct snd_pioneer_djm_device snd_pioneer_djm_devices[] = {
+	{ .name = "DJM-250Mk2", .controls = snd_pioneer_djm250mk2_option_groups, .ncontrols = 7},
+	{ .name = "DJM-750", .controls = snd_pioneer_djm750_option_groups, .ncontrols = 5}
+};
+
 // layout of the kcontrol->private_value:
 #define SND_PIONEER_DJM_VALUE_MASK 0x0000ffff
-#define SND_PIONEER_DJM_GROUP_MASK 0xffff0000
+#define SND_PIONEER_DJM_GROUP_MASK 0x00ff0000
+#define SND_PIONEER_DJM_DEVICE_MASK 0xff000000
 #define SND_PIONEER_DJM_GROUP_SHIFT 16
+#define SND_PIONEER_DJM_DEVICE_SHIFT 24
+
 
 static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info)
 {
-	u16 group_index = kctl->private_value >> SND_PIONEER_DJM_GROUP_SHIFT;
+	unsigned long private_value = kctl->private_value;
+	u8 device_idx = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
+	u8 group_idx = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+
+	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+
 	size_t count;
 	const char *name;
 	const struct snd_pioneer_djm_option_group *group;
 
-	if (group_index >= ARRAY_SIZE(snd_pioneer_djm_option_groups))
+	if (group_idx >= device.ncontrols)
 		return -EINVAL;
 
-	group = &snd_pioneer_djm_option_groups[group_index];
+	group = &device.controls[group_idx];
 	count = group->count;
 	if (info->value.enumerated.item >= count)
 		info->value.enumerated.item = count - 1;
@@ -2732,12 +2817,13 @@ static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_c
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u16 group, u16 value)
+static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer,
+		u8 device_idx, u8 group, u16 value)
 {
 	int err;
+	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
 
-	if (group >= ARRAY_SIZE(snd_pioneer_djm_option_groups)
-			|| value >= snd_pioneer_djm_option_groups[group].count)
+	if ((group >= device.ncontrols) || value >= device.controls[group].count)
 		return -EINVAL;
 
 	err = snd_usb_lock_shutdown(mixer->chip);
@@ -2747,9 +2833,8 @@ static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u1
 	err = snd_usb_ctl_msg(
 		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
 		USB_REQ_SET_FEATURE,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		snd_pioneer_djm_option_groups[group].options[value].wValue,
-		snd_pioneer_djm_option_groups[group].options[value].wIndex,
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, device.controls[group].options[value].wValue,
+		device.controls[group].options[value].wIndex,
 		NULL, 0);
 
 	snd_usb_unlock_shutdown(mixer->chip);
@@ -2767,27 +2852,34 @@ static int snd_pioneer_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ct
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
 	struct usb_mixer_interface *mixer = list->mixer;
 	unsigned long private_value = kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+
+	u8 device = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
 	u16 value = elem->value.enumerated.item[0];
 
-	kctl->private_value = (group << SND_PIONEER_DJM_GROUP_SHIFT) | value;
+	kctl->private_value = ((device << SND_PIONEER_DJM_DEVICE_SHIFT) |
+			      (group << SND_PIONEER_DJM_GROUP_SHIFT) |
+			      value);
 
-	return snd_pioneer_djm_controls_update(mixer, group, value);
+	return snd_pioneer_djm_controls_update(mixer, device, group, value);
 }
 
 static int snd_pioneer_djm_controls_resume(struct usb_mixer_elem_list *list)
 {
 	unsigned long private_value = list->kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+	u8 device = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
 	u16 value = (private_value & SND_PIONEER_DJM_VALUE_MASK);
-
-	return snd_pioneer_djm_controls_update(list->mixer, group, value);
+	return snd_pioneer_djm_controls_update(list->mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer)
+static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer,
+		const u8 device_idx)
 {
 	int err, i;
-	const struct snd_pioneer_djm_option_group *group;
+
+	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+
 	struct snd_kcontrol_new knew = {
 		.iface  = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
@@ -2797,11 +2889,17 @@ static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer)
 		.put  = snd_pioneer_djm_controls_put
 	};
 
-	for (i = 0; i < ARRAY_SIZE(snd_pioneer_djm_option_groups); i++) {
-		group = &snd_pioneer_djm_option_groups[i];
-		knew.name = group->name;
-		knew.private_value = (i << SND_PIONEER_DJM_GROUP_SHIFT) | group->default_value;
-		err = snd_pioneer_djm_controls_update(mixer, i, group->default_value);
+	u16 value;
+	for (i = 0; i < device.ncontrols; i++) {
+		value = device.controls[i].default_value;
+
+		knew.name = device.controls[i].name;
+		knew.private_value = (
+			(device_idx << SND_PIONEER_DJM_DEVICE_SHIFT) |
+			(i << SND_PIONEER_DJM_GROUP_SHIFT) |
+			value);
+
+		err = snd_pioneer_djm_controls_update(mixer, device_idx, i, value);
 		if (err)
 			return err;
 		err = add_single_ctl_with_resume(mixer, 0, snd_pioneer_djm_controls_resume,
@@ -2917,7 +3015,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_bbfpro_controls_create(mixer);
 		break;
 	case USB_ID(0x2b73, 0x0017): /* Pioneer DJ DJM-250MK2 */
-		err = snd_pioneer_djm_controls_create(mixer);
+		err = snd_pioneer_djm_controls_create(mixer, 0x00);
+		break;
+	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
+		err = snd_pioneer_djm_controls_create(mixer, 0x01);
 		break;
 	}
 
-- 
2.30.0

Re: [PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Fabian Lesniak]

Hi Olivia,

perfect time for this patch since I'm currently working on similar quirks for
the DJM-900NXS2 model. I will stick to your method for now. I do have some
minor comments below.

In general, I'm wondering whether it is a good way to implement more and more
Pioneer devices in such a hard coded way. mixer_quirks.c already has >3k LOC,
and the 900NXS2 support will add at least 100 more if written in the same
scheme. It may be good to either dynamically create controls depending on the
model or move pioneer support to an extra file. I'd like to hear what Takashi
thinks about that.

Cheers
Fabian

> +static const struct snd_pioneer_djm_device snd_pioneer_djm_devices[] = {
> +	{ .name = "DJM-250Mk2", .controls = snd_pioneer_djm250mk2_option_groups, .ncontrols = 7},
> +	{ .name = "DJM-750", .controls = snd_pioneer_djm750_option_groups, .ncontrols = 5}
> +};
These fixed values for ncontrols can easily be overlooked, consider ARRAY_SIZE
instead. Maybe introduce a macro similar to snd_pioneer_djm_option_group_item.

> +	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
This makes a local copy, which can be avoided by using a pointer instead:
const struct snd_pioneer_djm_device *device = &snd_pioneer_djm_devices[device_idx];

> usb_mixer_interface *mixer, u1 err = snd_usb_ctl_msg(
>  		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
>  		USB_REQ_SET_FEATURE,
> -		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> -		snd_pioneer_djm_option_groups[group].options[value].wValue,
> -		snd_pioneer_djm_option_groups[group].options[value].wIndex,
> +		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> device.controls[group].options[value].wValue,
> +		device.controls[group].options[value].wIndex,
>  		NULL, 0);
Rather keep these arguments aligned.

> -		err = snd_pioneer_djm_controls_create(mixer);
> +		err = snd_pioneer_djm_controls_create(mixer, 0x00);
> +		break;
> +	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
> +		err = snd_pioneer_djm_controls_create(mixer, 0x01);
>  		break;
I'd introduce defines for the different models instead of raw values.

Re: [PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Takashi Iwai]

On Fri, 29 Jan 2021 15:09:11 +0100,
Fabian Lesniak wrote:
> 
> Hi Olivia,
> 
> perfect time for this patch since I'm currently working on similar quirks for
> the DJM-900NXS2 model. I will stick to your method for now. I do have some
> minor comments below.
> 
> In general, I'm wondering whether it is a good way to implement more and more
> Pioneer devices in such a hard coded way. mixer_quirks.c already has >3k LOC,
> and the 900NXS2 support will add at least 100 more if written in the same
> scheme. It may be good to either dynamically create controls depending on the
> model or move pioneer support to an extra file. I'd like to hear what Takashi
> thinks about that.

If we can reduce the code, it's really appreciated.  But I'm afraid
that we won't reduce much for now, and the current amount is still
manageable.  Let's see how much we need for 900NXS2.


thanks,

Takashi


> 
> Cheers
> Fabian
> 
> > +static const struct snd_pioneer_djm_device snd_pioneer_djm_devices[] = {
> > +	{ .name = "DJM-250Mk2", .controls = snd_pioneer_djm250mk2_option_groups, .ncontrols = 7},
> > +	{ .name = "DJM-750", .controls = snd_pioneer_djm750_option_groups, .ncontrols = 5}
> > +};
> These fixed values for ncontrols can easily be overlooked, consider ARRAY_SIZE
> instead. Maybe introduce a macro similar to snd_pioneer_djm_option_group_item.
> 
> > +	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
> This makes a local copy, which can be avoided by using a pointer instead:
> const struct snd_pioneer_djm_device *device = &snd_pioneer_djm_devices[device_idx];
> 
> > usb_mixer_interface *mixer, u1 err = snd_usb_ctl_msg(
> >  		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
> >  		USB_REQ_SET_FEATURE,
> > -		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > -		snd_pioneer_djm_option_groups[group].options[value].wValue,
> > -		snd_pioneer_djm_option_groups[group].options[value].wIndex,
> > +		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > device.controls[group].options[value].wValue,
> > +		device.controls[group].options[value].wIndex,
> >  		NULL, 0);
> Rather keep these arguments aligned.
> 
> > -		err = snd_pioneer_djm_controls_create(mixer);
> > +		err = snd_pioneer_djm_controls_create(mixer, 0x00);
> > +		break;
> > +	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
> > +		err = snd_pioneer_djm_controls_create(mixer, 0x01);
> >  		break;
> I'd introduce defines for the different models instead of raw values.
> 
> 

Re: [PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Olivia Mackintosh]

Hi Fabian,

On Fri, Jan 29, 2021 at 03:09:11PM +0100, Fabian Lesniak wrote:
> Hi Olivia,
> 
> perfect time for this patch since I'm currently working on similar quirks for
> the DJM-900NXS2 model. I will stick to your method for now. I do have some
> minor comments below.
> 
> In general, I'm wondering whether it is a good way to implement more and more
> Pioneer devices in such a hard coded way. mixer_quirks.c already has >3k LOC,
> and the 900NXS2 support will add at least 100 more if written in the same
> scheme. It may be good to either dynamically create controls depending on the
> model or move pioneer support to an extra file. I'd like to hear what Takashi
> thinks about that.

I also wish to reduce the amount of device-specific configuration and do
have a (different) patch that aims to do this however it is incomplete.
The current lifecycle for creating, updating, getting option info and so
on made this challenging and so it needs futher thought. In particular,
the choices are enumerated by index (see the  ..._controls_info()
function) meaning it makes it hard to have canonical values.

Ideally, we should direct ..._controls_create to a flat structure that
only contains the input types for each channel. The wValues and wIndexes
can be derrived.

> Cheers
> Fabian
> 
> > +static const struct snd_pioneer_djm_device snd_pioneer_djm_devices[] = {
> > +	{ .name = "DJM-250Mk2", .controls = snd_pioneer_djm250mk2_option_groups, .ncontrols = 7},
> > +	{ .name = "DJM-750", .controls = snd_pioneer_djm750_option_groups, .ncontrols = 5}
> > +};
> These fixed values for ncontrols can easily be overlooked, consider ARRAY_SIZE
> instead. Maybe introduce a macro similar to snd_pioneer_djm_option_group_item.

This was a concern, I will change this.

> > +	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
> This makes a local copy, which can be avoided by using a pointer instead:
> const struct snd_pioneer_djm_device *device = &snd_pioneer_djm_devices[device_idx];

Thank you.

> > usb_mixer_interface *mixer, u1 err = snd_usb_ctl_msg(
> >  		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
> >  		USB_REQ_SET_FEATURE,
> > -		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > -		snd_pioneer_djm_option_groups[group].options[value].wValue,
> > -		snd_pioneer_djm_option_groups[group].options[value].wIndex,
> > +		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > device.controls[group].options[value].wValue,
> > +		device.controls[group].options[value].wIndex,
> >  		NULL, 0);
> Rather keep these arguments aligned.

Yes.

> > -		err = snd_pioneer_djm_controls_create(mixer);
> > +		err = snd_pioneer_djm_controls_create(mixer, 0x00);
> > +		break;
> > +	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
> > +		err = snd_pioneer_djm_controls_create(mixer, 0x01);
> >  		break;
> I'd introduce defines for the different models instead of raw values.

I agree, this currently may create the potential for dereferenced
NULL pointer if the index points to a non-existant item in
`...control_devices[]`. 

Thank you for your comments Fabian.

Kindest regards,
Olivia

Re: [PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[František Kučera]

Dne 29. 01. 21 v 15:09 Fabian Lesniak napsal(a):
> In general, I'm wondering whether it is a good way to implement more and more
> Pioneer devices in such a hard coded way. mixer_quirks.c already has >3k LOC,
> and the 900NXS2 support will add at least 100 more if written in the same
> scheme. It may be good to either dynamically create controls depending on the
> model or move pioneer support to an extra file.

The original idea was to reduce complexity rather than lines of code and keep it straightforward without many IF branching and special cases.

Maybe more data/configuration (declared arrays) can be shared among multiple devices – like the capture_level.

Does not the DJM750 support also mapping of MIC, AUX and REC OUT or any playback mapping? If it would, more configuration might be shared. And is it DJM-750MK2 or DJM-750-K? At least the specification of DJM-750-K talks about a sound card with 4 stereo inputs and 4 stereo outputs.

Does anybody know/have other DJM hardware? Does it use the same codes (wValue/wIndex)?

Franta

Re: [PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Fabian Lesniak]

Hi Franta,

I've just submitted mixer quirks for the 900NXS2, following the design of 
Olivia and you. Seems quite clean, but if anyone comes up with a clever idea 
on how to share code between the channel arrays, I'd highly appreciate that. 
My experiments so far turned out quite complex: I thought about adding flags 
like "DEVICE_HAS_DIGITAL", "DEVICE_HAS_AUX" etc. which are evaluated during 
control creation and usage. The code became unreadable and complex, so I 
ditched that idea. Creating the controls arrays dynamically would maybe help.

The 900NXS2 uses the same wValue/wIndex as your 250Mk2, just expanded to five 
channels. It does not allow to set playback channels via USB, that can only be 
done in hardware using the input source knob. I guess that is same for the 
DJM-700. The 900NXS2 allows querying the currently selected playback channel 
though, but I think this is of no great use so I did not implement it in this 
patch (although it was in my original draft from last year: https://
gist.github.com/flesniak/074ab23bbc833663b782f44174eae6a4). If you think it's 
worth it, I could have a look at that again.

Cheers
Fabian

Am Montag, 1. Februar 2021, 16:34:29 CET schrieb František Kučera:
> Dne 29. 01. 21 v 15:09 Fabian Lesniak napsal(a):
> > In general, I'm wondering whether it is a good way to implement more and
> > more Pioneer devices in such a hard coded way. mixer_quirks.c already has
> > >3k LOC, and the 900NXS2 support will add at least 100 more if written in
> > the same scheme. It may be good to either dynamically create controls
> > depending on the model or move pioneer support to an extra file.
> 
> The original idea was to reduce complexity rather than lines of code and
> keep it straightforward without many IF branching and special cases.
> 
> Maybe more data/configuration (declared arrays) can be shared among multiple
> devices – like the capture_level.
> 
> Does not the DJM750 support also mapping of MIC, AUX and REC OUT or any
> playback mapping? If it would, more configuration might be shared. And is
> it DJM-750MK2 or DJM-750-K? At least the specification of DJM-750-K talks
> about a sound card with 4 stereo inputs and 4 stereo outputs.
> 
> Does anybody know/have other DJM hardware? Does it use the same codes
> (wValue/wIndex)?
> 
> Franta

Re: [PATCH] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Olivia Mackintosh]

Hi all,

I did a little exploration of this before too and though I would share my
notes FWIW.

Some things we might be able to do right off the bat:
	1. Move wIndex from each item into the control group:
>		struct snd_pioneer_djm_option_group {
>			const char *name;
>			const struct snd_pioneer_djm_option *options;
>			const size_t count;
>			const u16 default_value;
>			const u16 windex;
>		};
>
	2. Generate labels based on wValue perhaps in the fashion of
		where each case is an enum or define (e.g. PHONO = 0x3):
>		static char *snd_pioneer_djm_get_label(u8 input_type) {
>		      switch (input_type) {
>		      case LINE:      return "Control Tone LINE\0";
>		      case CDLINE:    return "Control Tone CD/LINE\0";
>		      case PHONO:     return "Control Tone PHONO\0";
>		      case PFADER:    return "Post Fader\0";
>		      case XFADERA:   return "Cross Fader A\0";
>		      case XFADERB:   return "Cross Fader B\0";
>		      case MIC:       return "Mic\0";
>		      case RECOUT:    return "Rec Out\0";
>		      case AUX:       return "Aux\0";
>		      case NONE:      return "None\0";
>		      case PFADERCH1: return "Post Fader CH1\0";
>		      case PFADERCH2: return "Post Fader CH2\0";
>		      default:	return "\0"; // 'EINVAL'
>		};
>}	

This should get us 90% to where we need to be. I originally had lots of
huge code snippets that I were going to send but I think this is more
readable!

Other wildcard ideas I explored:
	1. Bitmask of values stored in u32 private_data. This could work
		since it only takes up 12bits but it seems like an abuse of
		private_data's intended function and it feels like the
		unwrapping process would be somewhat length and messy.

		I think this is similar to your idea Fabian.

---

Maybe these notes will help provide some inspiration or something.
If I have the time I'll pick up from here tomorrow. Comments/input/
different ideas very welcome :).

Kindest regards,
Olivia


On Mon, Feb 01, 2021 at 10:37:21PM +0100, Fabian Lesniak wrote:
> Hi Franta,
> 
> I've just submitted mixer quirks for the 900NXS2, following the design of 
> Olivia and you. Seems quite clean, but if anyone comes up with a clever idea 
> on how to share code between the channel arrays, I'd highly appreciate that. 
> My experiments so far turned out quite complex: I thought about adding flags 
> like "DEVICE_HAS_DIGITAL", "DEVICE_HAS_AUX" etc. which are evaluated during 
> control creation and usage. The code became unreadable and complex, so I 
> ditched that idea. Creating the controls arrays dynamically would maybe help.
> 
> The 900NXS2 uses the same wValue/wIndex as your 250Mk2, just expanded to five 
> channels. It does not allow to set playback channels via USB, that can only be 
> done in hardware using the input source knob. I guess that is same for the 
> DJM-700. The 900NXS2 allows querying the currently selected playback channel 
> though, but I think this is of no great use so I did not implement it in this 
> patch (although it was in my original draft from last year: https://
> gist.github.com/flesniak/074ab23bbc833663b782f44174eae6a4). If you think it's 
> worth it, I could have a look at that again.
> 
> Cheers
> Fabian
> 
> Am Montag, 1. Februar 2021, 16:34:29 CET schrieb František Kučera:
> > Dne 29. 01. 21 v 15:09 Fabian Lesniak napsal(a):
> > > In general, I'm wondering whether it is a good way to implement more and
> > > more Pioneer devices in such a hard coded way. mixer_quirks.c already has
> > > >3k LOC, and the 900NXS2 support will add at least 100 more if written in
> > > the same scheme. It may be good to either dynamically create controls
> > > depending on the model or move pioneer support to an extra file.
> > 
> > The original idea was to reduce complexity rather than lines of code and
> > keep it straightforward without many IF branching and special cases.
> > 
> > Maybe more data/configuration (declared arrays) can be shared among multiple
> > devices – like the capture_level.
> > 
> > Does not the DJM750 support also mapping of MIC, AUX and REC OUT or any
> > playback mapping? If it would, more configuration might be shared. And is
> > it DJM-750MK2 or DJM-750-K? At least the specification of DJM-750-K talks
> > about a sound card with 4 stereo inputs and 4 stereo outputs.
> > 
> > Does anybody know/have other DJM hardware? Does it use the same codes
> > (wValue/wIndex)?
> > 
> > Franta
> 
> 

[PATCH v2 0/2] Add DJM-750 and simplify

[Olivia Mackintosh]

This is a re-roll of my last patch with the addition of some
improvements. Hopefully it reduces the noise a little and simplifies the
adding of new devices. One thing I haven't done here is replace the
arrays of values for each channel with a named definition. In order to
do this, the actual wValue would need to include the channel mask.

Olivia Mackintosh (2):
  ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk
  ALSA: usb-audio: Simplify DJM mixer quirks

 sound/usb/mixer_quirks.c | 333 ++++++++++++++++++++++++---------------
 1 file changed, 210 insertions(+), 123 deletions(-)

-- 
2.30.0

[PATCH v2 1/2] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Olivia Mackintosh]

This allows for N different devices to use the pioneer mixer quirk for
setting capture/record type and recording level. The impementation has
not changed much with the exception of an additional mask on
private_value to allow storing of a device index:
	DEVICE MASK	0xff000000
	GROUP_MASK	0x00ff0000
	VALUE_MASK	0x0000ffff

There is perhaps room for removing the duplication in the lookup tables
(name, wValue, wIndex) and deriving these. See the code header comment
to understand how this can be achieved.

Feedback is very much appreciated as I'm not the most proficient C
programmer but am learning as I go.

Signed-off-by: Olivia Mackintosh <livvy@base.nu>
---
 sound/usb/mixer_quirks.c | 195 +++++++++++++++++++++++++++++----------
 1 file changed, 148 insertions(+), 47 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index abad1d61a536..8518691509d9 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -2603,28 +2603,68 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
 }
 
 /*
- * Pioneer DJ DJM-250MK2 and maybe other DJM models
+ * Pioneer DJ DJM Mixers
  *
- * For playback, no duplicate mapping should be set.
- * There are three mixer stereo channels (CH1, CH2, AUX)
- * and three stereo sources (Playback 1-2, Playback 3-4, Playback 5-6).
- * Each channel should be mapped just once to one source.
- * If mapped multiple times, only one source will play on given channel
- * (sources are not mixed together).
+ * These devices generally have options for soft-switching the playback and
+ * capture sources in addition to the recording level. Although different
+ * devices have different configurations, there seems to be canonical values
+ * for specific capture/playback types:
  *
- * For recording, duplicate mapping is OK. We will get the same signal multiple times.
+ * 	Capture 			| Playback
+ * 	================================|===========
+ * 	Control Tone Line	0x00	| Ch1	0x00
+ * 	Control Tone CD/Line	0x01	| Ch2	0x01
+ * 	Control Tone Phono	0x03	| Aux	0x04
+ * 	Post Fader		0x06	|
+ * 	Cross Fader A		0x07	|
+ * 	Cross Fader B		0x08	|
+ * 	Mic			0x09	|
+ * 	Aux			0x0d	|
+ * 	Rec_Out			0x0a	|
+ * 	None			0x0f	|
+ * 	Ch1 Post Fader		0x11	|
+ * 	Ch2 Post Fader		0x12	|
  *
- * Channels 7-8 are in both directions fixed to FX SEND / FX RETURN.
+ * The wValue for configuring playback/capture type also contains the channel
+ * the input type should apply to:
+ * 	CHANNEL NUMBER	0xff00
+ * 	INPUT TYPE	0x00ff
+ * e.g. Channel 2 set to 'Control Tone Phono' will have wValue 0x0203
+ *
+ * The wIndex values are below:
+ * 	CAPTURE		0x8002
+ * 	CAPTURE LEVEL	0x8003
+ * 	PLAYBACK	0x8016
  *
- * See also notes in the quirks-table.h file.
  */
 
+#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
+	.name = _name, \
+	.options = snd_pioneer_djm_options_##suffix, \
+	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
+	.default_value = _default_value }
+
+struct snd_pioneer_djm_device {
+	char *name;
+	const struct snd_pioneer_djm_option_group *controls;
+	const size_t ncontrols;
+};
+
 struct snd_pioneer_djm_option {
 	const u16 wIndex;
 	const u16 wValue;
 	const char *name;
 };
 
+struct snd_pioneer_djm_option_group {
+	const char *name;
+	const struct snd_pioneer_djm_option *options;
+	const size_t count;
+	const u16 default_value;
+};
+
+/* Common Options
+ */
 static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level[] = {
 	{ .name =  "-5 dB",                  .wValue = 0x0300, .wIndex = 0x8003 },
 	{ .name = "-10 dB",                  .wValue = 0x0200, .wIndex = 0x8003 },
@@ -2632,6 +2672,8 @@ static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level
 	{ .name = "-19 dB",                  .wValue = 0x0000, .wIndex = 0x8003 }
 };
 
+/* DJM250MK2 Options
+ */
 static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch12[] = {
 	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
 	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
@@ -2682,20 +2724,8 @@ static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_56[]
 	{ .name =  "AUX",                    .wValue = 0x0304, .wIndex = 0x8016 }
 };
 
-struct snd_pioneer_djm_option_group {
-	const char *name;
-	const struct snd_pioneer_djm_option *options;
-	const size_t count;
-	const u16 default_value;
-};
-
-#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
-	.name = _name, \
-	.options = snd_pioneer_djm_options_##suffix, \
-	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
-	.default_value = _default_value }
 
-static const struct snd_pioneer_djm_option_group snd_pioneer_djm_option_groups[] = {
+static const struct snd_pioneer_djm_option_group snd_pioneer_djm250mk2_option_groups[] = {
 	snd_pioneer_djm_option_group_item("Master Capture Level Capture Switch", capture_level, 0),
 	snd_pioneer_djm_option_group_item("Capture 1-2 Capture Switch",          capture_ch12,  2),
 	snd_pioneer_djm_option_group_item("Capture 3-4 Capture Switch",          capture_ch34,  2),
@@ -2705,22 +2735,77 @@ static const struct snd_pioneer_djm_option_group snd_pioneer_djm_option_groups[]
 	snd_pioneer_djm_option_group_item("Playback 5-6 Playback Switch",        playback_56,   2)
 };
 
+
+/* DJM750 */
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch1[] = {
+	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
+	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
+	{ .name =  "Post CH1 Fader",         .wValue = 0x0106, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0107, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0108, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch2[] = {
+	{ .name =  "CH2 Control Tone LINE",  .wValue = 0x0200, .wIndex = 0x8002 },
+	{ .name =  "CH2 Control Tone CDLINE",.wValue = 0x0201, .wIndex = 0x8002 },
+	{ .name =  "Post CH2 Fader",         .wValue = 0x0206, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0207, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0208, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch3[] = {
+	{ .name =  "CH3 Control Tone LINE",  .wValue = 0x0300, .wIndex = 0x8002 },
+	{ .name =  "CH3 Control Tone CDLINE",.wValue = 0x0301, .wIndex = 0x8002 },
+	{ .name =  "Post CH3 Fader",         .wValue = 0x0306, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0307, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0308, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch4[] = {
+	{ .name =  "CH4 Control Tone LINE",  .wValue = 0x0400, .wIndex = 0x8002 },
+	{ .name =  "CH4 Control Tone PHONO", .wValue = 0x0403, .wIndex = 0x8002 },
+	{ .name =  "Post CH4 Fader",         .wValue = 0x0406, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader A",          .wValue = 0x0407, .wIndex = 0x8002 },
+	{ .name =  "Cross Fader B",          .wValue = 0x0408, .wIndex = 0x8002 },
+};
+
+static const struct snd_pioneer_djm_option_group snd_pioneer_djm750_option_groups[] = {
+	snd_pioneer_djm_option_group_item("Capture Level", capture_level, 0),
+	snd_pioneer_djm_option_group_item("CH1 Input", djm750_capture_ch1, 2),
+	snd_pioneer_djm_option_group_item("CH2 Input", djm750_capture_ch2, 2),
+	snd_pioneer_djm_option_group_item("CH3 Input", djm750_capture_ch3, 0),
+	snd_pioneer_djm_option_group_item("CH4 Input", djm750_capture_ch4, 0),
+};
+
+static const struct snd_pioneer_djm_device snd_pioneer_djm_devices[] = {
+	{ .name = "DJM-250Mk2", .controls = snd_pioneer_djm250mk2_option_groups, .ncontrols = 7},
+	{ .name = "DJM-750", .controls = snd_pioneer_djm750_option_groups, .ncontrols = 5}
+};
+
 // layout of the kcontrol->private_value:
 #define SND_PIONEER_DJM_VALUE_MASK 0x0000ffff
-#define SND_PIONEER_DJM_GROUP_MASK 0xffff0000
+#define SND_PIONEER_DJM_GROUP_MASK 0x00ff0000
+#define SND_PIONEER_DJM_DEVICE_MASK 0xff000000
 #define SND_PIONEER_DJM_GROUP_SHIFT 16
+#define SND_PIONEER_DJM_DEVICE_SHIFT 24
+
 
 static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info)
 {
-	u16 group_index = kctl->private_value >> SND_PIONEER_DJM_GROUP_SHIFT;
+	unsigned long private_value = kctl->private_value;
+	u8 device_idx = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
+	u8 group_idx = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+
+	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+
 	size_t count;
 	const char *name;
 	const struct snd_pioneer_djm_option_group *group;
 
-	if (group_index >= ARRAY_SIZE(snd_pioneer_djm_option_groups))
+	if (group_idx >= device.ncontrols)
 		return -EINVAL;
 
-	group = &snd_pioneer_djm_option_groups[group_index];
+	group = &device.controls[group_idx];
 	count = group->count;
 	if (info->value.enumerated.item >= count)
 		info->value.enumerated.item = count - 1;
@@ -2732,12 +2817,13 @@ static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_c
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u16 group, u16 value)
+static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer,
+		u8 device_idx, u8 group, u16 value)
 {
 	int err;
+	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
 
-	if (group >= ARRAY_SIZE(snd_pioneer_djm_option_groups)
-			|| value >= snd_pioneer_djm_option_groups[group].count)
+	if ((group >= device.ncontrols) || value >= device.controls[group].count)
 		return -EINVAL;
 
 	err = snd_usb_lock_shutdown(mixer->chip);
@@ -2747,9 +2833,8 @@ static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u1
 	err = snd_usb_ctl_msg(
 		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
 		USB_REQ_SET_FEATURE,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		snd_pioneer_djm_option_groups[group].options[value].wValue,
-		snd_pioneer_djm_option_groups[group].options[value].wIndex,
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, device.controls[group].options[value].wValue,
+		device.controls[group].options[value].wIndex,
 		NULL, 0);
 
 	snd_usb_unlock_shutdown(mixer->chip);
@@ -2767,27 +2852,34 @@ static int snd_pioneer_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ct
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
 	struct usb_mixer_interface *mixer = list->mixer;
 	unsigned long private_value = kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+
+	u8 device = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
 	u16 value = elem->value.enumerated.item[0];
 
-	kctl->private_value = (group << SND_PIONEER_DJM_GROUP_SHIFT) | value;
+	kctl->private_value = ((device << SND_PIONEER_DJM_DEVICE_SHIFT) |
+			      (group << SND_PIONEER_DJM_GROUP_SHIFT) |
+			      value);
 
-	return snd_pioneer_djm_controls_update(mixer, group, value);
+	return snd_pioneer_djm_controls_update(mixer, device, group, value);
 }
 
 static int snd_pioneer_djm_controls_resume(struct usb_mixer_elem_list *list)
 {
 	unsigned long private_value = list->kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+	u8 device = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
 	u16 value = (private_value & SND_PIONEER_DJM_VALUE_MASK);
-
-	return snd_pioneer_djm_controls_update(list->mixer, group, value);
+	return snd_pioneer_djm_controls_update(list->mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer)
+static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer,
+		const u8 device_idx)
 {
 	int err, i;
-	const struct snd_pioneer_djm_option_group *group;
+
+	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+
 	struct snd_kcontrol_new knew = {
 		.iface  = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
@@ -2797,11 +2889,17 @@ static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer)
 		.put  = snd_pioneer_djm_controls_put
 	};
 
-	for (i = 0; i < ARRAY_SIZE(snd_pioneer_djm_option_groups); i++) {
-		group = &snd_pioneer_djm_option_groups[i];
-		knew.name = group->name;
-		knew.private_value = (i << SND_PIONEER_DJM_GROUP_SHIFT) | group->default_value;
-		err = snd_pioneer_djm_controls_update(mixer, i, group->default_value);
+	u16 value;
+	for (i = 0; i < device.ncontrols; i++) {
+		value = device.controls[i].default_value;
+
+		knew.name = device.controls[i].name;
+		knew.private_value = (
+			(device_idx << SND_PIONEER_DJM_DEVICE_SHIFT) |
+			(i << SND_PIONEER_DJM_GROUP_SHIFT) |
+			value);
+
+		err = snd_pioneer_djm_controls_update(mixer, device_idx, i, value);
 		if (err)
 			return err;
 		err = add_single_ctl_with_resume(mixer, 0, snd_pioneer_djm_controls_resume,
@@ -2917,7 +3015,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_bbfpro_controls_create(mixer);
 		break;
 	case USB_ID(0x2b73, 0x0017): /* Pioneer DJ DJM-250MK2 */
-		err = snd_pioneer_djm_controls_create(mixer);
+		err = snd_pioneer_djm_controls_create(mixer, 0x00);
+		break;
+	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
+		err = snd_pioneer_djm_controls_create(mixer, 0x01);
 		break;
 	}
 
-- 
2.30.0

[PATCH v2 2/2] ALSA: usb-audio: Simplify DJM mixer quirks

[Olivia Mackintosh]

More DJM devices are due to be addded to Mixer quirks. This removes some
of the device-specific duplication by doing the following:

	1. Don't specify wIndex on each value, just specify it on the
	   control.
	2. Lookup the control labels dynamically based on the wValue.
	3. Shorten the namespace prefix to "snd_djm_"

Signed-off-by: Olivia Mackintosh <livvy@base.nu>
---
 sound/usb/mixer_quirks.c | 370 +++++++++++++++++++--------------------
 1 file changed, 178 insertions(+), 192 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 8518691509d9..0080187e891c 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -2602,228 +2602,214 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
 	return 0;
 }
 
+
 /*
  * Pioneer DJ DJM Mixers
  *
  * These devices generally have options for soft-switching the playback and
  * capture sources in addition to the recording level. Although different
  * devices have different configurations, there seems to be canonical values
- * for specific capture/playback types:
- *
- * 	Capture 			| Playback
- * 	================================|===========
- * 	Control Tone Line	0x00	| Ch1	0x00
- * 	Control Tone CD/Line	0x01	| Ch2	0x01
- * 	Control Tone Phono	0x03	| Aux	0x04
- * 	Post Fader		0x06	|
- * 	Cross Fader A		0x07	|
- * 	Cross Fader B		0x08	|
- * 	Mic			0x09	|
- * 	Aux			0x0d	|
- * 	Rec_Out			0x0a	|
- * 	None			0x0f	|
- * 	Ch1 Post Fader		0x11	|
- * 	Ch2 Post Fader		0x12	|
- *
- * The wValue for configuring playback/capture type also contains the channel
- * the input type should apply to:
- * 	CHANNEL NUMBER	0xff00
- * 	INPUT TYPE	0x00ff
- * e.g. Channel 2 set to 'Control Tone Phono' will have wValue 0x0203
- *
- * The wIndex values are below:
- * 	CAPTURE		0x8002
- * 	CAPTURE LEVEL	0x8003
- * 	PLAYBACK	0x8016
- *
+ * for specific capture/playback types:  See the definitions below.
  */
 
-#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
+// Capture types
+#define SND_DJM_CAP_LINE	0x00
+#define SND_DJM_CAP_CDLINE	0x01
+#define SND_DJM_CAP_PHONO	0x03
+#define SND_DJM_CAP_PFADER	0x06
+#define SND_DJM_CAP_XFADERA	0x07
+#define SND_DJM_CAP_XFADERB	0x08
+#define SND_DJM_CAP_MIC		0x09
+#define SND_DJM_CAP_AUX		0x0d
+#define SND_DJM_CAP_RECOUT	0x0a
+#define SND_DJM_CAP_NONE	0x0f
+#define SND_DJM_CAP_CH1PFADER	0x11
+#define SND_DJM_CAP_CH2PFADER	0x12
+
+// Playback types
+#define SND_DJM_PB_CH1		0x00
+#define SND_DJM_PB_CH2		0x01
+#define SND_DJM_PB_AUX		0x04
+
+#define SND_DJM_WINDEX_CAP	0x8002
+#define SND_DJM_WINDEX_CAPLVL	0x8003
+#define SND_DJM_WINDEX_PB	0x8016
+
+// kcontrol->private_value layout
+#define SND_DJM_VALUE_MASK	0x0000ffff
+#define SND_DJM_GROUP_MASK	0x00ff0000
+#define SND_DJM_DEVICE_MASK	0xff000000
+#define SND_DJM_GROUP_SHIFT	16
+#define SND_DJM_DEVICE_SHIFT	24
+
+// device table index
+#define SND_DJM_250MK2_IDX	0x0
+#define SND_DJM_750_IDX		0x1
+
+
+#define SND_DJM_CTL(_name, suffix, _default_value, _windex) { \
 	.name = _name, \
-	.options = snd_pioneer_djm_options_##suffix, \
-	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
-	.default_value = _default_value }
+	.options = snd_djm_opts_##suffix, \
+	.noptions = ARRAY_SIZE(snd_djm_opts_##suffix), \
+	.default_value = _default_value, \
+	.wIndex = _windex }
 
-struct snd_pioneer_djm_device {
+#define SND_DJM_DEVICE(suffix) { \
+	.controls = snd_djm_ctls_##suffix, \
+	.ncontrols = ARRAY_SIZE(snd_djm_ctls_##suffix) }
+
+
+struct snd_djm_device {
 	char *name;
-	const struct snd_pioneer_djm_option_group *controls;
+	const struct snd_djm_ctl *controls;
 	const size_t ncontrols;
 };
 
-struct snd_pioneer_djm_option {
-	const u16 wIndex;
-	const u16 wValue;
+struct snd_djm_ctl {
 	const char *name;
-};
-
-struct snd_pioneer_djm_option_group {
-	const char *name;
-	const struct snd_pioneer_djm_option *options;
-	const size_t count;
+	const u16 *options;
+	const size_t noptions;
 	const u16 default_value;
+	const u16 wIndex;
 };
 
-/* Common Options
- */
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level[] = {
-	{ .name =  "-5 dB",                  .wValue = 0x0300, .wIndex = 0x8003 },
-	{ .name = "-10 dB",                  .wValue = 0x0200, .wIndex = 0x8003 },
-	{ .name = "-15 dB",                  .wValue = 0x0100, .wIndex = 0x8003 },
-	{ .name = "-19 dB",                  .wValue = 0x0000, .wIndex = 0x8003 }
+static char *snd_djm_get_label_caplevel(u16 wvalue) {
+	switch(wvalue) {
+	case 0x0000:	return "-19dB";
+	case 0x0100:	return "-15dB";
+	case 0x0200:	return "-10dB";
+	case 0x0300:	return "-5dB";
+	default:	return "\0"; // 'EINVAL'
+	}
 };
 
-/* DJM250MK2 Options
- */
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch12[] = {
-	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
-	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0106, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0107, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0108, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0109, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x010d, .wIndex = 0x8002 },
-	{ .name =  "REC OUT",                .wValue = 0x010a, .wIndex = 0x8002 }
+static char *snd_djm_get_label_cap(u16 wvalue) {
+	switch (wvalue & 0x00ff) {
+	case SND_DJM_CAP_LINE:		return "Control Tone LINE\0";
+	case SND_DJM_CAP_CDLINE:	return "Control Tone CD/LINE\0";
+	case SND_DJM_CAP_PHONO:		return "Control Tone PHONO\0";
+	case SND_DJM_CAP_PFADER:	return "Post Fader\0";
+	case SND_DJM_CAP_XFADERA:	return "Cross Fader A\0";
+	case SND_DJM_CAP_XFADERB:	return "Cross Fader B\0";
+	case SND_DJM_CAP_MIC:		return "Mic\0";
+	case SND_DJM_CAP_RECOUT:	return "Rec Out\0";
+	case SND_DJM_CAP_AUX:		return "Aux\0";
+	case SND_DJM_CAP_NONE:		return "None\0";
+	case SND_DJM_CAP_CH1PFADER:	return "Post Fader Ch1\0";
+	case SND_DJM_CAP_CH2PFADER:	return "Post Fader Ch2\0";
+	default:			return "\0"; // 'EINVAL'
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch34[] = {
-	{ .name =  "CH2 Control Tone PHONO", .wValue = 0x0203, .wIndex = 0x8002 },
-	{ .name =  "CH2 Control Tone LINE",  .wValue = 0x0200, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0206, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0207, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0208, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0209, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x020d, .wIndex = 0x8002 },
-	{ .name =  "REC OUT",                .wValue = 0x020a, .wIndex = 0x8002 }
+static char *snd_djm_get_label_pb(u16 wvalue) {
+	switch (wvalue & 0x00ff) {
+	case SND_DJM_PB_CH1:	return "Ch1\0";
+	case SND_DJM_PB_CH2:	return "Ch2\0";
+	case SND_DJM_PB_AUX:	return "Aux\0";
+	default:		return "\0"; // 'EINVAL'
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch56[] = {
-	{ .name =  "REC OUT",                .wValue = 0x030a, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0311, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0312, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0307, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0308, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0309, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x030d, .wIndex = 0x8002 }
+static char *snd_djm_get_label(u16 wvalue, u16 windex) {
+	switch(windex) {
+	case SND_DJM_WINDEX_CAPLVL:	return snd_djm_get_label_caplevel(wvalue);
+	case SND_DJM_WINDEX_CAP:	return snd_djm_get_label_cap(wvalue);
+	case SND_DJM_WINDEX_PB:		return snd_djm_get_label_pb(wvalue);
+	default:			return "\0"; // 'EINVAL';
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_12[] = {
-	{ .name =  "CH1",                    .wValue = 0x0100, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0101, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0104, .wIndex = 0x8016 }
-};
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_34[] = {
-	{ .name =  "CH1",                    .wValue = 0x0200, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0201, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0204, .wIndex = 0x8016 }
-};
+// DJM-250MK2
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_56[] = {
-	{ .name =  "CH1",                    .wValue = 0x0300, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0301, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0304, .wIndex = 0x8016 }
-};
+static const u16 snd_djm_opts_cap_level[] = {
+	0x0000, 0x0100, 0x0200, 0x0300 };
 
+static const u16 snd_djm_opts_250mk2_cap1[] = {
+	0x0103, 0x0100, 0x0106, 0x0107, 0x0108, 0x0109, 0x010d, 0x010a };
 
-static const struct snd_pioneer_djm_option_group snd_pioneer_djm250mk2_option_groups[] = {
-	snd_pioneer_djm_option_group_item("Master Capture Level Capture Switch", capture_level, 0),
-	snd_pioneer_djm_option_group_item("Capture 1-2 Capture Switch",          capture_ch12,  2),
-	snd_pioneer_djm_option_group_item("Capture 3-4 Capture Switch",          capture_ch34,  2),
-	snd_pioneer_djm_option_group_item("Capture 5-6 Capture Switch",          capture_ch56,  0),
-	snd_pioneer_djm_option_group_item("Playback 1-2 Playback Switch",        playback_12,   0),
-	snd_pioneer_djm_option_group_item("Playback 3-4 Playback Switch",        playback_34,   1),
-	snd_pioneer_djm_option_group_item("Playback 5-6 Playback Switch",        playback_56,   2)
-};
+static const u16 snd_djm_opts_250mk2_cap2[] = {
+	0x0203, 0x0200, 0x0206, 0x0207, 0x0208, 0x0209, 0x020d, 0x020a };
 
+static const u16 snd_djm_opts_250mk2_cap3[] = {
+	0x030a, 0x0311, 0x0312, 0x0307, 0x0308, 0x0309, 0x030d };
 
-/* DJM750 */
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch1[] = {
-	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
-	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0106, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0107, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0108, .wIndex = 0x8002 },
-};
+static const u16 snd_djm_opts_250mk2_pb1[] = { 0x0100, 0x0101, 0x0104 };
+static const u16 snd_djm_opts_250mk2_pb2[] = { 0x0200, 0x0201, 0x0204 };
+static const u16 snd_djm_opts_250mk2_pb3[] = { 0x0300, 0x0301, 0x0304 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch2[] = {
-	{ .name =  "CH2 Control Tone LINE",  .wValue = 0x0200, .wIndex = 0x8002 },
-	{ .name =  "CH2 Control Tone CDLINE",.wValue = 0x0201, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0206, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0207, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0208, .wIndex = 0x8002 },
+static const struct snd_djm_ctl snd_djm_ctls_250mk2[] = {
+	SND_DJM_CTL("Capture Level", cap_level, 0, SND_DJM_WINDEX_CAPLVL),
+	SND_DJM_CTL("Ch1 Input",   250mk2_cap1, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch2 Input",   250mk2_cap2, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch3 Input",   250mk2_cap3, 0, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch1 Output",   250mk2_pb1, 0, SND_DJM_WINDEX_PB),
+	SND_DJM_CTL("Ch2 Output",   250mk2_pb2, 1, SND_DJM_WINDEX_PB),
+	SND_DJM_CTL("Ch3 Output",   250mk2_pb3, 2, SND_DJM_WINDEX_PB)
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch3[] = {
-	{ .name =  "CH3 Control Tone LINE",  .wValue = 0x0300, .wIndex = 0x8002 },
-	{ .name =  "CH3 Control Tone CDLINE",.wValue = 0x0301, .wIndex = 0x8002 },
-	{ .name =  "Post CH3 Fader",         .wValue = 0x0306, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0307, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0308, .wIndex = 0x8002 },
-};
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_djm750_capture_ch4[] = {
-	{ .name =  "CH4 Control Tone LINE",  .wValue = 0x0400, .wIndex = 0x8002 },
-	{ .name =  "CH4 Control Tone PHONO", .wValue = 0x0403, .wIndex = 0x8002 },
-	{ .name =  "Post CH4 Fader",         .wValue = 0x0406, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0407, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0408, .wIndex = 0x8002 },
-};
+// DJM-750
+static const u16 snd_djm_opts_750_cap1[] = { 0x0100, 0x0103, 0x0106, 0x0107, 0x0108 };
+static const u16 snd_djm_opts_750_cap2[] = { 0x0200, 0x0201, 0x0206, 0x0207, 0x0208 };
+static const u16 snd_djm_opts_750_cap3[] = { 0x0300, 0x0301, 0x0306, 0x0307, 0x0308 };
+static const u16 snd_djm_opts_750_cap4[] = { 0x0400, 0x0403, 0x0406, 0x0407, 0x0408 };
 
-static const struct snd_pioneer_djm_option_group snd_pioneer_djm750_option_groups[] = {
-	snd_pioneer_djm_option_group_item("Capture Level", capture_level, 0),
-	snd_pioneer_djm_option_group_item("CH1 Input", djm750_capture_ch1, 2),
-	snd_pioneer_djm_option_group_item("CH2 Input", djm750_capture_ch2, 2),
-	snd_pioneer_djm_option_group_item("CH3 Input", djm750_capture_ch3, 0),
-	snd_pioneer_djm_option_group_item("CH4 Input", djm750_capture_ch4, 0),
+static const struct snd_djm_ctl snd_djm_ctls_750[] = {
+	SND_DJM_CTL("Capture Level", cap_level, 0, SND_DJM_WINDEX_CAPLVL),
+	SND_DJM_CTL("Ch1 Input",   750_cap1, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch2 Input",   750_cap2, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch3 Input",   750_cap3, 0, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch4 Input",   750_cap4, 0, SND_DJM_WINDEX_CAP)
 };
 
-static const struct snd_pioneer_djm_device snd_pioneer_djm_devices[] = {
-	{ .name = "DJM-250Mk2", .controls = snd_pioneer_djm250mk2_option_groups, .ncontrols = 7},
-	{ .name = "DJM-750", .controls = snd_pioneer_djm750_option_groups, .ncontrols = 5}
-};
 
-// layout of the kcontrol->private_value:
-#define SND_PIONEER_DJM_VALUE_MASK 0x0000ffff
-#define SND_PIONEER_DJM_GROUP_MASK 0x00ff0000
-#define SND_PIONEER_DJM_DEVICE_MASK 0xff000000
-#define SND_PIONEER_DJM_GROUP_SHIFT 16
-#define SND_PIONEER_DJM_DEVICE_SHIFT 24
+static const struct snd_djm_device snd_djm_devices[] = {
+	SND_DJM_DEVICE(250mk2),
+	SND_DJM_DEVICE(750)
+};
 
 
-static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info)
+static int snd_djm_controls_info(struct snd_kcontrol *kctl,
+		                 struct snd_ctl_elem_info *info)
 {
 	unsigned long private_value = kctl->private_value;
-	u8 device_idx = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
-	u8 group_idx = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+	u8 device_idx = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 ctl_idx = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
+	const struct snd_djm_device device = snd_djm_devices[device_idx];
+	const char *name;
+	const struct snd_djm_ctl *ctl;
+	size_t noptions;
 
-	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+	if (ctl_idx >= device.ncontrols)
+		return -EINVAL;
 
-	size_t count;
-	const char *name;
-	const struct snd_pioneer_djm_option_group *group;
+	ctl = &device.controls[ctl_idx];
+	noptions = ctl->noptions;
+	if (info->value.enumerated.item >= noptions)
+		info->value.enumerated.item = noptions - 1;
 
-	if (group_idx >= device.ncontrols)
+	name = snd_djm_get_label(
+		ctl->options[info->value.enumerated.item],
+		ctl->wIndex
+	);
+	if (strlen(name) == 0)
 		return -EINVAL;
 
-	group = &device.controls[group_idx];
-	count = group->count;
-	if (info->value.enumerated.item >= count)
-		info->value.enumerated.item = count - 1;
-	name = group->options[info->value.enumerated.item].name;
 	strscpy(info->value.enumerated.name, name, sizeof(info->value.enumerated.name));
 	info->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	info->count = 1;
-	info->value.enumerated.items = count;
+	info->value.enumerated.items = noptions;
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer,
-		u8 device_idx, u8 group, u16 value)
+static int snd_djm_controls_update(struct usb_mixer_interface *mixer,
+		                   u8 device_idx, u8 group, u16 value)
 {
 	int err;
-	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+	const  struct snd_djm_device *device = &snd_djm_devices[device_idx];
 
-	if ((group >= device.ncontrols) || value >= device.controls[group].count)
+	if ((group >= device->ncontrols) || value >= device->controls[group].noptions)
 		return -EINVAL;
 
 	err = snd_usb_lock_shutdown(mixer->chip);
@@ -2833,76 +2819,76 @@ static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer,
 	err = snd_usb_ctl_msg(
 		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
 		USB_REQ_SET_FEATURE,
-		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, device.controls[group].options[value].wValue,
-		device.controls[group].options[value].wIndex,
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		device->controls[group].options[value],
+		device->controls[group].wIndex,
 		NULL, 0);
 
 	snd_usb_unlock_shutdown(mixer->chip);
 	return err;
 }
 
-static int snd_pioneer_djm_controls_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
+static int snd_djm_controls_get(struct snd_kcontrol *kctl,
+		                struct snd_ctl_elem_value *elem)
 {
-	elem->value.enumerated.item[0] = kctl->private_value & SND_PIONEER_DJM_VALUE_MASK;
+	elem->value.enumerated.item[0] = kctl->private_value & SND_DJM_VALUE_MASK;
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
+static int snd_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
 {
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
 	struct usb_mixer_interface *mixer = list->mixer;
 	unsigned long private_value = kctl->private_value;
 
-	u8 device = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
-	u8 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+	u8 device = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
 	u16 value = elem->value.enumerated.item[0];
 
-	kctl->private_value = ((device << SND_PIONEER_DJM_DEVICE_SHIFT) |
-			      (group << SND_PIONEER_DJM_GROUP_SHIFT) |
+	kctl->private_value = ((device << SND_DJM_DEVICE_SHIFT) |
+			      (group << SND_DJM_GROUP_SHIFT) |
 			      value);
 
-	return snd_pioneer_djm_controls_update(mixer, device, group, value);
+	return snd_djm_controls_update(mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_resume(struct usb_mixer_elem_list *list)
+static int snd_djm_controls_resume(struct usb_mixer_elem_list *list)
 {
 	unsigned long private_value = list->kctl->private_value;
-	u8 device = (private_value & SND_PIONEER_DJM_DEVICE_MASK) >> SND_PIONEER_DJM_DEVICE_SHIFT;
-	u8 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
-	u16 value = (private_value & SND_PIONEER_DJM_VALUE_MASK);
-	return snd_pioneer_djm_controls_update(list->mixer, device, group, value);
+	u8 device = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
+	u16 value = (private_value & SND_DJM_VALUE_MASK);
+	return snd_djm_controls_update(list->mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer,
+static int snd_djm_controls_create(struct usb_mixer_interface *mixer,
 		const u8 device_idx)
 {
 	int err, i;
 
-	const struct snd_pioneer_djm_device device = snd_pioneer_djm_devices[device_idx];
+	const struct snd_djm_device device = snd_djm_devices[device_idx];
 
 	struct snd_kcontrol_new knew = {
 		.iface  = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
 		.index = 0,
-		.info = snd_pioneer_djm_controls_info,
-		.get  = snd_pioneer_djm_controls_get,
-		.put  = snd_pioneer_djm_controls_put
+		.info = snd_djm_controls_info,
+		.get  = snd_djm_controls_get,
+		.put  = snd_djm_controls_put
 	};
 
 	u16 value;
 	for (i = 0; i < device.ncontrols; i++) {
 		value = device.controls[i].default_value;
-
 		knew.name = device.controls[i].name;
 		knew.private_value = (
-			(device_idx << SND_PIONEER_DJM_DEVICE_SHIFT) |
-			(i << SND_PIONEER_DJM_GROUP_SHIFT) |
+			(device_idx << SND_DJM_DEVICE_SHIFT) |
+			(i << SND_DJM_GROUP_SHIFT) |
 			value);
-
-		err = snd_pioneer_djm_controls_update(mixer, device_idx, i, value);
+		err = snd_djm_controls_update(mixer, device_idx, i, value);
 		if (err)
 			return err;
-		err = add_single_ctl_with_resume(mixer, 0, snd_pioneer_djm_controls_resume,
+		err = add_single_ctl_with_resume(mixer, 0, snd_djm_controls_resume,
 						 &knew, NULL);
 		if (err)
 			return err;
@@ -3015,10 +3001,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_bbfpro_controls_create(mixer);
 		break;
 	case USB_ID(0x2b73, 0x0017): /* Pioneer DJ DJM-250MK2 */
-		err = snd_pioneer_djm_controls_create(mixer, 0x00);
+		err = snd_djm_controls_create(mixer, SND_DJM_250MK2_IDX);
 		break;
 	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
-		err = snd_pioneer_djm_controls_create(mixer, 0x01);
+		err = snd_djm_controls_create(mixer, SND_DJM_750_IDX);
 		break;
 	}
 
-- 
2.30.0

Re: [PATCH v2 0/2] Add DJM-750 and simplify

[Takashi Iwai]

On Thu, 04 Feb 2021 04:44:30 +0100,
Olivia Mackintosh wrote:
> 
> This is a re-roll of my last patch with the addition of some
> improvements. Hopefully it reduces the noise a little and simplifies the
> adding of new devices. One thing I haven't done here is replace the
> arrays of values for each channel with a named definition. In order to
> do this, the actual wValue would need to include the channel mask.
> 
> Olivia Mackintosh (2):
>   ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk
>   ALSA: usb-audio: Simplify DJM mixer quirks

checkpatch.pl complains a few things in both patches.  While many of
warnings can be ignored, some errors (e.g. the brace should be put in
the next line for the function definition) need to be addressed.

Could you fix and resubmit?


thanks,

Takashi

[PATCH v3 0/1] Add DJM-750 and simplify

[Olivia Mackintosh]

Hi Takashi,

I have corrected the various issues raised by checkpatch and also have
just merged the two patches together because they seem to be two
iterations of the same thing. Hopefully it will make it easier to check
and review. I'll check Patchwork and make sure it looks obvious that
I've done this.

Kindest regards,
Olivia

Olivia Mackintosh (1):
  ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

 sound/usb/mixer_quirks.c | 336 +++++++++++++++++++++++++--------------
 1 file changed, 214 insertions(+), 122 deletions(-)

-- 
2.30.0

[PATCH v3 1/1] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Olivia Mackintosh]

This allows for N different devices to use the pioneer mixer quirk for
setting capture/record type and recording level. The impementation has
not changed much with the exception of an additional mask on
private_value to allow storing of a device index:
	DEVICE MASK	0xff000000
	GROUP_MASK	0x00ff0000
	VALUE_MASK	0x0000ffff

There is perhaps room for removing the duplication in the lookup tables
(name, wValue, wIndex) and deriving these. See the code header comment
to understand how this can be achieved.

Feedback is very much appreciated as I'm not the most proficient C
programmer but am learning as I go.

Signed-off-by: Olivia Mackintosh <livvy@base.nu>
---
 sound/usb/mixer_quirks.c | 336 +++++++++++++++++++++++++--------------
 1 file changed, 214 insertions(+), 122 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index abad1d61a536..77b6bf029beb 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -2602,142 +2602,218 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
 	return 0;
 }
 
+
 /*
- * Pioneer DJ DJM-250MK2 and maybe other DJM models
- *
- * For playback, no duplicate mapping should be set.
- * There are three mixer stereo channels (CH1, CH2, AUX)
- * and three stereo sources (Playback 1-2, Playback 3-4, Playback 5-6).
- * Each channel should be mapped just once to one source.
- * If mapped multiple times, only one source will play on given channel
- * (sources are not mixed together).
- *
- * For recording, duplicate mapping is OK. We will get the same signal multiple times.
+ * Pioneer DJ DJM Mixers
  *
- * Channels 7-8 are in both directions fixed to FX SEND / FX RETURN.
- *
- * See also notes in the quirks-table.h file.
+ * These devices generally have options for soft-switching the playback and
+ * capture sources in addition to the recording level. Although different
+ * devices have different configurations, there seems to be canonical values
+ * for specific capture/playback types:  See the definitions below.
  */
 
-struct snd_pioneer_djm_option {
-	const u16 wIndex;
-	const u16 wValue;
-	const char *name;
-};
+// Capture types
+#define SND_DJM_CAP_LINE	0x00
+#define SND_DJM_CAP_CDLINE	0x01
+#define SND_DJM_CAP_PHONO	0x03
+#define SND_DJM_CAP_PFADER	0x06
+#define SND_DJM_CAP_XFADERA	0x07
+#define SND_DJM_CAP_XFADERB	0x08
+#define SND_DJM_CAP_MIC		0x09
+#define SND_DJM_CAP_AUX		0x0d
+#define SND_DJM_CAP_RECOUT	0x0a
+#define SND_DJM_CAP_NONE	0x0f
+#define SND_DJM_CAP_CH1PFADER	0x11
+#define SND_DJM_CAP_CH2PFADER	0x12
+
+// Playback types
+#define SND_DJM_PB_CH1		0x00
+#define SND_DJM_PB_CH2		0x01
+#define SND_DJM_PB_AUX		0x04
+
+#define SND_DJM_WINDEX_CAP	0x8002
+#define SND_DJM_WINDEX_CAPLVL	0x8003
+#define SND_DJM_WINDEX_PB	0x8016
+
+// kcontrol->private_value layout
+#define SND_DJM_VALUE_MASK	0x0000ffff
+#define SND_DJM_GROUP_MASK	0x00ff0000
+#define SND_DJM_DEVICE_MASK	0xff000000
+#define SND_DJM_GROUP_SHIFT	16
+#define SND_DJM_DEVICE_SHIFT	24
+
+// device table index
+#define SND_DJM_250MK2_IDX	0x0
+#define SND_DJM_750_IDX		0x1
+
+
+#define SND_DJM_CTL(_name, suffix, _default_value, _windex) { \
+	.name = _name, \
+	.options = snd_djm_opts_##suffix, \
+	.noptions = ARRAY_SIZE(snd_djm_opts_##suffix), \
+	.default_value = _default_value, \
+	.wIndex = _windex }
+
+#define SND_DJM_DEVICE(suffix) { \
+	.controls = snd_djm_ctls_##suffix, \
+	.ncontrols = ARRAY_SIZE(snd_djm_ctls_##suffix) }
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level[] = {
-	{ .name =  "-5 dB",                  .wValue = 0x0300, .wIndex = 0x8003 },
-	{ .name = "-10 dB",                  .wValue = 0x0200, .wIndex = 0x8003 },
-	{ .name = "-15 dB",                  .wValue = 0x0100, .wIndex = 0x8003 },
-	{ .name = "-19 dB",                  .wValue = 0x0000, .wIndex = 0x8003 }
+
+struct snd_djm_device {
+	char *name;
+	const struct snd_djm_ctl *controls;
+	const size_t ncontrols;
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch12[] = {
-	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
-	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0106, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0107, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0108, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0109, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x010d, .wIndex = 0x8002 },
-	{ .name =  "REC OUT",                .wValue = 0x010a, .wIndex = 0x8002 }
+struct snd_djm_ctl {
+	const char *name;
+	const u16 *options;
+	const size_t noptions;
+	const u16 default_value;
+	const u16 wIndex;
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch34[] = {
-	{ .name =  "CH2 Control Tone PHONO", .wValue = 0x0203, .wIndex = 0x8002 },
-	{ .name =  "CH2 Control Tone LINE",  .wValue = 0x0200, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0206, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0207, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0208, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0209, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x020d, .wIndex = 0x8002 },
-	{ .name =  "REC OUT",                .wValue = 0x020a, .wIndex = 0x8002 }
+static char *snd_djm_get_label_caplevel(u16 wvalue)
+{
+	switch (wvalue) {
+	case 0x0000:	return "-19dB";
+	case 0x0100:	return "-15dB";
+	case 0x0200:	return "-10dB";
+	case 0x0300:	return "-5dB";
+	default:	return "\0"; // 'EINVAL'
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch56[] = {
-	{ .name =  "REC OUT",                .wValue = 0x030a, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0311, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0312, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0307, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0308, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0309, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x030d, .wIndex = 0x8002 }
+static char *snd_djm_get_label_cap(u16 wvalue)
+{
+	switch (wvalue & 0x00ff) {
+	case SND_DJM_CAP_LINE:		return "Control Tone LINE\0";
+	case SND_DJM_CAP_CDLINE:	return "Control Tone CD/LINE\0";
+	case SND_DJM_CAP_PHONO:		return "Control Tone PHONO\0";
+	case SND_DJM_CAP_PFADER:	return "Post Fader\0";
+	case SND_DJM_CAP_XFADERA:	return "Cross Fader A\0";
+	case SND_DJM_CAP_XFADERB:	return "Cross Fader B\0";
+	case SND_DJM_CAP_MIC:		return "Mic\0";
+	case SND_DJM_CAP_RECOUT:	return "Rec Out\0";
+	case SND_DJM_CAP_AUX:		return "Aux\0";
+	case SND_DJM_CAP_NONE:		return "None\0";
+	case SND_DJM_CAP_CH1PFADER:	return "Post Fader Ch1\0";
+	case SND_DJM_CAP_CH2PFADER:	return "Post Fader Ch2\0";
+	default:			return "\0"; // 'EINVAL'
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_12[] = {
-	{ .name =  "CH1",                    .wValue = 0x0100, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0101, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0104, .wIndex = 0x8016 }
+static char *snd_djm_get_label_pb(u16 wvalue)
+{
+	switch (wvalue & 0x00ff) {
+	case SND_DJM_PB_CH1:	return "Ch1\0";
+	case SND_DJM_PB_CH2:	return "Ch2\0";
+	case SND_DJM_PB_AUX:	return "Aux\0";
+	default:		return "\0"; // 'EINVAL'
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_34[] = {
-	{ .name =  "CH1",                    .wValue = 0x0200, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0201, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0204, .wIndex = 0x8016 }
+static char *snd_djm_get_label(u16 wvalue, u16 windex)
+{
+	switch (windex) {
+	case SND_DJM_WINDEX_CAPLVL:	return snd_djm_get_label_caplevel(wvalue);
+	case SND_DJM_WINDEX_CAP:	return snd_djm_get_label_cap(wvalue);
+	case SND_DJM_WINDEX_PB:		return snd_djm_get_label_pb(wvalue);
+	default:			return "\0"; // 'EINVAL';
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_56[] = {
-	{ .name =  "CH1",                    .wValue = 0x0300, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0301, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0304, .wIndex = 0x8016 }
+
+// DJM-250MK2
+
+static const u16 snd_djm_opts_cap_level[] = {
+	0x0000, 0x0100, 0x0200, 0x0300 };
+
+static const u16 snd_djm_opts_250mk2_cap1[] = {
+	0x0103, 0x0100, 0x0106, 0x0107, 0x0108, 0x0109, 0x010d, 0x010a };
+
+static const u16 snd_djm_opts_250mk2_cap2[] = {
+	0x0203, 0x0200, 0x0206, 0x0207, 0x0208, 0x0209, 0x020d, 0x020a };
+
+static const u16 snd_djm_opts_250mk2_cap3[] = {
+	0x030a, 0x0311, 0x0312, 0x0307, 0x0308, 0x0309, 0x030d };
+
+static const u16 snd_djm_opts_250mk2_pb1[] = { 0x0100, 0x0101, 0x0104 };
+static const u16 snd_djm_opts_250mk2_pb2[] = { 0x0200, 0x0201, 0x0204 };
+static const u16 snd_djm_opts_250mk2_pb3[] = { 0x0300, 0x0301, 0x0304 };
+
+static const struct snd_djm_ctl snd_djm_ctls_250mk2[] = {
+	SND_DJM_CTL("Capture Level", cap_level, 0, SND_DJM_WINDEX_CAPLVL),
+	SND_DJM_CTL("Ch1 Input",   250mk2_cap1, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch2 Input",   250mk2_cap2, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch3 Input",   250mk2_cap3, 0, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch1 Output",   250mk2_pb1, 0, SND_DJM_WINDEX_PB),
+	SND_DJM_CTL("Ch2 Output",   250mk2_pb2, 1, SND_DJM_WINDEX_PB),
+	SND_DJM_CTL("Ch3 Output",   250mk2_pb3, 2, SND_DJM_WINDEX_PB)
 };
 
-struct snd_pioneer_djm_option_group {
-	const char *name;
-	const struct snd_pioneer_djm_option *options;
-	const size_t count;
-	const u16 default_value;
+
+// DJM-750
+static const u16 snd_djm_opts_750_cap1[] = { 0x0100, 0x0103, 0x0106, 0x0107, 0x0108 };
+static const u16 snd_djm_opts_750_cap2[] = { 0x0200, 0x0201, 0x0206, 0x0207, 0x0208 };
+static const u16 snd_djm_opts_750_cap3[] = { 0x0300, 0x0301, 0x0306, 0x0307, 0x0308 };
+static const u16 snd_djm_opts_750_cap4[] = { 0x0400, 0x0403, 0x0406, 0x0407, 0x0408 };
+
+static const struct snd_djm_ctl snd_djm_ctls_750[] = {
+	SND_DJM_CTL("Capture Level", cap_level, 0, SND_DJM_WINDEX_CAPLVL),
+	SND_DJM_CTL("Ch1 Input",   750_cap1, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch2 Input",   750_cap2, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch3 Input",   750_cap3, 0, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch4 Input",   750_cap4, 0, SND_DJM_WINDEX_CAP)
 };
 
-#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
-	.name = _name, \
-	.options = snd_pioneer_djm_options_##suffix, \
-	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
-	.default_value = _default_value }
-
-static const struct snd_pioneer_djm_option_group snd_pioneer_djm_option_groups[] = {
-	snd_pioneer_djm_option_group_item("Master Capture Level Capture Switch", capture_level, 0),
-	snd_pioneer_djm_option_group_item("Capture 1-2 Capture Switch",          capture_ch12,  2),
-	snd_pioneer_djm_option_group_item("Capture 3-4 Capture Switch",          capture_ch34,  2),
-	snd_pioneer_djm_option_group_item("Capture 5-6 Capture Switch",          capture_ch56,  0),
-	snd_pioneer_djm_option_group_item("Playback 1-2 Playback Switch",        playback_12,   0),
-	snd_pioneer_djm_option_group_item("Playback 3-4 Playback Switch",        playback_34,   1),
-	snd_pioneer_djm_option_group_item("Playback 5-6 Playback Switch",        playback_56,   2)
+
+static const struct snd_djm_device snd_djm_devices[] = {
+	SND_DJM_DEVICE(250mk2),
+	SND_DJM_DEVICE(750)
 };
 
-// layout of the kcontrol->private_value:
-#define SND_PIONEER_DJM_VALUE_MASK 0x0000ffff
-#define SND_PIONEER_DJM_GROUP_MASK 0xffff0000
-#define SND_PIONEER_DJM_GROUP_SHIFT 16
 
-static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info)
+static int snd_djm_controls_info(struct snd_kcontrol *kctl,
+				struct snd_ctl_elem_info *info)
 {
-	u16 group_index = kctl->private_value >> SND_PIONEER_DJM_GROUP_SHIFT;
-	size_t count;
+	unsigned long private_value = kctl->private_value;
+	u8 device_idx = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 ctl_idx = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
+	const struct snd_djm_device device = snd_djm_devices[device_idx];
 	const char *name;
-	const struct snd_pioneer_djm_option_group *group;
+	const struct snd_djm_ctl *ctl;
+	size_t noptions;
+
+	if (ctl_idx >= device.ncontrols)
+		return -EINVAL;
+
+	ctl = &device.controls[ctl_idx];
+	noptions = ctl->noptions;
+	if (info->value.enumerated.item >= noptions)
+		info->value.enumerated.item = noptions - 1;
 
-	if (group_index >= ARRAY_SIZE(snd_pioneer_djm_option_groups))
+	name = snd_djm_get_label(
+		ctl->options[info->value.enumerated.item],
+		ctl->wIndex
+	);
+	if (strlen(name) == 0)
 		return -EINVAL;
 
-	group = &snd_pioneer_djm_option_groups[group_index];
-	count = group->count;
-	if (info->value.enumerated.item >= count)
-		info->value.enumerated.item = count - 1;
-	name = group->options[info->value.enumerated.item].name;
 	strscpy(info->value.enumerated.name, name, sizeof(info->value.enumerated.name));
 	info->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	info->count = 1;
-	info->value.enumerated.items = count;
+	info->value.enumerated.items = noptions;
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u16 group, u16 value)
+static int snd_djm_controls_update(struct usb_mixer_interface *mixer,
+				u8 device_idx, u8 group, u16 value)
 {
 	int err;
+	const  struct snd_djm_device *device = &snd_djm_devices[device_idx];
 
-	if (group >= ARRAY_SIZE(snd_pioneer_djm_option_groups)
-			|| value >= snd_pioneer_djm_option_groups[group].count)
+	if ((group >= device->ncontrols) || value >= device->controls[group].noptions)
 		return -EINVAL;
 
 	err = snd_usb_lock_shutdown(mixer->chip);
@@ -2748,63 +2824,76 @@ static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u1
 		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
 		USB_REQ_SET_FEATURE,
 		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		snd_pioneer_djm_option_groups[group].options[value].wValue,
-		snd_pioneer_djm_option_groups[group].options[value].wIndex,
+		device->controls[group].options[value],
+		device->controls[group].wIndex,
 		NULL, 0);
 
 	snd_usb_unlock_shutdown(mixer->chip);
 	return err;
 }
 
-static int snd_pioneer_djm_controls_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
+static int snd_djm_controls_get(struct snd_kcontrol *kctl,
+				struct snd_ctl_elem_value *elem)
 {
-	elem->value.enumerated.item[0] = kctl->private_value & SND_PIONEER_DJM_VALUE_MASK;
+	elem->value.enumerated.item[0] = kctl->private_value & SND_DJM_VALUE_MASK;
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
+static int snd_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
 {
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
 	struct usb_mixer_interface *mixer = list->mixer;
 	unsigned long private_value = kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+
+	u8 device = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
 	u16 value = elem->value.enumerated.item[0];
 
-	kctl->private_value = (group << SND_PIONEER_DJM_GROUP_SHIFT) | value;
+	kctl->private_value = ((device << SND_DJM_DEVICE_SHIFT) |
+			      (group << SND_DJM_GROUP_SHIFT) |
+			      value);
 
-	return snd_pioneer_djm_controls_update(mixer, group, value);
+	return snd_djm_controls_update(mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_resume(struct usb_mixer_elem_list *list)
+static int snd_djm_controls_resume(struct usb_mixer_elem_list *list)
 {
 	unsigned long private_value = list->kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
-	u16 value = (private_value & SND_PIONEER_DJM_VALUE_MASK);
+	u8 device = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
+	u16 value = (private_value & SND_DJM_VALUE_MASK);
 
-	return snd_pioneer_djm_controls_update(list->mixer, group, value);
+	return snd_djm_controls_update(list->mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer)
+static int snd_djm_controls_create(struct usb_mixer_interface *mixer,
+		const u8 device_idx)
 {
 	int err, i;
-	const struct snd_pioneer_djm_option_group *group;
+	u16 value;
+
+	const struct snd_djm_device device = snd_djm_devices[device_idx];
+
 	struct snd_kcontrol_new knew = {
 		.iface  = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
 		.index = 0,
-		.info = snd_pioneer_djm_controls_info,
-		.get  = snd_pioneer_djm_controls_get,
-		.put  = snd_pioneer_djm_controls_put
+		.info = snd_djm_controls_info,
+		.get  = snd_djm_controls_get,
+		.put  = snd_djm_controls_put
 	};
 
-	for (i = 0; i < ARRAY_SIZE(snd_pioneer_djm_option_groups); i++) {
-		group = &snd_pioneer_djm_option_groups[i];
-		knew.name = group->name;
-		knew.private_value = (i << SND_PIONEER_DJM_GROUP_SHIFT) | group->default_value;
-		err = snd_pioneer_djm_controls_update(mixer, i, group->default_value);
+	for (i = 0; i < device.ncontrols; i++) {
+		value = device.controls[i].default_value;
+		knew.name = device.controls[i].name;
+		knew.private_value = (
+			(device_idx << SND_DJM_DEVICE_SHIFT) |
+			(i << SND_DJM_GROUP_SHIFT) |
+			value);
+		err = snd_djm_controls_update(mixer, device_idx, i, value);
 		if (err)
 			return err;
-		err = add_single_ctl_with_resume(mixer, 0, snd_pioneer_djm_controls_resume,
+		err = add_single_ctl_with_resume(mixer, 0, snd_djm_controls_resume,
 						 &knew, NULL);
 		if (err)
 			return err;
@@ -2917,7 +3006,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_bbfpro_controls_create(mixer);
 		break;
 	case USB_ID(0x2b73, 0x0017): /* Pioneer DJ DJM-250MK2 */
-		err = snd_pioneer_djm_controls_create(mixer);
+		err = snd_djm_controls_create(mixer, SND_DJM_250MK2_IDX);
+		break;
+	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
+		err = snd_djm_controls_create(mixer, SND_DJM_750_IDX);
 		break;
 	}
 
-- 
2.30.0

Re: [PATCH v3 1/1] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Takashi Iwai]

On Thu, 04 Feb 2021 20:39:06 +0100,
Olivia Mackintosh wrote:
> 
> This allows for N different devices to use the pioneer mixer quirk for
> setting capture/record type and recording level. The impementation has
> not changed much with the exception of an additional mask on
> private_value to allow storing of a device index:
> 	DEVICE MASK	0xff000000
> 	GROUP_MASK	0x00ff0000
> 	VALUE_MASK	0x0000ffff
> 
> There is perhaps room for removing the duplication in the lookup tables
> (name, wValue, wIndex) and deriving these. See the code header comment
> to understand how this can be achieved.
> 
> Feedback is very much appreciated as I'm not the most proficient C
> programmer but am learning as I go.
> 
> Signed-off-by: Olivia Mackintosh <livvy@base.nu>

The patch looks almost good, below are just some nitpicking.


> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -2602,142 +2602,218 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
>  	return 0;
>  }
>  
> +

No need to add more empty line here.

> +struct snd_djm_device {
> +	char *name;
> +	const struct snd_djm_ctl *controls;
> +	const size_t ncontrols;

The const to ncontrols is almost useless.

> +struct snd_djm_ctl {
> +	const char *name;
> +	const u16 *options;
> +	const size_t noptions;
> +	const u16 default_value;
> +	const u16 wIndex;

Ditto, const for integers are superfluous.

> +static char *snd_djm_get_label_caplevel(u16 wvalue)

This should have const instead.

> +{
> +	switch (wvalue) {
> +	case 0x0000:	return "-19dB";
> +	case 0x0100:	return "-15dB";
> +	case 0x0200:	return "-10dB";
> +	case 0x0300:	return "-5dB";
> +	default:	return "\0"; // 'EINVAL'

Let return NULL for the error instead.

> +static char *snd_djm_get_label_cap(u16 wvalue)

Use const.

> +{
> +	switch (wvalue & 0x00ff) {
> +	case SND_DJM_CAP_LINE:		return "Control Tone LINE\0";

The trainling '\0' is superfluous.  Ditto in other lines.

> +	default:			return "\0"; // 'EINVAL'

Let's use NULL.

> +static char *snd_djm_get_label_pb(u16 wvalue)

Use const.

> +{
> +	switch (wvalue & 0x00ff) {
> +	case SND_DJM_PB_CH1:	return "Ch1\0";
> +	case SND_DJM_PB_CH2:	return "Ch2\0";
> +	case SND_DJM_PB_AUX:	return "Aux\0";
> +	default:		return "\0"; // 'EINVAL'

Like the above.

> +static char *snd_djm_get_label(u16 wvalue, u16 windex)

Use const.

> +{
> +	switch (windex) {
> +	case SND_DJM_WINDEX_CAPLVL:	return snd_djm_get_label_caplevel(wvalue);
> +	case SND_DJM_WINDEX_CAP:	return snd_djm_get_label_cap(wvalue);
> +	case SND_DJM_WINDEX_PB:		return snd_djm_get_label_pb(wvalue);
> +	default:			return "\0"; // 'EINVAL';

Use NULL.

> -static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info)
> +static int snd_djm_controls_info(struct snd_kcontrol *kctl,
> +				struct snd_ctl_elem_info *info)
>  {
> -	u16 group_index = kctl->private_value >> SND_PIONEER_DJM_GROUP_SHIFT;
> -	size_t count;
> +	unsigned long private_value = kctl->private_value;
> +	u8 device_idx = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
> +	u8 ctl_idx = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
> +	const struct snd_djm_device device = snd_djm_devices[device_idx];

This will end up copying the whole struct on a stack.
Instead,
	const struct snd_djm_device *device = &snd_djm_devices[device_idx];
and access via
	device->ncontrols

> +	name = snd_djm_get_label(
> +		ctl->options[info->value.enumerated.item],
> +		ctl->wIndex
> +	);

It's better not to put a line-break after the open parenthesis.

	name = snd_djm_get_label(ctl->options[info->value.enumerated.item],
				 ctl->wIndex);

> +	if (strlen(name) == 0)
>  		return -EINVAL;

If the functions above return NULL for errors, this can be simplified
as:
	if (!name)
		return -EINVAL;

> -static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u16 group, u16 value)
> +static int snd_djm_controls_update(struct usb_mixer_interface *mixer,
> +				u8 device_idx, u8 group, u16 value)
>  {
>  	int err;
> +	const  struct snd_djm_device *device = &snd_djm_devices[device_idx];

Funnily, here you're doing right :)

> +static int snd_djm_controls_create(struct usb_mixer_interface *mixer,
> +		const u8 device_idx)
>  {
>  	int err, i;
> -	const struct snd_pioneer_djm_option_group *group;
> +	u16 value;
> +
> +	const struct snd_djm_device device = snd_djm_devices[device_idx];

... but here not.


thanks,

Takashi

[PATCH v4 0/1] Add DJM-750 and simplify

[Olivia Mackintosh]

Hi Takashi,

Thank you for your feedback, it's very helpful. Attached is an updated
patch which should hopefully be correct this time =)

Kindest regards,
Olivia

Olivia Mackintosh (1):
  ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

 sound/usb/mixer_quirks.c | 336 +++++++++++++++++++++++++--------------
 1 file changed, 216 insertions(+), 120 deletions(-)

-- 
2.30.0

[PATCH v4 1/1] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Olivia Mackintosh]

This allows for N different devices to use the pioneer mixer quirk for
setting capture/record type and recording level. The impementation has
not changed much with the exception of an additional mask on
private_value to allow storing of a device index:
	DEVICE MASK	0xff000000
	GROUP_MASK	0x00ff0000
	VALUE_MASK	0x0000ffff

This could be improved by changing the arrays of wValues for each
channel to contain named definitions (e.g. SND_DJM_CAP_LINE). It would
improve readability and perhaps would allow using the same array for
multiple channels. The channel number can be specified on the control
next to the wIndex.

Feedback is very much appreciated as I'm not the most proficient C
programmer but am learning as I go.

Signed-off-by: Olivia Mackintosh <livvy@base.nu>
---
 sound/usb/mixer_quirks.c | 336 +++++++++++++++++++++++++--------------
 1 file changed, 216 insertions(+), 120 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index abad1d61a536..9d0ac2aa9044 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -2603,141 +2603,221 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
 }
 
 /*
- * Pioneer DJ DJM-250MK2 and maybe other DJM models
+ * Pioneer DJ DJM Mixers
  *
- * For playback, no duplicate mapping should be set.
- * There are three mixer stereo channels (CH1, CH2, AUX)
- * and three stereo sources (Playback 1-2, Playback 3-4, Playback 5-6).
- * Each channel should be mapped just once to one source.
- * If mapped multiple times, only one source will play on given channel
- * (sources are not mixed together).
+ * These devices generally have options for soft-switching the playback and
+ * capture sources in addition to the recording level. Although different
+ * devices have different configurations, there seems to be canonical values
+ * for specific capture/playback types:  See the definitions of these below.
  *
- * For recording, duplicate mapping is OK. We will get the same signal multiple times.
- *
- * Channels 7-8 are in both directions fixed to FX SEND / FX RETURN.
- *
- * See also notes in the quirks-table.h file.
+ * The wValue is masked with the stereo channel number. e.g. Setting Ch2 to
+ * capture phono would be 0x0203. Capture, playback and capture level have
+ * different wIndexes.
  */
 
-struct snd_pioneer_djm_option {
-	const u16 wIndex;
-	const u16 wValue;
+// Capture types
+#define SND_DJM_CAP_LINE	0x00
+#define SND_DJM_CAP_CDLINE	0x01
+#define SND_DJM_CAP_PHONO	0x03
+#define SND_DJM_CAP_PFADER	0x06
+#define SND_DJM_CAP_XFADERA	0x07
+#define SND_DJM_CAP_XFADERB	0x08
+#define SND_DJM_CAP_MIC		0x09
+#define SND_DJM_CAP_AUX		0x0d
+#define SND_DJM_CAP_RECOUT	0x0a
+#define SND_DJM_CAP_NONE	0x0f
+#define SND_DJM_CAP_CH1PFADER	0x11
+#define SND_DJM_CAP_CH2PFADER	0x12
+
+// Playback types
+#define SND_DJM_PB_CH1		0x00
+#define SND_DJM_PB_CH2		0x01
+#define SND_DJM_PB_AUX		0x04
+
+#define SND_DJM_WINDEX_CAP	0x8002
+#define SND_DJM_WINDEX_CAPLVL	0x8003
+#define SND_DJM_WINDEX_PB	0x8016
+
+// kcontrol->private_value layout
+#define SND_DJM_VALUE_MASK	0x0000ffff
+#define SND_DJM_GROUP_MASK	0x00ff0000
+#define SND_DJM_DEVICE_MASK	0xff000000
+#define SND_DJM_GROUP_SHIFT	16
+#define SND_DJM_DEVICE_SHIFT	24
+
+// device table index
+#define SND_DJM_250MK2_IDX	0x0
+#define SND_DJM_750_IDX		0x1
+
+
+#define SND_DJM_CTL(_name, suffix, _default_value, _windex) { \
+	.name = _name, \
+	.options = snd_djm_opts_##suffix, \
+	.noptions = ARRAY_SIZE(snd_djm_opts_##suffix), \
+	.default_value = _default_value, \
+	.wIndex = _windex }
+
+#define SND_DJM_DEVICE(suffix) { \
+	.controls = snd_djm_ctls_##suffix, \
+	.ncontrols = ARRAY_SIZE(snd_djm_ctls_##suffix) }
+
+
+struct snd_djm_device {
 	const char *name;
+	const struct snd_djm_ctl *controls;
+	size_t ncontrols;
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_level[] = {
-	{ .name =  "-5 dB",                  .wValue = 0x0300, .wIndex = 0x8003 },
-	{ .name = "-10 dB",                  .wValue = 0x0200, .wIndex = 0x8003 },
-	{ .name = "-15 dB",                  .wValue = 0x0100, .wIndex = 0x8003 },
-	{ .name = "-19 dB",                  .wValue = 0x0000, .wIndex = 0x8003 }
+struct snd_djm_ctl {
+	const char *name;
+	const u16 *options;
+	size_t noptions;
+	u16 default_value;
+	u16 wIndex;
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch12[] = {
-	{ .name =  "CH1 Control Tone PHONO", .wValue = 0x0103, .wIndex = 0x8002 },
-	{ .name =  "CH1 Control Tone LINE",  .wValue = 0x0100, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0106, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0107, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0108, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0109, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x010d, .wIndex = 0x8002 },
-	{ .name =  "REC OUT",                .wValue = 0x010a, .wIndex = 0x8002 }
+static const char *snd_djm_get_label_caplevel(u16 wvalue)
+{
+	switch (wvalue) {
+	case 0x0000:	return "-19dB";
+	case 0x0100:	return "-15dB";
+	case 0x0200:	return "-10dB";
+	case 0x0300:	return "-5dB";
+	default:	return NULL;
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch34[] = {
-	{ .name =  "CH2 Control Tone PHONO", .wValue = 0x0203, .wIndex = 0x8002 },
-	{ .name =  "CH2 Control Tone LINE",  .wValue = 0x0200, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0206, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0207, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0208, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0209, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x020d, .wIndex = 0x8002 },
-	{ .name =  "REC OUT",                .wValue = 0x020a, .wIndex = 0x8002 }
+static const char *snd_djm_get_label_cap(u16 wvalue)
+{
+	switch (wvalue & 0x00ff) {
+	case SND_DJM_CAP_LINE:		return "Control Tone LINE";
+	case SND_DJM_CAP_CDLINE:	return "Control Tone CD/LINE";
+	case SND_DJM_CAP_PHONO:		return "Control Tone PHONO";
+	case SND_DJM_CAP_PFADER:	return "Post Fader";
+	case SND_DJM_CAP_XFADERA:	return "Cross Fader A";
+	case SND_DJM_CAP_XFADERB:	return "Cross Fader B";
+	case SND_DJM_CAP_MIC:		return "Mic";
+	case SND_DJM_CAP_RECOUT:	return "Rec Out";
+	case SND_DJM_CAP_AUX:		return "Aux";
+	case SND_DJM_CAP_NONE:		return "None";
+	case SND_DJM_CAP_CH1PFADER:	return "Post Fader Ch1";
+	case SND_DJM_CAP_CH2PFADER:	return "Post Fader Ch2";
+	default:			return NULL;
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_capture_ch56[] = {
-	{ .name =  "REC OUT",                .wValue = 0x030a, .wIndex = 0x8002 },
-	{ .name =  "Post CH1 Fader",         .wValue = 0x0311, .wIndex = 0x8002 },
-	{ .name =  "Post CH2 Fader",         .wValue = 0x0312, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader A",          .wValue = 0x0307, .wIndex = 0x8002 },
-	{ .name =  "Cross Fader B",          .wValue = 0x0308, .wIndex = 0x8002 },
-	{ .name =  "MIC",                    .wValue = 0x0309, .wIndex = 0x8002 },
-	{ .name =  "AUX",                    .wValue = 0x030d, .wIndex = 0x8002 }
+static const char *snd_djm_get_label_pb(u16 wvalue)
+{
+	switch (wvalue & 0x00ff) {
+	case SND_DJM_PB_CH1:	return "Ch1";
+	case SND_DJM_PB_CH2:	return "Ch2";
+	case SND_DJM_PB_AUX:	return "Aux";
+	default:		return NULL;
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_12[] = {
-	{ .name =  "CH1",                    .wValue = 0x0100, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0101, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0104, .wIndex = 0x8016 }
+static const char *snd_djm_get_label(u16 wvalue, u16 windex)
+{
+	switch (windex) {
+	case SND_DJM_WINDEX_CAPLVL:	return snd_djm_get_label_caplevel(wvalue);
+	case SND_DJM_WINDEX_CAP:	return snd_djm_get_label_cap(wvalue);
+	case SND_DJM_WINDEX_PB:		return snd_djm_get_label_pb(wvalue);
+	default:			return NULL;
+	}
 };
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_34[] = {
-	{ .name =  "CH1",                    .wValue = 0x0200, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0201, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0204, .wIndex = 0x8016 }
-};
 
-static const struct snd_pioneer_djm_option snd_pioneer_djm_options_playback_56[] = {
-	{ .name =  "CH1",                    .wValue = 0x0300, .wIndex = 0x8016 },
-	{ .name =  "CH2",                    .wValue = 0x0301, .wIndex = 0x8016 },
-	{ .name =  "AUX",                    .wValue = 0x0304, .wIndex = 0x8016 }
+// DJM-250MK2
+static const u16 snd_djm_opts_cap_level[] = {
+	0x0000, 0x0100, 0x0200, 0x0300 };
+
+static const u16 snd_djm_opts_250mk2_cap1[] = {
+	0x0103, 0x0100, 0x0106, 0x0107, 0x0108, 0x0109, 0x010d, 0x010a };
+
+static const u16 snd_djm_opts_250mk2_cap2[] = {
+	0x0203, 0x0200, 0x0206, 0x0207, 0x0208, 0x0209, 0x020d, 0x020a };
+
+static const u16 snd_djm_opts_250mk2_cap3[] = {
+	0x030a, 0x0311, 0x0312, 0x0307, 0x0308, 0x0309, 0x030d };
+
+static const u16 snd_djm_opts_250mk2_pb1[] = { 0x0100, 0x0101, 0x0104 };
+static const u16 snd_djm_opts_250mk2_pb2[] = { 0x0200, 0x0201, 0x0204 };
+static const u16 snd_djm_opts_250mk2_pb3[] = { 0x0300, 0x0301, 0x0304 };
+
+static const struct snd_djm_ctl snd_djm_ctls_250mk2[] = {
+	SND_DJM_CTL("Capture Level", cap_level, 0, SND_DJM_WINDEX_CAPLVL),
+	SND_DJM_CTL("Ch1 Input",   250mk2_cap1, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch2 Input",   250mk2_cap2, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch3 Input",   250mk2_cap3, 0, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch1 Output",   250mk2_pb1, 0, SND_DJM_WINDEX_PB),
+	SND_DJM_CTL("Ch2 Output",   250mk2_pb2, 1, SND_DJM_WINDEX_PB),
+	SND_DJM_CTL("Ch3 Output",   250mk2_pb3, 2, SND_DJM_WINDEX_PB)
 };
 
-struct snd_pioneer_djm_option_group {
-	const char *name;
-	const struct snd_pioneer_djm_option *options;
-	const size_t count;
-	const u16 default_value;
+
+// DJM-750
+static const u16 snd_djm_opts_750_cap1[] = {
+	0x0101, 0x0103, 0x0106, 0x0107, 0x0108, 0x0109, 0x010a, 0x010f };
+static const u16 snd_djm_opts_750_cap2[] = {
+	0x0200, 0x0201, 0x0206, 0x0207, 0x0208, 0x0209, 0x020a, 0x020f };
+static const u16 snd_djm_opts_750_cap3[] = {
+	0x0300, 0x0301, 0x0306, 0x0307, 0x0308, 0x0309, 0x030a, 0x030f };
+static const u16 snd_djm_opts_750_cap4[] = {
+	0x0401, 0x0403, 0x0406, 0x0407, 0x0408, 0x0409, 0x040a, 0x040f };
+
+static const struct snd_djm_ctl snd_djm_ctls_750[] = {
+	SND_DJM_CTL("Capture Level", cap_level, 0, SND_DJM_WINDEX_CAPLVL),
+	SND_DJM_CTL("Ch1 Input",   750_cap1, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch2 Input",   750_cap2, 2, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch3 Input",   750_cap3, 0, SND_DJM_WINDEX_CAP),
+	SND_DJM_CTL("Ch4 Input",   750_cap4, 0, SND_DJM_WINDEX_CAP)
 };
 
-#define snd_pioneer_djm_option_group_item(_name, suffix, _default_value) { \
-	.name = _name, \
-	.options = snd_pioneer_djm_options_##suffix, \
-	.count = ARRAY_SIZE(snd_pioneer_djm_options_##suffix), \
-	.default_value = _default_value }
-
-static const struct snd_pioneer_djm_option_group snd_pioneer_djm_option_groups[] = {
-	snd_pioneer_djm_option_group_item("Master Capture Level Capture Switch", capture_level, 0),
-	snd_pioneer_djm_option_group_item("Capture 1-2 Capture Switch",          capture_ch12,  2),
-	snd_pioneer_djm_option_group_item("Capture 3-4 Capture Switch",          capture_ch34,  2),
-	snd_pioneer_djm_option_group_item("Capture 5-6 Capture Switch",          capture_ch56,  0),
-	snd_pioneer_djm_option_group_item("Playback 1-2 Playback Switch",        playback_12,   0),
-	snd_pioneer_djm_option_group_item("Playback 3-4 Playback Switch",        playback_34,   1),
-	snd_pioneer_djm_option_group_item("Playback 5-6 Playback Switch",        playback_56,   2)
+
+static const struct snd_djm_device snd_djm_devices[] = {
+	SND_DJM_DEVICE(250mk2),
+	SND_DJM_DEVICE(750)
 };
 
-// layout of the kcontrol->private_value:
-#define SND_PIONEER_DJM_VALUE_MASK 0x0000ffff
-#define SND_PIONEER_DJM_GROUP_MASK 0xffff0000
-#define SND_PIONEER_DJM_GROUP_SHIFT 16
 
-static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info)
+static int snd_djm_controls_info(struct snd_kcontrol *kctl,
+				struct snd_ctl_elem_info *info)
 {
-	u16 group_index = kctl->private_value >> SND_PIONEER_DJM_GROUP_SHIFT;
-	size_t count;
+	unsigned long private_value = kctl->private_value;
+	u8 device_idx = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 ctl_idx = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
+	const struct snd_djm_device *device = &snd_djm_devices[device_idx];
 	const char *name;
-	const struct snd_pioneer_djm_option_group *group;
+	const struct snd_djm_ctl *ctl;
+	size_t noptions;
+
+	if (ctl_idx >= device->ncontrols)
+		return -EINVAL;
+
+	ctl = &device->controls[ctl_idx];
+	noptions = ctl->noptions;
+	if (info->value.enumerated.item >= noptions)
+		info->value.enumerated.item = noptions - 1;
 
-	if (group_index >= ARRAY_SIZE(snd_pioneer_djm_option_groups))
+	name = snd_djm_get_label(ctl->options[info->value.enumerated.item],
+				ctl->wIndex);
+	if (!name)
 		return -EINVAL;
 
-	group = &snd_pioneer_djm_option_groups[group_index];
-	count = group->count;
-	if (info->value.enumerated.item >= count)
-		info->value.enumerated.item = count - 1;
-	name = group->options[info->value.enumerated.item].name;
 	strscpy(info->value.enumerated.name, name, sizeof(info->value.enumerated.name));
 	info->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	info->count = 1;
-	info->value.enumerated.items = count;
+	info->value.enumerated.items = noptions;
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u16 group, u16 value)
+static int snd_djm_controls_update(struct usb_mixer_interface *mixer,
+				u8 device_idx, u8 group, u16 value)
 {
 	int err;
+	const struct snd_djm_device *device = &snd_djm_devices[device_idx];
 
-	if (group >= ARRAY_SIZE(snd_pioneer_djm_option_groups)
-			|| value >= snd_pioneer_djm_option_groups[group].count)
+	if ((group >= device->ncontrols) || value >= device->controls[group].noptions)
 		return -EINVAL;
 
 	err = snd_usb_lock_shutdown(mixer->chip);
@@ -2748,63 +2828,76 @@ static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u1
 		mixer->chip->dev, usb_sndctrlpipe(mixer->chip->dev, 0),
 		USB_REQ_SET_FEATURE,
 		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		snd_pioneer_djm_option_groups[group].options[value].wValue,
-		snd_pioneer_djm_option_groups[group].options[value].wIndex,
+		device->controls[group].options[value],
+		device->controls[group].wIndex,
 		NULL, 0);
 
 	snd_usb_unlock_shutdown(mixer->chip);
 	return err;
 }
 
-static int snd_pioneer_djm_controls_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
+static int snd_djm_controls_get(struct snd_kcontrol *kctl,
+				struct snd_ctl_elem_value *elem)
 {
-	elem->value.enumerated.item[0] = kctl->private_value & SND_PIONEER_DJM_VALUE_MASK;
+	elem->value.enumerated.item[0] = kctl->private_value & SND_DJM_VALUE_MASK;
 	return 0;
 }
 
-static int snd_pioneer_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
+static int snd_djm_controls_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *elem)
 {
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
 	struct usb_mixer_interface *mixer = list->mixer;
 	unsigned long private_value = kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
+
+	u8 device = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
 	u16 value = elem->value.enumerated.item[0];
 
-	kctl->private_value = (group << SND_PIONEER_DJM_GROUP_SHIFT) | value;
+	kctl->private_value = ((device << SND_DJM_DEVICE_SHIFT) |
+			      (group << SND_DJM_GROUP_SHIFT) |
+			      value);
 
-	return snd_pioneer_djm_controls_update(mixer, group, value);
+	return snd_djm_controls_update(mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_resume(struct usb_mixer_elem_list *list)
+static int snd_djm_controls_resume(struct usb_mixer_elem_list *list)
 {
 	unsigned long private_value = list->kctl->private_value;
-	u16 group = (private_value & SND_PIONEER_DJM_GROUP_MASK) >> SND_PIONEER_DJM_GROUP_SHIFT;
-	u16 value = (private_value & SND_PIONEER_DJM_VALUE_MASK);
+	u8 device = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
+	u8 group = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
+	u16 value = (private_value & SND_DJM_VALUE_MASK);
 
-	return snd_pioneer_djm_controls_update(list->mixer, group, value);
+	return snd_djm_controls_update(list->mixer, device, group, value);
 }
 
-static int snd_pioneer_djm_controls_create(struct usb_mixer_interface *mixer)
+static int snd_djm_controls_create(struct usb_mixer_interface *mixer,
+		const u8 device_idx)
 {
 	int err, i;
-	const struct snd_pioneer_djm_option_group *group;
+	u16 value;
+
+	const struct snd_djm_device *device = &snd_djm_devices[device_idx];
+
 	struct snd_kcontrol_new knew = {
 		.iface  = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
 		.index = 0,
-		.info = snd_pioneer_djm_controls_info,
-		.get  = snd_pioneer_djm_controls_get,
-		.put  = snd_pioneer_djm_controls_put
+		.info = snd_djm_controls_info,
+		.get  = snd_djm_controls_get,
+		.put  = snd_djm_controls_put
 	};
 
-	for (i = 0; i < ARRAY_SIZE(snd_pioneer_djm_option_groups); i++) {
-		group = &snd_pioneer_djm_option_groups[i];
-		knew.name = group->name;
-		knew.private_value = (i << SND_PIONEER_DJM_GROUP_SHIFT) | group->default_value;
-		err = snd_pioneer_djm_controls_update(mixer, i, group->default_value);
+	for (i = 0; i < device->ncontrols; i++) {
+		value = device->controls[i].default_value;
+		knew.name = device->controls[i].name;
+		knew.private_value = (
+			(device_idx << SND_DJM_DEVICE_SHIFT) |
+			(i << SND_DJM_GROUP_SHIFT) |
+			value);
+		err = snd_djm_controls_update(mixer, device_idx, i, value);
 		if (err)
 			return err;
-		err = add_single_ctl_with_resume(mixer, 0, snd_pioneer_djm_controls_resume,
+		err = add_single_ctl_with_resume(mixer, 0, snd_djm_controls_resume,
 						 &knew, NULL);
 		if (err)
 			return err;
@@ -2917,7 +3010,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		err = snd_bbfpro_controls_create(mixer);
 		break;
 	case USB_ID(0x2b73, 0x0017): /* Pioneer DJ DJM-250MK2 */
-		err = snd_pioneer_djm_controls_create(mixer);
+		err = snd_djm_controls_create(mixer, SND_DJM_250MK2_IDX);
+		break;
+	case USB_ID(0x08e4, 0x017f): /* Pioneer DJ DJM-750 */
+		err = snd_djm_controls_create(mixer, SND_DJM_750_IDX);
 		break;
 	}
 
-- 
2.30.0

Re: [PATCH v4 1/1] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

[Takashi Iwai]

On Fri, 05 Feb 2021 19:42:56 +0100,
Olivia Mackintosh wrote:
> 
> This allows for N different devices to use the pioneer mixer quirk for
> setting capture/record type and recording level. The impementation has
> not changed much with the exception of an additional mask on
> private_value to allow storing of a device index:
> 	DEVICE MASK	0xff000000
> 	GROUP_MASK	0x00ff0000
> 	VALUE_MASK	0x0000ffff
> 
> This could be improved by changing the arrays of wValues for each
> channel to contain named definitions (e.g. SND_DJM_CAP_LINE). It would
> improve readability and perhaps would allow using the same array for
> multiple channels. The channel number can be specified on the control
> next to the wIndex.
> 
> Feedback is very much appreciated as I'm not the most proficient C
> programmer but am learning as I go.
> 
> Signed-off-by: Olivia Mackintosh <livvy@base.nu>

Thanks, applied now.


Takashi