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

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

[Hans de Goede]

Hi,

Here is v6 of my patch(es) to allow userspace to get notified about
hw initiated brightness changes.

As agreed upon before the API has been changed to introduce a new
hw_brightness_changed attribute for this.

This has been tested on a Dell XPS 15, I'm travelling atm so I've
not tested the new version of the thinkpad patch.

The platform patches here are included mostly as an example of
users of the new API, they are currently not intended for merging
as we first need to have the LED class API for this merged.

Regards,

Hans

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

[Hans de Goede]

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.
+
 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
-- 
2.9.3

[PATCH v6 2/4] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
brightness is changed through the hotkey.

This will allow userspace to monitor (poll) for brightness changes on
these LEDs caused by the hotkey.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-This is a new patch in v3 of this patch-set
Changes in v4:
-No changes
Changes in v5:
-Switch to new led-trigger based API for notifying userspace about
 keyboard backlight brightness changes.
-Also call ledtrig_kbd_backlight() for laptops with a thinklight
-Rename the hotkey defines from THINKLIGHT to KBD_LIGHT since they
 are shared between the thinklight and the keyboard backlight
Changes in v6:
-Switch to new brightness_hw_changed LED class API
---
 drivers/platform/x86/thinkpad_acpi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index cacb43f..6249292 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -163,6 +163,7 @@ enum tpacpi_hkey_event_t {
 	TP_HKEY_EV_HOTKEY_BASE		= 0x1001, /* first hotkey (FN+F1) */
 	TP_HKEY_EV_BRGHT_UP		= 0x1010, /* Brightness up */
 	TP_HKEY_EV_BRGHT_DOWN		= 0x1011, /* Brightness down */
+	TP_HKEY_EV_KBD_LIGHT		= 0x1012, /* Thinklight/kbd backlight */
 	TP_HKEY_EV_VOL_UP		= 0x1015, /* Volume up or unmute */
 	TP_HKEY_EV_VOL_DOWN		= 0x1016, /* Volume down or unmute */
 	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
@@ -1959,7 +1960,7 @@ enum {	/* Positions of some of the keys in hotkey masks */
 	TP_ACPI_HKEY_HIBERNATE_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNF12,
 	TP_ACPI_HKEY_BRGHTUP_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNHOME,
 	TP_ACPI_HKEY_BRGHTDWN_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNEND,
-	TP_ACPI_HKEY_THNKLGHT_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
+	TP_ACPI_HKEY_KBD_LIGHT_MASK	= 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
 	TP_ACPI_HKEY_ZOOM_MASK		= 1 << TP_ACPI_HOTKEYSCAN_FNSPACE,
 	TP_ACPI_HKEY_VOLUP_MASK		= 1 << TP_ACPI_HOTKEYSCAN_VOLUMEUP,
 	TP_ACPI_HKEY_VOLDWN_MASK	= 1 << TP_ACPI_HOTKEYSCAN_VOLUMEDOWN,
@@ -2344,7 +2345,7 @@ static void hotkey_read_nvram(struct tp_nvram_state *n, const u32 m)
 		n->display_toggle = !!(d & TP_NVRAM_MASK_HKT_DISPLAY);
 		n->hibernate_toggle = !!(d & TP_NVRAM_MASK_HKT_HIBERNATE);
 	}
-	if (m & TP_ACPI_HKEY_THNKLGHT_MASK) {
+	if (m & TP_ACPI_HKEY_KBD_LIGHT_MASK) {
 		d = nvram_read_byte(TP_NVRAM_ADDR_THINKLIGHT);
 		n->thinklight_toggle = !!(d & TP_NVRAM_MASK_THINKLIGHT);
 	}
@@ -5193,6 +5194,7 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 	.led_classdev = {
 		.name		= "tpacpi::kbd_backlight",
 		.max_brightness	= 2,
+		.flags		= LED_BRIGHT_HW_CHANGED,
 		.brightness_set	= &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
 	}
@@ -5222,6 +5224,8 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 		return rc;
 	}
 
+	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
+				      TP_ACPI_HKEY_KBD_LIGHT_MASK);
 	return 0;
 }
 
