[PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[Takashi Iwai]

The current code for controlling mic mute LED in patch_sigmatel.c
blindly assumes that there is a single capture switch.  But, there can
be multiple multiple ones, and each of them flips the state, ended up
in an inconsistent state.

For fixing this problem, this patch adds kcontrol to be passed to the
hook function so that the callee can check which switch is being
accessed.  In stac_capture_led_hook(), the state is checked as a
bitmask, and turns on the LED when all capture switches are off.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_generic.c     |  8 ++++----
 sound/pci/hda/hda_generic.h     |  1 +
 sound/pci/hda/patch_conexant.c  |  3 ++-
 sound/pci/hda/patch_realtek.c   |  9 ++++++---
 sound/pci/hda/patch_sigmatel.c  | 27 +++++++++++++++++----------
 sound/pci/hda/thinkpad_helper.c |  1 +
 6 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 8321a97d5c05..d9a09bdd09db 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3269,7 +3269,7 @@ static int cap_put_caller(struct snd_kcontrol *kcontrol,
 	mutex_unlock(&codec->control_mutex);
 	snd_hda_codec_flush_cache(codec); /* flush the updates */
 	if (err >= 0 && spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, ucontrol);
+		spec->cap_sync_hook(codec, kcontrol, ucontrol);
 	return err;
 }
 
@@ -3390,7 +3390,7 @@ static int cap_single_sw_put(struct snd_kcontrol *kcontrol,
 		return ret;
 
 	if (spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, ucontrol);
+		spec->cap_sync_hook(codec, kcontrol, ucontrol);
 
 	return ret;
 }
@@ -3795,7 +3795,7 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
 		return 0;
 	snd_hda_activate_path(codec, path, true, false);
 	if (spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, NULL);
+		spec->cap_sync_hook(codec, NULL, NULL);
 	path_power_down_sync(codec, old_path);
 	return 1;
 }
@@ -5270,7 +5270,7 @@ static void init_input_src(struct hda_codec *codec)
 	}
 
 	if (spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, NULL);
+		spec->cap_sync_hook(codec, NULL, NULL);
 }
 
 /* set right pin controls for digital I/O */
diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
index 07f767231c9f..c908afbe4d94 100644
--- a/sound/pci/hda/hda_generic.h
+++ b/sound/pci/hda/hda_generic.h
@@ -274,6 +274,7 @@ struct hda_gen_spec {
 	void (*init_hook)(struct hda_codec *codec);
 	void (*automute_hook)(struct hda_codec *codec);
 	void (*cap_sync_hook)(struct hda_codec *codec,
+			      struct snd_kcontrol *kcontrol,
 			      struct snd_ctl_elem_value *ucontrol);
 
 	/* PCM hooks */
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 4e0ec146553d..bcf91bea3317 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -3291,7 +3291,8 @@ static void cxt_update_headset_mode(struct hda_codec *codec)
 }
 
 static void cxt_update_headset_mode_hook(struct hda_codec *codec,
-			     struct snd_ctl_elem_value *ucontrol)
+					 struct snd_kcontrol *kcontrol,
+					 struct snd_ctl_elem_value *ucontrol)
 {
 	cxt_update_headset_mode(codec);
 }
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 56a8f1876603..6aabaa1c19f5 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -708,7 +708,8 @@ static void alc_inv_dmic_sync(struct hda_codec *codec, bool force)
 }
 
 static void alc_inv_dmic_hook(struct hda_codec *codec,
-			     struct snd_ctl_elem_value *ucontrol)
+			      struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *ucontrol)
 {
 	alc_inv_dmic_sync(codec, false);
 }
