From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1A5E9C433E0 for ; Wed, 17 Jun 2020 11:55:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E15B520734 for ; Wed, 17 Jun 2020 11:55:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726331AbgFQLzG (ORCPT ); Wed, 17 Jun 2020 07:55:06 -0400 Received: from mx2.suse.de ([195.135.220.15]:57006 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725901AbgFQLzG (ORCPT ); Wed, 17 Jun 2020 07:55:06 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 8AC34AAC7; Wed, 17 Jun 2020 11:55:07 +0000 (UTC) Date: Wed, 17 Jun 2020 13:55:02 +0200 Message-ID: From: Takashi Iwai To: Kai-Heng Feng Cc: tiwai@suse.com, Jaroslav Kysela , Wenwen Wang , Pierre-Louis Bossart , =?UTF-8?B?TWljaGHFgiBNaXJvc8WC?= =?UTF-8?B?YXc=?= , Hui Wang , Kailang Yang , Jian-Hong Pan , Tomas Espeleta , Thomas Hebb , alsa-devel@alsa-project.org (moderated list:SOUND), linux-kernel@vger.kernel.org (open list) Subject: Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support In-Reply-To: <20200617102906.16156-1-kai.heng.feng@canonical.com> References: <20200617102906.16156-1-kai.heng.feng@canonical.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 >