[PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Kai-Heng Feng]

Currently, HDA codec LED class can only be used by GPIO controlled LED.
However, there are some new systems that control LED via COEF instead of
GPIO.

In order to support those systems, create a new helper that can be
facilitated by both COEF controlled and GPIO controlled LED.

In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
don't restore the LED properly after suspend.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v3:
 - Wording.
 - Properly prefix exported symbol.
v2:
 - Prevent platforms like Dell, Lenovoe and Huawei create double LED
   class devices.
 sound/pci/hda/hda_generic.c   |  7 ++++---
 sound/pci/hda/hda_generic.h   |  1 +
 sound/pci/hda/patch_realtek.c | 29 ++++++++++++++++-------------
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index f4e9d9445e18..e4f534e9a88c 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3897,7 +3897,7 @@ enum {
 	MICMUTE_LED_FOLLOW_MUTE,
 };
 
-static void call_micmute_led_update(struct hda_codec *codec)
+void snd_hda_gen_call_micmute_led_update(struct hda_codec *codec)
 {
 	struct hda_gen_spec *spec = codec->spec;
 	unsigned int val;
@@ -3924,6 +3924,7 @@ static void call_micmute_led_update(struct hda_codec *codec)
 	if (spec->micmute_led.update)
 		spec->micmute_led.update(codec);
 }
+EXPORT_SYMBOL_GPL(snd_hda_gen_call_micmute_led_update);
 
 static void update_micmute_led(struct hda_codec *codec,
 			       struct snd_kcontrol *kcontrol,
@@ -3945,7 +3946,7 @@ static void update_micmute_led(struct hda_codec *codec,
 			spec->micmute_led.capture |= mask;
 		else
 			spec->micmute_led.capture &= ~mask;
-		call_micmute_led_update(codec);
+		snd_hda_gen_call_micmute_led_update(codec);
 	}
 }
 
@@ -3982,7 +3983,7 @@ static int micmute_led_mode_put(struct snd_kcontrol *kcontrol,
 	if (mode == spec->micmute_led.led_mode)
 		return 0;
 	spec->micmute_led.led_mode = mode;
-	call_micmute_led_update(codec);
+	snd_hda_gen_call_micmute_led_update(codec);
 	return 1;
 }
 
diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
index fb9f1a90238b..062be242339a 100644
--- a/sound/pci/hda/hda_generic.h
+++ b/sound/pci/hda/hda_generic.h
@@ -353,6 +353,7 @@ unsigned int snd_hda_gen_path_power_filter(struct hda_codec *codec,
 void snd_hda_gen_stream_pm(struct hda_codec *codec, hda_nid_t nid, bool on);
 int snd_hda_gen_fix_pin_power(struct hda_codec *codec, hda_nid_t pin);
 
+void snd_hda_gen_call_micmute_led_update(struct hda_codec *codec);
 int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
 				void (*hook)(struct hda_codec *));
 void snd_hda_gen_fixup_micmute_led(struct hda_codec *codec,
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 6d73f8beadb6..0587d1e96b19 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4114,10 +4114,10 @@ static int micmute_led_set(struct led_classdev *led_cdev,
 			   enum led_brightness brightness)
 {
 	struct hda_codec *codec = dev_to_hda_codec(led_cdev->dev->parent);
-	struct alc_spec *spec = codec->spec;
+	struct hda_gen_spec *spec = codec->spec;
 
-	alc_update_gpio_led(codec, spec->gpio_mic_led_mask,
-			    spec->micmute_led_polarity, !!brightness);
+	spec->micmute_led.led_mode = !brightness;
+	snd_hda_gen_call_micmute_led_update(codec);
 	return 0;
 }
 
@@ -4126,7 +4126,17 @@ static struct led_classdev micmute_led_cdev = {
 	.max_brightness = 1,
 	.brightness_set_blocking = micmute_led_set,
 	.default_trigger = "audio-micmute",
+	.flags = LED_CORE_SUSPENDRESUME,
 };
+
+static void alc_register_micmute_led(struct hda_codec *codec)
+{
+		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+		if (devm_led_classdev_register(&codec->core.dev, &micmute_led_cdev))
+			codec_warn(codec, "failed to register micmute LED\n");
+}
+#else
+static inline void alc_register_micmute_led(struct hda_codec *codec) {}
 #endif
 
 /* setup mute and mic-mute GPIO bits, add hooks appropriately */
@@ -4136,9 +4146,6 @@ static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
 				  unsigned int micmute_mask)
 {
 	struct alc_spec *spec = codec->spec;
-#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
-	int err;
-#endif
 
 	alc_fixup_gpio(codec, action, mute_mask | micmute_mask);
 
@@ -4151,13 +4158,7 @@ static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
 	if (micmute_mask) {
 		spec->gpio_mic_led_mask = micmute_mask;
 		snd_hda_gen_add_micmute_led(codec, alc_gpio_micmute_update);
-
-#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
-		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-		err = devm_led_classdev_register(&codec->core.dev, &micmute_led_cdev);
-		if (err)
-			codec_warn(codec, "failed to register micmute LED\n");
-#endif
+		alc_register_micmute_led(codec);
 	}
 }
 
@@ -4305,6 +4306,7 @@ static void alc285_fixup_hp_coef_micmute_led(struct hda_codec *codec,
 		spec->mic_led_coefbit_on = 1<<13;
 		spec->mic_led_coefbit_off = 0;
 		snd_hda_gen_add_micmute_led(codec, alc_hp_cap_micmute_update);
+		alc_register_micmute_led(codec);
 	}
 }
 
