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

On Thu, 02 Feb 2017 17:56:05 +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.

In v3 I did changed the following: exchange MUA to keep code style, remove all globals, all declaration static (where possible), use standard snd_ctl_boolean_mono_info() when possible, NULL check for all kmalloc(), Remove redundant code, add comments where needed, fix macro arguments, replace uint8_t with u8/s8, remove all typedefs (if possible), add resume callback.

Signed-off-by: Detlef Urban onkel@paraair.de

Thanks, this version looks much better. But a few things:

--- /dev/null +++ b/sound/usb/mixer_us16x08.c @@ -0,0 +1,1772 @@ +/*

• • Tascam US-16x08 ALSA driver
• • Copyright (c) 2016 by Detlef Urban (onkel@paraair.de)
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

+#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/usb/audio-v2.h> +#include "linux/kthread.h"

Why kthread? Also why double-quote, not bracket?

+#include <sound/core.h> +#include <sound/control.h> +#include <asm-generic/errno-base.h>

This looks very wrong. Just include <linux/errno.h> if really needed (I guess you won't).

+/* wrapper function to send prepared URB buffer to usb device. Return -1

• • if something went wrong

It's always better to return a negative error code instead of -1.

• */

+static int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size) +{

• int count = -1;
• if (chip) {

• count = snd_usb_ctl_msg(chip->dev,
  

• 	usb_sndctrlpipe(chip->dev, 0),
  

• 	SND_US16X08_URB_REQUEST,
  

• 	SND_US16X08_URB_REQUESTTYPE,
  

• 	0, 0, buf, size);
  

The indentation looks wrong.

+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;
• struct snd_us16x08_common *store =

• (struct snd_us16x08_common *) elem->private_data;
  

• int index = ucontrol->id.index;
• char buf[sizeof(route_msg)];
• int val, val_org, err = 0;
• /* prepare the message buffer from template */
• memcpy(buf, route_msg, sizeof(route_msg));
• /* get the new value (no bias for routes) */
• val = ucontrol->value.integer.value[0];
• if (val < 2) {

• /* input comes from a master channel */
  

• val_org = val;
  

• buf[2] = 0x02;
  

• } else {

• /* input comes from a computer channel */
  

• buf[2] = 0x03;
  

• val_org = val - 2;
  

• }
• /* place new route selection in URB message */
• buf[5] = (unsigned char) (val_org & 0x0f) + 1;
• /* place route selector in URB message */
• buf[13] = index + 1;
• err = snd_us16x08_send_urb(chip, buf, sizeof(route_msg));
• if (err == 0) {

• store->value[index] = val;
  

• elem->cached &= 1 << index;
  

Do you mean "|="? Otherwise the cache value is set exclusively. (Ditto for all other places.)

+static int snd_us16x08_comp_get(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)

+{

• int val = 0;
• struct usb_mixer_elem_info *elem = kcontrol->private_data;
• struct snd_us16x08_comp_store *store =

• ((struct snd_us16x08_comp_store *) elem->private_data);
  

• int index = ucontrol->id.index;
• switch (elem->head.id) {
• case SND_US16X08_ID_COMP_THRESHOLD:

• val = store->valThreshold[index];
  

• break;
  

• case SND_US16X08_ID_COMP_RATIO:

• val = store->valRatio[index];
  

• break;
  

• case SND_US16X08_ID_COMP_ATTACK:

• val = store->valAttack[index];
  

• break;
  

• case SND_US16X08_ID_COMP_RELEASE:

• val = store->valRelease[index];
  

• break;
  

• case SND_US16X08_ID_COMP_GAIN:

• val = store->valGain[index];
  

• break;
  

• case SND_US16X08_ID_COMP_SWITCH:

• val = store->valSwitch[index];
  

• break;
  

Wouldn't it be easier to have something like:

val_idx = elem_id_to_idx(elem->head.id); val = store->values[val_id][index];

A series of switch/case doesn't look good.

+static int snd_us16x08_eqswitch_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;
• struct snd_us16x08_eq_all_store *store =

• ((struct snd_us16x08_eq_all_store *) elem->private_data);
  

• int index = ucontrol->id.index;
• char buf[sizeof(eqs_msq)];
• int val, err = 0;
• /* new control value incl. bias*/
• val = ucontrol->value.integer.value[0] + SND_US16X08_KCBIAS(kcontrol);
• /* prepare URB message from EQ template */
• memcpy(buf, eqs_msq, sizeof(eqs_msq));
• store->low_store->valSwitch[index] = val;
• store->midlow_store->valSwitch[index] = val;
• store->midhigh_store->valSwitch[index] = val;
• store->high_store->valSwitch[index] = val;
• /* place channel index in URB message */
• buf[5] = index + 1;
• /* all four EQ bands have to be enabled/disabled in once */
• buf[20] = store->low_store->valSwitch[index];
• buf[17] = store->low_store->valWidth[index];
• buf[14] = store->low_store->valFreq[index];
• buf[11] = store->low_store->valdB[index];
• buf[8] = 0x01;
• err = snd_us16x08_send_urb(chip, buf, sizeof(eqs_msq));

No error check?

• /* give time to the device to handle the request */
• mdelay(15);

Does it have to be mdelay(), not msleep() or its variant? mdelay() with such a long time is usually something wrong.

+/* setup EQ store and assign default values */ +static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(int band_index) +{

• int i = 0;

Superfluous initialization.

+/* suspend/resume */

+static void snd_us16x08_resume_route(struct usb_mixer_elem_info *elem) +{

• int i;
• struct snd_us16x08_common *store =

• (struct snd_us16x08_common *) elem->private_data;
  

• for (i = 0; i < elem->channels; i++)

• if (elem->cached & (1 << i))
  

• 	store->value[i] = elem->cache_val[i];
  

Does it make sense? The resume function resumes the device, and it's not about the memory we manage by ourselves. Usually the resume function writes back the cached value to the device.

If store->value[] and elem->cache_val[] is always identical, we can drop even store->value[].

+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",
  

• .freeer = snd_usb_mixer_elem_free
  

• },
• { /* EQ low gain */

• .kcontrol_new = &snd_us16x08_eq_gain_ctl,
  

• .control_id = SND_US16X08_ID_EQLOWLEVEL,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Low gain",
  

• .freeer = snd_usb_mixer_elem_free
  

• },
• { /* EQ low freq */

• .kcontrol_new = &snd_us16x08_eq_low_freq_ctl,
  

• .control_id = SND_US16X08_ID_EQLOWFREQ,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Low freq",
  

• .freeer = NULL
  

• },
• { /* EQ mid low gain */

• .kcontrol_new = &snd_us16x08_eq_gain_ctl,
  

• .control_id = SND_US16X08_ID_EQLOWMIDLEVEL,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Mid low gain",
  

• .freeer = snd_usb_mixer_elem_free
  

• },
• { /* EQ mid low freq */

• .kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
  

• .control_id = SND_US16X08_ID_EQLOWMIDFREQ,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Mid low freq",
  

• .freeer = NULL
  

• },
• { /* EQ mid low Q */

• .kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
  

• .control_id = SND_US16X08_ID_EQLOWMIDWIDTH,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Mid low Q",
  

• .freeer = NULL
  

• },
• { /* EQ mid high gain */

• .kcontrol_new = &snd_us16x08_eq_gain_ctl,
  

• .control_id = SND_US16X08_ID_EQHIGHMIDLEVEL,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Mid high gain",
  

• .freeer = snd_usb_mixer_elem_free
  

• },
• { /* EQ mid high freq */

• .kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
  

• .control_id = SND_US16X08_ID_EQHIGHMIDFREQ,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Mid high freq",
  

• .freeer = NULL
  

• },
• { /* EQ mid high Q */

• .kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
  

• .control_id = SND_US16X08_ID_EQHIGHMIDWIDTH,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "Mid high Q",
  

• .freeer = NULL
  

• },
• { /* EQ high gain */

• .kcontrol_new = &snd_us16x08_eq_gain_ctl,
  

• .control_id = SND_US16X08_ID_EQHIGHLEVEL,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "High gain",
  

• .freeer = snd_usb_mixer_elem_free
  

• },
• { /* EQ low freq */

• .kcontrol_new = &snd_us16x08_eq_high_freq_ctl,
  

• .control_id = SND_US16X08_ID_EQHIGHFREQ,
  

• .type = USB_MIXER_U8,
  

• .num_channels = 16,
  

• .name = "High freq",
  

These mixer control names look somehow too ambiguous...

+static int snd_us16x08_controls_create_eq(struct usb_mixer_interface *mixer) +{

• int i;
• int err;
• void *store = NULL;
• struct snd_us16x08_eq_store *eq_low_store =

• snd_us16x08_create_eq_store(0x01);
  

• struct snd_us16x08_eq_store *eq_midlow_store =

• snd_us16x08_create_eq_store(0x02);
  

• struct snd_us16x08_eq_store *eq_midhigh_store =

• snd_us16x08_create_eq_store(0x03);
  

• struct snd_us16x08_eq_store *eq_high_store =

• snd_us16x08_create_eq_store(0x04);
  

• struct snd_us16x08_eq_all_store *eq_all_store =

• kmalloc(sizeof(struct snd_us16x08_eq_all_store), GFP_KERNEL);
  

• /* check for allocation error */
• if (eq_low_store == NULL || eq_midlow_store == NULL ||

• eq_midhigh_store == NULL || eq_high_store == NULL ||
  

• eq_all_store == NULL)
  

• return -ENOMEM;
  

This leaks the memory. You forgot to release non-NULL ones.

+int snd_us16x08_controls_create(struct usb_mixer_interface *mixer) +{

• int i;
• int err;
• struct snd_us16x08_common *route_store;
• struct snd_us16x08_comp_store *comp_store;
• struct snd_us16x08_meter_store *meter_store;
• struct snd_us16x08_common *master_store;
• struct snd_us16x08_bus_store *bus_store;
• struct snd_us16x08_channel_store *channel_store;
• /* just check for non-MIDI interface */
• if (mixer->hostif->desc.bInterfaceNumber == 3) {

• /* create compressor mixer elements */
  

• comp_store = snd_us16x08_create_comp_store();
  

• if (comp_store == NULL)
  

• 	return -ENOMEM;
  

• /* create bus routing store */
  

• route_store = snd_us16x08_create_route_store();
  

• if (route_store == NULL)
  

• 	return -ENOMEM;
  

• /* create meters store */
  

• meter_store = snd_us16x08_create_meter_store();
  

• if (meter_store == NULL)
  

• 	return -ENOMEM;
  

• /* create master store */
  

• master_store = snd_us16x08_create_mix_store(127);
  

• if (master_store == NULL)
  

• 	return -ENOMEM;
  

• /* create bus store */
  

• bus_store = snd_us16x08_create_bus_store(0);
  

• if (bus_store == NULL)
  

• 	return -ENOMEM;
  

• /* create channel store */
  

• channel_store = snd_us16x08_create_channel_store();
  

• if (channel_store == NULL)
  

• 	return -ENOMEM;
  

Ditto.

• err = add_new_ctl(mixer, &snd_us16x08_route_ctl,
  

• 	SND_US16X08_ID_ROUTE, USB_MIXER_U8, 8, "Route",
  

• 	(void *) route_store, snd_usb_mixer_elem_free);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_master_ctl,
  

• 	SND_US16X08_ID_FADER, USB_MIXER_U8, 1, "Master",
  

• 	(void *) master_store,
  

• 	snd_usb_mixer_elem_free);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_bus_ctl,
  

• 	SND_US16X08_ID_BYPASS, USB_MIXER_U8, 1, "Bypass",
  

• 	(void *) bus_store, snd_usb_mixer_elem_free);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_bus_ctl,
  

• 	SND_US16X08_ID_BUSS_OUT, USB_MIXER_U8, 1, "Buss out",
  

• 	(void *) bus_store, NULL);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_bus_ctl,
  

• 	SND_US16X08_ID_MUTE, USB_MIXER_U8, 1, "Master mute",
  

• 	(void *) bus_store, NULL);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_ch_boolean_ctl,
  

• 	SND_US16X08_ID_PHASE, USB_MIXER_U8, 16, "Phase",
  

• 	(void *) channel_store,
  

• 	snd_usb_mixer_elem_free);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_ch_int_ctl,
  

• 	SND_US16X08_ID_FADER, USB_MIXER_S16, 16, "Fader",
  

• 	(void *) channel_store, NULL);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_ch_boolean_ctl,
  

• 	SND_US16X08_ID_MUTE, USB_MIXER_BOOLEAN, 16, "Mute",
  

• 	(void *) channel_store, NULL);
  

• if (err < 0)
  

• 	return err;
  

• err = add_new_ctl(mixer, &snd_us16x08_ch_int_ctl,
  

• 	SND_US16X08_ID_PAN, USB_MIXER_U16, 16, "Pan",
  

• 	(void *) channel_store, NULL);
  

Can we use a table for registration, too? This sequence of calls looks... well, a lot of room to optimize :)

+struct snd_us16x08_eq_store {

• u8 valdB[SND_US16X08_MAX_CHANNELS];
• u8 valFreq[SND_US16X08_MAX_CHANNELS];
• u8 valWidth[SND_US16X08_MAX_CHANNELS];
• u8 valSwitch[SND_US16X08_MAX_CHANNELS];

Please avoid thisSomething, but use this_something (dB might be an exception, though). In this case, val prefix can be dropped, too. It's just superfluous.

+struct snd_us16x08_comp_store {

• u8 valThreshold[SND_US16X08_MAX_CHANNELS];
• u8 valAttack[SND_US16X08_MAX_CHANNELS];
• u8 valRelease[SND_US16X08_MAX_CHANNELS];
• u8 valRatio[SND_US16X08_MAX_CHANNELS];
• u8 valGain[SND_US16X08_MAX_CHANNELS];
• u8 valSwitch[SND_US16X08_MAX_CHANNELS];

+};

Ditto.

thanks,

Takashi