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 09:04 PM, Jacek Anaszewski wrote:
> On 01/26/2017 08:51 PM, Jacek Anaszewski wrote:
>> On 01/26/2017 09:33 AM, Pali Rohár wrote:
>>> On Wednesday 25 January 2017 22:35:53 Jacek Anaszewski wrote:
>>>> On 01/25/2017 05:49 PM, Pali Rohár wrote:
>>>>> On Wednesday 25 January 2017 17:11:27 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.
>>>
>>> In which situation this attribute may be different?
>>>
>>> I think some information should be present in this description...
>>
>> The first sentence in description says:
>>
>> "Last hardware set brightness level for this LED" - i.e. it may be
>> different from the _actual_ brightness, which could have been set by
>> LED class driver.
>>
>>>>>> 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>
>>>>>
>>>>> Just speculation: What about using name 'actual_brightness'? It provides
>>>>> same output on read as actual_brightness from /sys/class/backlight/.
>>>>
>>>> This name is ambiguous. Propagating it to the LED subsystem only because
>>>> it exists in backlight is not sufficient argument IMHO.
>>>
>>> I thought that having same name could help userspace and also to have
>>> consistency between similar subsystems. But ok, that was just my
>>> speculation.
>>
>> Please also note the "actual" would be ambiguous especially in view
>> of my above explanation.
>>
>>> I have just one small suggestion in current name "brightness_hw_changed".
>>> As it provides regular read() capability I would suggest to not indicate
>>> "activity" (changed) in name...
>>
>> Without "changed" one could think that brightness file (the one we have
>> currently) doesn't reflect hardware state.
>
> A new, possibly better name has just come to my mind.
> Since the file has two purposes: to notify about brightness changes
> made by hardware and caching last such a change, then we could call
> it hw_brightness_cache. Now there is no activity in the file name.

I must say I don't like that name the "cache" bit suggests that the hardware
is caching system, which is not the case. As for having an acivity in the
name, there is no rule against that.

Regards,

Hans



>