From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fout-a2-smtp.messagingengine.com (fout-a2-smtp.messagingengine.com [103.168.172.145]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 16EC7223DFB; Sat, 14 Mar 2026 19:03:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.145 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773514982; cv=none; b=m9lVEp7FYNdsQWxbHydTkHFGqNbvAs0qcIjZeBekprpcBYU8TyLIiTaoSAEuekJgY6va+KNUW2W0iJG7An/5SfJmGALqWFkKFNGSyxxXKsvK1vSSQ879o6C4VpM2foKFjhMWuqywCILHTjk48yw+8KF3S9JkciW5ai/cKqaGdJo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773514982; c=relaxed/simple; bh=jSFCI51XBxlead68JqLE6rCRgoGnGkYIWv9uv/LTJyk=; h=MIME-Version:Date:From:To:Cc:Message-Id:In-Reply-To:References: Subject:Content-Type; b=ddZsQnFNHQNhO698jRJGE9CBZoxxLExMVap4pc6fScfxQ1DNvryl+MPmyBdsPEkk+X8PtIbLNy8WzNndREVt7QidXGVG0bmVodWJohl/1oDfSjpdlnRcGcB5gXhy9KUj6Zoj90Qx7w7QSbaC01+uS1Qx9EnaAWM1EuSy05fpAbI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=squebb.ca; spf=pass smtp.mailfrom=squebb.ca; dkim=pass (2048-bit key) header.d=squebb.ca header.i=@squebb.ca header.b=nXNyq6D3; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=OrgNub0N; arc=none smtp.client-ip=103.168.172.145 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=squebb.ca Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=squebb.ca Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=squebb.ca header.i=@squebb.ca header.b="nXNyq6D3"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="OrgNub0N" Received: from phl-compute-02.internal (phl-compute-02.internal [10.202.2.42]) by mailfout.phl.internal (Postfix) with ESMTP id 6501CEC05A6; Sat, 14 Mar 2026 15:03:00 -0400 (EDT) Received: from phl-imap-08 ([10.202.2.84]) by phl-compute-02.internal (MEProxy); Sat, 14 Mar 2026 15:03:00 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=squebb.ca; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm2; t=1773514980; x=1773601380; bh=6B5noVKI/sd7+b+g/lP2eIjDmMdPnEsr3uVANQiYAHk=; b= nXNyq6D3B5V4RG1uiVGI5tIkHMExhKdKRYMN1Sc+Qo1s2vFYB+u+3I3DMIujFOyA 6mrjumeu4jYp9/pug4zQgBYj+JojL3sBxyVxaSaUo36vLx8TJHCYXQ3TPogGeT2T o5UmLTzq/1MrgWiJeIcdoK4WL2ce19EktXVJHKORG/uqXK73WjxskZYXk/TaRvxE M4dVgUz0vNo8wABj6p/gmqoVmUiICAL+IvQbjcccVI01S3VRHiNVcaxziJd+TVsM oKGo9rDQj/ZCGj21o63Lpa2huTJ7n/E12y6FZROoO6h3dNYqoRx8bKgWaWv7pj/x kWtfT1zpv3X5QhtfjRm7MQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1773514980; x= 1773601380; bh=6B5noVKI/sd7+b+g/lP2eIjDmMdPnEsr3uVANQiYAHk=; b=O rgNub0Nt2fZunw3zYF/uESbSQuIci6M/AiXTgs+mWdRL4xrAwlWNUm4q6QdLHFs9 B2IYpoFQWka1PlR8GbSNUNEL20yvPg8BKfiTGG+MmCkNF5o7n6S4FlqFAzhDItdK QOQ1L2CJymIZzmrlH3KfUUrvtChUf/Vp+heXxeysvCyPCYZl3X2VxiX+3Xi2zwIu 4nFiVr03LtQJlr9j14D2KgW0Ea0bV6LpiLtE+zQlbnIx/kUBKxXUdXwjZQI+caw8 7aY9K7U3BM6wpUWpo6pfBoKe5SGWFTxrn4PdZQEkOMV48TDTvfSqxJoek6+2Xqvz JhsNkHIym67hgqdDaEmSA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefgedrtddtgddvleeffeeiucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhepofggfffhvfevkfgjfhfutgfgsehtqhertdertdejnecuhfhrohhmpedfofgrrhhk ucfrvggrrhhsohhnfdcuoehmphgvrghrshhonhdqlhgvnhhovhhosehsqhhuvggssgdrtg grqeenucggtffrrghtthgvrhhnpeehgfelkedujeeugffgudeffffhhfethfegvedtjeeu teellefghfefiefffeekudenucffohhmrghinhepkhguvgdrohhrghenucevlhhushhtvg hrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmphgvrghrshhonhdqlhgv nhhovhhosehsqhhuvggssgdrtggrpdhnsggprhgtphhtthhopedukedpmhhouggvpehsmh htphhouhhtpdhrtghpthhtohepsghlvghunhhgsegthhhrohhmihhumhdrohhrghdprhgt phhtthhopehgrhhovggtkhestghhrhhomhhiuhhmrdhorhhgpdhrtghpthhtohepuggvrh gvkhhjohhhnhdrtghlrghrkhesghhmrghilhdrtghomhdprhgtphhtthhopehikhgvphgr nhhhtgesghhmrghilhdrtghomhdprhgtphhtthhopehvihhshhhnuhhotghvsehgmhgrih hlrdgtohhmpdhrtghpthhtohephhgrnhhsgheskhgvrhhnvghlrdhorhhgpdhrtghpthht ohepkhgrsggvlheskhgvrhhnvghlrdhorhhgpdhrtghpthhtoheplhgvvgeskhgvrhhnvg hlrdhorhhgpdhrtghpthhtohepphgrvhgvlheskhgvrhhnvghlrdhorhhg X-ME-Proxy: Feedback-ID: ibe194615:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id 9023D2CE0072; Sat, 14 Mar 2026 15:02:59 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-ThreadId: Aq-ebexNQroV Date: Sat, 14 Mar 2026 15:02:39 -0400 From: "Mark Pearson" To: "Rong Zhang" , "Andrew Lunn" , "Lee Jones" Cc: "Pavel Machek" , =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= , "Benson Leung" , "Guenter Roeck" , =?UTF-8?Q?Marek_Beh=C3=BAn?= , "Derek J . Clark" , "Hans de Goede" , =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= , "Ike Panhc" , "Vishnu Sankar" , "Vishnu Sankar" , linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, chrome-platform@lists.linux.dev, "platform-driver-x86@vger.kernel.org" Message-Id: <2ac0c46c-805d-4749-a6e0-94b16b104a72@app.fastmail.com> In-Reply-To: References: <20260227190617.271388-1-i@rong.moe> <20260227190617.271388-2-i@rong.moe> <500dfc4b-ebd7-4637-bed0-130fc571923c@lunn.ch> <353713de-b121-4e27-a46f-6ca1941888a6@lunn.ch> <8b25e1ea9bff414442fa47475b9c9ac5b29049d7.camel@rong.moe> <60e60c42-f776-424a-a5b4-7286d33175d1@lunn.ch> <4e839e55-4daa-45cd-b403-d1d6eae63d8d@app.fastmail.com> Subject: Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Fri, Mar 13, 2026, at 10:01 AM, Rong Zhang wrote: > Hi Mark, > > On Thu, 2026-03-12 at 17:46 -0400, Mark Pearson wrote: >>=20 >> On Thu, Mar 12, 2026, at 2:01 PM, Rong Zhang wrote: >> > Hi Andrew, >> >=20 >> > On Wed, 2026-03-11 at 22:29 +0100, Andrew Lunn wrote: >> > > > We just can't prevent the EC from responding to the Fn+Space sh= ortcut. >> > > > So it's essentially user's choice to switch to the hw control t= rigger >> > > > and make it offloaded to hardware (sorry if my cover letter and= replies >> > > > didn't express this well). >> > >=20 >> > > Do you have any control over the EC? >> > >=20 >> > > You have a two bosses dilemma. You need to eliminate one of the >> > > bosses. Either the EC controls the LED, or Linux does. Having both >> > > controlling it is just going to work out badly. >> >=20 >> > I agree that the manufacturers designed the interface poorly :-/ >> >=20 >> > I am not affiliated with any laptop manufacturers so I am just spea= king >> > for myself: >> >=20 >> > IMO the real boss is the user. Both the shortcut (Fn+Space) and the >> > ACPI interface are just "message channels" for the EC to know about= the >> > user's choice. >> >=20 >> > Being able to press such a shortcut always implies that the user is >> > physically in front of the device. In this case it no longer about >> > whether Linux or the EC controls the LED, but both should respect >> > user's choice. That was why brightness_hw_changed was introduced to >> > respect user's choice and pass it to the userspace. So far there has >> > been ~10 drivers utilizing the brightness_hw_changed interface. >> >=20 >> I am affiliated with a laptop manufacturer :) Happy to take suggestio= ns on what should be improved or is missing (can't promise anything but = happy to consider it and take it for review). >>=20 >> We can set the brightness, get the status, and the FW sends events wh= en it changes - all supported on Linux (for Lenovo devices). This looks = like a pretty decent API to me. What is it missing? > > IMO, one missing thing is that there's no approach for the OS to > prevent the EC from responding to Fn+Space. Hence no 100% pure software > control is possible. We end up have a mixed SW + EC control. > > Another missing thing is that there's no approach for the OS to > query/set the ALS-to-brightness curve (or trip points, whatever you > call it) of the EC driven auto brightness mode. Therefore, should we > have a kernel mode ALS-based software trigger, we would never know if > our curve could be offloaded. > > That being said, I don't think either of these two missing things is a > big deal, since most laptops (if not all) are just designed to work > like this and I don't think we would have a kernel mode ALS-based > software trigger any soon. No 100% pure software control wasn't, isn't > and shouldn't be a blocker of using an LED classdev. As I've said, > that's exactly why brightness_hw_changed was introduced. > Got it - thanks for the input. My personal opinion (not feedback from the FW team) - disabling the FN+spacebar is not a useful feature. It's not exactly s= omething you would trigger accidentally. I can't think when people would= need to disable it rather than just choosing to not use it. - Programming the curve would be more interesting - but becomes a bit o= f a pro-user use case. Happy to suggest it, but it's a nice-to-have feat= ure IMO. Agreed that neither of these seem a valid reason to block the implementa= tion. >>=20 >> I don't understand the two bosses issue I'm afraid. The user ('Linux'= in your description?) tells the EC what it wants the LED to be, and the= EC does it. The EC is not a boss. > > The "user" means the one (i.e., a human) who is using the device. And > "message channels" mean: > > User -> pressing Fn+Space -> EC -> update keyboard backlight > > User -> LED classdev / manufacturer utilities -> ACPI -> EC -> update > keyboard backlight > > They are all about the user, as a human, tells their intention to the > device. Of course there may be some userspace software or kernel > trigger blinking the LED, but again, that's still the user's choice and > intention. Hence I don't think EC is a boss either, and the user is the > real boss. > I think we're in agreement :) >>=20 >> > >=20 >> > > > As my previous reply said, it's common that an LED driver can't= prevent >> > > > hardware from changing its state autonomously. Prior to the >> > > > introduction of auto brightness mode, brightness_hw_changed is = enough >> > > > to handle this. The issue only emerges when recent models start= to >> > > > provide an auto brightness mode based on the ALS sensor. >> > >=20 >> > > Do you have a software driven brightness mode based on an ALS? Wh= at >> > > API do you use to control this? Can you use that API, and acceler= ate >> > > it? >> >=20 >> > All devices I've seen implement an EC driven auto brightness mode b= ased >> > on an ALS. >> >=20 >> > @Mark, do you know any device implementing a software driven auto >> > brightness mode? >> >=20 >>=20 >> I don't - to my knowledge in auto mode it's always driven by the HW/F= W. > > Thanks. > >>=20 >> If there was a SW approach it would read the sensor and set the brigh= tness to low/medium/high (and not to auto) so I'm struggling to understa= nd the issue here. What am I missing? > > My understanding of Andrew's words (see also his previous replies) is: > > hw control trigger is fundamentally an accelerated (offloaded) > software trigger. Only if there is a software-driven ALS-based > trigger and the curve matches the FW one can we treat the auto > brightness mode as a hw control trigger. > > But those laptops with an ALS and keyboard backlight are not designed > to work like this, and the curve may be very specific to the ALS and > the luminance of the keyboard backlight. So I asked you to confirm if > there is any device designed to use an software driven auto brightness > mode (even under Windows). > > Hmm, forgot to ask about that... Is there any device comes with ALS- > based auto brightness mode but Linux cannot read the the ALS? If such > devices exist, the "accelerated software trigger" model is no longer > relevant. > I don't know of any SW driven auto brightness mode. Afraid I also don't know about reading the ALS. I'll see if I can find o= ut - but I don't think it's directly important to this series which is a= bout supporting the new HW automated feature. We're not trying to implement an automated SW based control feature with= this series :) > Also that's why we have private LED triggers -- it's useful when the > HW/FW has its own triggering functionality. For example, "cros-ec-led" > has a private trigger and declares it as its hw control trigger. > ack >>=20 >> > >=20 >> > > > FYI, desktop environments (e.g., GNOME, KDE) can control the ba= cklight >> > > > brightness of keyboards through sliders and heavily depend on >> > > > brightness_hw_changed to update the sliders and display OSD onc= e the >> > > > shortcut is pressed. >> > >=20 >> > > Hold up. Terminology problem. I'm a networking guy, i know networ= king >> > > terms. By slider, do you mean a software scroll bar sort of thing= ?=C2=A0 >> >=20 >> > Yes. See >> > https://blogs.kde.org/2024/09/04/brightness-controls-for-all-your-d= isplays/ >> >=20 >> > (it was about display backlight but it also showed the keyboard one= in >> > the same image) >> >=20 >> > > I'm >> > > an XFCE users. I can control the display backlight with a slider = on >> > > the battery charge applet. And i can use Fn F4/F5. I've not seen a >> > > software scroll bar for the keyboard backlight, but i think >> > > allows me to change the keyboard backlight. >> > >=20 >> > > So we have a slider, which is purely software, Linux. And we have= key >> > > presses, which you are calling shortcut, which the EC acts on, and >> > > might tell Linux, maybe, but not about the key press, but the act= ion >> > > the EC took because of the key press. >> >=20 >> > "might tell", "maybe" >> >=20 >> > It always tells the OS that the state of keyboard backlight has >> > changed. >> >=20 >> > >=20 >> > > You have some API to the EC to ask it nicely to act on the softwa= re >> > > slide, but it is the EC which really controls the LED, not Linux. >> > >=20 >> > > To me a Linux LED is a poor fit for what you want, and i think a >> > > trigger is even worse. The problems you have are because the >> > > LED+trigger model, plus using the hardware for acceleration, does= not >> > > fit with the EC actually controlling the hardware. >> > >=20 >> > > I would suggest you look at the API the EC exports and find a bet= ter >> > > model for it. >> >=20 >> > An LED classdev may be unable to perfectly fit this, but nothing is >> > perfect and so far it's the best thing we have. It's a fortunate to >> > have the LED subsystem. Windows, without a similar interface, ends = up >> > being filled with disgusting software pre-installed by the >> > manufacturer. >> >=20 >>=20 >> Afraid I don't understand what we are debating here. >>=20 >> Isn't the whole goal of this patch to make it so LED classdev is a be= tter fit to address missing functionality? Why would switching to someth= ing else (I have no idea what) be better? Especially given the the keybo= ard backlight is currently a LED device, and changing that would potenti= ally break things for users. >>=20 >> From my perspective if I could just tear this out and have a Lenovo o= nly keyboard_backlight implementation under (for example) /sys/devices/t= hinkpad_acpi it would be so much easier. But I don't think it is the rig= ht thing to do. My experience is if we define a common approach then all= vendors will use it going forward - which is better for the Linux exper= ience overall. >> Or we don't have fully implemented features for Linux users? That's k= inda sucky. > > I agreed with you. Just some supplemental information: > > ideapad-laptop has an custom attribute "fn_lock". This used to be > the=C2=A0only way for userspace to turn on/off FnLock. The attribute d= oes > not support any notification mechanism. > > Since devices with FnLock also come with an LED indicating the status > of FnLock, an LED classdev has been added with a new > LED_FUNCTION_FNLOCK macro for dt-bindings and UAPI. See commit > 7ab6c64663a0 ("dt-bindings: leds: Add LED_FUNCTION_FNLOCK") and commit > 07f48f668fac ("platform/x86: ideapad-laptop: add FnLock LED class > device"). Since then, userspace receives notifications through > brightness_hw_changed when the user presses Fn+Esc. > > I think this shows that an LED classdev, as a common interface, has its > vitality even when being used in a very specific use case. > >>=20 >> I don't think the two bosses argument is valid (or at least I don't u= nderstand it). Are there any other critical implementation details that = make this a poor choice and will bite us in the long run? >>=20 >> I personally find the implementation more complicated than I original= ly expected, but having looked at it and understood better what Rong was= proposing I understand the benefits and I think it works. We're still c= hecking it out on Thinkpad to confirm that, but this patch is a RFC so I= think that's part of the process. >>=20 >> > IMO the presence of brightness_hw_changed proves that an LED classd= ev, >> > as long as appropriate interfaces are provided, can work well with = such >> > hardware. And I don't think there is too much difference between EC >> > setting a static brightness value due to a shortcut being pressed a= nd >> > EC turning on/off the auto brightness mode due to the same shortcut= -- >> > if we can handle the former well, we can also implement a similar >> > mechanism for the latter. >> >=20 >> >=20 >> > Do you have any recommendations for a "better model"? >> >=20 >> > Did you mean do not register LED classdevs at all? This isn't really >> > viable and will break userspace. Some drivers has been using LED >> > classdevs for keyboard backlight for over a decade. And many >> > `*::kbd_backlight' LEDs rely on brightness_hw_changed, so it's very >> > common that we can't take 100% control over EC. LED classdevs and E= C- >> > controlled keyboard backlight have lived in harmony for a long time. >> >=20 >> > If we still register the keybaord backlight as an LED classdev but = use >> > a custom attribute to report/set the auto brightness mode. IMO this= is >> > even uglier than LED+trigger, as writing to such a non-brightness >> > attribute will interfere with the brightness attribute and the acti= ve >> > trigger and vice versa. Even if we rule out the EC's action, such >> > interference still exists as long as users use the attribute. >> >=20 >> >=20 >> > As for the software-vs-hardware priority issue, how about adding an >> > attribute "hw_change_policy" so that users can select if the softwa= re >> > state should be always reimposed to hardware? >>=20 >> Is this needed? When wouldn't this be the case? >>=20 >> If the SW sets a specific brightness that should become the setting. = It would override any previous choices and tell the HW that is what is w= anted now - don't change it (until the user says otherwise). >> If we're in auto mode and the HW changes the brightness - it doesn't = change the setting from auto mode, just the reported brightness level if= queried. > > My understanding of Andrew's words is: > > Linux LED should always be the boss. Tell the HW: don't change it *= even > if the user presses Fn+Space*. Failing to accomplish this means tha= t we > are in "a two bosses dilemma", hence "a Linux LED is a poor fit for > what you want" and "a trigger is even worse". > > Since Andrew cares about the software's precedence, the best thing we > can do is adding an attribute for users to select their preference. The > attribute's default value must not be reimpose, otherwise it breaks > existing userspace programs relying on brightness_hw_changed and > confuses most users. > For the current HW, the attribute is not going to be supported. There's = no way to disable FN+space as it goes directly to the BIOS (which then n= otifies the OS). I guess the OS can get the notification and then say 'nope - despite the= fact they just pressed the FN + spacebar, they actually don't want anyt= hing to change', and revert the setting to the previous setting.=20 If the user had to specifically set the attribute to do this it's fine -= but it feels like something that nobody would ever use and a whole bunc= h of extra complexity, to me > But yeah, personally I don't think it's needed either. It's been 9 > years since the introduction of brightness_hw_changed, and there's no > complaint about HW/FW overriding the software configured brightness. > After all, it's the user who decides to press the shortcut and asks the > EC to change the brightness or turn on/off the auto brightness mode. > I think you and I are both in agreement.=20 Andrew - are you still against the current proposal or have your concern= s been addressed? How do we proceed on finding something that we can move forward with? Mark