[PATCH 0/2] Introduce Poly/Plantronics mute event support

[PATCH 0/2] Introduce Poly/Plantronics mute event support

[Terry Junge]

Hi Jiri and Takashi,

I'm not sure how it works with two different maintained trees
but this patch set needs to be applied and flow upstream together.

If the HID patch is applied without the ALSA patch then mute sync
issues will occur with multiple Poly/Plantronics product families.

This patch set was tested by Wade and myself with multiple
Poly/Plantronics product family headsets.

Hi Wade,

Please feel free to add your Signed-off-by: and/or Tested-by: tags,
as you see fit.

Thanks,
Terry

Terry Junge (2):
  HID: hid-plantronics: Add mic mute mapping and generalize quirks
  ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
    names

 drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
 sound/usb/mixer_quirks.c      |  35 +++++++++
 2 files changed, 100 insertions(+), 70 deletions(-)


base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
-- 
2.43.0

[PATCH 1/2] HID: hid-plantronics: Add mic mute mapping and generalize quirks

[Terry Junge]

Added mapping for headset mute key events.

Removed PLT_QUIRK_DOUBLE_VOLUME_KEYS quirk and made it generic.
The quirk logic did not keep track of the actual previous key
so any key event occurring in less than or equal to 5ms was ignored.

Removed PLT_QUIRK_FOLLOWED_OPPOSITE_VOLUME_KEYS quirk.
It had the same logic issue as the double key quirk and was actually
masking the as designed behavior of most of the headsets.
It's occurrence should be minimized with the ALSA control naming
quirk that is part of the patch set.

Signed-off-by: Terry Junge <linuxhid@cosmicgizmosystems.com>
---
 drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
 1 file changed, 65 insertions(+), 70 deletions(-)

diff --git a/drivers/hid/hid-plantronics.c b/drivers/hid/hid-plantronics.c
index 25cfd964dc25..06f9f3eef61c 100644
--- a/drivers/hid/hid-plantronics.c
+++ b/drivers/hid/hid-plantronics.c
@@ -23,30 +23,34 @@
 
 #define PLT_VOL_UP		0x00b1
 #define PLT_VOL_DOWN		0x00b2
+#define PLT_MIC_MUTE		0x00b5
 
 #define PLT1_VOL_UP		(PLT_HID_1_0_PAGE | PLT_VOL_UP)
 #define PLT1_VOL_DOWN		(PLT_HID_1_0_PAGE | PLT_VOL_DOWN)
+#define PLT1_MIC_MUTE		(PLT_HID_1_0_PAGE | PLT_MIC_MUTE)
 #define PLT2_VOL_UP		(PLT_HID_2_0_PAGE | PLT_VOL_UP)
 #define PLT2_VOL_DOWN		(PLT_HID_2_0_PAGE | PLT_VOL_DOWN)
+#define PLT2_MIC_MUTE		(PLT_HID_2_0_PAGE | PLT_MIC_MUTE)
+#define HID_TELEPHONY_MUTE	(HID_UP_TELEPHONY | 0x2f)
+#define HID_CONSUMER_MUTE	(HID_UP_CONSUMER | 0xe2)
 
 #define PLT_DA60		0xda60
 #define PLT_BT300_MIN		0x0413
 #define PLT_BT300_MAX		0x0418
 
-
 #define PLT_ALLOW_CONSUMER (field->application == HID_CP_CONSUMERCONTROL && \
-			    (usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER)
+			   (usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER && \
+			   usage->hid != HID_CONSUMER_MUTE)
 
-#define PLT_QUIRK_DOUBLE_VOLUME_KEYS BIT(0)
-#define PLT_QUIRK_FOLLOWED_OPPOSITE_VOLUME_KEYS BIT(1)
+#define PLT_ALLOW_MUTE (usage->hid == HID_TELEPHONY_MUTE)
 
 #define PLT_DOUBLE_KEY_TIMEOUT 5 /* ms */
