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