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

[Olivia Mackintosh <livvy@base.nu>]

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