@@ -4319,6 +4321,7 @@ static void alc236_fixup_hp_coef_micmute_led(struct hda_codec *codec,
 		spec->mic_led_coefbit_on = 2<<2;
 		spec->mic_led_coefbit_off = 1<<2;
 		snd_hda_gen_add_micmute_led(codec, alc_hp_cap_micmute_update);
+		alc_register_micmute_led(codec);
 	}
 }
 
-- 
2.17.1

[PATCH v3 2/2] ALSA: hda/realtek: Add mute LED and micmute LED support for HP systems

[Kai-Heng Feng]

There are two more HP systems control mute LED from HDA codec and need
to expose micmute led class so SoF can control micmute LED.

Add quirks to support them.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v3:
 - No change.
v2:
 - Wording.

 sound/pci/hda/patch_realtek.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 0587d1e96b19..cd1a3619806a 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -7473,6 +7473,8 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 	SND_PCI_QUIRK(0x103c, 0x83b9, "HP Spectre x360", ALC269_FIXUP_HP_MUTE_LED_MIC3),
 	SND_PCI_QUIRK(0x103c, 0x8497, "HP Envy x360", ALC269_FIXUP_HP_MUTE_LED_MIC3),
 	SND_PCI_QUIRK(0x103c, 0x84e7, "HP Pavilion 15", ALC269_FIXUP_HP_MUTE_LED_MIC3),
+	SND_PCI_QUIRK(0x103c, 0x869d, "HP", ALC236_FIXUP_HP_MUTE_LED),
+	SND_PCI_QUIRK(0x103c, 0x8729, "HP", ALC285_FIXUP_HP_GPIO_LED),
 	SND_PCI_QUIRK(0x103c, 0x8736, "HP", ALC285_FIXUP_HP_GPIO_LED),
 	SND_PCI_QUIRK(0x103c, 0x877a, "HP", ALC285_FIXUP_HP_MUTE_LED),
 	SND_PCI_QUIRK(0x103c, 0x877d, "HP", ALC236_FIXUP_HP_MUTE_LED),
-- 
2.17.1

Re: [PATCH v3 2/2] ALSA: hda/realtek: Add mute LED and micmute LED support for HP systems

[Takashi Iwai]

On Wed, 17 Jun 2020 12:29:02 +0200,
Kai-Heng Feng wrote:
> 
> There are two more HP systems control mute LED from HDA codec and need
> to expose micmute led class so SoF can control micmute LED.
> 
> Add quirks to support them.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

I guess this can be applied independently from the patch#1?
If so, I'll queue this for for-linus branch.


thanks,

Takashi


> ---
> v3:
>  - No change.
> v2:
>  - Wording.
> 
>  sound/pci/hda/patch_realtek.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 0587d1e96b19..cd1a3619806a 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -7473,6 +7473,8 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
>  	SND_PCI_QUIRK(0x103c, 0x83b9, "HP Spectre x360", ALC269_FIXUP_HP_MUTE_LED_MIC3),
>  	SND_PCI_QUIRK(0x103c, 0x8497, "HP Envy x360", ALC269_FIXUP_HP_MUTE_LED_MIC3),
>  	SND_PCI_QUIRK(0x103c, 0x84e7, "HP Pavilion 15", ALC269_FIXUP_HP_MUTE_LED_MIC3),
> +	SND_PCI_QUIRK(0x103c, 0x869d, "HP", ALC236_FIXUP_HP_MUTE_LED),
> +	SND_PCI_QUIRK(0x103c, 0x8729, "HP", ALC285_FIXUP_HP_GPIO_LED),
>  	SND_PCI_QUIRK(0x103c, 0x8736, "HP", ALC285_FIXUP_HP_GPIO_LED),
>  	SND_PCI_QUIRK(0x103c, 0x877a, "HP", ALC285_FIXUP_HP_MUTE_LED),
>  	SND_PCI_QUIRK(0x103c, 0x877d, "HP", ALC236_FIXUP_HP_MUTE_LED),
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/2] ALSA: hda/realtek: Add mute LED and micmute LED support for HP systems

[Kai-Heng Feng]

> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Wed, 17 Jun 2020 12:29:02 +0200,
> Kai-Heng Feng wrote:
>> 
>> There are two more HP systems control mute LED from HDA codec and need
>> to expose micmute led class so SoF can control micmute LED.
>> 
>> Add quirks to support them.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> I guess this can be applied independently from the patch#1?
> If so, I'll queue this for for-linus branch.

Yes please. Thanks!

Kai-Heng

> 
> 
> thanks,
> 
> Takashi
> 
> 
>> ---
>> v3:
>> - No change.
>> v2:
>> - Wording.
>> 
>> sound/pci/hda/patch_realtek.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>> index 0587d1e96b19..cd1a3619806a 100644
>> --- a/sound/pci/hda/patch_realtek.c
>> +++ b/sound/pci/hda/patch_realtek.c
>> @@ -7473,6 +7473,8 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
>> 	SND_PCI_QUIRK(0x103c, 0x83b9, "HP Spectre x360", ALC269_FIXUP_HP_MUTE_LED_MIC3),
>> 	SND_PCI_QUIRK(0x103c, 0x8497, "HP Envy x360", ALC269_FIXUP_HP_MUTE_LED_MIC3),
>> 	SND_PCI_QUIRK(0x103c, 0x84e7, "HP Pavilion 15", ALC269_FIXUP_HP_MUTE_LED_MIC3),
>> +	SND_PCI_QUIRK(0x103c, 0x869d, "HP", ALC236_FIXUP_HP_MUTE_LED),
>> +	SND_PCI_QUIRK(0x103c, 0x8729, "HP", ALC285_FIXUP_HP_GPIO_LED),
>> 	SND_PCI_QUIRK(0x103c, 0x8736, "HP", ALC285_FIXUP_HP_GPIO_LED),
>> 	SND_PCI_QUIRK(0x103c, 0x877a, "HP", ALC285_FIXUP_HP_MUTE_LED),
>> 	SND_PCI_QUIRK(0x103c, 0x877d, "HP", ALC236_FIXUP_HP_MUTE_LED),
>> -- 
>> 2.17.1
>> 

