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

[Takashi Iwai <tiwai@suse.de>]

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