@@ -9169,6 +9173,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 			volume_alsa_notify_change();
 		}
 	}
+	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_KBD_LIGHT)
+		led_classdev_notify_brightness_hw_changed(
+			&tpacpi_led_kbdlight.led_classdev,
+			kbdlight_sysfs_get(NULL));
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
-- 
2.9.3

[PATCH v6 3/4] platform/x86/dell-*: Add a generic dell-laptop notifier chain

[Hans de Goede]

There are several cases where events handled in one of the dell-* drivers
need to be propagated to another dell-* driver.

This commits add 3 generic functions:
dell_laptop_register_notifier()
dell_laptop_unregister_notifier()
dell_laptop_call_notifier()

It currently only defines 1 action:
DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED

Which is intended to propagate kbd_backlight_brightness_changed wmi
events from dell-wmi to dell-laptop (which contains the actual kbd
backlight driver).

These functions are put in dell-smbios as both dell-wmi and dell-laptop
use smbios functions and I do not want to put the notifier head in
either driver, as that will make the 2 drivers depend on each other.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
Changes in v3:
-No changes
Changes in v4:
-Rename functions from dell_smbios_*_notifier to dell_laptop_*_notifier
Changes in v5:
-Dropped due to new led-trigger based approach making this unnecessary
Changes in v6:
-Re-introduced as led-trigger based approach got nacked
---
 drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h | 11 +++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index d2412ab..bceb382 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_find_token);
 
+static ATOMIC_NOTIFIER_HEAD(dell_laptop_chain_head);
+
+int dell_laptop_register_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&dell_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_register_notifier);
+
+int dell_laptop_unregister_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&dell_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_unregister_notifier);
+
+void dell_laptop_call_notifier(unsigned long action, void *data)
+{
+	atomic_notifier_call_chain(&dell_laptop_chain_head, action, data);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_call_notifier);
+
 static void __init parse_da_table(const struct dmi_header *dm)
 {
 	/* Final token is a terminator, so we don't want to copy it */
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..45cbc22 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -16,6 +16,8 @@
 #ifndef _DELL_SMBIOS_H_
 #define _DELL_SMBIOS_H_
 
+struct notifier_block;
+
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
 
@@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
 void dell_smbios_send_request(int class, int select);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
+
+enum dell_laptop_notifier_actions {
+	DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED,
+};
+
+int dell_laptop_register_notifier(struct notifier_block *nb);
+int dell_laptop_unregister_notifier(struct notifier_block *nb);
+void dell_laptop_call_notifier(unsigned long action, void *data);
+
 #endif
-- 
2.9.3

[PATCH v6 4/4] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Make dell-wmi notify on hotkey kbd brightness changes, listen for this
in dell-laptop and call led_classdev_notify_brightness_hw_changed.

This will allow userspace to monitor (poll) for brightness changes on
these LEDs caused by the hotkey.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Changes in v2:
-Use the new dell_smbios*notify functionality
Changes in v3:
-Simplify the if condition for calling led_notify_brightness_change
Changes in v4:
-Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
Changes in v5:
-Switch to new led-trigger based API for notifying userspace about
 keyboard backlight brightness changes.
Changes in v6:
-Switch to new brightness_hw_changed LED class API
---
 drivers/platform/x86/dell-laptop.c | 30 +++++++++++++++++++++++++++++-
 drivers/platform/x86/dell-wmi.c    | 14 +++++++++-----
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 14392a0..7af485e 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1937,6 +1937,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 
 static struct led_classdev kbd_led = {
 	.name           = "dell::kbd_backlight",
+	.flags		= LED_BRIGHT_HW_CHANGED,
 	.brightness_set_blocking = kbd_led_level_set,
 	.brightness_get = kbd_led_level_get,
 	.groups         = kbd_led_groups,
@@ -1944,6 +1945,8 @@ static struct led_classdev kbd_led = {
 
 static int __init kbd_led_init(struct device *dev)
 {
+	int ret;
+
 	kbd_init();
 	if (!kbd_led_present)
 		return -ENODEV;
@@ -1955,7 +1958,11 @@ static int __init kbd_led_init(struct device *dev)
 		if (kbd_led.max_brightness)
 			kbd_led.max_brightness--;
 	}
-	return led_classdev_register(dev, &kbd_led);
+	ret = led_classdev_register(dev, &kbd_led);
+	if (ret)
+		kbd_led_present = false;
+
+	return ret;
 }
 
 static void brightness_set_exit(struct led_classdev *led_cdev,
@@ -1972,6 +1979,24 @@ static void kbd_led_exit(void)
 	led_classdev_unregister(&kbd_led);
 }
 
+static int dell_laptop_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	switch (action) {
+	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
+		if (kbd_led_present)
+			led_classdev_notify_brightness_hw_changed(
+				&kbd_led, kbd_led_level_get(&kbd_led));
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dell_laptop_notifier = {
+	.notifier_call = dell_laptop_notifier_call,
+};
+
 static int __init dell_init(void)
 {
 	struct calling_interface_buffer *buffer;
@@ -2015,6 +2040,8 @@ static int __init dell_init(void)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
 				    &dell_debugfs_fops);
 
+	dell_laptop_register_notifier(&dell_laptop_notifier);
+
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
 
@@ -2066,6 +2093,7 @@ static int __init dell_init(void)
 
 static void __exit dell_exit(void)
 {
+	dell_laptop_unregister_notifier(&dell_laptop_notifier);
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 75e6370..15740b5 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -297,11 +297,11 @@ static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
 	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
 
 	/* Keyboard backlight level changed */
-	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
-	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x01e1, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02ea, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02eb, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02ec, { KEY_KBDILLUMTOGGLE } },
+	{ KE_IGNORE, 0x02f6, { KEY_KBDILLUMTOGGLE } },
 };
 
 static struct input_dev *dell_wmi_input_dev;
@@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
+	if (key->keycode == KEY_KBDILLUMTOGGLE)
+		dell_laptop_call_notifier(
+			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
2.9.3

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

[Pali Rohár]

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Jacek Anaszewski]

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
> 

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

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.

-- 
Best regards,
Jacek Anaszewski

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

[Pali Rohár]

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Jacek Anaszewski]

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.

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

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.

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

[-- 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 --]

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

[Hans de Goede]

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



>

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

[Hans de Goede]

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
>

Re: [PATCH v6 2/4] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Andy Shevchenko]

On Wed, Jan 25, 2017 at 6:11 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
> kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
> brightness is changed through the hotkey.
>
> This will allow userspace to monitor (poll) for brightness changes on
> these LEDs caused by the hotkey.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I didn't see v7 of this and consequent patches. What's going on?

> ---
> Changes in v3:
> -This is a new patch in v3 of this patch-set
> Changes in v4:
> -No changes
> Changes in v5:
> -Switch to new led-trigger based API for notifying userspace about
>  keyboard backlight brightness changes.
> -Also call ledtrig_kbd_backlight() for laptops with a thinklight
> -Rename the hotkey defines from THINKLIGHT to KBD_LIGHT since they
>  are shared between the thinklight and the keyboard backlight
> Changes in v6:
> -Switch to new brightness_hw_changed LED class API
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index cacb43f..6249292 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -163,6 +163,7 @@ enum tpacpi_hkey_event_t {
>         TP_HKEY_EV_HOTKEY_BASE          = 0x1001, /* first hotkey (FN+F1) */
>         TP_HKEY_EV_BRGHT_UP             = 0x1010, /* Brightness up */
>         TP_HKEY_EV_BRGHT_DOWN           = 0x1011, /* Brightness down */
> +       TP_HKEY_EV_KBD_LIGHT            = 0x1012, /* Thinklight/kbd backlight */
>         TP_HKEY_EV_VOL_UP               = 0x1015, /* Volume up or unmute */
>         TP_HKEY_EV_VOL_DOWN             = 0x1016, /* Volume down or unmute */
>         TP_HKEY_EV_VOL_MUTE             = 0x1017, /* Mixer output mute */
> @@ -1959,7 +1960,7 @@ enum {    /* Positions of some of the keys in hotkey masks */
>         TP_ACPI_HKEY_HIBERNATE_MASK     = 1 << TP_ACPI_HOTKEYSCAN_FNF12,
>         TP_ACPI_HKEY_BRGHTUP_MASK       = 1 << TP_ACPI_HOTKEYSCAN_FNHOME,
>         TP_ACPI_HKEY_BRGHTDWN_MASK      = 1 << TP_ACPI_HOTKEYSCAN_FNEND,
> -       TP_ACPI_HKEY_THNKLGHT_MASK      = 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
> +       TP_ACPI_HKEY_KBD_LIGHT_MASK     = 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
>         TP_ACPI_HKEY_ZOOM_MASK          = 1 << TP_ACPI_HOTKEYSCAN_FNSPACE,
>         TP_ACPI_HKEY_VOLUP_MASK         = 1 << TP_ACPI_HOTKEYSCAN_VOLUMEUP,
>         TP_ACPI_HKEY_VOLDWN_MASK        = 1 << TP_ACPI_HOTKEYSCAN_VOLUMEDOWN,
> @@ -2344,7 +2345,7 @@ static void hotkey_read_nvram(struct tp_nvram_state *n, const u32 m)
>                 n->display_toggle = !!(d & TP_NVRAM_MASK_HKT_DISPLAY);
>                 n->hibernate_toggle = !!(d & TP_NVRAM_MASK_HKT_HIBERNATE);
>         }
> -       if (m & TP_ACPI_HKEY_THNKLGHT_MASK) {
> +       if (m & TP_ACPI_HKEY_KBD_LIGHT_MASK) {
>                 d = nvram_read_byte(TP_NVRAM_ADDR_THINKLIGHT);
>                 n->thinklight_toggle = !!(d & TP_NVRAM_MASK_THINKLIGHT);
>         }
> @@ -5193,6 +5194,7 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
>         .led_classdev = {
>                 .name           = "tpacpi::kbd_backlight",
>                 .max_brightness = 2,
> +               .flags          = LED_BRIGHT_HW_CHANGED,
>                 .brightness_set = &kbdlight_sysfs_set,
>                 .brightness_get = &kbdlight_sysfs_get,
>         }
> @@ -5222,6 +5224,8 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
>                 return rc;
>         }
>
> +       tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
> +                                     TP_ACPI_HKEY_KBD_LIGHT_MASK);
>         return 0;
>  }
>
> @@ -9169,6 +9173,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>                         volume_alsa_notify_change();
>                 }
>         }
> +       if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_KBD_LIGHT)
> +               led_classdev_notify_brightness_hw_changed(
> +                       &tpacpi_led_kbdlight.led_classdev,
> +                       kbdlight_sysfs_get(NULL));
>  }
>
>  static void hotkey_driver_event(const unsigned int scancode)
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/4] platform/x86/thinkpad: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 01/28/2017 02:36 PM, Andy Shevchenko wrote:
> On Wed, Jan 25, 2017 at 6:11 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Make thinkpad_acpi call led_classdev_notify_brightness_hw_changed on the
>> kbd_led led_classdev registered by thinkpad_acpi when the kbd backlight
>> brightness is changed through the hotkey.
>>
>> This will allow userspace to monitor (poll) for brightness changes on
>> these LEDs caused by the hotkey.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> I didn't see v7 of this and consequent patches. What's going on?

