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

[Takashi Iwai]

On Mon, 30 Jan 2017 14:20:18 +0100,
Detlef Urban wrote:
> 
> I bet on "global variables have been removed".

Good to hear, but I'm not sure.  See below.

> But still unclear to me is if use of private
> data field in struct usb_mixer_elem_info is
> uncommon?

It's normal to use it.

> 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.
> 
> Signed-off-by: Detlef Urban <onkel at paraair.de>

Well, the patch style looks still very incompatible.
Please run checkpatch.pl and fix the issues reported there at first.

Also, if it's your MUA that breaks the spacing of the patch, then
please fix your MUA setup, or use attachment as a last resort.


About the non-coding-style issues in the patch:

> +int snd_us16x08_recv_urb(struct snd_usb_audio *chip,
> +    unsigned char *buf, int size)

Missing static?

> +int snd_us16x08_recv_urb_0xa1(struct snd_usb_audio *chip,
> +    char *buffer, int size)

Ditto.  I won't mention in all others.  Please check each function
whether it really must be non-static or not.

> +{
> +
> +    unsigned char buf[64] = {0,};
> +    int err = snd_usb_ctl_msg(chip->dev,
> +        usb_rcvctrlpipe(chip->dev, 0),
> +        2,
> +        0xa1, 0x0100, 0x100, buf, 14);
> +    dev_err(&chip->dev->dev,
> +        "us16x08: snd_us16x08_recv_urb: error: %d\n", err);

Is the intention to show always the error?  If yes, please give some
more comments to the function itself.

> +
> +    return 0;
> +}
> +
> +/* wrapper function to send prepared URB buffer to usb device. Return -1
> + * if something went wrong (FIXME: have to have more helpful error numbers)

Better to fix now :)

> + */
> +int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size)
....
> +struct meter_resp {
> +    uint8_t a0;
> +    uint8_t a1;
> +    uint8_t a2;
> +    uint8_t b0;
> +    uint8_t b1;
> +    uint8_t b2;
> +    uint8_t c0;
> +    uint8_t c1;
> +    uint8_t c2;
> +    uint8_t c3;
> +};
> +struct meter_resp *resp;
> +unsigned char meter_urb_buf[64] = {0,};

What about these are global...?  I thought you bet it.

> +int snd_us16x08_eqswitch_info(struct snd_kcontrol *kcontrol,
> +    struct snd_ctl_elem_info *uinfo)
> +{
> +    uinfo->count = 1;
> +    uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
> +    uinfo->value.integer.max = 1;
> +    uinfo->value.integer.min = 0;
> +    return 0;
> +}

Use the standard snd_ctl_boolean_mono_info().

> +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;
> +
> +    val = ucontrol->value.integer.value[0];
> +
> +    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;
> +
> +    buf[5] = index + 1;
> +
> +    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));
> +
> +    mdelay(15);

Need to be mdelay(), not msleep() or variant?

