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

[Fabian Lesniak <fabian@lesniak-it.de>]

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.