-#define PLT_FOLLOWED_OPPOSITE_KEY_TIMEOUT 220 /* ms */
 
 struct plt_drv_data {
 	unsigned long device_type;
-	unsigned long last_volume_key_ts;
-	u32 quirks;
+	unsigned long last_key_ts;
+	unsigned long double_key_to;
+	__u16 last_key;
 };
 
 static int plantronics_input_mapping(struct hid_device *hdev,
@@ -63,29 +67,34 @@ static int plantronics_input_mapping(struct hid_device *hdev,
 	if (field->application == HID_GD_JOYSTICK)
 		goto defaulted;
 
-	/* handle volume up/down mapping */
 	/* non-standard types or multi-HID interfaces - plt_type is PID */
 	if (!(plt_type & HID_USAGE_PAGE)) {
 		switch (plt_type) {
 		case PLT_DA60:
 			if (PLT_ALLOW_CONSUMER)
 				goto defaulted;
-			goto ignored;
+			if (usage->hid == HID_CONSUMER_MUTE) {
+				mapped_key = KEY_MICMUTE;
+				goto mapped;
+			}
+			break;
 		default:
-			if (PLT_ALLOW_CONSUMER)
+			if (PLT_ALLOW_CONSUMER || PLT_ALLOW_MUTE)
 				goto defaulted;
 		}
+		goto ignored;
 	}
-	/* handle standard types - plt_type is 0xffa0uuuu or 0xffa2uuuu */
-	/* 'basic telephony compliant' - allow default consumer page map */
-	else if ((plt_type & HID_USAGE) >= PLT_BASIC_TELEPHONY &&
-		 (plt_type & HID_USAGE) != PLT_BASIC_EXCEPTION) {
-		if (PLT_ALLOW_CONSUMER)
-			goto defaulted;
-	}
-	/* not 'basic telephony' - apply legacy mapping */
-	/* only map if the field is in the device's primary vendor page */
-	else if (!((field->application ^ plt_type) & HID_USAGE_PAGE)) {
+
+	/* handle standard consumer control mapping */
+	/* and standard telephony mic mute mapping */
+	if (PLT_ALLOW_CONSUMER || PLT_ALLOW_MUTE)
+		goto defaulted;
+
+	/* handle vendor unique types - plt_type is 0xffa0uuuu or 0xffa2uuuu */
+	/* if not 'basic telephony compliant' - map vendor unique controls */
+	if (!((plt_type & HID_USAGE) >= PLT_BASIC_TELEPHONY &&
+	      (plt_type & HID_USAGE) != PLT_BASIC_EXCEPTION) &&
+	      !((field->application ^ plt_type) & HID_USAGE_PAGE))
 		switch (usage->hid) {
 		case PLT1_VOL_UP:
 		case PLT2_VOL_UP:
@@ -95,8 +104,11 @@ static int plantronics_input_mapping(struct hid_device *hdev,
 		case PLT2_VOL_DOWN:
 			mapped_key = KEY_VOLUMEDOWN;
 			goto mapped;
+		case PLT1_MIC_MUTE:
+		case PLT2_MIC_MUTE:
+			mapped_key = KEY_MICMUTE;
+			goto mapped;
 		}
-	}
 
 /*
  * Future mapping of call control or other usages,
@@ -105,6 +117,8 @@ static int plantronics_input_mapping(struct hid_device *hdev,
  */
 
 ignored:
+	hid_dbg(hdev, "usage: %08x (appl: %08x) - ignored\n",
+		usage->hid, field->application);
 	return -1;
 
 defaulted:
@@ -123,38 +137,26 @@ static int plantronics_event(struct hid_device *hdev, struct hid_field *field,
 			     struct hid_usage *usage, __s32 value)
 {
 	struct plt_drv_data *drv_data = hid_get_drvdata(hdev);
+	unsigned long prev_tsto, cur_ts;
+	__u16 prev_key, cur_key;
 
-	if (drv_data->quirks & PLT_QUIRK_DOUBLE_VOLUME_KEYS) {
-		unsigned long prev_ts, cur_ts;
-
-		/* Usages are filtered in plantronics_usages. */
+	/* Usages are filtered in plantronics_usages. */
 
-		if (!value) /* Handle key presses only. */
-			return 0;
+	/* HZ too low for ms resolution - double key detection disabled */
+	/* or it is a key release - handle key presses only. */
+	if (!drv_data->double_key_to || !value)
+		return 0;
 
-		prev_ts = drv_data->last_volume_key_ts;
-		cur_ts = jiffies;
-		if (jiffies_to_msecs(cur_ts - prev_ts) <= PLT_DOUBLE_KEY_TIMEOUT)
-			return 1; /* Ignore the repeated key. */
+	prev_tsto = drv_data->last_key_ts + drv_data->double_key_to;
+	cur_ts = drv_data->last_key_ts = jiffies;
+	prev_key = drv_data->last_key;
+	cur_key = drv_data->last_key = usage->code;
 
-		drv_data->last_volume_key_ts = cur_ts;
+	/* If the same key occurs in <= double_key_to -- ignore it */
+	if (prev_key == cur_key && time_before_eq(cur_ts, prev_tsto)) {
+		hid_dbg(hdev, "double key %d ignored\n", cur_key);
+		return 1; /* Ignore the repeated key. */
 	}
-	if (drv_data->quirks & PLT_QUIRK_FOLLOWED_OPPOSITE_VOLUME_KEYS) {
-		unsigned long prev_ts, cur_ts;
-
-		/* Usages are filtered in plantronics_usages. */
-
-		if (!value) /* Handle key presses only. */
-			return 0;
-
-		prev_ts = drv_data->last_volume_key_ts;
-		cur_ts = jiffies;
-		if (jiffies_to_msecs(cur_ts - prev_ts) <= PLT_FOLLOWED_OPPOSITE_KEY_TIMEOUT)
-			return 1; /* Ignore the followed opposite volume key. */
-
-		drv_data->last_volume_key_ts = cur_ts;
-	}
-
 	return 0;
 }
 
@@ -196,12 +198,16 @@ static int plantronics_probe(struct hid_device *hdev,
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");
-		goto err;
+		return ret;
 	}
 
 	drv_data->device_type = plantronics_device_type(hdev);
-	drv_data->quirks = id->driver_data;
-	drv_data->last_volume_key_ts = jiffies - msecs_to_jiffies(PLT_DOUBLE_KEY_TIMEOUT);
+	drv_data->double_key_to = msecs_to_jiffies(PLT_DOUBLE_KEY_TIMEOUT);
+	drv_data->last_key_ts = jiffies - drv_data->double_key_to;
+
+	/* if HZ does not allow ms resolution - disable double key detection */
+	if (drv_data->double_key_to < PLT_DOUBLE_KEY_TIMEOUT)
+		drv_data->double_key_to = 0;
 
 	hid_set_drvdata(hdev, drv_data);
 
@@ -210,29 +216,10 @@ static int plantronics_probe(struct hid_device *hdev,
 	if (ret)
 		hid_err(hdev, "hw start failed\n");
 
-err:
 	return ret;
 }
 
 static const struct hid_device_id plantronics_devices[] = {
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS,
-					 USB_DEVICE_ID_PLANTRONICS_BLACKWIRE_3210_SERIES),
-		.driver_data = PLT_QUIRK_DOUBLE_VOLUME_KEYS },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS,
-					 USB_DEVICE_ID_PLANTRONICS_BLACKWIRE_3220_SERIES),
-		.driver_data = PLT_QUIRK_DOUBLE_VOLUME_KEYS },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS,
-					 USB_DEVICE_ID_PLANTRONICS_BLACKWIRE_3215_SERIES),
-		.driver_data = PLT_QUIRK_DOUBLE_VOLUME_KEYS },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS,
-					 USB_DEVICE_ID_PLANTRONICS_BLACKWIRE_3225_SERIES),
-		.driver_data = PLT_QUIRK_DOUBLE_VOLUME_KEYS },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS,
-					 USB_DEVICE_ID_PLANTRONICS_BLACKWIRE_3325_SERIES),
-		.driver_data = PLT_QUIRK_FOLLOWED_OPPOSITE_VOLUME_KEYS },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS,
-					 USB_DEVICE_ID_PLANTRONICS_ENCOREPRO_500_SERIES),
-		.driver_data = PLT_QUIRK_FOLLOWED_OPPOSITE_VOLUME_KEYS },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
 	{ }
 };
@@ -241,6 +228,14 @@ MODULE_DEVICE_TABLE(hid, plantronics_devices);
 static const struct hid_usage_id plantronics_usages[] = {
 	{ HID_CP_VOLUMEUP, EV_KEY, HID_ANY_ID },
 	{ HID_CP_VOLUMEDOWN, EV_KEY, HID_ANY_ID },
+	{ HID_TELEPHONY_MUTE, EV_KEY, HID_ANY_ID },
+	{ HID_CONSUMER_MUTE, EV_KEY, HID_ANY_ID },
+	{ PLT2_VOL_UP, EV_KEY, HID_ANY_ID },
+	{ PLT2_VOL_DOWN, EV_KEY, HID_ANY_ID },
+	{ PLT2_MIC_MUTE, EV_KEY, HID_ANY_ID },
+	{ PLT1_VOL_UP, EV_KEY, HID_ANY_ID },
+	{ PLT1_VOL_DOWN, EV_KEY, HID_ANY_ID },
+	{ PLT1_MIC_MUTE, EV_KEY, HID_ANY_ID },
 	{ HID_TERMINATOR, HID_TERMINATOR, HID_TERMINATOR }
 };
 
-- 
2.43.0

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

[Terry Junge]

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>
---
 sound/usb/mixer_quirks.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 8bbf070b3676..20d63efd5498 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -4215,6 +4215,37 @@ static void snd_dragonfly_quirk_db_scale(struct usb_mixer_interface *mixer,
 	}
 }
 
+static void snd_fix_plt_control_name(struct snd_kcontrol *kctl)
+{
+	static const char * const names_to_remove[] = {
+		"Earphone",
+		"Microphone",
+		"Receive",
+		"Transmit",
+		NULL
+	};
+	const char * const *n2r;
+	char *dst, *src;
+	size_t len;
+
+	for (n2r = names_to_remove; *n2r; ++n2r) {
+		dst = strstr(kctl->id.name, *n2r);
+		if (dst) {
+			src = dst + strlen(*n2r);
+			len = strlen(src) + 1;
+			if ((char *)kctl->id.name != dst && *(dst - 1) == ' ')
+				--dst;
+			memmove(dst, src, len);
+		}
+	}
+	if (kctl->id.name[0] == ' ') {
+		char rcat[sizeof(kctl->id.name)] = { "Headset" };
+
+		strlcat(rcat, kctl->id.name, sizeof(rcat));
+		strscpy(kctl->id.name, rcat, sizeof(kctl->id.name));
+	}
+}
+
 void snd_usb_mixer_fu_apply_quirk(struct usb_mixer_interface *mixer,
 				  struct usb_mixer_elem_info *cval, int unitid,
 				  struct snd_kcontrol *kctl)
@@ -4232,5 +4263,9 @@ void snd_usb_mixer_fu_apply_quirk(struct usb_mixer_interface *mixer,
 			cval->min_mute = 1;
 		break;
 	}
+
+	/* ALSA-ify some Plantronics headset control names */
+	if (USB_ID_VENDOR(mixer->chip->usb_id) == 0x047f)
+		snd_fix_plt_control_name(kctl);
 }
 
-- 
2.43.0

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

[Takashi Iwai]

On Sun, 24 Nov 2024 21:32:41 +0100,
Terry Junge wrote:
> 
> 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>

Thanks, this description looks much more understandable now.
Meanwhile...

> ---
>  sound/usb/mixer_quirks.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index 8bbf070b3676..20d63efd5498 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -4215,6 +4215,37 @@ static void snd_dragonfly_quirk_db_scale(struct usb_mixer_interface *mixer,
>  	}
>  }
>  
> +static void snd_fix_plt_control_name(struct snd_kcontrol *kctl)
> +{
> +	static const char * const names_to_remove[] = {
> +		"Earphone",
> +		"Microphone",
> +		"Receive",
> +		"Transmit",
> +		NULL
> +	};
> +	const char * const *n2r;
> +	char *dst, *src;
> +	size_t len;
> +
> +	for (n2r = names_to_remove; *n2r; ++n2r) {
> +		dst = strstr(kctl->id.name, *n2r);
> +		if (dst) {
> +			src = dst + strlen(*n2r);
> +			len = strlen(src) + 1;
> +			if ((char *)kctl->id.name != dst && *(dst - 1) == ' ')
> +				--dst;
> +			memmove(dst, src, len);
> +		}
> +	}
> +	if (kctl->id.name[0] == ' ') {
> +		char rcat[sizeof(kctl->id.name)] = { "Headset" };
> +
> +		strlcat(rcat, kctl->id.name, sizeof(rcat));
> +		strscpy(kctl->id.name, rcat, sizeof(kctl->id.name));
> +	}
> +}

... the code itself isn't really trivial (due to the poor string
handling by nature of C language), so it's better to put some brief
comment what this function really does, too.


thanks,

Takashi

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

[Terry Junge]

On 11/25/24 1:25 AM, Takashi Iwai wrote:
> On Sun, 24 Nov 2024 21:32:41 +0100,
> Terry Junge wrote:
>>
>> 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>
> 
> Thanks, this description looks much more understandable now.
> Meanwhile...
> 
>> ---
>>  sound/usb/mixer_quirks.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
>> index 8bbf070b3676..20d63efd5498 100644
>> --- a/sound/usb/mixer_quirks.c
>> +++ b/sound/usb/mixer_quirks.c
>> @@ -4215,6 +4215,37 @@ static void snd_dragonfly_quirk_db_scale(struct usb_mixer_interface *mixer,
>>  	}
>>  }
>>  
>> +static void snd_fix_plt_control_name(struct snd_kcontrol *kctl)
>> +{
>> +	static const char * const names_to_remove[] = {
>> +		"Earphone",
>> +		"Microphone",
>> +		"Receive",
>> +		"Transmit",
>> +		NULL
>> +	};
>> +	const char * const *n2r;
>> +	char *dst, *src;
>> +	size_t len;
>> +
>> +	for (n2r = names_to_remove; *n2r; ++n2r) {
>> +		dst = strstr(kctl->id.name, *n2r);
>> +		if (dst) {
>> +			src = dst + strlen(*n2r);
>> +			len = strlen(src) + 1;
>> +			if ((char *)kctl->id.name != dst && *(dst - 1) == ' ')
>> +				--dst;
>> +			memmove(dst, src, len);
>> +		}
>> +	}
>> +	if (kctl->id.name[0] == ' ') {
>> +		char rcat[sizeof(kctl->id.name)] = { "Headset" };
>> +
>> +		strlcat(rcat, kctl->id.name, sizeof(rcat));
>> +		strscpy(kctl->id.name, rcat, sizeof(kctl->id.name));
>> +	}
>> +}
> 
> ... the code itself isn't really trivial (due to the poor string
> handling by nature of C language), so it's better to put some brief
> comment what this function really does, too.

Thanks Takashi,

I'll comment the function in v2 and send it out.

> 
> 
> thanks,
> 
> Takashi

RE: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Wang, Wade]

Hi Terry,

Thet are OK for me, because my outlook will change patch format, would you mind to add these info for me directly. Thanks

Regards
Wade

