[alsa-devel] [PATCH 4/4 v5] ALSA: usb-audio: Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20

[Takashi Iwai]

At Thu,  6 Nov 2014 08:33:38 -0600,
Chris J Arges wrote:
> 
> --- /dev/null
> +++ b/sound/usb/mixer_scarlett.c
....
> +int scarlett_init = 0;

What's this?


> +/********************** Enum Strings *************************/
> +
> +static const struct scarlett_mixer_elem_enum_info opt_pad = {
> +	.start = 0,
> +	.len = 2,
> +	.names = (char *[]){

Better to keep names field const to avoid the cast.
It might be needed to cast for the dynamic arrays, but then you need
the cast only once there, instead of spreading cast over all codes.

> +static int scarlett_ctl_enum_get(struct snd_kcontrol *kctl,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct scarlett_mixer_elem_enum_info *opt = elem->private_data;
> +	int err, val;
> +
> +	err = snd_usb_get_cur_mix_value(elem, 0, 0, &val);
> +	if (err < 0)
> +		return err;
> +
> +	if ((opt->start == -1) && (val > opt->len)) /* >= 0x20 */
> +		val = 0;
> +	else
> +		val = clamp(val - opt->start, 0, opt->len-1);

Please give a bit more explanation here.  This code relies on the fact
that the hardware returns a high value for non-selection.

> +
> +	ucontrol->value.enumerated.item[0] = val;
> +
> +	return 0;
> +}
> +
> +static int scarlett_ctl_enum_put(struct snd_kcontrol *kctl,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct scarlett_mixer_elem_enum_info *opt = elem->private_data;
> +	int changed = 0;
> +	int err, oval, val;
> +
> +	err = snd_usb_get_cur_mix_value(elem, 0, 0, &oval);
> +	if (err < 0)
> +		return err;
> +
> +	val = ucontrol->value.integer.value[0];
> +	val = val + opt->start;

Ditto here.  It assumes that the write of an invalid value would
result in the non-selection.

> +static int add_new_ctl(struct usb_mixer_interface *mixer,
> +		       const struct snd_kcontrol_new *ncontrol,
> +		       int index, int offset, int num,
> +		       int val_type, int channels, const char *name,
> +		       const struct scarlett_mixer_elem_enum_info *opt,
> +		       struct usb_mixer_elem_info **elem_ret
> +)
> +{
...
> +	snprintf(kctl->id.name, sizeof(kctl->id.name), "%s", name);

strlcpy() is more straightforward.

> +int scarlett_create_strings_from_info(struct scarlett_device_info *info)
> +{
> +	int i = 0;
> +	char **names;
> +
> +	names = kmalloc_array(info->opt_master.len, sizeof(char *), GFP_KERNEL);
> +	if (!names) {
> +		kfree(names);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < info->opt_master.len; i++) {
> +		names[i] = kmalloc_array(SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				   sizeof(char), GFP_KERNEL);
> +		if (!names[i]) {
> +			kfree(names);
> +			return -ENOMEM;

Memory leaks.  names[0..i-1] have to be freed, too.


thanks,

Takashi