Re: [PATCH v3 2/2] ALSA: hda/realtek: Add mute LED and micmute LED support for HP systems

[Takashi Iwai]

On Wed, 17 Jun 2020 17:25:10 +0200,
Kai-Heng Feng wrote:
> 
> 
> 
> > On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Wed, 17 Jun 2020 12:29:02 +0200,
> > Kai-Heng Feng wrote:
> >> 
> >> There are two more HP systems control mute LED from HDA codec and need
> >> to expose micmute led class so SoF can control micmute LED.
> >> 
> >> Add quirks to support them.
> >> 
> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > 
> > I guess this can be applied independently from the patch#1?
> > If so, I'll queue this for for-linus branch.
> 
> Yes please. Thanks!

OK, applied now (with Cc to stable).


thanks,

Takashi

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Takashi Iwai]

On Wed, 17 Jun 2020 12:29:01 +0200,
Kai-Heng Feng wrote:
> 
> Currently, HDA codec LED class can only be used by GPIO controlled LED.
> However, there are some new systems that control LED via COEF instead of
> GPIO.
> 
> In order to support those systems, create a new helper that can be
> facilitated by both COEF controlled and GPIO controlled LED.
> 
> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
> don't restore the LED properly after suspend.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Thanks for the quick follow up, the issues I pointed were fixed.

But, now looking at the code change again, I'm no longer sure whether
it's the right move.

Basically, the led cdev should serve only for turning on/off the LED
as given.  But your patch changes it to call the generic mixer
updater, which is rather the one who would call the led cdev state
update itself.  That is, it's other way round.

IMO, what we need is to make all places calling
snd_hda_gen_add_micmute_led() to create led cdev, and change those
calls with snd_hda_gen_fixup_micmute_led().

It'll be a bit more changes and likely not fitting with 5.8, but the
whole result will be more consistent.


thanks,

Takashi


