| Message ID | 20190815013824.13373-1-jeronimo@borque.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda - Fixes inverted Conexant GPIO mic mute led | expand |
On Thu, 15 Aug 2019 03:38:24 +0200, <jeronimo@borque.com.ar> wrote: > > 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 (Mic mute > led was on when the input enabled) Fixing it in the call to > cxt_update_gpio_led at the cxt_gpio_micmute_update hook. > Maybe more changes are required to be consistent everywhere. > > Signed-off-by: Jeronimo Borque <jeronimo@borque.com.ar> Could you check which value you have in "Mic Mute-LED Mode" mixer element? I guess it's "Follow Mute". If so, change it to "Follow Capture". If this works, it means that the driver works as expected but the problem is only about the default value. The default value set in the generic parser is based on other machine's standard (LED on at mic off), while some machines might expect differently. On such machines, we need to set the different value initially in the quirk fixup. thanks, Takashi
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.
Thanks,
Jerónimo
El jue., 15 de ago. de 2019 a la(s) 02:58, Takashi Iwai (tiwai@suse.de)
escribió:
> On Thu, 15 Aug 2019 03:38:24 +0200,
> <jeronimo@borque.com.ar> wrote:
> >
> > 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 (Mic mute
> > led was on when the input enabled) Fixing it in the call to
> > cxt_update_gpio_led at the cxt_gpio_micmute_update hook.
> > Maybe more changes are required to be consistent everywhere.
> >
> > Signed-off-by: Jeronimo Borque <jeronimo@borque.com.ar>
>
> Could you check which value you have in "Mic Mute-LED Mode" mixer
> element? I guess it's "Follow Mute". If so, change it to "Follow
> Capture".
>
> If this works, it means that the driver works as expected but the
> problem is only about the default value. The default value set in the
> generic parser is based on other machine's standard (LED on at mic
> off), while some machines might expect differently. On such machines,
> we need to set the different value initially in the quirk fixup.
>
>
> thanks,
>
> Takashi
>
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? - GPIO pin high = mic LED on or off? - How is the expected behavior on Windows? Mute is on when mic is muted, or mute-on when mic is ready? thanks, Takashi > > Thanks, > Jerónimo > > El jue., 15 de ago. de 2019 a la(s) 02:58, Takashi Iwai (tiwai@suse.de) > escribió: > > On Thu, 15 Aug 2019 03:38:24 +0200, > <jeronimo@borque.com.ar> wrote: > > > > 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 (Mic mute > > led was on when the input enabled) Fixing it in the call to > > cxt_update_gpio_led at the cxt_gpio_micmute_update hook. > > Maybe more changes are required to be consistent everywhere. > > > > Signed-off-by: Jeronimo Borque <jeronimo@borque.com.ar> > > Could you check which value you have in "Mic Mute-LED Mode" mixer > element? I guess it's "Follow Mute". If so, change it to "Follow > Capture". > > If this works, it means that the driver works as expected but the > problem is only about the default value. The default value set in the > generic parser is based on other machine's standard (LED on at mic > off), while some machines might expect differently. On such machines, > we need to set the different value initially in the quirk fixup. > > thanks, > > Takashi > >
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, Jerónimo > > thanks, > > Takashi > > > > > Thanks, > > Jerónimo > > > > El jue., 15 de ago. de 2019 a la(s) 02:58, Takashi Iwai (tiwai@suse.de) > > escribió: > > > > On Thu, 15 Aug 2019 03:38:24 +0200, > > <jeronimo@borque.com.ar> wrote: > > > > > > 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 (Mic mute > > > led was on when the input enabled) Fixing it in the call to > > > cxt_update_gpio_led at the cxt_gpio_micmute_update hook. > > > Maybe more changes are required to be consistent everywhere. > > > > > > Signed-off-by: Jeronimo Borque <jeronimo@borque.com.ar> > > > > Could you check which value you have in "Mic Mute-LED Mode" mixer > > element? I guess it's "Follow Mute". If so, change it to "Follow > > Capture". > > > > If this works, it means that the driver works as expected but the > > problem is only about the default value. The default value set in > the > > generic parser is based on other machine's standard (LED on at mic > > off), while some machines might expect differently. On such > machines, > > we need to set the different value initially in the quirk fixup. > > > > thanks, > > > > Takashi > > > > >
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 */
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index f299f137eaea..8edf0d1290b5 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -636,6 +636,10 @@ static void cxt_update_gpio_led(struct hda_codec *codec, unsigned int mask, spec->gpio_led &= ~mask; else spec->gpio_led |= mask; + + codec_dbg(codec, "mask:%d enabled:%d gpio_led:%d\n", + mask, enabled, spec->gpio_led); + if (spec->gpio_led != oldval) snd_hda_codec_write(codec, 0x01, 0, AC_VERB_SET_GPIO_DATA, spec->gpio_led); @@ -656,7 +660,7 @@ static void cxt_gpio_micmute_update(struct hda_codec *codec) struct conexant_spec *spec = codec->spec; cxt_update_gpio_led(codec, spec->gpio_mic_led_mask, - spec->gen.micmute_led.led_value); + !spec->gen.micmute_led.led_value); } @@ -669,7 +673,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;