> +struct snd_us16x08_fader *snd_us16x08_create_mix_store(int default_val)
> +{
> +    int i;
> +    struct snd_us16x08_fader *tmp =
> +        kmalloc(sizeof(struct snd_us16x08_fader), GFP_KERNEL);

Missing NULL check.

> +
> +    for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++)
> +        tmp->value[i] = default_val;
> +    return tmp;
> +}
> +
> +struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
> +{
> +    int i = 0;
> +    struct snd_us16x08_comp_store *tmp =
> +        kmalloc(sizeof(struct snd_us16x08_comp_store), GFP_KERNEL);

Ditto.

> +
> +    for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
> +        tmp->valThreshold[i] = 0x20; /*     0dB */
> +        tmp->valRatio[i] = 0x00; /*         1:1 */
> +        tmp->valGain[i] = 0x00; /*          0dB */
> +        tmp->valSwitch[i] = 0x00; /*        on */
> +        tmp->valAttack[i] = 0x02; /*        2ms */
> +        tmp->valRelease[i] = 0x01; /*       10ms */
> +    }
> +    return tmp;
> +}
> +
> +struct snd_us16x08_bypass_store *snd_us16x08_create_single_store(
> +    int default_val)
> +{
> +    struct snd_us16x08_bypass_store *tmp =
> +        kmalloc(sizeof(struct snd_us16x08_bypass_store), GFP_KERNEL);

Ditto...  For the whole!

> +void snd_us16x08_free_eq_store(struct snd_kcontrol *kctl)
> +{
> +    snd_usb_mixer_elem_free(kctl);
> +}
> +
> +void snd_us16x08_free_mix_store(struct snd_kcontrol *kctl)
> +{
> +    snd_usb_mixer_elem_free(kctl);
> +}

Why two different functions for the very same thing?


> +static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
> +    struct snd_ctl_elem_value *ucontrol)
> +{
> +    int i, set;
> +    int val;
> +    struct usb_mixer_elem_info *elem = kcontrol->private_data;
> +    struct snd_usb_audio *chip = elem->head.mixer->chip;
> +    struct snd_us16x08_meter_store *store = elem->private_data;
> +    /* struct snd_us16x08_comp_store *comp_store; */
> +
> +    if (elem) {
> +        store = (struct snd_us16x08_meter_store *) elem->private_data;
> +        chip = elem->head.mixer->chip;
> +    }
> +
> +    switch (kcontrol->private_value) {
> +    case 0:
> +        snd_us16x08_send_urb(chip, mix_init_msg1, 4);
> +        snd_us16x08_recv_urb(chip, meter_urb_buf,
> +            sizeof(meter_urb_buf));
> +        kcontrol->private_value++;
> +        break;
> +    case 1:
> +        snd_us16x08_recv_urb(chip, meter_urb_buf,
> +            sizeof(meter_urb_buf));
> +        kcontrol->private_value++;
> +        break;
> +    case 2:
> +        snd_us16x08_recv_urb(chip, meter_urb_buf,
> +            sizeof(meter_urb_buf));
> +        kcontrol->private_value++;
> +        break;
> +    case 3:
> +        mix_init_msg2[2] = snd_get_meter_comp_index(store);
> +        snd_us16x08_send_urb(chip, mix_init_msg2, 10);
> +        snd_us16x08_recv_urb(chip, meter_urb_buf,
> +            sizeof(meter_urb_buf));
> +        kcontrol->private_value = 0;
> +        break;
> +    }
.....

WTF does this function do...?  Please add more comments.


> +int snd_us16x08_controls_create_eq(struct usb_mixer_interface *mixer)
> +{
> +
> +    int err;
> +    char name[64];
> +    struct usb_mixer_elem_info *elem;
> +
> +    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);
> +
> +    eq_all_store->low_store = eq_low_store;
> +    eq_all_store->midlow_store = eq_midlow_store;
> +    eq_all_store->midhigh_store = eq_midhigh_store;
> +    eq_all_store->high_store = eq_high_store;

You didn't check NULL at all here...


> +    snprintf(name, sizeof(name), "5 Low");
> +    err = add_new_ctl(mixer, &snd_us16x08_eqlevel_ctl,
> +        SND_US16X08_ID_EQLOWLEVEL, 0x00, 0, USB_MIXER_U8, 1, name,
> +        (void *) eq_low_store, snd_us16x08_free_eq_store, &elem);
> +    if (err < 0)
> +        return err;
> +
> +    snprintf(name, sizeof(name), "51 LowFreq");
....

Please use a table for this kind of code.


> +int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
> +{
> +
> +    int err;
> +    char name[64];
> +    struct usb_mixer_elem_info *elem;
> +    struct snd_us16x08_fader *route_store;
> +    struct snd_us16x08_comp_store *comp_store;
> +    struct snd_us16x08_meter_store *meter_store;
> +
> +    if (mixer->chip->num_interfaces > 0) {

What is this?


> +        route_store = snd_us16x08_create_mix_store(0);

Missing error check.

> +        route_store->value[0] = 0;
> +        route_store->value[1] = 1;
> +        route_store->value[2] = 4;
> +        route_store->value[3] = 5;
> +        route_store->value[4] = 6;
> +        route_store->value[5] = 7;
> +        route_store->value[6] = 8;
> +        route_store->value[7] = 9;
> +        snprintf(name, sizeof(name), "Route");
> +        err = add_new_ctl(mixer, &snd_us16x08_route_ctl,
> +            0x00, 0x00, 0,
> +            USB_MIXER_U8, 1, name, (void *) route_store,
> +            snd_us16x08_free_mix_store, &elem);
> +        if (err < 0)
> +            return err;
> +
> +        snprintf(name, sizeof(name), "Master");
.....

Please use a table.

> --- /dev/null
> +++ b/sound/usb/mixer_us16x08.h
> @@ -0,0 +1,132 @@
> +#ifndef __USB_MIXER_US16X08_H
> +#define __USB_MIXER_US16X08_H
> +
> +
> +#define SND_US16X08_MIN_CHANNELS 0

Zero channel is allowed...?

> +#define SND_US16X08_MAX_CHANNELS 16
> +
> +/* set macro for kcontrol private_value */
> +#define SND_US16X08_KCSET(bias, step, min, max)  \
> +    ((bias << 24) | (step << 16) | (min << 8) | max)

Put parentheses to each argument in a macro.

> +struct snd_us16x08_fader {
> +    uint8_t value[SND_US16X08_MAX_CHANNELS];

Use u8 for the kernel code.

> +struct snd_us16x08_comp_store {
> +    int8_t valThreshold[SND_US16X08_MAX_CHANNELS];
> +    uint8_t valAttack[SND_US16X08_MAX_CHANNELS];
> +    uint8_t valRelease[SND_US16X08_MAX_CHANNELS];
> +    uint8_t valRatio[SND_US16X08_MAX_CHANNELS];
> +    uint8_t valGain[SND_US16X08_MAX_CHANNELS];
> +    uint8_t valSwitch[SND_US16X08_MAX_CHANNELS];
> +};
> +
> +struct snd_us16x08_meter_store {
> +    long meter_level[SND_US16X08_MAX_CHANNELS];
> +    long master_level[2]; /* level of meter for master output */

The size of long depends on the architecture.  Do you want 32bit or
64bit integer?  For the latter, use s64 or u64.  For the former, just
use the standard int.

> +    int comp_index;
> +    int comp_active_index;
> +    long comp_level[16]; /* compressor reduction level of current
> channel */

Ditto.

> +    struct snd_us16x08_comp_store *comp_store;
> +};
> +
> +struct snd_us16x08_bypass_store {
> +    int value;
> +};

What's the merit of this struct?

> +typedef void(*opt_free)(struct snd_kcontrol *kctl);

Don't use typedef unless really required.

> +int snd_us16x08_mix_info(struct snd_kcontrol *kcontrol,
> +    struct snd_ctl_elem_info *uinfo);
> +
> +int snd_us16x08_controls_create(struct usb_mixer_interface *mixer);
> +
> +int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size);
> +
> +int snd_us16x08_cur_channel(void);
> +
> +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 void *opt,
> +    opt_free freeer, struct usb_mixer_elem_info **elem_ret);
> +
> +int snd_us16x08_controls_create_eq(struct usb_mixer_interface *mixer);
> +int snd_us16x08_controls_create_comp(struct usb_mixer_interface *mixer);
> +
> +struct snd_us16x08_comp_store *get_comp_store(void);

Where is it defined?


Also, did you test suspend/resume?  Your code seems missing the
restore mixer callback.  Without the proper restore callback, S3 may
work, but S4 won't.


thanks,

Takashi