On Mon, 21 Sep 2020 21:09:36 +0200,
František Kučera wrote:
>
> From: František Kučera <franta-linux@xxxxxxxxxxx>
>
> 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
