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@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;elsebreak;}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