Re: [alsa-devel] [PATCH] ALSA: usb-audio: Add support for Presonus Studio 1810c

[Nick Kossifidis <mickflemm@gmail.com>]

Hello Takashi, happy new year and thanks for the feedback,


On Fri, Dec 27, 2019 at 10:48 AM Takashi Iwai <tiwai@suse.de> wrote:
> > diff --git a/sound/usb/format.c b/sound/usb/format.c
> > index d79db7130..edf3f2a55 100644
> > --- a/sound/usb/format.c
> > +++ b/sound/usb/format.c
> > @@ -262,6 +262,23 @@ static int parse_uac2_sample_rate_range(struct snd_usb_audio *chip,
> >               }
> >
> >               for (rate = min; rate <= max; rate += res) {
> > +
> > +                     /*
> > +                      * Presonus Studio 1810c anounces invalid
> > +                      * sampling rates for its streams.
> > +                      */
> > +                     if (chip->usb_id == USB_ID(0x0194f, 0x010c) &&
> > +                     ((rate > 48000 && fp->altsetting == 1) ||
> > +                      ((rate < 88200 || rate > 96000)
> > +                       && fp->altsetting == 2) ||
> > +                      ((rate < 176400 || rate > 192000)
> > +                       && fp->altsetting == 3))) {
> > +                             if (res)
> > +                                     continue;
> > +                             else
> > +                                     break;
> > +                     }
>
> It's hard to imagine what result this would lead to, because the
> conditions are so complex.
> Maybe better to prepare a fixed table instead?  Or, can we simply take
> a fixed quirk?
>

I thought of using a fixed table but on the other hand the card does
support reporting rates via the UAC2-compliant command and I noticed
there are similar quirks on parse_audio_format_rates_v1. Maybe have a
new quirk function that we can call from within the for loop for both
UAC1/2 to filter misreported rates in one place ? I think it'll be
cleaner that way.

> > +static int
> > +snd_s1810c_switch_set(struct snd_kcontrol *kctl,
> > +                   struct snd_ctl_elem_value *ctl_elem)
> > +{
> > +     struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
> > +     struct usb_mixer_interface *mixer = list->mixer;
> > +     uint32_t curval = 0;
> > +     uint32_t newval = (uint32_t) ctl_elem->value.integer.value[0];
> > +     int ret = 0;
> > +
> > +     ret = snd_s1810c_get_switch_state(mixer, kctl, &curval);
> > +     if (ret < 0)
> > +             return 0;
> > +
> > +     if (curval == newval)
> > +             return 0;
> > +
> > +     kctl->private_value &= ~(0x1 << 16);
> > +     kctl->private_value |= (unsigned int)(newval & 0x1) << 16;
> > +     ret = snd_s1810c_set_switch_state(mixer, kctl);
>
> Hm, this can be racy.  The control get/put isn't protected, so you
> might get the inconsistency here when multiple kctls are accessed
> concurrently.
>

There is a lock on get/set_switch_state to serialize access to the
card, it should take care of that.

> > +     .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > +     .name = "Line 1/2 source type",
>
> The control name should consist of words starting with a capital
> letter, so it should be "Line 1/2 Source Type".
> Also, usually a control name needs a standard suffix.  Though, this
> has a special return enum type, so it can be OK as is.
>
> However...
>
> > +     .info = snd_s1810c_line_sw_info,
> > +     .get = snd_s1810c_switch_get,
> > +     .put = snd_s1810c_switch_set,
>
> ... this shows that the combination is invalid.  The enum type doesn't
> get/put the values in integer fields.  It's incompatible.
>

I didn't get that one, isn't enum type an integer ? All controls added
here are on/off switches and use the same get/set functions, I just
wanted to provide some more info for some of them so I added custom
.info functions.

> > +     .private_value = (SC1810C_STATE_LINE_SW | SC1810C_CTL_LINE_SW << 8)
> > +};
> > +
> > +static struct snd_kcontrol_new snd_s1810c_mute_sw = {
> > +     .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > +     .name = "Mute Main Out",
>
> ... and this one, for example, should deserve for "Switch" suffix, as
> it's a standard boolean switch (which use integer fields unlike
> enum).
>

Isn't "Switch" superfluous ? I don't see anything similar on the mixer
controls of my other sound cards, e.g. it says "Mic Boost" not "Switch
Mic Boost on". Also I still don't get the enum part, how can this be a
standard boolean switch and the others not ? You mean because it uses
snd_ctl_boolean_mono_info ?

> > +/* Entry point, called from mixer_quirks.c */
> > +int snd_sc1810_init_mixer(struct usb_mixer_interface *mixer)
> > +{
> > +     struct s1810_mixer_state *private = NULL;
> > +     struct snd_usb_audio *chip = mixer->chip;
> > +     struct usb_device *dev = chip->dev;
> > +     int ret = 0;
> > +
> > +     /* Run this only once */
> > +     if (!list_empty(&chip->mixer_list))
> > +             return 0;
> > +
> > +     dev_info(&dev->dev,
> > +              "Presonus Studio 1810c, device_setup: %u\n", chip->setup);
> > +     if (chip->setup == 1)
> > +             dev_info(&dev->dev, "(8out/18in @ 48KHz)\n");
> > +     else if (chip->setup == 2)
> > +             dev_info(&dev->dev, "(6out/8in @ 192KHz)\n");
> > +     else
> > +             dev_info(&dev->dev, "(8out/14in @ 96KHz)\n");
> > +
> > +     ret = snd_s1810c_init_mixer_maps(chip);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     private = vzalloc(sizeof(struct s1810_mixer_state));
>
> I don't think vmalloc is needed here, as the object is so small.
> Use kzalloc() and kfree() instead (unless I overlook something).
>

Good point, I'll switch to kzalloc/kfree, this will also make the bot happy.


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel