Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755861AbcKKI0D (ORCPT ); Fri, 11 Nov 2016 03:26:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51198 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753938AbcKKI0C (ORCPT ); Fri, 11 Nov 2016 03:26:02 -0500 Subject: Re: PM regression with LED changes in next-20161109 To: Pavel Machek References: <20161109192301.GS26979@atomide.com> <28234714-3994-6747-9cf8-1ff0b3257f7a@gmail.com> <5bd5333e-0dbb-6333-0a48-ca4d3a990f9c@samsung.com> <20161110162925.GA28832@amd> <20161110204852.GA31728@amd> Cc: Jacek Anaszewski , Jacek Anaszewski , Tony Lindgren , linux-leds@vger.kernel.org, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Darren Hart From: Hans de Goede Message-ID: <64de0593-606a-2fff-3726-b0293a624210@redhat.com> Date: Fri, 11 Nov 2016 09:25:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161110204852.GA31728@amd> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 11 Nov 2016 08:26:01 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3112 Lines: 77 Hi, On 10-11-16 21:48, Pavel Machek wrote: > Hi! > >>>> It seems that we should get back to your initial approach. i.e. only >>>> brightness changes caused by hardware should be reported. >>> >>> I don't think enabling poll() here is good idea. Some hardware won't >>> be able to tell you that it changed the state. Returning maximum >>> brightness trigger is going to use seems easier/better. >> >> The idea here is to allow userspace to poll() on the brightness >> sysfs atrribute to detect changes autonomously done by the hardware, >> such as e.g. happens on both Dell and Thinkpad laptops when pressing >> the keyboard backlight cycle hotkey. Note that these keys do not >> generate key-press events, the cycling through the brightness levels >> (including off) is done entirely in firmware. > > Ok, so you can do that for keyboard backlight on thinkpad... I guess > you handle that as a special trigger on the keyboard leds? No, as said this is all done in firmware, as in this is all dealt with by (presumably) the acpi-ec (acpi-embedded-controller) the kernel does not do anything here, the key is "hardwired" to control the keyboard backlight from the kernels pov. > Can other > triggers, such as heartbeat, be assigned to that "led"? > >> But we do get other ACPI events for this which we can use to let >> userspace know this happens, which is something which user- >> interfaces which allow control over the kbd backlight want to know. > > Yes, you can do that for keyboard backlight... but on thinkpads there > are more leds, such as battery led. That can blink on battery low, and > I don't think you can read the current status from hardware. Well the battery LED does not show up under /sys/class/led so that is not relevant for this situation, anyways ... > Getting current state of led blinking with cpu trigger is also not > quite a good idea. I agree with you that it would be better if reading the brightness sysfs attribute would always return the max brightness for LEDs which are blinking or have a trigger set. But it seems that Jacek disagrees, I will leave further discussion of this up to you and Jacek. > So IMO this should not be done in generic code. Instead, > kbd-backlight trigger should have special attribute, and that one > should be pollable. Again there is no kbd-backlight trigger. >> I understand that we will not always be able to do this, here is the >> Documentation/ABI/testing/sysfs-class-led text I have in mind: >> >> The file supports poll() to detect changes, changes are only >> signalled when this file is written or when the hardware / >> firmware changes the brightness itself and the driver can detect >> this. Changes done by kernel triggers / software blinking are >> not signalled. >> >> Note the "and the driver can detect this" language, that has been there >> since v1 of the poll() notification patch since I already expected not >> all hardware to be able to signal this. > > Lets move it to separate attribute, for triggers that can do that, > please. As explained above this has nothing to do with triggers... Regards, Hans