@@ -3211,7 +3212,8 @@ static void alc269_fixup_hp_gpio_mute_hook(void *private_data, int enabled)
 
 /* turn on/off mic-mute LED per capture hook */
 static void alc269_fixup_hp_gpio_mic_mute_hook(struct hda_codec *codec,
-			       struct snd_ctl_elem_value *ucontrol)
+					       struct snd_kcontrol *kcontrol,
+					       struct snd_ctl_elem_value *ucontrol)
 {
 	struct alc_spec *spec = codec->spec;
 	unsigned int oldval = spec->gpio_led;
@@ -3521,7 +3523,8 @@ static void alc_update_headset_mode(struct hda_codec *codec)
 }
 
 static void alc_update_headset_mode_hook(struct hda_codec *codec,
-			     struct snd_ctl_elem_value *ucontrol)
+					 struct snd_kcontrol *kcontrol,
+					 struct snd_ctl_elem_value *ucontrol)
 {
 	alc_update_headset_mode(codec);
 }
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
index 6998cf29b9bc..7311badf6a94 100644
--- a/sound/pci/hda/patch_sigmatel.c
+++ b/sound/pci/hda/patch_sigmatel.c
@@ -194,7 +194,7 @@ struct sigmatel_spec {
 	int default_polarity;
 
 	unsigned int mic_mute_led_gpio; /* capture mute LED GPIO */
-	bool mic_mute_led_on; /* current mic mute state */
+	unsigned int mic_enabled; /* current mic mute state (bitmask) */
 
 	/* stream */
 	unsigned int stream_delay;
@@ -324,19 +324,26 @@ static void stac_gpio_set(struct hda_codec *codec, unsigned int mask,
 
 /* hook for controlling mic-mute LED GPIO */
 static void stac_capture_led_hook(struct hda_codec *codec,
-			       struct snd_ctl_elem_value *ucontrol)
+				  struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
 {
 	struct sigmatel_spec *spec = codec->spec;
-	bool mute;
+	unsigned int mask;
+	bool cur_mute, prev_mute;
 
-	if (!ucontrol)
+	if (!kcontrol || !ucontrol)
 		return;
 
-	mute = !(ucontrol->value.integer.value[0] ||
-		 ucontrol->value.integer.value[1]);
-	if (spec->mic_mute_led_on != mute) {
-		spec->mic_mute_led_on = mute;
-		if (mute)
+	mask = 1U << snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
+	prev_mute = !spec->mic_enabled;
+	if (ucontrol->value.integer.value[0] ||
+	    ucontrol->value.integer.value[1])
+		spec->mic_enabled |= mask;
+	else
+		spec->mic_enabled &= ~mask;
+	cur_mute = !spec->mic_enabled;
+	if (cur_mute != prev_mute) {
+		if (cur_mute)
 			spec->gpio_data |= spec->mic_mute_led_gpio;
 		else
 			spec->gpio_data &= ~spec->mic_mute_led_gpio;
@@ -4462,7 +4469,7 @@ static void stac_setup_gpio(struct hda_codec *codec)
 	if (spec->mic_mute_led_gpio) {
 		spec->gpio_mask |= spec->mic_mute_led_gpio;
 		spec->gpio_dir |= spec->mic_mute_led_gpio;
-		spec->mic_mute_led_on = true;
+		spec->mic_enabled = 0;
 		spec->gpio_data |= spec->mic_mute_led_gpio;
 
 		spec->gen.cap_sync_hook = stac_capture_led_hook;
diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 5799fbc24c28..8fe3b8c18ed4 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -39,6 +39,7 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
 }
 
 static void update_tpacpi_micmute_led(struct hda_codec *codec,
+				      struct snd_kcontrol *kcontrol,
 				      struct snd_ctl_elem_value *ucontrol)
 {
 	if (!ucontrol || !led_set_func)
-- 
1.8.5.2

Re: [PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[David Henningsson]

On 01/30/2014 06:04 PM, Takashi Iwai wrote:
> The current code for controlling mic mute LED in patch_sigmatel.c
> blindly assumes that there is a single capture switch.  But, there can
> be multiple multiple ones, and each of them flips the state, ended up
> in an inconsistent state.
> 
> For fixing this problem, this patch adds kcontrol to be passed to the
> hook function so that the callee can check which switch is being
> accessed.  In stac_capture_led_hook(), the state is checked as a
> bitmask, and turns on the LED when all capture switches are off.

Is capture switch with non-zero indices turned off by default (and not
turned on by e g alsactl init)? Otherwise it sounds like this could be
regressing OOTB behaviour, i e, the mic mute LED worked with
people/PulseAudio just toggling Capture Switch 0, but now requires
manual toggling of the other capture switches...?

Anyway, the same reasoning would potentially apply to Conexant and
Realtek, so perhaps it's time to move some more of mic mute LED handling
into hda_generic?

> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_generic.c     |  8 ++++----
>  sound/pci/hda/hda_generic.h     |  1 +
>  sound/pci/hda/patch_conexant.c  |  3 ++-
>  sound/pci/hda/patch_realtek.c   |  9 ++++++---
>  sound/pci/hda/patch_sigmatel.c  | 27 +++++++++++++++++----------
>  sound/pci/hda/thinkpad_helper.c |  1 +
>  6 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 8321a97d5c05..d9a09bdd09db 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3269,7 +3269,7 @@ static int cap_put_caller(struct snd_kcontrol *kcontrol,
>  	mutex_unlock(&codec->control_mutex);
>  	snd_hda_codec_flush_cache(codec); /* flush the updates */
>  	if (err >= 0 && spec->cap_sync_hook)
> -		spec->cap_sync_hook(codec, ucontrol);
> +		spec->cap_sync_hook(codec, kcontrol, ucontrol);
>  	return err;
>  }
>  
> @@ -3390,7 +3390,7 @@ static int cap_single_sw_put(struct snd_kcontrol *kcontrol,
>  		return ret;
>  
>  	if (spec->cap_sync_hook)
> -		spec->cap_sync_hook(codec, ucontrol);
> +		spec->cap_sync_hook(codec, kcontrol, ucontrol);
>  
>  	return ret;
>  }
> @@ -3795,7 +3795,7 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
>  		return 0;
>  	snd_hda_activate_path(codec, path, true, false);
>  	if (spec->cap_sync_hook)
> -		spec->cap_sync_hook(codec, NULL);
> +		spec->cap_sync_hook(codec, NULL, NULL);
>  	path_power_down_sync(codec, old_path);
>  	return 1;
>  }
> @@ -5270,7 +5270,7 @@ static void init_input_src(struct hda_codec *codec)
>  	}
>  
>  	if (spec->cap_sync_hook)
> -		spec->cap_sync_hook(codec, NULL);
> +		spec->cap_sync_hook(codec, NULL, NULL);
>  }
>  
>  /* set right pin controls for digital I/O */
> diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
> index 07f767231c9f..c908afbe4d94 100644
> --- a/sound/pci/hda/hda_generic.h
> +++ b/sound/pci/hda/hda_generic.h
> @@ -274,6 +274,7 @@ struct hda_gen_spec {
>  	void (*init_hook)(struct hda_codec *codec);
>  	void (*automute_hook)(struct hda_codec *codec);
>  	void (*cap_sync_hook)(struct hda_codec *codec,
> +			      struct snd_kcontrol *kcontrol,
>  			      struct snd_ctl_elem_value *ucontrol);
>  
>  	/* PCM hooks */
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index 4e0ec146553d..bcf91bea3317 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -3291,7 +3291,8 @@ static void cxt_update_headset_mode(struct hda_codec *codec)
>  }
>  
>  static void cxt_update_headset_mode_hook(struct hda_codec *codec,
> -			     struct snd_ctl_elem_value *ucontrol)
> +					 struct snd_kcontrol *kcontrol,
> +					 struct snd_ctl_elem_value *ucontrol)
>  {
>  	cxt_update_headset_mode(codec);
>  }
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 56a8f1876603..6aabaa1c19f5 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -708,7 +708,8 @@ static void alc_inv_dmic_sync(struct hda_codec *codec, bool force)
>  }
>  
>  static void alc_inv_dmic_hook(struct hda_codec *codec,
> -			     struct snd_ctl_elem_value *ucontrol)
> +			      struct snd_kcontrol *kcontrol,
> +			      struct snd_ctl_elem_value *ucontrol)
>  {
>  	alc_inv_dmic_sync(codec, false);
>  }
> @@ -3211,7 +3212,8 @@ static void alc269_fixup_hp_gpio_mute_hook(void *private_data, int enabled)
>  
>  /* turn on/off mic-mute LED per capture hook */
>  static void alc269_fixup_hp_gpio_mic_mute_hook(struct hda_codec *codec,
> -			       struct snd_ctl_elem_value *ucontrol)
> +					       struct snd_kcontrol *kcontrol,
> +					       struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct alc_spec *spec = codec->spec;
>  	unsigned int oldval = spec->gpio_led;
> @@ -3521,7 +3523,8 @@ static void alc_update_headset_mode(struct hda_codec *codec)
>  }
>  
>  static void alc_update_headset_mode_hook(struct hda_codec *codec,
> -			     struct snd_ctl_elem_value *ucontrol)
> +					 struct snd_kcontrol *kcontrol,
> +					 struct snd_ctl_elem_value *ucontrol)
>  {
>  	alc_update_headset_mode(codec);
>  }
> diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
> index 6998cf29b9bc..7311badf6a94 100644
> --- a/sound/pci/hda/patch_sigmatel.c
> +++ b/sound/pci/hda/patch_sigmatel.c
> @@ -194,7 +194,7 @@ struct sigmatel_spec {
>  	int default_polarity;
>  
>  	unsigned int mic_mute_led_gpio; /* capture mute LED GPIO */
> -	bool mic_mute_led_on; /* current mic mute state */
> +	unsigned int mic_enabled; /* current mic mute state (bitmask) */
>  
>  	/* stream */
>  	unsigned int stream_delay;
> @@ -324,19 +324,26 @@ static void stac_gpio_set(struct hda_codec *codec, unsigned int mask,
>  
>  /* hook for controlling mic-mute LED GPIO */
>  static void stac_capture_led_hook(struct hda_codec *codec,
> -			       struct snd_ctl_elem_value *ucontrol)
> +				  struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct sigmatel_spec *spec = codec->spec;
> -	bool mute;
> +	unsigned int mask;
> +	bool cur_mute, prev_mute;
>  
> -	if (!ucontrol)
> +	if (!kcontrol || !ucontrol)
>  		return;
>  
> -	mute = !(ucontrol->value.integer.value[0] ||
> -		 ucontrol->value.integer.value[1]);
> -	if (spec->mic_mute_led_on != mute) {
> -		spec->mic_mute_led_on = mute;
> -		if (mute)
> +	mask = 1U << snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
> +	prev_mute = !spec->mic_enabled;
> +	if (ucontrol->value.integer.value[0] ||
> +	    ucontrol->value.integer.value[1])
> +		spec->mic_enabled |= mask;
> +	else
> +		spec->mic_enabled &= ~mask;
> +	cur_mute = !spec->mic_enabled;
> +	if (cur_mute != prev_mute) {
> +		if (cur_mute)
>  			spec->gpio_data |= spec->mic_mute_led_gpio;
>  		else
>  			spec->gpio_data &= ~spec->mic_mute_led_gpio;
> @@ -4462,7 +4469,7 @@ static void stac_setup_gpio(struct hda_codec *codec)
>  	if (spec->mic_mute_led_gpio) {
>  		spec->gpio_mask |= spec->mic_mute_led_gpio;
>  		spec->gpio_dir |= spec->mic_mute_led_gpio;
> -		spec->mic_mute_led_on = true;
> +		spec->mic_enabled = 0;
>  		spec->gpio_data |= spec->mic_mute_led_gpio;
>  
>  		spec->gen.cap_sync_hook = stac_capture_led_hook;
> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
> index 5799fbc24c28..8fe3b8c18ed4 100644
> --- a/sound/pci/hda/thinkpad_helper.c
> +++ b/sound/pci/hda/thinkpad_helper.c
> @@ -39,6 +39,7 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
>  }
>  
>  static void update_tpacpi_micmute_led(struct hda_codec *codec,
> +				      struct snd_kcontrol *kcontrol,
>  				      struct snd_ctl_elem_value *ucontrol)
>  {
>  	if (!ucontrol || !led_set_func)
> 



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[Takashi Iwai]

At Fri, 31 Jan 2014 17:52:55 +0100,
David Henningsson wrote:
> 
> On 01/30/2014 06:04 PM, Takashi Iwai wrote:
> > The current code for controlling mic mute LED in patch_sigmatel.c
> > blindly assumes that there is a single capture switch.  But, there can
> > be multiple multiple ones, and each of them flips the state, ended up
> > in an inconsistent state.
> > 
> > For fixing this problem, this patch adds kcontrol to be passed to the
> > hook function so that the callee can check which switch is being
> > accessed.  In stac_capture_led_hook(), the state is checked as a
> > bitmask, and turns on the LED when all capture switches are off.
> 
> Is capture switch with non-zero indices turned off by default (and not
> turned on by e g alsactl init)? Otherwise it sounds like this could be
> regressing OOTB behaviour, i e, the mic mute LED worked with
> people/PulseAudio just toggling Capture Switch 0, but now requires
> manual toggling of the other capture switches...?

I thought alsactl init doesn't take all captures on but only the
first capture.

I don't mind to change the behavior only to check the first capture,
and ignore the rest.  Although it sounds a bit inconsistent, it might
be slightly better aligned to the existing behavior.

> Anyway, the same reasoning would potentially apply to Conexant and
> Realtek, so perhaps it's time to move some more of mic mute LED handling
> into hda_generic?

The hook itself can be used not only for the LED.  So, filtering the
action in the generic parser side doesn't fit, IMO.


Takashi

> 
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/pci/hda/hda_generic.c     |  8 ++++----
> >  sound/pci/hda/hda_generic.h     |  1 +
> >  sound/pci/hda/patch_conexant.c  |  3 ++-
> >  sound/pci/hda/patch_realtek.c   |  9 ++++++---
> >  sound/pci/hda/patch_sigmatel.c  | 27 +++++++++++++++++----------
> >  sound/pci/hda/thinkpad_helper.c |  1 +
> >  6 files changed, 31 insertions(+), 18 deletions(-)
> > 
> > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > index 8321a97d5c05..d9a09bdd09db 100644
> > --- a/sound/pci/hda/hda_generic.c
> > +++ b/sound/pci/hda/hda_generic.c
> > @@ -3269,7 +3269,7 @@ static int cap_put_caller(struct snd_kcontrol *kcontrol,
> >  	mutex_unlock(&codec->control_mutex);
> >  	snd_hda_codec_flush_cache(codec); /* flush the updates */
> >  	if (err >= 0 && spec->cap_sync_hook)
> > -		spec->cap_sync_hook(codec, ucontrol);
> > +		spec->cap_sync_hook(codec, kcontrol, ucontrol);
> >  	return err;
> >  }
> >  
> > @@ -3390,7 +3390,7 @@ static int cap_single_sw_put(struct snd_kcontrol *kcontrol,
> >  		return ret;
> >  
> >  	if (spec->cap_sync_hook)
> > -		spec->cap_sync_hook(codec, ucontrol);
> > +		spec->cap_sync_hook(codec, kcontrol, ucontrol);
> >  
> >  	return ret;
> >  }
> > @@ -3795,7 +3795,7 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
> >  		return 0;
> >  	snd_hda_activate_path(codec, path, true, false);
> >  	if (spec->cap_sync_hook)
> > -		spec->cap_sync_hook(codec, NULL);
> > +		spec->cap_sync_hook(codec, NULL, NULL);
> >  	path_power_down_sync(codec, old_path);
> >  	return 1;
> >  }
> > @@ -5270,7 +5270,7 @@ static void init_input_src(struct hda_codec *codec)
> >  	}
> >  
> >  	if (spec->cap_sync_hook)
> > -		spec->cap_sync_hook(codec, NULL);
> > +		spec->cap_sync_hook(codec, NULL, NULL);
> >  }
> >  
> >  /* set right pin controls for digital I/O */
> > diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
> > index 07f767231c9f..c908afbe4d94 100644
> > --- a/sound/pci/hda/hda_generic.h
> > +++ b/sound/pci/hda/hda_generic.h
> > @@ -274,6 +274,7 @@ struct hda_gen_spec {
> >  	void (*init_hook)(struct hda_codec *codec);
> >  	void (*automute_hook)(struct hda_codec *codec);
> >  	void (*cap_sync_hook)(struct hda_codec *codec,
> > +			      struct snd_kcontrol *kcontrol,
> >  			      struct snd_ctl_elem_value *ucontrol);
> >  
> >  	/* PCM hooks */
> > diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> > index 4e0ec146553d..bcf91bea3317 100644
> > --- a/sound/pci/hda/patch_conexant.c
> > +++ b/sound/pci/hda/patch_conexant.c
> > @@ -3291,7 +3291,8 @@ static void cxt_update_headset_mode(struct hda_codec *codec)
> >  }
> >  
> >  static void cxt_update_headset_mode_hook(struct hda_codec *codec,
> > -			     struct snd_ctl_elem_value *ucontrol)
> > +					 struct snd_kcontrol *kcontrol,
> > +					 struct snd_ctl_elem_value *ucontrol)
> >  {
> >  	cxt_update_headset_mode(codec);
> >  }
> > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> > index 56a8f1876603..6aabaa1c19f5 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -708,7 +708,8 @@ static void alc_inv_dmic_sync(struct hda_codec *codec, bool force)
> >  }
> >  
> >  static void alc_inv_dmic_hook(struct hda_codec *codec,
> > -			     struct snd_ctl_elem_value *ucontrol)
> > +			      struct snd_kcontrol *kcontrol,
> > +			      struct snd_ctl_elem_value *ucontrol)
> >  {
> >  	alc_inv_dmic_sync(codec, false);
> >  }
> > @@ -3211,7 +3212,8 @@ static void alc269_fixup_hp_gpio_mute_hook(void *private_data, int enabled)
> >  
> >  /* turn on/off mic-mute LED per capture hook */
> >  static void alc269_fixup_hp_gpio_mic_mute_hook(struct hda_codec *codec,
> > -			       struct snd_ctl_elem_value *ucontrol)
> > +					       struct snd_kcontrol *kcontrol,
> > +					       struct snd_ctl_elem_value *ucontrol)
> >  {
> >  	struct alc_spec *spec = codec->spec;
> >  	unsigned int oldval = spec->gpio_led;
> > @@ -3521,7 +3523,8 @@ static void alc_update_headset_mode(struct hda_codec *codec)
> >  }
> >  
> >  static void alc_update_headset_mode_hook(struct hda_codec *codec,
> > -			     struct snd_ctl_elem_value *ucontrol)
> > +					 struct snd_kcontrol *kcontrol,
> > +					 struct snd_ctl_elem_value *ucontrol)
> >  {
> >  	alc_update_headset_mode(codec);
> >  }
> > diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
> > index 6998cf29b9bc..7311badf6a94 100644
> > --- a/sound/pci/hda/patch_sigmatel.c
> > +++ b/sound/pci/hda/patch_sigmatel.c
> > @@ -194,7 +194,7 @@ struct sigmatel_spec {
> >  	int default_polarity;
> >  
> >  	unsigned int mic_mute_led_gpio; /* capture mute LED GPIO */
> > -	bool mic_mute_led_on; /* current mic mute state */
> > +	unsigned int mic_enabled; /* current mic mute state (bitmask) */
> >  
> >  	/* stream */
> >  	unsigned int stream_delay;
> > @@ -324,19 +324,26 @@ static void stac_gpio_set(struct hda_codec *codec, unsigned int mask,
> >  
> >  /* hook for controlling mic-mute LED GPIO */
> >  static void stac_capture_led_hook(struct hda_codec *codec,
> > -			       struct snd_ctl_elem_value *ucontrol)
> > +				  struct snd_kcontrol *kcontrol,
> > +				  struct snd_ctl_elem_value *ucontrol)
> >  {
> >  	struct sigmatel_spec *spec = codec->spec;
> > -	bool mute;
> > +	unsigned int mask;
> > +	bool cur_mute, prev_mute;
> >  
> > -	if (!ucontrol)
> > +	if (!kcontrol || !ucontrol)
> >  		return;
> >  
> > -	mute = !(ucontrol->value.integer.value[0] ||
> > -		 ucontrol->value.integer.value[1]);
> > -	if (spec->mic_mute_led_on != mute) {
> > -		spec->mic_mute_led_on = mute;
> > -		if (mute)
> > +	mask = 1U << snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
> > +	prev_mute = !spec->mic_enabled;
> > +	if (ucontrol->value.integer.value[0] ||
> > +	    ucontrol->value.integer.value[1])
> > +		spec->mic_enabled |= mask;
> > +	else
> > +		spec->mic_enabled &= ~mask;
> > +	cur_mute = !spec->mic_enabled;
> > +	if (cur_mute != prev_mute) {
> > +		if (cur_mute)
> >  			spec->gpio_data |= spec->mic_mute_led_gpio;
> >  		else
> >  			spec->gpio_data &= ~spec->mic_mute_led_gpio;
> > @@ -4462,7 +4469,7 @@ static void stac_setup_gpio(struct hda_codec *codec)
> >  	if (spec->mic_mute_led_gpio) {
> >  		spec->gpio_mask |= spec->mic_mute_led_gpio;
> >  		spec->gpio_dir |= spec->mic_mute_led_gpio;
> > -		spec->mic_mute_led_on = true;
> > +		spec->mic_enabled = 0;
> >  		spec->gpio_data |= spec->mic_mute_led_gpio;
> >  
> >  		spec->gen.cap_sync_hook = stac_capture_led_hook;
> > diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
> > index 5799fbc24c28..8fe3b8c18ed4 100644
> > --- a/sound/pci/hda/thinkpad_helper.c
> > +++ b/sound/pci/hda/thinkpad_helper.c
> > @@ -39,6 +39,7 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
> >  }
> >  
> >  static void update_tpacpi_micmute_led(struct hda_codec *codec,
> > +				      struct snd_kcontrol *kcontrol,
> >  				      struct snd_ctl_elem_value *ucontrol)
> >  {
> >  	if (!ucontrol || !led_set_func)
> > 
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 

Re: [PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[Takashi Iwai]

At Fri, 31 Jan 2014 18:02:03 +0100,
Takashi Iwai wrote:
> 
> At Fri, 31 Jan 2014 17:52:55 +0100,
> David Henningsson wrote:
> > 
> > On 01/30/2014 06:04 PM, Takashi Iwai wrote:
> > > The current code for controlling mic mute LED in patch_sigmatel.c
> > > blindly assumes that there is a single capture switch.  But, there can
> > > be multiple multiple ones, and each of them flips the state, ended up
> > > in an inconsistent state.
> > > 
> > > For fixing this problem, this patch adds kcontrol to be passed to the
> > > hook function so that the callee can check which switch is being
> > > accessed.  In stac_capture_led_hook(), the state is checked as a
> > > bitmask, and turns on the LED when all capture switches are off.
> > 
> > Is capture switch with non-zero indices turned off by default (and not
> > turned on by e g alsactl init)? Otherwise it sounds like this could be
> > regressing OOTB behaviour, i e, the mic mute LED worked with
> > people/PulseAudio just toggling Capture Switch 0, but now requires
> > manual toggling of the other capture switches...?
> 
> I thought alsactl init doesn't take all captures on but only the
> first capture.
> 
> I don't mind to change the behavior only to check the first capture,
> and ignore the rest.  Although it sounds a bit inconsistent, it might
> be slightly better aligned to the existing behavior.

In that way, the patch will be even simpler like below.
Maybe we should take this one.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: hda - Fix inconsistent Mic mute LED

The current code for controlling mic mute LED in patch_sigmatel.c
blindly assumes that there is a single capture switch.  But, there can
be multiple multiple ones, and each of them flips the state, ended up
in an inconsistent state.

For fixing this problem, this patch adds kcontrol to be passed to the
hook function so that the callee can check which switch is being
accessed.  In stac_capture_led_hook(), the only first capture switch
is checked for LED controls and the other capture switches are
ignored.  This keeps the previous behavior with PulseAudio and others,
at least.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_generic.c     | 8 ++++----
 sound/pci/hda/hda_generic.h     | 1 +
 sound/pci/hda/patch_conexant.c  | 3 ++-
 sound/pci/hda/patch_realtek.c   | 9 ++++++---
 sound/pci/hda/patch_sigmatel.c  | 9 +++++++--
 sound/pci/hda/thinkpad_helper.c | 1 +
 6 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 8321a97d5c05..d9a09bdd09db 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3269,7 +3269,7 @@ static int cap_put_caller(struct snd_kcontrol *kcontrol,
 	mutex_unlock(&codec->control_mutex);
 	snd_hda_codec_flush_cache(codec); /* flush the updates */
 	if (err >= 0 && spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, ucontrol);
+		spec->cap_sync_hook(codec, kcontrol, ucontrol);
 	return err;
 }
 
@@ -3390,7 +3390,7 @@ static int cap_single_sw_put(struct snd_kcontrol *kcontrol,
 		return ret;
 
 	if (spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, ucontrol);
+		spec->cap_sync_hook(codec, kcontrol, ucontrol);
 
 	return ret;
 }
@@ -3795,7 +3795,7 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
 		return 0;
 	snd_hda_activate_path(codec, path, true, false);
 	if (spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, NULL);
+		spec->cap_sync_hook(codec, NULL, NULL);
 	path_power_down_sync(codec, old_path);
 	return 1;
 }
@@ -5270,7 +5270,7 @@ static void init_input_src(struct hda_codec *codec)
 	}
 
 	if (spec->cap_sync_hook)
-		spec->cap_sync_hook(codec, NULL);
+		spec->cap_sync_hook(codec, NULL, NULL);
 }
 
 /* set right pin controls for digital I/O */
diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
index 07f767231c9f..c908afbe4d94 100644
--- a/sound/pci/hda/hda_generic.h
+++ b/sound/pci/hda/hda_generic.h
@@ -274,6 +274,7 @@ struct hda_gen_spec {
 	void (*init_hook)(struct hda_codec *codec);
 	void (*automute_hook)(struct hda_codec *codec);
 	void (*cap_sync_hook)(struct hda_codec *codec,
+			      struct snd_kcontrol *kcontrol,
 			      struct snd_ctl_elem_value *ucontrol);
 
 	/* PCM hooks */
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 4e0ec146553d..bcf91bea3317 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -3291,7 +3291,8 @@ static void cxt_update_headset_mode(struct hda_codec *codec)
 }
 
 static void cxt_update_headset_mode_hook(struct hda_codec *codec,
-			     struct snd_ctl_elem_value *ucontrol)
+					 struct snd_kcontrol *kcontrol,
+					 struct snd_ctl_elem_value *ucontrol)
 {
 	cxt_update_headset_mode(codec);
 }
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 56a8f1876603..6aabaa1c19f5 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -708,7 +708,8 @@ static void alc_inv_dmic_sync(struct hda_codec *codec, bool force)
 }
 
 static void alc_inv_dmic_hook(struct hda_codec *codec,
-			     struct snd_ctl_elem_value *ucontrol)
+			      struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *ucontrol)
 {
 	alc_inv_dmic_sync(codec, false);
 }
