Re: [PATCH v2 2/2] ALSA: usb-audio: Add quirk for Plantronics headsets to fix control names

[Takashi Iwai <tiwai@suse.de>]

On Tue, 24 Dec 2024 07:56:36 +0100,
Wade Wang wrote:
> 
> From: Terry Junge <linuxhid@cosmicgizmosystems.com>
> 
> Many Poly/Plantronics headset families name the feature, input,
> and/or output units in a such a way to produce control names
> that are not recognized by user space. As such, the volume and
> mute events do not get routed to the headset's audio controls.
> 
> As an example from a product family:
> 
> The microphone mute control is named
> Headset Microphone Capture Switch
> and the headset volume control is named
> Headset Earphone Playback Volume
> 
> The quirk fixes these to become
> Headset Capture Switch
> Headset Playback Volume
> 
> Signed-off-by: Terry Junge <linuxhid@cosmicgizmosystems.com>
> Signed-off-by: Wade Wang <wade.wang@hp.com>
> Cc: stable@vger.kernel.org
> ---
> V1 -> V2: Add comments, usb_audio_dbg() calls, fix leading space case
> 
>  sound/usb/mixer_quirks.c | 66 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index 23fcd680167d..5728c03dc49f 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -4216,6 +4216,67 @@ static void snd_dragonfly_quirk_db_scale(struct usb_mixer_interface *mixer,
>  	}
>  }
>  
> +/*
> + * Some Plantronics headsets have control names that don't meet ALSA naming
> + * standards. This function removes nonstandard source names. By the time
> + * this function is called the control name will look like one of these:
> + * "source names Playback Volume"
> + * "source names Playback Switch"
> + * "source names Capture Volume"
> + * "source names Capture Switch"
> + * First it scans through the list and removes any found name(s) by moving the
> + * remaining string and its null terminator over the found name and its leading
> + * space, if it has one.
> + * Second, if all source names were removed, it puts back "Headset"
> + * otherwise removes a leading space, if there is one.
> + */
> +static void snd_fix_plt_control_name(struct usb_mixer_interface *mixer,
> +				     struct snd_kcontrol *kctl)
> +{
> +	/* no variant of "Sidetone" should be added to this list */
> +	static const char * const names_to_remove[] = {
> +		"Earphone",
> +		"Microphone",
> +		"Receive",
> +		"Transmit",
> +		NULL
> +	};
> +	const char * const *n2r;
> +	char *dst, *src, *last = NULL;
> +	size_t len = 0;
> +
> +	for (n2r = names_to_remove; *n2r; ++n2r) {
> +		dst = strstr(kctl->id.name, *n2r);
> +		if (dst) {
> +			usb_audio_dbg(mixer->chip, "found %s in %s\n",
> +					*n2r, kctl->id.name);
> +			src = dst + strlen(*n2r);
> +			len = strlen(src) + 1;
> +			if ((char *)kctl->id.name != dst && *(dst - 1) == ' ')
> +				--dst;
> +			last = memmove(dst, src, len);
> +			usb_audio_dbg(mixer->chip, "now %s\n", kctl->id.name);
> +		}
> +	}
> +	if (!len) {
> +		usb_audio_dbg(mixer->chip, "no change in %s\n", kctl->id.name);
> +		return;
> +	}
> +	if (len <= sizeof " Playback Volume" && (char *)kctl->id.name == last) {
> +		char rcat[sizeof(kctl->id.name)] = { "Headset" };
> +
> +		strlcat(rcat, kctl->id.name, sizeof(rcat));
> +		strscpy(kctl->id.name, rcat, sizeof(kctl->id.name));
> +		usb_audio_dbg(mixer->chip, "now %s\n", kctl->id.name);
> +	} else if (kctl->id.name[0] == ' ') {
> +		dst = kctl->id.name;
> +		src = dst + 1;
> +		len = strlen(src) + 1;
> +		memmove(dst, src, len);
> +		usb_audio_dbg(mixer->chip, "now %s\n", kctl->id.name);
> +	}
> +}

Thanks to your updated comments, it's a bit better understandable
now.  However, IMO, it's still too complex than needed.

Basically what we want is to make those kctl names just like "Headset
Playback Switch" from the original "Earphone Headset Playback Switch".
If so, a simpler code would be something like:

static void fix_plantronics_control_name(struct usb_mixer_interface *mixer,
					 struct snd_kcontrol *kctl)
{
	static const char * const prefix_to_match[] = {
		"Headset", "Earphone", "Microphone", "Receive", "Transmit"
	};
	static const char * const suffix[] = {
		"Playback Volume", "Playback Switch",
		"Capture Volume", "Capture Switch"
	};
	int i;
	
	for (i = 0; i < ARRAY_SIZE(prefix_to_match); i++) {
		if (strstr(kctl->id.name, prefix_to_match[i]))
			break;
	}
	if (i >= ARRAY_SIZE(prefix_to_match))
		return;

	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
		if (strstr(kctl->id.name, suffix[i])) {
			usb_audio_dbg(mixer->chip, "fix kctl name %s\n",
				      kctl->id.name);
			sprintf(kctl->id.name, "Headset %s", suffix[i]);
			return;
		}
	}
}


One may put a space around the word if we want to make sure that it's
a separated word, but I hope you get the idea by the example above.
This is no hot code path and it runs only once at probe, so the code
readability and understandability are much more important than
efficiency, after all.


thanks,

Takashi