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 */

I'm sorry. I've sent a wrong patch file. I'll resubmit the right patch shortly. Thanks, Jerónimo

El dom., 18 de ago. de 2019 a la(s) 18:23, jeronimo@borque.com.ar escribió:

From: Jeronimo Borque jeronimo@borque.com.ar

"enabled" parameter historically referred to the device input or output, not to the led indicator. After the changes added with the led helper functions the mic mute led logic refers to the led and not to the mic input which caused led indicator to be negated. Fixing logic in cxt_update_gpio_led and updated cxt_fixup_gpio_mute_hook Also updated debug messages to ease further debugging if necessary.

Suggested-by: Takashi Iwai tiwai@suse.de Signed-off-by: Jeronimo Borque jeronimo@borque.com.ar

---

sound/pci/hda/patch_conexant.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 14298ef45b21..d1c93dfa158b 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -611,18 +611,20 @@ 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;
  

•   codec_dbg(codec, "mask:%d enabled:%d gpio_led:%d\n
  

•                   mask, led_on, spec->gpio_led);
    if (spec->gpio_led != oldval)
            snd_hda_codec_write(codec, 0x01, 0, AC_VERB_SET_GPIO_DATA,
                                spec->gpio_led);
  

@@ -633,8 +635,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 */ @@ -656,7 +658,6 @@ static void cxt_fixup_mute_led_gpio(struct hda_codec *codec, { 0x01, AC_VERB_SET_GPIO_DIRECTION, 0x03 }, {} };

•   codec_info(codec, "action: %d gpio_led: %d\n", action,
  

spec->gpio_led);

    if (action == HDA_FIXUP_ACT_PRE_PROBE) {
            spec->gen.vmaster_mute.hook = cxt_fixup_gpio_mute_hook;

-- 2.21.0