> ---
> v3:
>  - Wording.
>  - Properly prefix exported symbol.
> v2:
>  - Prevent platforms like Dell, Lenovoe and Huawei create double LED
>    class devices.
>  sound/pci/hda/hda_generic.c   |  7 ++++---
>  sound/pci/hda/hda_generic.h   |  1 +
>  sound/pci/hda/patch_realtek.c | 29 ++++++++++++++++-------------
>  3 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index f4e9d9445e18..e4f534e9a88c 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3897,7 +3897,7 @@ enum {
>  	MICMUTE_LED_FOLLOW_MUTE,
>  };
>  
> -static void call_micmute_led_update(struct hda_codec *codec)
> +void snd_hda_gen_call_micmute_led_update(struct hda_codec *codec)
>  {
>  	struct hda_gen_spec *spec = codec->spec;
>  	unsigned int val;
> @@ -3924,6 +3924,7 @@ static void call_micmute_led_update(struct hda_codec *codec)
>  	if (spec->micmute_led.update)
>  		spec->micmute_led.update(codec);
>  }
> +EXPORT_SYMBOL_GPL(snd_hda_gen_call_micmute_led_update);
>  
>  static void update_micmute_led(struct hda_codec *codec,
>  			       struct snd_kcontrol *kcontrol,
> @@ -3945,7 +3946,7 @@ static void update_micmute_led(struct hda_codec *codec,
>  			spec->micmute_led.capture |= mask;
>  		else
>  			spec->micmute_led.capture &= ~mask;
> -		call_micmute_led_update(codec);
> +		snd_hda_gen_call_micmute_led_update(codec);
>  	}
>  }
>  
> @@ -3982,7 +3983,7 @@ static int micmute_led_mode_put(struct snd_kcontrol *kcontrol,
>  	if (mode == spec->micmute_led.led_mode)
>  		return 0;
>  	spec->micmute_led.led_mode = mode;
> -	call_micmute_led_update(codec);
> +	snd_hda_gen_call_micmute_led_update(codec);
>  	return 1;
>  }
>  
> diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
> index fb9f1a90238b..062be242339a 100644
> --- a/sound/pci/hda/hda_generic.h
> +++ b/sound/pci/hda/hda_generic.h
> @@ -353,6 +353,7 @@ unsigned int snd_hda_gen_path_power_filter(struct hda_codec *codec,
>  void snd_hda_gen_stream_pm(struct hda_codec *codec, hda_nid_t nid, bool on);
>  int snd_hda_gen_fix_pin_power(struct hda_codec *codec, hda_nid_t pin);
>  
> +void snd_hda_gen_call_micmute_led_update(struct hda_codec *codec);
>  int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
>  				void (*hook)(struct hda_codec *));
>  void snd_hda_gen_fixup_micmute_led(struct hda_codec *codec,
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 6d73f8beadb6..0587d1e96b19 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -4114,10 +4114,10 @@ static int micmute_led_set(struct led_classdev *led_cdev,
>  			   enum led_brightness brightness)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(led_cdev->dev->parent);
> -	struct alc_spec *spec = codec->spec;
> +	struct hda_gen_spec *spec = codec->spec;
>  
> -	alc_update_gpio_led(codec, spec->gpio_mic_led_mask,
> -			    spec->micmute_led_polarity, !!brightness);
> +	spec->micmute_led.led_mode = !brightness;
> +	snd_hda_gen_call_micmute_led_update(codec);
>  	return 0;
>  }
>  
> @@ -4126,7 +4126,17 @@ static struct led_classdev micmute_led_cdev = {
>  	.max_brightness = 1,
>  	.brightness_set_blocking = micmute_led_set,
>  	.default_trigger = "audio-micmute",
> +	.flags = LED_CORE_SUSPENDRESUME,
>  };
> +
> +static void alc_register_micmute_led(struct hda_codec *codec)
> +{
> +		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +		if (devm_led_classdev_register(&codec->core.dev, &micmute_led_cdev))
> +			codec_warn(codec, "failed to register micmute LED\n");
> +}
> +#else
> +static inline void alc_register_micmute_led(struct hda_codec *codec) {}
>  #endif
>  
>  /* setup mute and mic-mute GPIO bits, add hooks appropriately */
> @@ -4136,9 +4146,6 @@ static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
>  				  unsigned int micmute_mask)
>  {
>  	struct alc_spec *spec = codec->spec;
> -#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
> -	int err;
> -#endif
>  
>  	alc_fixup_gpio(codec, action, mute_mask | micmute_mask);
>  
> @@ -4151,13 +4158,7 @@ static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
>  	if (micmute_mask) {
>  		spec->gpio_mic_led_mask = micmute_mask;
>  		snd_hda_gen_add_micmute_led(codec, alc_gpio_micmute_update);
> -
> -#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
> -		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -		err = devm_led_classdev_register(&codec->core.dev, &micmute_led_cdev);
> -		if (err)
> -			codec_warn(codec, "failed to register micmute LED\n");
> -#endif
> +		alc_register_micmute_led(codec);
>  	}
>  }
>  
> @@ -4305,6 +4306,7 @@ static void alc285_fixup_hp_coef_micmute_led(struct hda_codec *codec,
>  		spec->mic_led_coefbit_on = 1<<13;
>  		spec->mic_led_coefbit_off = 0;
>  		snd_hda_gen_add_micmute_led(codec, alc_hp_cap_micmute_update);
> +		alc_register_micmute_led(codec);
>  	}
>  }
>  
> @@ -4319,6 +4321,7 @@ static void alc236_fixup_hp_coef_micmute_led(struct hda_codec *codec,
>  		spec->mic_led_coefbit_on = 2<<2;
>  		spec->mic_led_coefbit_off = 1<<2;
>  		snd_hda_gen_add_micmute_led(codec, alc_hp_cap_micmute_update);
> +		alc_register_micmute_led(codec);
>  	}
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Kai-Heng Feng]

> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Wed, 17 Jun 2020 12:29:01 +0200,
> Kai-Heng Feng wrote:
>> 
>> Currently, HDA codec LED class can only be used by GPIO controlled LED.
>> However, there are some new systems that control LED via COEF instead of
>> GPIO.
>> 
>> In order to support those systems, create a new helper that can be
>> facilitated by both COEF controlled and GPIO controlled LED.
>> 
>> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
>> don't restore the LED properly after suspend.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> Thanks for the quick follow up, the issues I pointed were fixed.
> 
> But, now looking at the code change again, I'm no longer sure whether
> it's the right move.
> 
> Basically, the led cdev should serve only for turning on/off the LED
> as given.  But your patch changes it to call the generic mixer
> updater, which is rather the one who would call the led cdev state
> update itself.  That is, it's other way round.
> 
> IMO, what we need is to make all places calling
> snd_hda_gen_add_micmute_led() to create led cdev, and change those
> calls with snd_hda_gen_fixup_micmute_led().

Ok, so it's the same as patch v1.
How should we handle vendors other than HP?
Only create led cdev if the ID matches to HP?

> 
> It'll be a bit more changes and likely not fitting with 5.8, but the
> whole result will be more consistent.

A bit off topic, but do you think it's reasonable to also create led cdev for mute LED, in addition to micmute LED?
I just found that the LEDs are still on during system suspend, and led cdev has the ability to turn off the LEDs on system suspend.

Kai-Heng

