[PATCH 1/2] ALSA: hda: Make codec controlled LED support more generic

[PATCH 1/2] ALSA: hda: Make codec controlled LED support more generic

[Kai-Heng Feng]

Currently, only HDA codec GPIO controlled LED class is supported, and
only via platform specific quirk.

There are systems that control LED via COEF instead of GPIO, and to
support those systems, move the LED class registration to
snd_hda_gen_add_micmute_led(), so all systems can facilitate the same
interface.

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>
---
 sound/pci/hda/hda_generic.c   | 28 ++++++++++++++++++++++++++++
 sound/pci/hda/patch_realtek.c | 30 ------------------------------
 2 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index f4e9d9445e18..4242407734c0 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -4006,6 +4006,28 @@ static const struct snd_kcontrol_new micmute_led_mode_ctl = {
  *
  * Returns 0 if the hook is established or a negative error code.
  */
+
+#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
+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 hda_gen_spec *spec = codec->spec;
+
+	spec->micmute_led.led_mode = !brightness;
+	call_micmute_led_update(codec);
+	return 0;
+}
+
+static struct led_classdev micmute_led_cdev = {
+	.name = "hda::micmute",
+	.max_brightness = 1,
+	.brightness_set_blocking = micmute_led_set,
+	.default_trigger = "audio-micmute",
+	.flags = LED_CORE_SUSPENDRESUME,
+};
+#endif
+
 int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
 				void (*hook)(struct hda_codec *))
 {
@@ -4019,6 +4041,12 @@ int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
 	spec->cap_sync_hook = update_micmute_led;
 	if (!snd_hda_gen_add_kctl(spec, NULL, &micmute_led_mode_ctl))
 		return -ENOMEM;
+
+#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
+	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");
+#endif
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led);
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 6d73f8beadb6..cead44a6c6cd 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4109,26 +4109,6 @@ static void alc_gpio_micmute_update(struct hda_codec *codec)
 			    spec->gen.micmute_led.led_value);
 }
 
-#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
-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;
-
-	alc_update_gpio_led(codec, spec->gpio_mic_led_mask,
-			    spec->micmute_led_polarity, !!brightness);
-	return 0;
-}
-
-static struct led_classdev micmute_led_cdev = {
-	.name = "hda::micmute",
-	.max_brightness = 1,
-	.brightness_set_blocking = micmute_led_set,
-	.default_trigger = "audio-micmute",
-};
-#endif
-
 /* setup mute and mic-mute GPIO bits, add hooks appropriately */
 static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
 				  int action,
@@ -4136,9 +4116,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 +4128,6 @@ 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
 	}
 }
 
-- 
2.17.1

[PATCH 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 and control
micmute LED from SoF. Add IDs to support them.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 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 cead44a6c6cd..f7398633d736 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -7440,6 +7440,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 1/2] ALSA: hda: Make codec controlled LED support more generic

[Takashi Iwai]

On Tue, 16 Jun 2020 06:46:58 +0200,
Kai-Heng Feng wrote:
> 
> Currently, only HDA codec GPIO controlled LED class is supported, and
> only via platform specific quirk.
> 
> There are systems that control LED via COEF instead of GPIO, and to
> support those systems, move the LED class registration to
> snd_hda_gen_add_micmute_led(), so all systems can facilitate the same
> interface.

This will result in creating the led cdev always when the mic-mute LED
feature is used, not only for HP but also for Dell and Huawei.
Is this the intended result?


thanks,

Takashi

> 
> 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>
> ---
>  sound/pci/hda/hda_generic.c   | 28 ++++++++++++++++++++++++++++
>  sound/pci/hda/patch_realtek.c | 30 ------------------------------
>  2 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index f4e9d9445e18..4242407734c0 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -4006,6 +4006,28 @@ static const struct snd_kcontrol_new micmute_led_mode_ctl = {
>   *
>   * Returns 0 if the hook is established or a negative error code.
>   */
> +
> +#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
> +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 hda_gen_spec *spec = codec->spec;
> +
> +	spec->micmute_led.led_mode = !brightness;
> +	call_micmute_led_update(codec);
> +	return 0;
> +}
> +
> +static struct led_classdev micmute_led_cdev = {
> +	.name = "hda::micmute",
> +	.max_brightness = 1,
> +	.brightness_set_blocking = micmute_led_set,
> +	.default_trigger = "audio-micmute",
> +	.flags = LED_CORE_SUSPENDRESUME,
> +};
> +#endif
> +
>  int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
>  				void (*hook)(struct hda_codec *))
>  {
> @@ -4019,6 +4041,12 @@ int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
>  	spec->cap_sync_hook = update_micmute_led;
>  	if (!snd_hda_gen_add_kctl(spec, NULL, &micmute_led_mode_ctl))
>  		return -ENOMEM;
> +
> +#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
> +	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");
> +#endif
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led);
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 6d73f8beadb6..cead44a6c6cd 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -4109,26 +4109,6 @@ static void alc_gpio_micmute_update(struct hda_codec *codec)
>  			    spec->gen.micmute_led.led_value);
>  }
>  
> -#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
> -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;
> -
> -	alc_update_gpio_led(codec, spec->gpio_mic_led_mask,
> -			    spec->micmute_led_polarity, !!brightness);
> -	return 0;
> -}
> -
> -static struct led_classdev micmute_led_cdev = {
> -	.name = "hda::micmute",
> -	.max_brightness = 1,
> -	.brightness_set_blocking = micmute_led_set,
> -	.default_trigger = "audio-micmute",
> -};
> -#endif
> -
>  /* setup mute and mic-mute GPIO bits, add hooks appropriately */
>  static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
>  				  int action,
> @@ -4136,9 +4116,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 +4128,6 @@ 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
>  	}
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] ALSA: hda: Make codec controlled LED support more generic

