Hello Takashi, happy new year and thanks for the feedback,
On Fri, Dec 27, 2019 at 10:48 AM Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
