Re: [PATCH] ALSA: hda - Fixes inverted Conexant GPIO mic mute led

[Takashi Iwai <tiwai@suse.de>]

On Fri, 16 Aug 2019 01:08:34 +0200,
Jerónimo Borque wrote:
> 
> El jue., 15 de ago. de 2019 a la(s) 14:06, Takashi Iwai (tiwai@suse.de)
> escribió:
> 
>     On Thu, 15 Aug 2019 18:33:50 +0200,
>     Jerónimo Borque wrote:
>     >
>     > Hi Takashi,
>     > Modifying Mic Mute-LED Mode does indeed alter the behavior. The thing is
>     that
>     > this ends being confusing as in all machines I've been testing this
>     setting
>     > Mic Mute-LED Mode to "Follow Capture" actually makes it follow mute, as
>     > setting it to "On" turns the LED off.
>     > There is other setting called "mute_led_polarity" but this does not
>     work, as
>     > currently mic mute LED and mute LED do not follow the same logic.
>     > What I think may be causing confusion is "cxt_update_gpio_led" "enabled"
>     > parameter. Setting "enabled" to "true" sets the GPIO pin to 0 causing
>     the led
>     > to be turned off. I think "enabled" used to refer to the input capture
>     or
>     > output status and not to the LED being lit or not. Output or input not
>     enabled
>     > (enabled==false) caused the LED to be turned on.
>     > This logic in the function negates it on the GPIO output.
>     >
>     > if (enabled)
>     >     spec->gpio_led &= ~mask;
>     > else
>     >     spec->gpio_led |= mask;
>     >
>     > May be I can do a more comprehensive fix, reversing the behavior of 
>     > "cxt_update_gpio_led" "enabled" parameter to refer the GPIO output value
>     (
>     > enabled==true => GPIO pin output high )
>     > Then also modify the call to "cxt_update_gpio_led" in
>     > "cxt_fixup_gpio_mute_hook" to make it work consistently.
>    
>     OK, if the "On" turns the LED off, it's indeed inverted.
>     Then we'd need to consider both fixing the inverted behavior and the
>     default mic-mute mode.
>    
>     Could you confirm the following?
>    
>     - Which models and codecs are checked?
> 
> I've tested on HP ZBook 15U G3 (Conexant CX20724) and HP Probook 440 G4
> (Conexant CX8200)
> 
>     - GPIO pin high = mic LED on or off?
> 
>  GPIO pin high = mic LED on
> 
>     - How is the expected behavior on Windows?
>        Mute is on when mic is muted, or mute-on when mic is ready?
> 
> Mute led is on when mic is muted.

Thanks, it's clearer now.

Then I'd rather like to correct cxt_update_gpio_led(), i.e. taking the
argument led_on instead of enabled, and correct the caller in
cxt_fixup_gpio_mute_hook() to invert instead.  Something like below.
Could you retest and resubmit with that change?


thanks,

Takashi

--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -611,18 +611,18 @@ static void cxt_fixup_hp_gate_mic_jack(struct hda_codec *codec,
 
 /* update LED status via GPIO */
 static void cxt_update_gpio_led(struct hda_codec *codec, unsigned int mask,
-				bool enabled)
+				bool led_on)
 {
 	struct conexant_spec *spec = codec->spec;
 	unsigned int oldval = spec->gpio_led;
 
 	if (spec->mute_led_polarity)
-		enabled = !enabled;
+		led_on = !led_on;
 
-	if (enabled)
-		spec->gpio_led &= ~mask;
-	else
+	if (led_on)
 		spec->gpio_led |= mask;
+	else
+		spec->gpio_led &= ~mask;
 	if (spec->gpio_led != oldval)
 		snd_hda_codec_write(codec, 0x01, 0, AC_VERB_SET_GPIO_DATA,
 				    spec->gpio_led);
@@ -634,7 +634,8 @@ static void cxt_fixup_gpio_mute_hook(void *private_data, int enabled)
 	struct hda_codec *codec = private_data;
 	struct conexant_spec *spec = codec->spec;
 
-	cxt_update_gpio_led(codec, spec->gpio_mute_led_mask, enabled);
+	/* muted -> LED on */
+	cxt_update_gpio_led(codec, spec->gpio_mute_led_mask, !enabled);
 }
 
 /* turn on/off mic-mute LED via GPIO per capture hook */
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel