Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S940975AbcKOOlK (ORCPT ); Tue, 15 Nov 2016 09:41:10 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:38925 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936237AbcKOOlH (ORCPT ); Tue, 15 Nov 2016 09:41:07 -0500 X-AuditID: cbfec7f1-f79f46d0000008eb-29-582b1e7e75c3 Subject: Re: LEDs that change brightness "itself" -- that's a trigger. Re: PM regression with LED changes in next-20161109 To: Hans de Goede , Pavel Machek Cc: 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: Jacek Anaszewski Message-id: Date: Tue, 15 Nov 2016 15:41:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprPKsWRmVeSWpSXmKPExsWy7djP87p1ctoRBhcuG1t0LTSweHN8OpPF 7a0bWCw2Pb7GanF51xw2i61v1jFazF7Sz2Jx99RRNov9V7wcOD2+fZ3E4rFz1l12j80rtDw2 L6n3eL/vKpvHitXf2T0+b5ILYI/isklJzcksSy3St0vgyphz8RFjwWH1im3HNrI3MM5W6GLk 5JAQMJF43ruTHcIWk7hwbz0biC0ksJRRYuo72S5GLiD7M6PEp+nfmGAa3rYchCpaxihx/n0g RNEzRonjC54zgiSEBWokLm1uZu5i5OAQEXCRaG/2BqlhFvjJKNG7vgusmU3AUOLni9dMIDW8 AnYS6ydrgYRZBFQlvm8EKeHgEBWIkNh9NxUkzCsgKPFj8j0WEJsTqPrygv/MIDazgKPEg0U7 WSFseYnNa94yg6ySEDjELrH+Rh87yBwJAVmJTQeYIc53kXi3dSqULSzx6vgWqN9lJDo7DjJB 9E5mlLh47CYrhLOaUWJjZycLRJW1RMP/XywQ2/gkJm2bzgyxgFeio00IosRD4taeM1BDHSVa N9xlhITPajaJtY+6GScwys9C8tAsJE/MQvLEAkbmVYwiqaXFuempxUZ6xYm5xaV56XrJ+bmb GIHJ5vS/4x93ML4/YXWIUYCDUYmHd8dRzQgh1sSy4srcQ4wSHMxKIryKEtoRQrwpiZVVqUX5 8UWlOanFhxilOViUxHn3LLgSLiSQnliSmp2aWpBaBJNl4uCUamBkkOqfe2zSE+GN3+29zn03 UTpQwnP01RQOuW0VJU9+XJS5JGxuZ+160+GNz8FUsyLnawnH8xumKSt+bYnbZ3Vl7tMy1e7Z e36luTJ37Z/AeYrNrS+jXyvrnSSrK/+vQ1HbnM+l22u0KBnkT2gLYT3ovODxk77ZinmsqRkG q/Wz7k/IaA6enaPEUpyRaKjFXFScCACe479LMgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrPIsWRmVeSWpSXmKPExsVy+t/xy7rNctoRBhe/Slp0LTSweHN8OpPF 7a0bWCw2Pb7GanF51xw2i61v1jFazF7Sz2Jx99RRNov9V7wcOD2+fZ3E4rFz1l12j80rtDw2 L6n3eL/vKpvHitXf2T0+b5ILYI9ys8lITUxJLVJIzUvOT8nMS7dVCg1x07VQUshLzE21VYrQ 9Q0JUlIoS8wpBfKMDNCAg3OAe7CSvl2CW8aci48YCw6rV2w7tpG9gXG2QhcjJ4eEgInE25aD bBC2mMSFe+uBbC4OIYEljBKHf55nAUkICTxjlHi9xgXEFhaokTg9/QtjFyMHh4iAi0R7szdE yXI2iR23QkB6mQV+Mkr8ebUFrJdNwFDi54vXTCD1vAJ2Eusna4GEWQRUJb5v7ALbKyoQIXFr 1UdGEJtXQFDix+R7YK2cQOWXF/xnBrGZBWwlFrxfxwJhy0tsXvOWeQKjwCwkLbOQlM1CUraA kXkVo0hqaXFuem6xoV5xYm5xaV66XnJ+7iZGYORtO/Zz8w7GSxuDDzEKcDAq8fDuOKoZIcSa WFZcmXuIUYKDWUmEV1FCO0KINyWxsiq1KD++qDQntfgQoynQExOZpUST84FJIa8k3tDE0NzS 0MjYwsLcyEhJnLfkw5VwIYH0xJLU7NTUgtQimD4mDk6pBkb3eYlSsg4cy59d7F/+xNwn/HDi sa8/Z8ppzfy9Qdv/1U2/6G8xhqf2fVziZLzNd9fM9q7vj2SYT4fWumx6q1zfcsKkIvbBVKHi 7a+rmPr0z157znR2nZ5bxw5Rqw7mCwvfrbbZ1GpdPcF265J5rWuKnQwWFj175nh8b274jCef 9r5lvZB+f5qvEktxRqKhFnNRcSIAT697aNICAAA= X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20161115144102eucas1p1d22f718d2e78637f5f2991539dab24c9 X-Msg-Generator: CA X-Sender-IP: 182.198.249.179 X-Local-Sender: =?UTF-8?B?SmFjZWsgQW5hc3pld3NraRtTUlBPTC1TeXN0ZW0gRlcgIChN?= =?UTF-8?B?Qikb7IK87ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?SmFjZWsgQW5hc3pld3NraRtTUlBPTC1TeXN0ZW0gRlcgIChN?= =?UTF-8?B?QikbU2Ftc3VuZyBFbGVjdHJvbmljcxtTZW5pb3IgU29mdHdhcmUgRW5naW5l?= =?UTF-8?B?ZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjc1MjY=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20161115103141epcas2p251f2521354a8cd7b9dfcfe55bfa482f0 X-RootMTR: 20161115103141epcas2p251f2521354a8cd7b9dfcfe55bfa482f0 References: <9b476f85-d45e-deb6-335d-fc56f6d90350@redhat.com> <127cdd42-6fd8-c671-60b7-3826b351577f@samsung.com> <15cafbf5-d842-e184-2fd4-65f8272f505a@redhat.com> <20161115103133.GA22860@amd> <4e392d5d-eb10-f285-517e-976a55c3e318@samsung.com> <20161115111154.GA5482@amd> <128aae59-b790-42f1-7d66-81391c9330c3@redhat.com> <20161115114859.GA7018@amd> <9e7d5e0f-c4ca-6930-63b9-83dc28517f33@samsung.com> <9d42546d-5917-891c-2b18-ccf7f7328624@redhat.com> <3bb538bb-3c80-70c7-2c44-46b174c90503@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4939 Lines: 120 On 11/15/2016 03:30 PM, Hans de Goede wrote: > Hi, > > On 15-11-16 15:04, Jacek Anaszewski wrote: >> On 11/15/2016 02:48 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 15-11-16 14:28, Jacek Anaszewski wrote: >>>> On 11/15/2016 01:06 PM, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 15-11-16 12:48, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>>>> The LED you are talking about _has_ a trigger, implemented in >>>>>>>>>> hardware. That trigger can change LED brightness behind kernel's >>>>>>>>>> (and >>>>>>>>>> userspace's) back. Don't pretend the trigger does not exist, it >>>>>>>>>> does. >>>>>>>>>> >>>>>>>>>> And when you do that, you'll have nice place to report changes to >>>>>>>>>> userspace -- trigger can now export that information, and offer >>>>>>>>>> poll() >>>>>>>>>> interface. >>>>>>>>> >>>>>>>>> Well, that sounds interesting. It is logically justifiable. >>>>>>>> >>>>>>>> Thanks. >>>>>>>> >>>>>>>>> I initially proposed exactly this solution, with recently >>>>>>>>> added userspace LED being a trigger listener. It seems a bit >>>>>>>>> awkward though. How would you listen to the trigger events? >>>>>>>> >>>>>>>> Trigger exposes a file in sysfs, with poll() working on that file >>>>>>> >>>>>>> Hmm, a new file would give the advantage of making it easy for >>>>>>> userspace to see if the trigger is poll-able, this is likely >>>>>>> better then my own proposal I just send. >>>>>> >>>>>> Good. >>>>>> >>>>>>>> (and >>>>>>>> probably read exposing the current brightness). >>>>>>> >>>>>>> If we do this, can we please make it mirror brightness, iow >>>>>>> also make it writable, that will make it easier for userspace >>>>>>> to deal with it. We can simply re-use the existing show / store >>>>>>> methods for brightness for this. >>>>>> >>>>>> Actually, echo 0 > brightness disables the trigger, IIRC. I'd avoid >>>>>> that here, you want to be able to turn off the backlight but still >>>>>> keep the trigger (and be notified of future changes). >>>>> >>>>> True, that is easy to do the store method will just need to call >>>>> led_set_brightness_nosleep instead of led_set_brightness, this >>>>> will skip the checks to stop blinking in led_set_brightness and >>>>> otherwise is equivalent. >>>>> >>>>>>> I suggest we call it: >>>>>>> >>>>>>> trigger_brightness >>>>>>> >>>>>>> And only register it when a poll-able trigger is present. >>>>>> >>>>>> I'd call it 'current_brightness', but that's no big deal. Yes, only >>>>>> registering it for poll-able triggers makes sense. >>>>> >>>>> current_brightness works for me. I will take a shot a patch-set >>>>> implementing this. >>>> >>>> Word "current" is not precise here. >>>> >>>> It can be thought of as either last brightness set by the >>>> user or the brightness currently written to the device >>>> (returned by brightness file). >>>> >>>> There is a semantic discrepancy in our requirements - >>>> we want the file representing both permanent brightness >>>> set by the user and brightness set by the hardware. >>>> >>>> The two stand in contradiction to each other since >>>> brightness set by the user can be adjusted by the hardware. >>>> >>>> Reading the file shouldn't update brightness property of >>>> struct led_classdev, so it shouldn't call led_update_brightness() >>>> but it still should allow reading brightness set by the >>>> hardware, as a result of each POLLPRI event. So in fact in >>>> the same time it should report both according to our requirements >>>> which is impossible. Do we need three brightness files ? >>> >>> I don't think so, current_brightness actually is an accurate >>> name, if the brightness was last changed by writing from >>> sysfs, the keyboard backlight will honor that and the current_brightness >>> attribute will show the brightness last set through writing it, >>> which matches the actual current brightness of the keyboard backlight. >>> >>> Likewise if it was changed with the hotkey last then the keyboard >>> backlight brightness will be changed and reading from current_brightness >>> will return the new actual brightness. Basically reading from this >>> file will be no different then reading from the normal "brightness" >>> file the difference will be in that it is poll-able and that >>> writing 0 turns off the LED without stopping blinking. >> >> If so then when software blinking enabled, it will return 0 on low >> blink cycle no matter what current brightness level is. > > For software blinking there will not be a current_brightness atrribute, > as we will only add that for LEDs with poll-able triggers. > > Also during the low cycle the LED is off, so its brightness at the > moment of reading (iow currently) is 0. OK, I wanted to make sure that we know what we've agreed on. Earlier there were doubts raised about brightness during software blinking being periodically reported 0, but actually your reasoning seems to be the best answer to that. -- Best regards, Jacek Anaszewski