-----Original Message-----
From: Terry Junge <linuxhid@cosmicgizmosystems.com> 
Sent: Monday, November 25, 2024 4:33 AM
To: Jiri Kosina <jikos@kernel.org>; Takashi Iwai <tiwai@suse.com>; Wang, Wade <wade.wang@hp.com>; Benjamin Tissoires <bentiss@kernel.org>; Jaroslav Kysela <perex@perex.cz>
Cc: Terry Junge <linuxhid@cosmicgizmosystems.com>; linux-input@vger.kernel.org; linux-sound@vger.kernel.org
Subject: [PATCH 0/2] Introduce Poly/Plantronics mute event support

CAUTION: External Email

Hi Jiri and Takashi,

I'm not sure how it works with two different maintained trees but this patch set needs to be applied and flow upstream together.

If the HID patch is applied without the ALSA patch then mute sync issues will occur with multiple Poly/Plantronics product families.

This patch set was tested by Wade and myself with multiple Poly/Plantronics product family headsets.

Hi Wade,

Please feel free to add your Signed-off-by: and/or Tested-by: tags, as you see fit.

Thanks,
Terry

Terry Junge (2):
  HID: hid-plantronics: Add mic mute mapping and generalize quirks
  ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
    names

 drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
 sound/usb/mixer_quirks.c      |  35 +++++++++
 2 files changed, 100 insertions(+), 70 deletions(-)


base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
--
2.43.0

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Takashi Iwai]

On Sun, 24 Nov 2024 21:32:39 +0100,
Terry Junge wrote:
> 
> Hi Jiri and Takashi,
> 
> I'm not sure how it works with two different maintained trees
> but this patch set needs to be applied and flow upstream together.
> 
> If the HID patch is applied without the ALSA patch then mute sync
> issues will occur with multiple Poly/Plantronics product families.

Both patches can be applied individually, and even if only one of them
is applied, it won't hurt.  So I guess both subsystems can take the
corresponding one at any time.


thanks,

Takashi

> 
> This patch set was tested by Wade and myself with multiple
> Poly/Plantronics product family headsets.
> 
> Hi Wade,
> 
> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
> as you see fit.
> 
> Thanks,
> Terry
> 
> Terry Junge (2):
>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
>     names
> 
>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
>  sound/usb/mixer_quirks.c      |  35 +++++++++
>  2 files changed, 100 insertions(+), 70 deletions(-)
> 
> 
> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
> -- 
> 2.43.0
> 

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Terry Junge]

On 11/25/24 12:55 AM, Takashi Iwai wrote:
> On Sun, 24 Nov 2024 21:32:39 +0100,
> Terry Junge wrote:
>>
>> Hi Jiri and Takashi,
>>
>> I'm not sure how it works with two different maintained trees
>> but this patch set needs to be applied and flow upstream together.
>>
>> If the HID patch is applied without the ALSA patch then mute sync
>> issues will occur with multiple Poly/Plantronics product families.
> 
> Both patches can be applied individually, and even if only one of them
> is applied, it won't hurt.  So I guess both subsystems can take the
> corresponding one at any time.
> 

Hi Takashi,

I've tested out the behavior with each patch individually applied and
have found that, IMHO, the mute functionality and synchronization is
worse than the current behavior with neither patch. However, with both
patches applied the mixer UI microphone mute control and the headset
mute button are fully synchronized.

There must be a way that both patches can be tied together and be
applied simultaneously.

If it would help, and be allowed, I can submit a single patch that
contains both changes.

Thanks,
Terry

> 
> thanks,
> 
> Takashi
> 
>>
>> This patch set was tested by Wade and myself with multiple
>> Poly/Plantronics product family headsets.
>>
>> Hi Wade,
>>
>> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
>> as you see fit.
>>
>> Thanks,
>> Terry
>>
>> Terry Junge (2):
>>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
>>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
>>     names
>>
>>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
>>  sound/usb/mixer_quirks.c      |  35 +++++++++
>>  2 files changed, 100 insertions(+), 70 deletions(-)
>>
>>
>> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
>> -- 
>> 2.43.0
>>

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Takashi Iwai]

On Mon, 02 Dec 2024 23:35:51 +0100,
Terry Junge wrote:
> 
> On 11/25/24 12:55 AM, Takashi Iwai wrote:
> > On Sun, 24 Nov 2024 21:32:39 +0100,
> > Terry Junge wrote:
> >>
> >> Hi Jiri and Takashi,
> >>
> >> I'm not sure how it works with two different maintained trees
> >> but this patch set needs to be applied and flow upstream together.
> >>
> >> If the HID patch is applied without the ALSA patch then mute sync
> >> issues will occur with multiple Poly/Plantronics product families.
> > 
> > Both patches can be applied individually, and even if only one of them
> > is applied, it won't hurt.  So I guess both subsystems can take the
> > corresponding one at any time.
> > 
> 
> Hi Takashi,
> 
> I've tested out the behavior with each patch individually applied and
> have found that, IMHO, the mute functionality and synchronization is
> worse than the current behavior with neither patch. However, with both
> patches applied the mixer UI microphone mute control and the headset
> mute button are fully synchronized.

That's odd.  How can it worsen?  As far as I understand from the patch
descriptions, the USB-audio patch corrects only the mixer volume
control names, while the HID patch changes the quirk to be generalized
(to be dropped the next key in a short period).  If only USB audio
patch is applied, it doesn't matter as the volume binding didn't
happen before the patch.  OTOH, ditto, if only HID patch is applied.
Am I missing anything here?

> There must be a way that both patches can be tied together and be
> applied simultaneously.
> 
> If it would help, and be allowed, I can submit a single patch that
> contains both changes.

Applying both from the single tree is possible, sure.  One of two
needs an ack from the subsystem maintainers.


thanks,

Takashi

> 
> Thanks,
> Terry
> 
> > 
> > thanks,
> > 
> > Takashi
> > 
> >>
> >> This patch set was tested by Wade and myself with multiple
> >> Poly/Plantronics product family headsets.
> >>
> >> Hi Wade,
> >>
> >> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
> >> as you see fit.
> >>
> >> Thanks,
> >> Terry
> >>
> >> Terry Junge (2):
> >>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
> >>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
> >>     names
> >>
> >>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
> >>  sound/usb/mixer_quirks.c      |  35 +++++++++
> >>  2 files changed, 100 insertions(+), 70 deletions(-)
> >>
> >>
> >> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
> >> -- 
> >> 2.43.0
> >>
> 

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Terry Junge]