> 
> 
> thanks,
> 
> Takashi
> 
> 
>> ---
>> v3:
>> - Wording.
>> - Properly prefix exported symbol.
>> v2:
>> - Prevent platforms like Dell, Lenovoe and Huawei create double LED
>>   class devices.
>> sound/pci/hda/hda_generic.c   |  7 ++++---
>> sound/pci/hda/hda_generic.h   |  1 +
>> sound/pci/hda/patch_realtek.c | 29 ++++++++++++++++-------------
>> 3 files changed, 21 insertions(+), 16 deletions(-)
>> 
>> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
>> index f4e9d9445e18..e4f534e9a88c 100644
>> --- a/sound/pci/hda/hda_generic.c
>> +++ b/sound/pci/hda/hda_generic.c
>> @@ -3897,7 +3897,7 @@ enum {
>> 	MICMUTE_LED_FOLLOW_MUTE,
>> };
>> 
>> -static void call_micmute_led_update(struct hda_codec *codec)
>> +void snd_hda_gen_call_micmute_led_update(struct hda_codec *codec)
>> {
>> 	struct hda_gen_spec *spec = codec->spec;
>> 	unsigned int val;
>> @@ -3924,6 +3924,7 @@ static void call_micmute_led_update(struct hda_codec *codec)
>> 	if (spec->micmute_led.update)
>> 		spec->micmute_led.update(codec);
>> }
>> +EXPORT_SYMBOL_GPL(snd_hda_gen_call_micmute_led_update);
>> 
>> static void update_micmute_led(struct hda_codec *codec,
>> 			       struct snd_kcontrol *kcontrol,
>> @@ -3945,7 +3946,7 @@ static void update_micmute_led(struct hda_codec *codec,
>> 			spec->micmute_led.capture |= mask;
>> 		else
>> 			spec->micmute_led.capture &= ~mask;
>> -		call_micmute_led_update(codec);
>> +		snd_hda_gen_call_micmute_led_update(codec);
>> 	}
>> }
>> 
>> @@ -3982,7 +3983,7 @@ static int micmute_led_mode_put(struct snd_kcontrol *kcontrol,
>> 	if (mode == spec->micmute_led.led_mode)
>> 		return 0;
>> 	spec->micmute_led.led_mode = mode;
>> -	call_micmute_led_update(codec);
>> +	snd_hda_gen_call_micmute_led_update(codec);
>> 	return 1;
>> }
>> 
>> diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
>> index fb9f1a90238b..062be242339a 100644
>> --- a/sound/pci/hda/hda_generic.h
>> +++ b/sound/pci/hda/hda_generic.h
>> @@ -353,6 +353,7 @@ unsigned int snd_hda_gen_path_power_filter(struct hda_codec *codec,
>> void snd_hda_gen_stream_pm(struct hda_codec *codec, hda_nid_t nid, bool on);
>> int snd_hda_gen_fix_pin_power(struct hda_codec *codec, hda_nid_t pin);
>> 
>> +void snd_hda_gen_call_micmute_led_update(struct hda_codec *codec);
>> int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
>> 				void (*hook)(struct hda_codec *));
>> void snd_hda_gen_fixup_micmute_led(struct hda_codec *codec,
>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>> index 6d73f8beadb6..0587d1e96b19 100644
>> --- a/sound/pci/hda/patch_realtek.c
>> +++ b/sound/pci/hda/patch_realtek.c
>> @@ -4114,10 +4114,10 @@ static int micmute_led_set(struct led_classdev *led_cdev,
>> 			   enum led_brightness brightness)
>> {
>> 	struct hda_codec *codec = dev_to_hda_codec(led_cdev->dev->parent);
>> -	struct alc_spec *spec = codec->spec;
>> +	struct hda_gen_spec *spec = codec->spec;
>> 
>> -	alc_update_gpio_led(codec, spec->gpio_mic_led_mask,
>> -			    spec->micmute_led_polarity, !!brightness);
>> +	spec->micmute_led.led_mode = !brightness;
>> +	snd_hda_gen_call_micmute_led_update(codec);
>> 	return 0;
>> }
>> 
>> @@ -4126,7 +4126,17 @@ static struct led_classdev micmute_led_cdev = {
>> 	.max_brightness = 1,
>> 	.brightness_set_blocking = micmute_led_set,
>> 	.default_trigger = "audio-micmute",
>> +	.flags = LED_CORE_SUSPENDRESUME,
>> };
>> +
>> +static void alc_register_micmute_led(struct hda_codec *codec)
>> +{
>> +		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>> +		if (devm_led_classdev_register(&codec->core.dev, &micmute_led_cdev))
>> +			codec_warn(codec, "failed to register micmute LED\n");
>> +}
>> +#else
>> +static inline void alc_register_micmute_led(struct hda_codec *codec) {}
>> #endif
>> 
>> /* setup mute and mic-mute GPIO bits, add hooks appropriately */
>> @@ -4136,9 +4146,6 @@ static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
>> 				  unsigned int micmute_mask)
>> {
>> 	struct alc_spec *spec = codec->spec;
>> -#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
>> -	int err;
>> -#endif
>> 
>> 	alc_fixup_gpio(codec, action, mute_mask | micmute_mask);
>> 
>> @@ -4151,13 +4158,7 @@ static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
>> 	if (micmute_mask) {
>> 		spec->gpio_mic_led_mask = micmute_mask;
>> 		snd_hda_gen_add_micmute_led(codec, alc_gpio_micmute_update);
>> -
>> -#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
>> -		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>> -		err = devm_led_classdev_register(&codec->core.dev, &micmute_led_cdev);
>> -		if (err)
>> -			codec_warn(codec, "failed to register micmute LED\n");
>> -#endif
>> +		alc_register_micmute_led(codec);
>> 	}
>> }
>> 
>> @@ -4305,6 +4306,7 @@ static void alc285_fixup_hp_coef_micmute_led(struct hda_codec *codec,
>> 		spec->mic_led_coefbit_on = 1<<13;
>> 		spec->mic_led_coefbit_off = 0;
>> 		snd_hda_gen_add_micmute_led(codec, alc_hp_cap_micmute_update);
>> +		alc_register_micmute_led(codec);
>> 	}
>> }
>> 
>> @@ -4319,6 +4321,7 @@ static void alc236_fixup_hp_coef_micmute_led(struct hda_codec *codec,
>> 		spec->mic_led_coefbit_on = 2<<2;
>> 		spec->mic_led_coefbit_off = 1<<2;
>> 		snd_hda_gen_add_micmute_led(codec, alc_hp_cap_micmute_update);
>> +		alc_register_micmute_led(codec);
>> 	}
>> }
>> 
>> -- 
>> 2.17.1
>> 

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Takashi Iwai]

