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

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 3224 bytes --]

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."

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]