[alsa-devel] [PATCH v5 1/1] ALSA: Tascam US-16x08 DSP mixer quirk

[Takashi Iwai]

On Fri, 10 Feb 2017 11:24:07 +0100,
OnkelDead wrote:
> 
> Add mixer quirk for Tascam US-16x08 usb interface.
> Even that this an usb compliant device,
> the input channels and DSP functions (EQ/Compressor)
> arn't accessible by default.
> 
> v5 fixes some wrong snd_kcontrol_new xx.put return codes
> 
> Signed-off-by: Detlef Urban <onkel at paraair.de>

Thanks, now it looks really better than previous versions!
One very minor issue in the coding style is:

> +static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kcontrol->private_data;
> +	struct snd_usb_audio *chip = elem->head.mixer->chip;
> +
> +	int index = ucontrol->id.index;
> +	char buf[sizeof(route_msg)];
> +	int val, val_org, err = 0;

Don't put a blank line among the variable definitions / declarations.

Also, another question is:

> +	err = snd_us16x08_send_urb(chip, buf, sizeof(route_msg));
> +
> +	if (err > 0) {
> +		elem->cached |= 1 << index;
> +		elem->cache_val[index] = val;
> +	} else {
> +		usb_audio_err(chip, "Failed to set routing, err:%d\n", err);
> +	}
> +
> +	return err > 0 ? 1 : 0;

How often such an error would happen?  If it may really happen (not
only in theory), it'd result in a flood of kernel error messages, and
we may need to suppress it.  OTOH, if this is really an error, the
callback should return an error code instead of silently ignoring it,
so that user-space can catch the error, too.

That said, it's a question whether you want to ignore it
intentionally, or it should be reported back to the upper level.

And the last issue is what I already mentioned: the control names.
For example:

> +/* table of EQ controls */
> +static struct snd_us16x08_control_params eq_controls[] = {
> +	{ /* EQ switch */
> +		.kcontrol_new = &snd_us16x08_eq_switch_ctl,
> +		.control_id = SND_US16X08_ID_EQENABLE,
> +		.type = USB_MIXER_BOOLEAN,
> +		.num_channels = 16,
> +		.name = "EQ enable",

... which is a very non-standard name.  There is some standard naming
rule, as defined in Documentation/sound/designs/control-names.rst.
Although we don't limit the control API usage strictly, it'd be better
to align with the rule; i.e. at least putting the proper suffix for
"Switch", "Volume", etc.

By covering these issues, I'd love to put this stuff into 4.11 if you
can resubmit it in time.


thanks,

Takashi