On Wed, 17 Jun 2020 17:24:30 +0200,
Kai-Heng Feng wrote:
> 
> 
> 
> > On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Wed, 17 Jun 2020 12:29:01 +0200,
> > Kai-Heng Feng wrote:
> >> 
> >> Currently, HDA codec LED class can only be used by GPIO controlled LED.
> >> However, there are some new systems that control LED via COEF instead of
> >> GPIO.
> >> 
> >> In order to support those systems, create a new helper that can be
> >> facilitated by both COEF controlled and GPIO controlled LED.
> >> 
> >> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
> >> don't restore the LED properly after suspend.
> >> 
> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > 
> > Thanks for the quick follow up, the issues I pointed were fixed.
> > 
> > But, now looking at the code change again, I'm no longer sure whether
> > it's the right move.
> > 
> > Basically, the led cdev should serve only for turning on/off the LED
> > as given.  But your patch changes it to call the generic mixer
> > updater, which is rather the one who would call the led cdev state
> > update itself.  That is, it's other way round.
> > 
> > IMO, what we need is to make all places calling
> > snd_hda_gen_add_micmute_led() to create led cdev, and change those
> > calls with snd_hda_gen_fixup_micmute_led().
> 
> Ok, so it's the same as patch v1.
> How should we handle vendors other than HP?
> Only create led cdev if the ID matches to HP?

It's fine to create a LED classdev for other vendors, too.  But the
problem is that it wasn't consistent.  With the LED classdev, we
should use only cdev, instead of mixing up different ways.

I wrote a few patches to convert those mic-mute LED stuff to classdev,
including some cleanups.  The patches are found in
topic/hda-micmute-led branch of sound git tree.  Could you check it?

Note that it's totally untested.  Also it doesn't contain yet
LED_CORE_SUSPENDRESUME, which should be done in another patch in
anyway.

> > It'll be a bit more changes and likely not fitting with 5.8, but the
> > whole result will be more consistent.
> 
> A bit off topic, but do you think it's reasonable to also create led cdev for mute LED, in addition to micmute LED?
> I just found that the LEDs are still on during system suspend, and led cdev has the ability to turn off the LEDs on system suspend.

Yes, it makes sense, too.  But the playback mute handling is a bit
more complicated than the mic-mute LED because it's implemented with a
vmaster hook.  I'll take a look later.


thanks,

Takashi

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Kai-Heng Feng]

> On Jun 17, 2020, at 23:50, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Wed, 17 Jun 2020 17:24:30 +0200,
> Kai-Heng Feng wrote:
>> 
>> 
>> 
>>> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
>>> 
>>> On Wed, 17 Jun 2020 12:29:01 +0200,
>>> Kai-Heng Feng wrote:
>>>> 
>>>> Currently, HDA codec LED class can only be used by GPIO controlled LED.
>>>> However, there are some new systems that control LED via COEF instead of
>>>> GPIO.
>>>> 
>>>> In order to support those systems, create a new helper that can be
>>>> facilitated by both COEF controlled and GPIO controlled LED.
>>>> 
>>>> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
>>>> don't restore the LED properly after suspend.
>>>> 
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> 
>>> Thanks for the quick follow up, the issues I pointed were fixed.
>>> 
>>> But, now looking at the code change again, I'm no longer sure whether
>>> it's the right move.
>>> 
>>> Basically, the led cdev should serve only for turning on/off the LED
>>> as given.  But your patch changes it to call the generic mixer
>>> updater, which is rather the one who would call the led cdev state
>>> update itself.  That is, it's other way round.
>>> 
>>> IMO, what we need is to make all places calling
>>> snd_hda_gen_add_micmute_led() to create led cdev, and change those
>>> calls with snd_hda_gen_fixup_micmute_led().
>> 
>> Ok, so it's the same as patch v1.
>> How should we handle vendors other than HP?
>> Only create led cdev if the ID matches to HP?
> 
> It's fine to create a LED classdev for other vendors, too.  But the
> problem is that it wasn't consistent.  With the LED classdev, we
> should use only cdev, instead of mixing up different ways.

Ok, now I get what you meant...

> 
> I wrote a few patches to convert those mic-mute LED stuff to classdev,
> including some cleanups.  The patches are found in
> topic/hda-micmute-led branch of sound git tree.  Could you check it?
> 
> Note that it's totally untested.  Also it doesn't contain yet
> LED_CORE_SUSPENDRESUME, which should be done in another patch in
> anyway.

Other than LED_CORE_SUSPENDRESUME, it works great!

Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> 
>>> It'll be a bit more changes and likely not fitting with 5.8, but the
>>> whole result will be more consistent.
>> 
>> A bit off topic, but do you think it's reasonable to also create led cdev for mute LED, in addition to micmute LED?
>> I just found that the LEDs are still on during system suspend, and led cdev has the ability to turn off the LEDs on system suspend.
> 
> Yes, it makes sense, too.  But the playback mute handling is a bit
> more complicated than the mic-mute LED because it's implemented with a
> vmaster hook.  I'll take a look later.

Thanks. I'll be happy to test it.

Kai-Heng

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Takashi Iwai]

