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

From: Kai-Heng Feng <kai.heng.feng@canonical.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: tiwai@suse.com, "Jaroslav Kysela" <perex@perex.cz>,
	"Michał Mirosław" <mirq-linux@rere.qmqm.pl>,
	"Wenwen Wang" <wenwen@cs.uga.edu>,
	"Hui Wang" <hui.wang@canonical.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Kailang Yang" <kailang@realtek.com>,
	"Jian-Hong Pan" <jian-hong@endlessm.com>,
	"Tomas Espeleta" <tomas.espeleta@gmail.com>,
	"Thomas Hebb" <tommyhebb@gmail.com>,
	"moderated list:SOUND" <alsa-devel@alsa-project.org>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] ALSA: hda: Make codec controlled LED support more generic
Date: Tue, 16 Jun 2020 16:36:41 +0800	[thread overview]
Message-ID: <B11E093E-6A30-4820-AC0E-C2887F5462F2@canonical.com> (raw)
In-Reply-To: <s5himfr30cv.wl-tiwai@suse.de>

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