On 12/2/24 11:32 PM, Takashi Iwai wrote:
> On Mon, 02 Dec 2024 23:35:51 +0100,
> Terry Junge wrote:
>>
>> On 11/25/24 12:55 AM, Takashi Iwai wrote:
>>> On Sun, 24 Nov 2024 21:32:39 +0100,
>>> Terry Junge wrote:
>>>>
>>>> Hi Jiri and Takashi,
>>>>
>>>> I'm not sure how it works with two different maintained trees
>>>> but this patch set needs to be applied and flow upstream together.
>>>>
>>>> If the HID patch is applied without the ALSA patch then mute sync
>>>> issues will occur with multiple Poly/Plantronics product families.
>>>
>>> Both patches can be applied individually, and even if only one of them
>>> is applied, it won't hurt.  So I guess both subsystems can take the
>>> corresponding one at any time.
>>>
>>
>> Hi Takashi,
>>
>> I've tested out the behavior with each patch individually applied and
>> have found that, IMHO, the mute functionality and synchronization is
>> worse than the current behavior with neither patch. However, with both
>> patches applied the mixer UI microphone mute control and the headset
>> mute button are fully synchronized.
> 
> That's odd.  How can it worsen?  As far as I understand from the patch
> descriptions, the USB-audio patch corrects only the mixer volume
> control names, while the HID patch changes the quirk to be generalized
> (to be dropped the next key in a short period).  If only USB audio
> patch is applied, it doesn't matter as the volume binding didn't
> happen before the patch.  OTOH, ditto, if only HID patch is applied.
> Am I missing anything here?

The USB-audio patch also corrects the names for the mixer switch controls.
The HID patch also adds mapping of the mute button to the KEY_MICMUTE event.
It's not the playback volume handling that gets worse, it's the capture
switch handling that gets worse.

The USB-audio patch allows the mixer to bind to the headset's mute control in
the feature unit and mute/un-mute the headset microphone while the HID patch
allows the headset to request the mixer to toggle the microphone mute state.

Both patches are needed for microphone mute synchronization to work.

thanks,

Terry

> 
>> There must be a way that both patches can be tied together and be
>> applied simultaneously.
>>
>> If it would help, and be allowed, I can submit a single patch that
>> contains both changes.
> 
> Applying both from the single tree is possible, sure.  One of two
> needs an ack from the subsystem maintainers.
> 
> 
> thanks,
> 
> Takashi
> 
>>
>> Thanks,
>> Terry
>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>>
>>>> This patch set was tested by Wade and myself with multiple
>>>> Poly/Plantronics product family headsets.
>>>>
>>>> Hi Wade,
>>>>
>>>> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
>>>> as you see fit.
>>>>
>>>> Thanks,
>>>> Terry
>>>>
>>>> Terry Junge (2):
>>>>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
>>>>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
>>>>     names
>>>>
>>>>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
>>>>  sound/usb/mixer_quirks.c      |  35 +++++++++
>>>>  2 files changed, 100 insertions(+), 70 deletions(-)
>>>>
>>>>
>>>> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
>>>> -- 
>>>> 2.43.0
>>>>
>>

RE: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Wang, Wade]

Hi Terry,

If only USB-audio patch is applied, the situation of mute key is not worse than the current, at least, volume adjustment issue can be fixed. So it should be OK to apply USB audio patch first.

Regards
Wade

-----Original Message-----
From: Terry Junge <linuxhid@cosmicgizmosystems.com> 
Sent: Wednesday, December 4, 2024 1:24 PM
To: Takashi Iwai <tiwai@suse.de>
Cc: Jiri Kosina <jikos@kernel.org>; Takashi Iwai <tiwai@suse.com>; Wang, Wade <wade.wang@hp.com>; Benjamin Tissoires <bentiss@kernel.org>; Jaroslav Kysela <perex@perex.cz>; linux-input@vger.kernel.org; linux-sound@vger.kernel.org
Subject: Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

CAUTION: External Email

On 12/2/24 11:32 PM, Takashi Iwai wrote:
> On Mon, 02 Dec 2024 23:35:51 +0100,
> Terry Junge wrote:
>>
>> On 11/25/24 12:55 AM, Takashi Iwai wrote:
>>> On Sun, 24 Nov 2024 21:32:39 +0100,
>>> Terry Junge wrote:
>>>>
>>>> Hi Jiri and Takashi,
>>>>
>>>> I'm not sure how it works with two different maintained trees
>>>> but this patch set needs to be applied and flow upstream together.
>>>>
>>>> If the HID patch is applied without the ALSA patch then mute sync
>>>> issues will occur with multiple Poly/Plantronics product families.
>>>
>>> Both patches can be applied individually, and even if only one of them
>>> is applied, it won't hurt.  So I guess both subsystems can take the
>>> corresponding one at any time.
>>>
>>
>> Hi Takashi,
>>
>> I've tested out the behavior with each patch individually applied and
>> have found that, IMHO, the mute functionality and synchronization is
>> worse than the current behavior with neither patch. However, with both
>> patches applied the mixer UI microphone mute control and the headset
>> mute button are fully synchronized.
>
> That's odd.  How can it worsen?  As far as I understand from the patch
> descriptions, the USB-audio patch corrects only the mixer volume
> control names, while the HID patch changes the quirk to be generalized
> (to be dropped the next key in a short period).  If only USB audio
> patch is applied, it doesn't matter as the volume binding didn't
> happen before the patch.  OTOH, ditto, if only HID patch is applied.
> Am I missing anything here?

The USB-audio patch also corrects the names for the mixer switch controls.
The HID patch also adds mapping of the mute button to the KEY_MICMUTE event.
It's not the playback volume handling that gets worse, it's the capture
switch handling that gets worse.

The USB-audio patch allows the mixer to bind to the headset's mute control in
the feature unit and mute/un-mute the headset microphone while the HID patch
allows the headset to request the mixer to toggle the microphone mute state.

Both patches are needed for microphone mute synchronization to work.

thanks,

Terry

>
>> There must be a way that both patches can be tied together and be
>> applied simultaneously.
>>
>> If it would help, and be allowed, I can submit a single patch that
>> contains both changes.
>
> Applying both from the single tree is possible, sure.  One of two
> needs an ack from the subsystem maintainers.
>
>
> thanks,
>
> Takashi
>
>>
>> Thanks,
>> Terry
>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>>
>>>> This patch set was tested by Wade and myself with multiple
>>>> Poly/Plantronics product family headsets.
>>>>
>>>> Hi Wade,
>>>>
>>>> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
>>>> as you see fit.
>>>>
>>>> Thanks,
>>>> Terry
>>>>
>>>> Terry Junge (2):
>>>>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
>>>>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
>>>>     names
>>>>
>>>>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
>>>>  sound/usb/mixer_quirks.c      |  35 +++++++++
>>>>  2 files changed, 100 insertions(+), 70 deletions(-)
>>>>
>>>>
>>>> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
>>>> --
>>>> 2.43.0
>>>>
>>

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Takashi Iwai]

