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=-3.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 88C15C433DF for ; Wed, 17 Jun 2020 15:50:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6D59E20773 for ; Wed, 17 Jun 2020 15:50:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726918AbgFQPue (ORCPT ); Wed, 17 Jun 2020 11:50:34 -0400 Received: from mx2.suse.de ([195.135.220.15]:49596 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726511AbgFQPud (ORCPT ); Wed, 17 Jun 2020 11:50:33 -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 768A9AFBF; Wed, 17 Jun 2020 15:50:35 +0000 (UTC) Date: Wed, 17 Jun 2020 17:50:30 +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 , "moderated list:SOUND" , open list Subject: Re: [PATCH v3 1/2] ALSA: hda/realtek: Add COEF controlled micmute LED support In-Reply-To: <934401DE-7A4E-4B2C-8B06-E2AA203A9336@canonical.com> References: <20200617102906.16156-1-kai.heng.feng@canonical.com> <934401DE-7A4E-4B2C-8B06-E2AA203A9336@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 17:24:30 +0200, Kai-Heng Feng wrote: > > > > > On Jun 17, 2020, at 19:55, Takashi Iwai 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 > > > > 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