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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.





[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux