| Message ID | 20170125161130.5424-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
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. > > 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/.
H Hans, 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. > + > What: /sys/class/leds/<led>/trigger > Date: March 2006 > KernelVersion: 2.6.17 > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 326ee6e..27aa7b6 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -103,6 +103,52 @@ static const struct attribute_group *led_groups[] = { > NULL, > }; > > +static ssize_t brightness_hw_changed_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", led_cdev->brightness_hw_changed); > +} > + > +static DEVICE_ATTR_RO(brightness_hw_changed); > + > +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev) > +{ > + struct device *dev = led_cdev->dev; > + int ret; > + > + ret = device_create_file(dev, &dev_attr_brightness_hw_changed); > + if (ret) { > + dev_err(dev, "Error creating brightness_hw_changed\n"); > + return ret; > + } > + > + led_cdev->brightness_hw_changed_kn = > + sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed"); > + if (!led_cdev->brightness_hw_changed_kn) { > + dev_err(dev, "Error getting brightness_hw_changed kn\n"); > + device_remove_file(dev, &dev_attr_brightness_hw_changed); > + return -ENXIO; > + } > + > + return 0; > +} > + > +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev) > +{ > + sysfs_put(led_cdev->brightness_hw_changed_kn); > + device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed); > +} > + > +void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + led_cdev->brightness_hw_changed = brightness; > + sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn); > +} > +EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed); > > /** > * led_classdev_suspend - suspend an led_classdev. > * @led_cdev: the led_classdev to suspend. > @@ -204,6 +250,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) > dev_warn(parent, "Led %s renamed to %s due to name collision", > led_cdev->name, dev_name(led_cdev->dev)); > > + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { > + ret = led_add_brightness_hw_changed(led_cdev); > + if (ret) { > + device_unregister(led_cdev->dev); > + return ret; > + } > + } > + > led_cdev->work_flags = 0; > #ifdef CONFIG_LEDS_TRIGGERS > init_rwsem(&led_cdev->trigger_lock); > @@ -256,6 +310,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev) > > flush_work(&led_cdev->set_brightness_work); > > + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) > + led_remove_brightness_hw_changed(led_cdev); > + > device_unregister(led_cdev->dev); > > down_write(&leds_list_lock); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 569cb53..aa452c8 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -13,6 +13,7 @@ > #define __LINUX_LEDS_H_INCLUDED > > #include <linux/device.h> > +#include <linux/kernfs.h> > #include <linux/list.h> > #include <linux/mutex.h> > #include <linux/rwsem.h> > @@ -35,6 +36,7 @@ struct led_classdev { > const char *name; > enum led_brightness brightness; > enum led_brightness max_brightness; > + enum led_brightness brightness_hw_changed; Could we add Kconfig option for this feature and build the whole infrastructure only if enabled? > int flags; > > /* Lower 16 bits reflect status */ > @@ -46,6 +48,7 @@ struct led_classdev { > #define LED_DEV_CAP_FLASH (1 << 18) > #define LED_HW_PLUGGABLE (1 << 19) > #define LED_PANIC_INDICATOR (1 << 20) > +#define LED_BRIGHT_HW_CHANGED (1 << 21) > > /* set_brightness_work / blink_timer flags, atomic, private. */ > unsigned long work_flags; > @@ -99,6 +102,9 @@ struct led_classdev { > struct work_struct set_brightness_work; > int delayed_set_value; > > + /* For LEDs with brightness_hw_changed sysfs attribute */ > + struct kernfs_node *brightness_hw_changed_kn; > + > #ifdef CONFIG_LEDS_TRIGGERS > /* Protects the trigger data below */ > struct rw_semaphore trigger_lock; > @@ -123,6 +129,8 @@ extern void devm_led_classdev_unregister(struct device *parent, > struct led_classdev *led_cdev); > extern void led_classdev_suspend(struct led_classdev *led_cdev); > extern void led_classdev_resume(struct led_classdev *led_cdev); > +extern void led_classdev_notify_brightness_hw_changed( > + struct led_classdev *led_cdev, enum led_brightness brightness); > > /** > * led_blink_set - set blinking with software fallback >
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. >> >> 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.
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... > >> 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. 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...
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.
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.
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
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 >
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 >
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. + What: /sys/class/leds/<led>/trigger Date: March 2006 KernelVersion: 2.6.17 diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 326ee6e..27aa7b6 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -103,6 +103,52 @@ static const struct attribute_group *led_groups[] = { NULL, }; +static ssize_t brightness_hw_changed_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + + return sprintf(buf, "%u\n", led_cdev->brightness_hw_changed); +} + +static DEVICE_ATTR_RO(brightness_hw_changed); + +static int led_add_brightness_hw_changed(struct led_classdev *led_cdev) +{ + struct device *dev = led_cdev->dev; + int ret; + + ret = device_create_file(dev, &dev_attr_brightness_hw_changed); + if (ret) { + dev_err(dev, "Error creating brightness_hw_changed\n"); + return ret; + } + + led_cdev->brightness_hw_changed_kn = + sysfs_get_dirent(dev->kobj.sd, "brightness_hw_changed"); + if (!led_cdev->brightness_hw_changed_kn) { + dev_err(dev, "Error getting brightness_hw_changed kn\n"); + device_remove_file(dev, &dev_attr_brightness_hw_changed); + return -ENXIO; + } + + return 0; +} + +static void led_remove_brightness_hw_changed(struct led_classdev *led_cdev) +{ + sysfs_put(led_cdev->brightness_hw_changed_kn); + device_remove_file(led_cdev->dev, &dev_attr_brightness_hw_changed); +} + +void led_classdev_notify_brightness_hw_changed(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + led_cdev->brightness_hw_changed = brightness; + sysfs_notify_dirent(led_cdev->brightness_hw_changed_kn); +} +EXPORT_SYMBOL_GPL(led_classdev_notify_brightness_hw_changed); + /** * led_classdev_suspend - suspend an led_classdev. * @led_cdev: the led_classdev to suspend. @@ -204,6 +250,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) dev_warn(parent, "Led %s renamed to %s due to name collision", led_cdev->name, dev_name(led_cdev->dev)); + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { + ret = led_add_brightness_hw_changed(led_cdev); + if (ret) { + device_unregister(led_cdev->dev); + return ret; + } + } + led_cdev->work_flags = 0; #ifdef CONFIG_LEDS_TRIGGERS init_rwsem(&led_cdev->trigger_lock); @@ -256,6 +310,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev) flush_work(&led_cdev->set_brightness_work); + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) + led_remove_brightness_hw_changed(led_cdev); + device_unregister(led_cdev->dev); down_write(&leds_list_lock); diff --git a/include/linux/leds.h b/include/linux/leds.h index 569cb53..aa452c8 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -13,6 +13,7 @@ #define __LINUX_LEDS_H_INCLUDED #include <linux/device.h> +#include <linux/kernfs.h> #include <linux/list.h> #include <linux/mutex.h> #include <linux/rwsem.h> @@ -35,6 +36,7 @@ struct led_classdev { const char *name; enum led_brightness brightness; enum led_brightness max_brightness; + enum led_brightness brightness_hw_changed; int flags; /* Lower 16 bits reflect status */ @@ -46,6 +48,7 @@ struct led_classdev { #define LED_DEV_CAP_FLASH (1 << 18) #define LED_HW_PLUGGABLE (1 << 19) #define LED_PANIC_INDICATOR (1 << 20) +#define LED_BRIGHT_HW_CHANGED (1 << 21) /* set_brightness_work / blink_timer flags, atomic, private. */ unsigned long work_flags; @@ -99,6 +102,9 @@ struct led_classdev { struct work_struct set_brightness_work; int delayed_set_value; + /* For LEDs with brightness_hw_changed sysfs attribute */ + struct kernfs_node *brightness_hw_changed_kn; + #ifdef CONFIG_LEDS_TRIGGERS /* Protects the trigger data below */ struct rw_semaphore trigger_lock; @@ -123,6 +129,8 @@ extern void devm_led_classdev_unregister(struct device *parent, struct led_classdev *led_cdev); extern void led_classdev_suspend(struct led_classdev *led_cdev); extern void led_classdev_resume(struct led_classdev *led_cdev); +extern void led_classdev_notify_brightness_hw_changed( + struct led_classdev *led_cdev, enum led_brightness brightness); /** * led_blink_set - set blinking with software fallback
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(+)