> On Jun 17, 2020, at 19:55, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxxxxxxxx>
>
> 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
>>