On Wed, 04 Dec 2024 06:23:30 +0100,
Terry Junge wrote:
> 
> On 12/2/24 11:32 PM, Takashi Iwai wrote:
> > On Mon, 02 Dec 2024 23:35:51 +0100,
> > Terry Junge wrote:
> >>
> >> On 11/25/24 12:55 AM, Takashi Iwai wrote:
> >>> On Sun, 24 Nov 2024 21:32:39 +0100,
> >>> Terry Junge wrote:
> >>>>
> >>>> Hi Jiri and Takashi,
> >>>>
> >>>> I'm not sure how it works with two different maintained trees
> >>>> but this patch set needs to be applied and flow upstream together.
> >>>>
> >>>> If the HID patch is applied without the ALSA patch then mute sync
> >>>> issues will occur with multiple Poly/Plantronics product families.
> >>>
> >>> Both patches can be applied individually, and even if only one of them
> >>> is applied, it won't hurt.  So I guess both subsystems can take the
> >>> corresponding one at any time.
> >>>
> >>
> >> Hi Takashi,
> >>
> >> I've tested out the behavior with each patch individually applied and
> >> have found that, IMHO, the mute functionality and synchronization is
> >> worse than the current behavior with neither patch. However, with both
> >> patches applied the mixer UI microphone mute control and the headset
> >> mute button are fully synchronized.
> > 
> > That's odd.  How can it worsen?  As far as I understand from the patch
> > descriptions, the USB-audio patch corrects only the mixer volume
> > control names, while the HID patch changes the quirk to be generalized
> > (to be dropped the next key in a short period).  If only USB audio
> > patch is applied, it doesn't matter as the volume binding didn't
> > happen before the patch.  OTOH, ditto, if only HID patch is applied.
> > Am I missing anything here?
> 
> The USB-audio patch also corrects the names for the mixer switch controls.
> The HID patch also adds mapping of the mute button to the KEY_MICMUTE event.
> It's not the playback volume handling that gets worse, it's the capture
> switch handling that gets worse.

That's what I don't understand -- how can it get worse?  The key
binding didn't work beforehand, no?  Now HID patch handles the
mic-mute key event, but what can it *break* without USB-audio patch?

> The USB-audio patch allows the mixer to bind to the headset's mute control in
> the feature unit and mute/un-mute the headset microphone while the HID patch
> allows the headset to request the mixer to toggle the microphone mute state.
> 
> Both patches are needed for microphone mute synchronization to work.

Sure, but the question is what becomes *worse* by one of them.


thanks,

Takashi

> 
> thanks,
> 
> Terry
> 
> > 
> >> There must be a way that both patches can be tied together and be
> >> applied simultaneously.
> >>
> >> If it would help, and be allowed, I can submit a single patch that
> >> contains both changes.
> > 
> > Applying both from the single tree is possible, sure.  One of two
> > needs an ack from the subsystem maintainers.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> >>
> >> Thanks,
> >> Terry
> >>
> >>>
> >>> thanks,
> >>>
> >>> Takashi
> >>>
> >>>>
> >>>> This patch set was tested by Wade and myself with multiple
> >>>> Poly/Plantronics product family headsets.
> >>>>
> >>>> Hi Wade,
> >>>>
> >>>> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
> >>>> as you see fit.
> >>>>
> >>>> Thanks,
> >>>> Terry
> >>>>
> >>>> Terry Junge (2):
> >>>>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
> >>>>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
> >>>>     names
> >>>>
> >>>>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
> >>>>  sound/usb/mixer_quirks.c      |  35 +++++++++
> >>>>  2 files changed, 100 insertions(+), 70 deletions(-)
> >>>>
> >>>>
> >>>> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
> >>>> -- 
> >>>> 2.43.0
> >>>>
> >>
> 

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Terry Junge]

On 12/3/24 10:56 PM, Takashi Iwai wrote:
> On Wed, 04 Dec 2024 06:23:30 +0100,
> Terry Junge wrote:
>>
>> On 12/2/24 11:32 PM, Takashi Iwai wrote:
>>> On Mon, 02 Dec 2024 23:35:51 +0100,
>>> Terry Junge wrote:
>>>>
>>>> On 11/25/24 12:55 AM, Takashi Iwai wrote:
>>>>> On Sun, 24 Nov 2024 21:32:39 +0100,
>>>>> Terry Junge wrote:
>>>>>>
>>>>>> Hi Jiri and Takashi,
>>>>>>
>>>>>> I'm not sure how it works with two different maintained trees
>>>>>> but this patch set needs to be applied and flow upstream together.
>>>>>>
>>>>>> If the HID patch is applied without the ALSA patch then mute sync
>>>>>> issues will occur with multiple Poly/Plantronics product families.
>>>>>
>>>>> Both patches can be applied individually, and even if only one of them
>>>>> is applied, it won't hurt.  So I guess both subsystems can take the
>>>>> corresponding one at any time.
>>>>>
>>>>
>>>> Hi Takashi,
>>>>
>>>> I've tested out the behavior with each patch individually applied and
>>>> have found that, IMHO, the mute functionality and synchronization is
>>>> worse than the current behavior with neither patch. However, with both
>>>> patches applied the mixer UI microphone mute control and the headset
>>>> mute button are fully synchronized.
>>>
>>> That's odd.  How can it worsen?  As far as I understand from the patch
>>> descriptions, the USB-audio patch corrects only the mixer volume
>>> control names, while the HID patch changes the quirk to be generalized
>>> (to be dropped the next key in a short period).  If only USB audio
>>> patch is applied, it doesn't matter as the volume binding didn't
>>> happen before the patch.  OTOH, ditto, if only HID patch is applied.
>>> Am I missing anything here?
>>
>> The USB-audio patch also corrects the names for the mixer switch controls.
>> The HID patch also adds mapping of the mute button to the KEY_MICMUTE event.
>> It's not the playback volume handling that gets worse, it's the capture
>> switch handling that gets worse.
> 
> That's what I don't understand -- how can it get worse?  The key
> binding didn't work beforehand, no?  Now HID patch handles the
> mic-mute key event, but what can it *break* without USB-audio patch?

Sorry, it's a long story...
All this describes the behavior I see with Ubuntu 24.04.1.


The current mute behavior with no patch:

Headset cannot communicate mute toggle intent to mixer
Mixer cannot communicate mute state to headset