On Thu, 18 Jun 2020 07:15:21 +0200,
Kai-Heng Feng wrote:
> 
> 
> 
> > On Jun 17, 2020, at 23:50, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Wed, 17 Jun 2020 17:24:30 +0200,
> > Kai-Heng Feng wrote:
> >> 
> >> 
> >> 
> >>> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
> >>> 
> >>> On Wed, 17 Jun 2020 12:29:01 +0200,
> >>> Kai-Heng Feng wrote:
> >>>> 
> >>>> Currently, HDA codec LED class can only be used by GPIO controlled LED.
> >>>> However, there are some new systems that control LED via COEF instead of
> >>>> GPIO.
> >>>> 
> >>>> In order to support those systems, create a new helper that can be
> >>>> facilitated by both COEF controlled and GPIO controlled LED.
> >>>> 
> >>>> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
> >>>> don't restore the LED properly after suspend.
> >>>> 
> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>> 
> >>> Thanks for the quick follow up, the issues I pointed were fixed.
> >>> 
> >>> But, now looking at the code change again, I'm no longer sure whether
> >>> it's the right move.
> >>> 
> >>> Basically, the led cdev should serve only for turning on/off the LED
> >>> as given.  But your patch changes it to call the generic mixer
> >>> updater, which is rather the one who would call the led cdev state
> >>> update itself.  That is, it's other way round.
> >>> 
> >>> IMO, what we need is to make all places calling
> >>> snd_hda_gen_add_micmute_led() to create led cdev, and change those
> >>> calls with snd_hda_gen_fixup_micmute_led().
> >> 
> >> Ok, so it's the same as patch v1.
> >> How should we handle vendors other than HP?
> >> Only create led cdev if the ID matches to HP?
> > 
> > It's fine to create a LED classdev for other vendors, too.  But the
> > problem is that it wasn't consistent.  With the LED classdev, we
> > should use only cdev, instead of mixing up different ways.
> 
> Ok, now I get what you meant...
> 
> > 
> > I wrote a few patches to convert those mic-mute LED stuff to classdev,
> > including some cleanups.  The patches are found in
> > topic/hda-micmute-led branch of sound git tree.  Could you check it?
> > 
> > Note that it's totally untested.  Also it doesn't contain yet
> > LED_CORE_SUSPENDRESUME, which should be done in another patch in
> > anyway.
> 
> Other than LED_CORE_SUSPENDRESUME, it works great!
> 
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Good to hear!

> >>> It'll be a bit more changes and likely not fitting with 5.8, but the
> >>> whole result will be more consistent.
> >> 
> >> A bit off topic, but do you think it's reasonable to also create led cdev for mute LED, in addition to micmute LED?
> >> I just found that the LEDs are still on during system suspend, and led cdev has the ability to turn off the LEDs on system suspend.
> > 
> > Yes, it makes sense, too.  But the playback mute handling is a bit
> > more complicated than the mic-mute LED because it's implemented with a
> > vmaster hook.  I'll take a look later.
> 
> Thanks. I'll be happy to test it.

OK, I worked on this further and converted the whole mute LED handling
with LED classdev.

The topic/hda-micmute-led branch was updated again.  Could you give it
a try?  If that's OK, I'll add your tested-by tag and submit the
patches to ML later.

The old patchset is saved in topic/hda-micmute-led-old branch just for
a reference.


thanks,

Takashi

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Kai-Heng Feng]

> On Jun 18, 2020, at 15:32, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Thu, 18 Jun 2020 07:15:21 +0200,
> Kai-Heng Feng wrote:
>> 
>> 
>> 
>>> On Jun 17, 2020, at 23:50, Takashi Iwai <tiwai@suse.de> wrote:
>>> 
>>> On Wed, 17 Jun 2020 17:24:30 +0200,
>>> Kai-Heng Feng wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
>>>>> 
>>>>> On Wed, 17 Jun 2020 12:29:01 +0200,
>>>>> Kai-Heng Feng wrote:
>>>>>> 
>>>>>> Currently, HDA codec LED class can only be used by GPIO controlled LED.
>>>>>> However, there are some new systems that control LED via COEF instead of
>>>>>> GPIO.
>>>>>> 
>>>>>> In order to support those systems, create a new helper that can be
>>>>>> facilitated by both COEF controlled and GPIO controlled LED.
>>>>>> 
>>>>>> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
>>>>>> don't restore the LED properly after suspend.
>>>>>> 
>>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>> 
>>>>> Thanks for the quick follow up, the issues I pointed were fixed.
>>>>> 
>>>>> But, now looking at the code change again, I'm no longer sure whether
>>>>> it's the right move.
>>>>> 
>>>>> Basically, the led cdev should serve only for turning on/off the LED
>>>>> as given.  But your patch changes it to call the generic mixer
>>>>> updater, which is rather the one who would call the led cdev state
>>>>> update itself.  That is, it's other way round.
>>>>> 
>>>>> IMO, what we need is to make all places calling
>>>>> snd_hda_gen_add_micmute_led() to create led cdev, and change those
>>>>> calls with snd_hda_gen_fixup_micmute_led().
>>>> 
>>>> Ok, so it's the same as patch v1.
>>>> How should we handle vendors other than HP?
>>>> Only create led cdev if the ID matches to HP?
>>> 
>>> It's fine to create a LED classdev for other vendors, too.  But the
>>> problem is that it wasn't consistent.  With the LED classdev, we
>>> should use only cdev, instead of mixing up different ways.
>> 
>> Ok, now I get what you meant...
>> 
>>> 
>>> I wrote a few patches to convert those mic-mute LED stuff to classdev,
>>> including some cleanups.  The patches are found in
>>> topic/hda-micmute-led branch of sound git tree.  Could you check it?
>>> 
>>> Note that it's totally untested.  Also it doesn't contain yet
>>> LED_CORE_SUSPENDRESUME, which should be done in another patch in
>>> anyway.
>> 
>> Other than LED_CORE_SUSPENDRESUME, it works great!
>> 
>> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> Good to hear!
> 
>>>>> It'll be a bit more changes and likely not fitting with 5.8, but the
>>>>> whole result will be more consistent.
>>>> 
>>>> A bit off topic, but do you think it's reasonable to also create led cdev for mute LED, in addition to micmute LED?
>>>> I just found that the LEDs are still on during system suspend, and led cdev has the ability to turn off the LEDs on system suspend.
>>> 
>>> Yes, it makes sense, too.  But the playback mute handling is a bit
>>> more complicated than the mic-mute LED because it's implemented with a
>>> vmaster hook.  I'll take a look later.
>> 
>> Thanks. I'll be happy to test it.
> 
> OK, I worked on this further and converted the whole mute LED handling
> with LED classdev.
> 
> The topic/hda-micmute-led branch was updated again.  Could you give it
> a try?  If that's OK, I'll add your tested-by tag and submit the
> patches to ML later.

