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

[Takashi Iwai]

On Mon, 06 Jan 2020 02:00:39 +0100,
Nick Kossifidis wrote:
> 
> Hello Takashi, happy new year and thanks for the feedback,
> 
> 
> On Fri, Dec 27, 2019 at 10:48 AM Takashi Iwai <tiwai at 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.

Let's try in some different ways and compare how they look like.
If the current code is the best in the end, we should give some more
comments about the resultant rate tables.

> 
> > > +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.

The lock doesn't cover the two lines above fiddling with
kctl->private_value.  That's the racy point.


> > > +     .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 ?

No, it takes a different type, value.enumerated.item[].  It's an int
array, while value.integer.value[] (which is used for
SNDRV_CTL_ELEM_TYPE_BOOLEAN and SNDRV_CTL_ELEM_TYPE_INTEGER) is a long
array, hence the size differs on 64bit arch.


> 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 ?

There is a standard control name definition, see
Documentation/sound/designs/control-names.rst.  "Mic Boost" is one of
standard names, hence it's OK.  Although many drivers already don't
follow that rule, you'll still see some conceptual idea there.


thanks,

Takashi