[PATCH] ALSA: usb-audio: Add mixer support for Pioneer DJ DJM-250MK2

[Takashi Iwai]

On Mon, 21 Sep 2020 21:09:36 +0200,
František Kučera wrote:
> 
> From: František Kučera <franta-linux at frantovo.cz>
> 
> This patch extends support for DJM-250MK2 and allows mapping
> playback and capture channels to available sources.
> Configures the card through USB commands.

First off, your Signed-off-by line is missing.  This should have been
pointed by checkpatch.pl.

About the code changes:

> +static const struct snd_pioneer_djm_option SND_PIONEER_DJM_OPTIONS_CAPTURE_LEVEL[] = {

Avoid using the capital letters unless macros.
Ditto for other snd_pioneer_djm_option items.

> +struct snd_pioneer_djm_option_group {
> +	const char *name;
> +	const struct snd_pioneer_djm_option *options;
> +	const size_t count;
> +	const u16 default_value;
> +} snd_pioneer_djm_option_group;

Why you define an object here (snd_pioneer_djm_option_group), not only
struct?  I guess it was forgotten to remove when dropping typedef?

> +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;
> +	size_t count;
> +	const char *name;
> +	const struct snd_pioneer_djm_option_group *group;
> +
> +	if (group_index < ARRAY_SIZE(SND_PIONEER_DJM_OPTION_GROUPS)) {
> +		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;
> +		strlcpy(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;
> +		return 0;
> +	} else {
> +		return -EINVAL;
> +	}

This can be a bit simpler if you write like:

	if (group_index >= ARRAY_SIZE(....))
		return -EINVAL;

	group = &SND_PIONEER_DJM_OPTION_GROUPS[group_index];
	count = group->count;
	.....
	
The same applied to other functions.

> +static int snd_pioneer_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;

Avoid the unnecessary blank line in the above.


> +	u16 value = elem->value.enumerated.item[0];
> +
> +	kctl->private_value = group << SND_PIONEER_DJM_GROUP_SHIFT | value;

Better to wrap write parentheses around the bit operation for avoiding
confusions.  (Also a similar expression is found in another place).


thanks,

Takashi