Re: [PATCH v6 1/4] leds: class: Add new optional brightness_hw_changed attribute

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 01/26/2017 10:12 PM, Pavel Machek wrote:
> Hi!
>
>> Thanks for the patch. I have a few comments below.
>>
>> On 01/25/2017 05:11 PM, Hans de Goede wrote:
>>> Some LEDs may have their brightness level changed autonomously
>>> (outside of kernel control) by hardware / firmware. This commit
>>> adds support for an optional brightness_hw_changed attribute to
>>> signal such changes to userspace (if a driver can detect them):
>>>
>>> What:		/sys/class/leds/<led>/brightness_hw_changed
>>> Date:		January 2017
>>> KernelVersion:	4.11
>>> Description:
>>> 		Last hardware set brightness level for this LED. Some LEDs
>>> 		may be changed autonomously by hardware/firmware. Only LEDs
>>> 		where this happens and the driver can detect this, will
>>> 		have this file.
>>>
>>> 		This file supports poll() to detect when the hardware
>>> 		changes the brightness.
>>>
>>> 		Reading this file will return the last brightness level set
>>> 		by the hardware, this may be different from the current
>>> 		brightness.
>>>
>>> Drivers which want to support this, simply add LED_BRIGHT_HW_CHANGED to
>>> their flags field and call led_classdev_notify_brightness_hw_changed()
>>> with the hardware set brightness when they detect a hardware / firmware
>>> triggered brightness change.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v6:
>>> -New patch in v6 of this set, replacing previous trigger based API
>>> ---
>>>  Documentation/ABI/testing/sysfs-class-led | 16 +++++++++
>>>  drivers/leds/led-class.c                  | 57 +++++++++++++++++++++++++++++++
>>>  include/linux/leds.h                      |  8 +++++
>>>  3 files changed, 81 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
>>> index 491cdee..9f6e75d 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -23,6 +23,22 @@ Description:
>>>  		If the LED does not support different brightness levels, this
>>>  		should be 1.
>>>
>>> +What:		/sys/class/leds/<led>/brightness_hw_changed
>>> +Date:		January 2017
>>> +KernelVersion:	4.11
>>> +Description:
>>> +		Last hardware set brightness level for this LED. Some LEDs
>>> +		may be changed autonomously by hardware/firmware. Only LEDs
>>> +		where this happens and the driver can detect this, will have
>>> +		this file.
>>> +
>>> +		This file supports poll() to detect when the hardware changes
>>> +		the brightness.
>>> +
>>> +		Reading this file will return the last brightness level set
>>> +		by the hardware, this may be different from the current
>>> +		brightness.
>>
>> Let's return -ENODATA when no such an event occurred till the moment
>> of file reading.
>
> I'd do that.
>
> Actually, maybe we should make this undefined behaviour. "Reading this
> file will return the last brightness level set by the hardware. If the
> software changed the brightness in the meantime, maybe due to active
> trigger, the result is undefined, and it may return error."

As Jacek has said before in the case of software changed brightness
between the poll() waking up and reading the file, we should return
the brightness as set by the last hardware triggered change.

I've added the -ENODATA error (and documented it) for the v7 I'm
about to send.

Regards,

Hans




>
> 									Pavel
>