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

Cryolitia PukNgae
2025-09-15T07:45:35Z

On 12/09/2025 23.09, Takashi Iwai wrote:
> 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.
>

Thanks for your review. To be honest, I haven't understand how
`static unsigned int quirk_flags[SNDRV_CARDS]` works. e.g., based on the
current array form, how to pass a flag, and what does the index of the
array means.

Could you please explain it for me. thx.

> Takashi
>

Cryolitia PukNgae