[Kai-Heng Feng]

> On Jun 16, 2020, at 16:01, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Tue, 16 Jun 2020 06:46:58 +0200,
> Kai-Heng Feng wrote:
>> 
>> Currently, only HDA codec GPIO controlled LED class is supported, and
>> only via platform specific quirk.
>> 
>> There are systems that control LED via COEF instead of GPIO, and to
>> support those systems, move the LED class registration to
>> snd_hda_gen_add_micmute_led(), so all systems can facilitate the same
>> interface.
> 
> This will result in creating the led cdev always when the mic-mute LED
> feature is used, not only for HP but also for Dell and Huawei.
> Is this the intended result?

Ok, I can see there are other vendors calling snd_hda_gen_fixup_micmute_led() to create LED class.
And no, that's not the intended result.

I guess a chained function can be a better approach.

I'll send another series.

Kai-Heng

> 
> 
> thanks,
> 
> Takashi
> 
>> 
>> 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>
>> ---
>> sound/pci/hda/hda_generic.c   | 28 ++++++++++++++++++++++++++++
>> sound/pci/hda/patch_realtek.c | 30 ------------------------------
>> 2 files changed, 28 insertions(+), 30 deletions(-)
>> 
>> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
>> index f4e9d9445e18..4242407734c0 100644
>> --- a/sound/pci/hda/hda_generic.c
>> +++ b/sound/pci/hda/hda_generic.c
>> @@ -4006,6 +4006,28 @@ static const struct snd_kcontrol_new micmute_led_mode_ctl = {
>>  *
>>  * Returns 0 if the hook is established or a negative error code.
>>  */
>> +
>> +#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
>> +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 hda_gen_spec *spec = codec->spec;
>> +
>> +	spec->micmute_led.led_mode = !brightness;
>> +	call_micmute_led_update(codec);
>> +	return 0;
>> +}
>> +
>> +static struct led_classdev micmute_led_cdev = {
>> +	.name = "hda::micmute",
>> +	.max_brightness = 1,
>> +	.brightness_set_blocking = micmute_led_set,
>> +	.default_trigger = "audio-micmute",
>> +	.flags = LED_CORE_SUSPENDRESUME,
>> +};
>> +#endif
>> +
>> int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
>> 				void (*hook)(struct hda_codec *))
>> {
>> @@ -4019,6 +4041,12 @@ int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
>> 	spec->cap_sync_hook = update_micmute_led;
>> 	if (!snd_hda_gen_add_kctl(spec, NULL, &micmute_led_mode_ctl))
>> 		return -ENOMEM;
>> +
>> +#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
>> +	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");
>> +#endif
>> 	return 0;
>> }
>> EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led);
>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>> index 6d73f8beadb6..cead44a6c6cd 100644
>> --- a/sound/pci/hda/patch_realtek.c
>> +++ b/sound/pci/hda/patch_realtek.c
>> @@ -4109,26 +4109,6 @@ static void alc_gpio_micmute_update(struct hda_codec *codec)
>> 			    spec->gen.micmute_led.led_value);
>> }
>> 
>> -#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
>> -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;
>> -
>> -	alc_update_gpio_led(codec, spec->gpio_mic_led_mask,
>> -			    spec->micmute_led_polarity, !!brightness);
>> -	return 0;
>> -}
>> -
>> -static struct led_classdev micmute_led_cdev = {
>> -	.name = "hda::micmute",
>> -	.max_brightness = 1,
>> -	.brightness_set_blocking = micmute_led_set,
>> -	.default_trigger = "audio-micmute",
>> -};
>> -#endif
>> -
>> /* setup mute and mic-mute GPIO bits, add hooks appropriately */
>> static void alc_fixup_hp_gpio_led(struct hda_codec *codec,
>> 				  int action,
>> @@ -4136,9 +4116,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 +4128,6 @@ 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
>> 	}
>> }
>> 
>> -- 
>> 2.17.1
>>