As I said in the cover-letter of v6 the platform patches were mainly
there to document the usage of the new LED api. Since there were no
changes to the api I did not post a v7 of the platform/x86 patches,
also I've not yet tested the thinkpad changes as adjusted for the
new LED api.

Once the LED side is merged, I'll re-post the platform patches.

Regards,

Hans




>
>> ---
>> Changes in v3:
>> -This is a new patch in v3 of this patch-set
>> Changes in v4:
>> -No changes
>> Changes in v5:
>> -Switch to new led-trigger based API for notifying userspace about
>>  keyboard backlight brightness changes.
>> -Also call ledtrig_kbd_backlight() for laptops with a thinklight
>> -Rename the hotkey defines from THINKLIGHT to KBD_LIGHT since they
>>  are shared between the thinklight and the keyboard backlight
>> Changes in v6:
>> -Switch to new brightness_hw_changed LED class API
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index cacb43f..6249292 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -163,6 +163,7 @@ enum tpacpi_hkey_event_t {
>>         TP_HKEY_EV_HOTKEY_BASE          = 0x1001, /* first hotkey (FN+F1) */
>>         TP_HKEY_EV_BRGHT_UP             = 0x1010, /* Brightness up */
>>         TP_HKEY_EV_BRGHT_DOWN           = 0x1011, /* Brightness down */
>> +       TP_HKEY_EV_KBD_LIGHT            = 0x1012, /* Thinklight/kbd backlight */
>>         TP_HKEY_EV_VOL_UP               = 0x1015, /* Volume up or unmute */
>>         TP_HKEY_EV_VOL_DOWN             = 0x1016, /* Volume down or unmute */
>>         TP_HKEY_EV_VOL_MUTE             = 0x1017, /* Mixer output mute */
>> @@ -1959,7 +1960,7 @@ enum {    /* Positions of some of the keys in hotkey masks */
>>         TP_ACPI_HKEY_HIBERNATE_MASK     = 1 << TP_ACPI_HOTKEYSCAN_FNF12,
>>         TP_ACPI_HKEY_BRGHTUP_MASK       = 1 << TP_ACPI_HOTKEYSCAN_FNHOME,
>>         TP_ACPI_HKEY_BRGHTDWN_MASK      = 1 << TP_ACPI_HOTKEYSCAN_FNEND,
>> -       TP_ACPI_HKEY_THNKLGHT_MASK      = 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
>> +       TP_ACPI_HKEY_KBD_LIGHT_MASK     = 1 << TP_ACPI_HOTKEYSCAN_FNPAGEUP,
>>         TP_ACPI_HKEY_ZOOM_MASK          = 1 << TP_ACPI_HOTKEYSCAN_FNSPACE,
>>         TP_ACPI_HKEY_VOLUP_MASK         = 1 << TP_ACPI_HOTKEYSCAN_VOLUMEUP,
>>         TP_ACPI_HKEY_VOLDWN_MASK        = 1 << TP_ACPI_HOTKEYSCAN_VOLUMEDOWN,
>> @@ -2344,7 +2345,7 @@ static void hotkey_read_nvram(struct tp_nvram_state *n, const u32 m)
>>                 n->display_toggle = !!(d & TP_NVRAM_MASK_HKT_DISPLAY);
>>                 n->hibernate_toggle = !!(d & TP_NVRAM_MASK_HKT_HIBERNATE);
>>         }
>> -       if (m & TP_ACPI_HKEY_THNKLGHT_MASK) {
>> +       if (m & TP_ACPI_HKEY_KBD_LIGHT_MASK) {
>>                 d = nvram_read_byte(TP_NVRAM_ADDR_THINKLIGHT);
>>                 n->thinklight_toggle = !!(d & TP_NVRAM_MASK_THINKLIGHT);
>>         }
>> @@ -5193,6 +5194,7 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
>>         .led_classdev = {
>>                 .name           = "tpacpi::kbd_backlight",
>>                 .max_brightness = 2,
>> +               .flags          = LED_BRIGHT_HW_CHANGED,
>>                 .brightness_set = &kbdlight_sysfs_set,
>>                 .brightness_get = &kbdlight_sysfs_get,
>>         }
>> @@ -5222,6 +5224,8 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
>>                 return rc;
>>         }
>>
>> +       tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
>> +                                     TP_ACPI_HKEY_KBD_LIGHT_MASK);
>>         return 0;
>>  }
>>
>> @@ -9169,6 +9173,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>>                         volume_alsa_notify_change();
>>                 }
>>         }
>> +       if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_KBD_LIGHT)
>> +               led_classdev_notify_brightness_hw_changed(
>> +                       &tpacpi_led_kbdlight.led_classdev,
>> +                       kbdlight_sysfs_get(NULL));
>>  }
>>
>>  static void hotkey_driver_event(const unsigned int scancode)
>> --
>> 2.9.3
>>
>
>
>