Due to the name of the microphone mute control a software based mute control is
created that mutes the capture stream after it is received from the headset. It
is toggled on/off with the UI mixer dialog. Toggling this control is not
reflected in the headset, mute LED does not toggle, "Mute On" and "Mute Off"
prompts do not play (on some models). This mute control is totally independent
of the headset's mute state.

The headset also has a mute control that is toggled on/off with the headset mute
button. It mutes the capture stream before it is sent to the host. Mute state is
reflected in the mute LED and/or mute prompts. As the headset's mute control did
not get bound to the mixer, this mute control is also totally independent of the
software mute control.

If either of these mute controls are muted then the capture stream is muted.
Only if both controls are unmuted will the capture stream be live. So if the
user mutes with the UI mute control then they need to unmute with the UI
control. If they mute with the headset button then they need to unmute with
the headset button.


The mute behavior with only the HID patch:

Headset can communicate mute toggle intent to mixer
Mixer cannot communicate mute state to headset

A software mute control is created with the same characteristics as described
above with one addition. Not only does the software mute control toggle by using
the UI mixer dialog. It can now be toggled by the headset mute button sending the
mic-mute key event. This mute control is no longer totally independent.

The headset mute control is still as described above with the addition of
firing a mic-mute event when the mute button is pressed. So every press of the
mute button toggles the headset mute state and toggles the state of the
software mute control. This mute control is still totally independent. You can
only toggle it with the headset mute button.

So now the following scenario is possible...
Remember that if either control is muted the capture stream will be muted.

Mute using the UI dialog - software control is muted, headset control is not
Press headset mute button - headset control is muted, software control is not
Press headset mute button - software control is muted, headset control is not
...
Press headset mute button - headset control is muted, software control is not
Press headset mute button - software control is muted, headset control is not
Unmute using the UI dialog - both software and headset controls are not muted
Press headset mute button - both software and headset controls are muted
Press headset mute button - both software and headset controls are not muted

So the user should not use the UI dialog to toggle microphone mute.


The mute behavior with only the USB-audio patch:

Headset cannot communicate mute toggle intent to mixer
Mixer can communicate mute state to headset

With the names corrected the mixer now binds to the audio controls in the
headset. A software mute control is no longer created. Toggling the mute
control in the UI dialog actually sends the mute or unmute setting to
the headset's feature unit. As such, the mixer is in direct control of the
headset's mute state.

The headset mute button is not sending mic-mute events so it is unable to
request the mixer to toggle the mute state. The headset will comply with the
audio control mute/unmute commands as they come in. The headset's response to
mute button presses depends on the last audio control mute command received
from the host. If the last command was unmute then the mute button presses will
toggle the local mute state as described in earlier paragraphs. If the last
command was mute then the mute button presses will *not* unmute the headset.

So if the user mutes the microphone using the UI dialog, the headset will mute.
Then pressing the headset's mute button will result in the headset playing its
ummute prompt (beeps or "Mute Off") followed immediately by playing its mute
prompt (beeps or "Mute On") and the headset remains muted. At this point the
only way to unmute is by using the UI dialog.
 
So the user should not use the UI dialog to toggle microphone mute.
 

The mute behavior with both patches:

Headset can communicate mute toggle intent to mixer
Mixer can communicate mute state to headset

The microphone can be muted or unmuted by using the UI dialog or the headset's
mute button, in any order.


So...
Neither of the single patch scenarios is actually *broken*. The user can
mute or unmute the headset although some confusion can occur if the UI dialog
is utilized.

So it comes down to user experience and possibility for confusion to rate them
from best to worst.

Are either of the single patch scenarios better than or equal to the no patch
scenario?

thanks,

Terry

> 
>> The USB-audio patch allows the mixer to bind to the headset's mute control in
>> the feature unit and mute/un-mute the headset microphone while the HID patch
>> allows the headset to request the mixer to toggle the microphone mute state.
>>
>> Both patches are needed for microphone mute synchronization to work.
> 
> Sure, but the question is what becomes *worse* by one of them.
> 
> 
> thanks,
> 
> Takashi
> 
>>
>> thanks,
>>
>> Terry
>>
>>>
>>>> There must be a way that both patches can be tied together and be
>>>> applied simultaneously.
>>>>
>>>> If it would help, and be allowed, I can submit a single patch that
>>>> contains both changes.
>>>
>>> Applying both from the single tree is possible, sure.  One of two
>>> needs an ack from the subsystem maintainers.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>>
>>>> Thanks,
>>>> Terry
>>>>
>>>>>
>>>>> thanks,
>>>>>
>>>>> Takashi
>>>>>
>>>>>>
>>>>>> This patch set was tested by Wade and myself with multiple
>>>>>> Poly/Plantronics product family headsets.
>>>>>>
>>>>>> Hi Wade,
>>>>>>
>>>>>> Please feel free to add your Signed-off-by: and/or Tested-by: tags,
>>>>>> as you see fit.
>>>>>>
>>>>>> Thanks,
>>>>>> Terry
>>>>>>
>>>>>> Terry Junge (2):
>>>>>>   HID: hid-plantronics: Add mic mute mapping and generalize quirks
>>>>>>   ALSA: usb-audio: Add quirk for Plantronics headsets to fix control
>>>>>>     names
>>>>>>
>>>>>>  drivers/hid/hid-plantronics.c | 135 ++++++++++++++++------------------
>>>>>>  sound/usb/mixer_quirks.c      |  35 +++++++++
>>>>>>  2 files changed, 100 insertions(+), 70 deletions(-)
>>>>>>
>>>>>>
>>>>>> base-commit: 28eb75e178d389d325f1666e422bc13bbbb9804c
>>>>>> -- 
>>>>>> 2.43.0
>>>>>>
>>>>
>>

Re: [PATCH 0/2] Introduce Poly/Plantronics mute event support

[Takashi Iwai]