@@ -3211,7 +3212,8 @@ static void alc269_fixup_hp_gpio_mute_hook(void *private_data, int enabled)
 
 /* turn on/off mic-mute LED per capture hook */
 static void alc269_fixup_hp_gpio_mic_mute_hook(struct hda_codec *codec,
-			       struct snd_ctl_elem_value *ucontrol)
+					       struct snd_kcontrol *kcontrol,
+					       struct snd_ctl_elem_value *ucontrol)
 {
 	struct alc_spec *spec = codec->spec;
 	unsigned int oldval = spec->gpio_led;
@@ -3521,7 +3523,8 @@ static void alc_update_headset_mode(struct hda_codec *codec)
 }
 
 static void alc_update_headset_mode_hook(struct hda_codec *codec,
-			     struct snd_ctl_elem_value *ucontrol)
+					 struct snd_kcontrol *kcontrol,
+					 struct snd_ctl_elem_value *ucontrol)
 {
 	alc_update_headset_mode(codec);
 }
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
index 6998cf29b9bc..230b4b98d0da 100644
--- a/sound/pci/hda/patch_sigmatel.c
+++ b/sound/pci/hda/patch_sigmatel.c
@@ -324,12 +324,17 @@ static void stac_gpio_set(struct hda_codec *codec, unsigned int mask,
 
 /* hook for controlling mic-mute LED GPIO */
 static void stac_capture_led_hook(struct hda_codec *codec,
-			       struct snd_ctl_elem_value *ucontrol)
+				  struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
 {
 	struct sigmatel_spec *spec = codec->spec;
 	bool mute;
 
-	if (!ucontrol)
+	if (!kcontrol || !ucontrol)
+		return;
+
+	/* response only with the first capture switch */
+	if (snd_ctl_get_ioffidx(kcontrol, &ucontrol->id) != 0)
 		return;
 
 	mute = !(ucontrol->value.integer.value[0] ||
diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 5799fbc24c28..8fe3b8c18ed4 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -39,6 +39,7 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
 }
 
 static void update_tpacpi_micmute_led(struct hda_codec *codec,
+				      struct snd_kcontrol *kcontrol,
 				      struct snd_ctl_elem_value *ucontrol)
 {
 	if (!ucontrol || !led_set_func)
-- 
1.8.5.2

Re: [PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[David Henningsson]

On 01/31/2014 06:02 PM, Takashi Iwai wrote:
> At Fri, 31 Jan 2014 17:52:55 +0100,
> David Henningsson wrote:
>>
>> On 01/30/2014 06:04 PM, Takashi Iwai wrote:
>>> The current code for controlling mic mute LED in patch_sigmatel.c
>>> blindly assumes that there is a single capture switch.  But, there can
>>> be multiple multiple ones, and each of them flips the state, ended up
>>> in an inconsistent state.
>>>
>>> For fixing this problem, this patch adds kcontrol to be passed to the
>>> hook function so that the callee can check which switch is being
>>> accessed.  In stac_capture_led_hook(), the state is checked as a
>>> bitmask, and turns on the LED when all capture switches are off.
>>
>> Is capture switch with non-zero indices turned off by default (and not
>> turned on by e g alsactl init)? Otherwise it sounds like this could be
>> regressing OOTB behaviour, i e, the mic mute LED worked with
>> people/PulseAudio just toggling Capture Switch 0, but now requires
>> manual toggling of the other capture switches...?
> 
> I thought alsactl init doesn't take all captures on but only the
> first capture.
> 
> I don't mind to change the behavior only to check the first capture,
> and ignore the rest.  Although it sounds a bit inconsistent, it might
> be slightly better aligned to the existing behavior.

>From a security perspective the bitmask would be better. I think we just
need to double check that the Capture Switch 1 and 2 are not enabled by
default (in both kernel code and alsactl init).

>> Anyway, the same reasoning would potentially apply to Conexant and
>> Realtek, so perhaps it's time to move some more of mic mute LED handling
>> into hda_generic?
> 
> The hook itself can be used not only for the LED.  So, filtering the
> action in the generic parser side doesn't fit, IMO.

My thought were more on the lines of moving the bitmask check to the
generic parser, and add a new "toggle_mic_mute_led(bool on)" hook. In
order to minimise the duplication of code between
conexant/sigmatel/realtek codecs.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[Takashi Iwai]

At Fri, 31 Jan 2014 18:28:35 +0100,
David Henningsson wrote:
> 
> On 01/31/2014 06:02 PM, Takashi Iwai wrote:
> > At Fri, 31 Jan 2014 17:52:55 +0100,
> > David Henningsson wrote:
> >>
> >> On 01/30/2014 06:04 PM, Takashi Iwai wrote:
> >>> The current code for controlling mic mute LED in patch_sigmatel.c
> >>> blindly assumes that there is a single capture switch.  But, there can
> >>> be multiple multiple ones, and each of them flips the state, ended up
> >>> in an inconsistent state.
> >>>
> >>> For fixing this problem, this patch adds kcontrol to be passed to the
> >>> hook function so that the callee can check which switch is being
> >>> accessed.  In stac_capture_led_hook(), the state is checked as a
> >>> bitmask, and turns on the LED when all capture switches are off.
> >>
> >> Is capture switch with non-zero indices turned off by default (and not
> >> turned on by e g alsactl init)? Otherwise it sounds like this could be
> >> regressing OOTB behaviour, i e, the mic mute LED worked with
> >> people/PulseAudio just toggling Capture Switch 0, but now requires
> >> manual toggling of the other capture switches...?
> > 
> > I thought alsactl init doesn't take all captures on but only the
> > first capture.
> > 
> > I don't mind to change the behavior only to check the first capture,
> > and ignore the rest.  Although it sounds a bit inconsistent, it might
> > be slightly better aligned to the existing behavior.
> 
> From a security perspective the bitmask would be better. I think we just
> need to double check that the Capture Switch 1 and 2 are not enabled by
> default (in both kernel code and alsactl init).

Well, what if it were default on?  Then we should take a security
risk?  So, here is the question.  If we regard this security risk more
serious, we should take the first one.  If we regard the compatibility
more important, we need to take the second.

> >> Anyway, the same reasoning would potentially apply to Conexant and
> >> Realtek, so perhaps it's time to move some more of mic mute LED handling
> >> into hda_generic?
> > 
> > The hook itself can be used not only for the LED.  So, filtering the
> > action in the generic parser side doesn't fit, IMO.
> 
> My thought were more on the lines of moving the bitmask check to the
> generic parser, and add a new "toggle_mic_mute_led(bool on)" hook. In
> order to minimise the duplication of code between
> conexant/sigmatel/realtek codecs.

Yes, but I prefer keeping simplicity and flexibility there.  The
callback is still simple.  Now it receives kcontrol in addition, and
that's all.


Takashi

Re: [PATCH] ALSA: hda - Fix inconsistent Mic mute LED

[Takashi Iwai]

At Fri, 31 Jan 2014 18:41:47 +0100,
Takashi Iwai wrote:
> 
> At Fri, 31 Jan 2014 18:28:35 +0100,
> David Henningsson wrote:
> > 
> > On 01/31/2014 06:02 PM, Takashi Iwai wrote:
> > > At Fri, 31 Jan 2014 17:52:55 +0100,
> > > David Henningsson wrote:
> > >>
> > >> On 01/30/2014 06:04 PM, Takashi Iwai wrote:
> > >>> The current code for controlling mic mute LED in patch_sigmatel.c
> > >>> blindly assumes that there is a single capture switch.  But, there can
> > >>> be multiple multiple ones, and each of them flips the state, ended up
> > >>> in an inconsistent state.
> > >>>
> > >>> For fixing this problem, this patch adds kcontrol to be passed to the
> > >>> hook function so that the callee can check which switch is being
> > >>> accessed.  In stac_capture_led_hook(), the state is checked as a
> > >>> bitmask, and turns on the LED when all capture switches are off.
> > >>
> > >> Is capture switch with non-zero indices turned off by default (and not
> > >> turned on by e g alsactl init)? Otherwise it sounds like this could be
> > >> regressing OOTB behaviour, i e, the mic mute LED worked with
> > >> people/PulseAudio just toggling Capture Switch 0, but now requires
> > >> manual toggling of the other capture switches...?
> > > 
> > > I thought alsactl init doesn't take all captures on but only the
> > > first capture.
> > > 
> > > I don't mind to change the behavior only to check the first capture,
> > > and ignore the rest.  Although it sounds a bit inconsistent, it might
> > > be slightly better aligned to the existing behavior.
> > 
> > From a security perspective the bitmask would be better. I think we just
> > need to double check that the Capture Switch 1 and 2 are not enabled by
> > default (in both kernel code and alsactl init).
> 
> Well, what if it were default on?  Then we should take a security
> risk?  So, here is the question.  If we regard this security risk more
> serious, we should take the first one.  If we regard the compatibility
> more important, we need to take the second.

I decided the first approach, the original patch.
Showing wrongly the mute LED *ON* is worse than possible regressions.

If anyone has opposition, please speak up.


thanks,

Takashi