Thanks for the work, it works great.

Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> 
> The old patchset is saved in topic/hda-micmute-led-old branch just for
> a reference.
> 
> 
> thanks,
> 
> Takashi

Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support

[Takashi Iwai]

On Thu, 18 Jun 2020 12:16:15 +0200,
Kai-Heng Feng wrote:
> 
> 
> 
> > On Jun 18, 2020, at 15:32, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Thu, 18 Jun 2020 07:15:21 +0200,
> > Kai-Heng Feng wrote:
> >> 
> >> 
> >> 
> >>> On Jun 17, 2020, at 23:50, Takashi Iwai <tiwai@suse.de> wrote:
> >>> 
> >>> On Wed, 17 Jun 2020 17:24:30 +0200,
> >>> Kai-Heng Feng wrote:
> >>>> 
> >>>> 
> >>>> 
> >>>>> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@suse.de> wrote:
> >>>>> 
> >>>>> On Wed, 17 Jun 2020 12:29:01 +0200,
> >>>>> Kai-Heng Feng wrote:
> >>>>>> 
> >>>>>> Currently, HDA codec LED class can only be used by GPIO controlled LED.
> >>>>>> However, there are some new systems that control LED via COEF instead of
> >>>>>> GPIO.
> >>>>>> 
> >>>>>> In order to support those systems, create a new helper that can be
> >>>>>> facilitated by both COEF controlled and GPIO controlled LED.
> >>>>>> 
> >>>>>> In addition to that, add LED_CORE_SUSPENDRESUME flag since some systems
> >>>>>> don't restore the LED properly after suspend.
> >>>>>> 
> >>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>>>> 
> >>>>> Thanks for the quick follow up, the issues I pointed were fixed.
> >>>>> 
> >>>>> But, now looking at the code change again, I'm no longer sure whether
> >>>>> it's the right move.
> >>>>> 
> >>>>> Basically, the led cdev should serve only for turning on/off the LED
> >>>>> as given.  But your patch changes it to call the generic mixer
> >>>>> updater, which is rather the one who would call the led cdev state
> >>>>> update itself.  That is, it's other way round.
> >>>>> 
> >>>>> IMO, what we need is to make all places calling
> >>>>> snd_hda_gen_add_micmute_led() to create led cdev, and change those
> >>>>> calls with snd_hda_gen_fixup_micmute_led().
> >>>> 
> >>>> Ok, so it's the same as patch v1.
> >>>> How should we handle vendors other than HP?
> >>>> Only create led cdev if the ID matches to HP?
> >>> 
> >>> It's fine to create a LED classdev for other vendors, too.  But the
> >>> problem is that it wasn't consistent.  With the LED classdev, we
> >>> should use only cdev, instead of mixing up different ways.
> >> 
> >> Ok, now I get what you meant...
> >> 
> >>> 
> >>> I wrote a few patches to convert those mic-mute LED stuff to classdev,
> >>> including some cleanups.  The patches are found in
> >>> topic/hda-micmute-led branch of sound git tree.  Could you check it?
> >>> 
> >>> Note that it's totally untested.  Also it doesn't contain yet
> >>> LED_CORE_SUSPENDRESUME, which should be done in another patch in
> >>> anyway.
> >> 
> >> Other than LED_CORE_SUSPENDRESUME, it works great!
> >> 
> >> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > 
> > Good to hear!
> > 
> >>>>> It'll be a bit more changes and likely not fitting with 5.8, but the
> >>>>> whole result will be more consistent.
> >>>> 
> >>>> A bit off topic, but do you think it's reasonable to also create led cdev for mute LED, in addition to micmute LED?
> >>>> I just found that the LEDs are still on during system suspend, and led cdev has the ability to turn off the LEDs on system suspend.
> >>> 
> >>> Yes, it makes sense, too.  But the playback mute handling is a bit
> >>> more complicated than the mic-mute LED because it's implemented with a
> >>> vmaster hook.  I'll take a look later.
> >> 
> >> Thanks. I'll be happy to test it.
> > 
> > OK, I worked on this further and converted the whole mute LED handling
> > with LED classdev.
> > 
> > The topic/hda-micmute-led branch was updated again.  Could you give it
> > a try?  If that's OK, I'll add your tested-by tag and submit the
> > patches to ML later.
> 
> Thanks for the work, it works great.
> 
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Great, then I'll submit a patch set.
Thanks for quick testing!


Takashi