On Thu, 05 Dec 2024 23:19:00 +0100,
Terry Junge wrote:
> 
> On 12/3/24 10:56 PM, Takashi Iwai wrote:
> > On Wed, 04 Dec 2024 06:23:30 +0100,
> > Terry Junge wrote:
> >>
> >> On 12/2/24 11:32 PM, Takashi Iwai wrote:
> >>> On Mon, 02 Dec 2024 23:35:51 +0100,
> >>> Terry Junge wrote:
> >>>>
> >>>> On 11/25/24 12:55 AM, Takashi Iwai wrote:
> >>>>> On Sun, 24 Nov 2024 21:32:39 +0100,
> >>>>> Terry Junge wrote:
> >>>>>>
> >>>>>> Hi Jiri and Takashi,
> >>>>>>
> >>>>>> I'm not sure how it works with two different maintained trees
> >>>>>> but this patch set needs to be applied and flow upstream together.
> >>>>>>
> >>>>>> If the HID patch is applied without the ALSA patch then mute sync
> >>>>>> issues will occur with multiple Poly/Plantronics product families.
> >>>>>
> >>>>> Both patches can be applied individually, and even if only one of them
> >>>>> is applied, it won't hurt.  So I guess both subsystems can take the
> >>>>> corresponding one at any time.
> >>>>>
> >>>>
> >>>> Hi Takashi,
> >>>>
> >>>> I've tested out the behavior with each patch individually applied and
> >>>> have found that, IMHO, the mute functionality and synchronization is
> >>>> worse than the current behavior with neither patch. However, with both
> >>>> patches applied the mixer UI microphone mute control and the headset
> >>>> mute button are fully synchronized.
> >>>
> >>> That's odd.  How can it worsen?  As far as I understand from the patch
> >>> descriptions, the USB-audio patch corrects only the mixer volume
> >>> control names, while the HID patch changes the quirk to be generalized
> >>> (to be dropped the next key in a short period).  If only USB audio
> >>> patch is applied, it doesn't matter as the volume binding didn't
> >>> happen before the patch.  OTOH, ditto, if only HID patch is applied.
> >>> Am I missing anything here?
> >>
> >> The USB-audio patch also corrects the names for the mixer switch controls.
> >> The HID patch also adds mapping of the mute button to the KEY_MICMUTE event.
> >> It's not the playback volume handling that gets worse, it's the capture
> >> switch handling that gets worse.
> > 
> > That's what I don't understand -- how can it get worse?  The key
> > binding didn't work beforehand, no?  Now HID patch handles the
> > mic-mute key event, but what can it *break* without USB-audio patch?
> 
> Sorry, it's a long story...
> All this describes the behavior I see with Ubuntu 24.04.1.
> 
> 
> The current mute behavior with no patch:
> 
> Headset cannot communicate mute toggle intent to mixer
> Mixer cannot communicate mute state to headset
> 
> Due to the name of the microphone mute control a software based mute control is
> created that mutes the capture stream after it is received from the headset. It
> is toggled on/off with the UI mixer dialog. Toggling this control is not
> reflected in the headset, mute LED does not toggle, "Mute On" and "Mute Off"
> prompts do not play (on some models). This mute control is totally independent
> of the headset's mute state.
> 
> The headset also has a mute control that is toggled on/off with the headset mute
> button. It mutes the capture stream before it is sent to the host. Mute state is
> reflected in the mute LED and/or mute prompts. As the headset's mute control did
> not get bound to the mixer, this mute control is also totally independent of the
> software mute control.
> 
> If either of these mute controls are muted then the capture stream is muted.
> Only if both controls are unmuted will the capture stream be live. So if the
> user mutes with the UI mute control then they need to unmute with the UI
> control. If they mute with the headset button then they need to unmute with
> the headset button.
> 
> 
> The mute behavior with only the HID patch:
> 
> Headset can communicate mute toggle intent to mixer
> Mixer cannot communicate mute state to headset
> 
> A software mute control is created with the same characteristics as described
> above with one addition. Not only does the software mute control toggle by using
> the UI mixer dialog. It can now be toggled by the headset mute button sending the
> mic-mute key event. This mute control is no longer totally independent.
> 
> The headset mute control is still as described above with the addition of
> firing a mic-mute event when the mute button is pressed. So every press of the
> mute button toggles the headset mute state and toggles the state of the
> software mute control. This mute control is still totally independent. You can
> only toggle it with the headset mute button.
> 
> So now the following scenario is possible...
> Remember that if either control is muted the capture stream will be muted.
> 
> Mute using the UI dialog - software control is muted, headset control is not
> Press headset mute button - headset control is muted, software control is not
> Press headset mute button - software control is muted, headset control is not
> ...
> Press headset mute button - headset control is muted, software control is not
> Press headset mute button - software control is muted, headset control is not
> Unmute using the UI dialog - both software and headset controls are not muted
> Press headset mute button - both software and headset controls are muted
> Press headset mute button - both software and headset controls are not muted
> 
> So the user should not use the UI dialog to toggle microphone mute.
> 
> The mute behavior with only the USB-audio patch:
> 
> Headset cannot communicate mute toggle intent to mixer
> Mixer can communicate mute state to headset
> 
> With the names corrected the mixer now binds to the audio controls in the
> headset. A software mute control is no longer created. Toggling the mute
> control in the UI dialog actually sends the mute or unmute setting to
> the headset's feature unit. As such, the mixer is in direct control of the
> headset's mute state.
> 
> The headset mute button is not sending mic-mute events so it is unable to
> request the mixer to toggle the mute state. The headset will comply with the
> audio control mute/unmute commands as they come in. The headset's response to
> mute button presses depends on the last audio control mute command received
> from the host. If the last command was unmute then the mute button presses will
> toggle the local mute state as described in earlier paragraphs. If the last
> command was mute then the mute button presses will *not* unmute the headset.
> 
> So if the user mutes the microphone using the UI dialog, the headset will mute.
> Then pressing the headset's mute button will result in the headset playing its
> ummute prompt (beeps or "Mute Off") followed immediately by playing its mute
> prompt (beeps or "Mute On") and the headset remains muted. At this point the
> only way to unmute is by using the UI dialog.
>  
> So the user should not use the UI dialog to toggle microphone mute.
>  
> 
> The mute behavior with both patches:
> 
> Headset can communicate mute toggle intent to mixer
> Mixer can communicate mute state to headset
> 
> The microphone can be muted or unmuted by using the UI dialog or the headset's
> mute button, in any order.
> 
> 
> So...
> Neither of the single patch scenarios is actually *broken*. The user can
> mute or unmute the headset although some confusion can occur if the UI dialog
> is utilized.
> 
> So it comes down to user experience and possibility for confusion to rate them
> from best to worst.
> 
> Are either of the single patch scenarios better than or equal to the no patch
> scenario?

I don't think it'll be so big problem to apply individually.  Even if
only one of two patches is applied, it doesn't break things
seriously.  Certainly there can be inconsistency due to individual
mute behavior, but that's not much worse than the current situation,
IMO.

And, as long as we keep Cc-to-stable tag in both patches, both of them
will be taken to stable trees.  Also, since the changes aren't too
intrusive, it's no big problem to apply both patches to the same tree
by receiving Ack from the other subsystem maintainers, too.


thanks,

Takashi