Re: [PATCH v2 0/3] ALSA: usb-audio: add module param device_quirk_flags

Takashi Iwai
2025-09-12T15:10:01Z

On Fri, 12 Sep 2025 08:48:57 +0200,
Cryolitia PukNgae via B4 Relay wrote:
>
> As an implementation of what has been discussed previously[1].
>
> > An open question is whether we may want yet a new module option or
> > rather extend the existing quirk option to accept the strings
> > instead. Basically, when the given argument has a colon, it's a new
> > syntax. If it's only a number, it's an old syntax, and parse like
> > before. But, I'm open for either way (a new option or extend the
> > existing one).
>
> I would like to add a new param. The existed param
> `static unsigned int quirk_flags[SNDRV_CARDS]` seems to related to
> some sequence the card probed. To be honest, I havn't fully understood
> it. And it seems hard to improve it while keeping compatibility.
>
> 1. https://lore.kernel.org/all/87h5xm5g7f.wl-tiwai@suse.de/
>
> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
> ---
> Changes in v2:
> - Cleaned up some internal rebase confusion, sorry for that
> - Link to v1: https://lore.kernel.org/r/20250912-sound-v1-0-cc9cfd9f2d01@uniontech.com
>
> ---
> Cryolitia PukNgae (3):
> ALSA: usb-audio: add two-way convert between name and bit for QUIRK_FLAG_*
> ALSA: usb-audio: add module param device_quirk_flags
> ALSA: doc: add docs about device_device_quirk_flags in snd-usb-audio

Well, what I had in mind is something like:

--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -73,7 +73,7 @@ static bool lowlatency = true;
static char *quirk_alias[SNDRV_CARDS];
static char *delayed_register[SNDRV_CARDS];
static bool implicit_fb[SNDRV_CARDS];
-static unsigned int quirk_flags[SNDRV_CARDS];
+static char *quirk_flags[SNDRV_CARDS];

bool snd_usb_use_vmalloc = true;
bool snd_usb_skip_validation;
@@ -103,8 +103,8 @@ module_param_array(delayed_register, charp, NULL, 0444);
MODULE_PARM_DESC(delayed_register, "Quirk for delayed registration, given by id:iface, e.g. 0123abcd:4.");
module_param_array(implicit_fb, bool, NULL, 0444);
MODULE_PARM_DESC(implicit_fb, "Apply generic implicit feedback sync mode.");
-module_param_array(quirk_flags, uint, NULL, 0444);
-MODULE_PARM_DESC(quirk_flags, "Driver quirk bit flags.");
+module_param_array(quirk_flags, charp, NULL, 0444);
+MODULE_PARM_DESC(quirk_flags, "Driver quirk overrides.");
module_param_named(use_vmalloc, snd_usb_use_vmalloc, bool, 0444);
MODULE_PARM_DESC(use_vmalloc, "Use vmalloc for PCM intermediate buffers (default: yes).");
module_param_named(skip_validation, snd_usb_skip_validation, bool, 0444);
@@ -692,6 +692,22 @@ static void usb_audio_make_longname(struct usb_device *dev,
}
}

+static void set_quirk_flags(struct snd_usb_audio *chip, int idx)
+{
+ int i;
+
+ /* old style option found: the position-based integer value */
+ if (quirk_flags[idx] &&
+ !kstrtou32(quirk_flags[idx], 0, &chip->quirk_flags))
+ return;
+
+ /* take the default quirk from the quirk table */
+ snd_usb_init_quirk_flags(chip);
+ /* add or correct quirk bits from options */
+ for (i = 0; i < ARRAY_SIZE(quirk_flags); i++)
+ snd_usb_apply_quirk_option(chip, quirk_flags[i]);
+}
+
/*
* create a chip instance and set its names.
*/
@@ -750,10 +766,7 @@ static int snd_usb_audio_create(struct usb_interface *intf,
INIT_LIST_HEAD(&chip->midi_v2_list);
INIT_LIST_HEAD(&chip->mixer_list);

- if (quirk_flags[idx])
- chip->quirk_flags = quirk_flags[idx];
- else
- snd_usb_init_quirk_flags(chip);
+ set_quirk_flags(chip, idx);

card->private_free = snd_usb_audio_free;

.... and snd_usb_apply_quirk_option() adds or corrects the quirk bits
based on the string value if it matches with the probed device.
This function will be similar like your parser.

In that way, the old quirk_flags options work as-is, while you can use
a new style by passing values with "X:Y:Z" style.

Takashi