[PATCH v4 1/4] leds: core: Add support for poll()ing the sysfs brightness attr for changes.

[PATCH v4 1/4] leds: core: Add support for poll()ing the sysfs brightness attr for changes.

[Hans de Goede]

Add support for userspace using poll() for POLL_PRI on the sysfs brightness
attr to watch for brightness changes.

This commit adds a led_notify_brightness_change helper function for
waking up any poll() waiters; and calls this after any brightness changes
(after any led_classdev->brightness_set[_blocking] calls have completed).

In some use-cases led hardware may autonomously change its brightness,
e.g. the keyboard backlight used on some laptops is controlled by a
hardwired (firmware handled) hotkey. led_notify_brightness_change is
exported for use by drivers which can detect such autonomous changes.

This commit also updates the Documentation/ABI/testing/sysfs-class-led
documentation to document that userspace may now poll on the brightness
attribute.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Wakeup / notify userspace on any brightness changes, not just on
 autonomous changes done by the hw
Changes in v3:
-Rebase on linux-leds/for-next
Changes in v4:
-No Changes
---
 Documentation/ABI/testing/sysfs-class-led |  8 ++++++--
 drivers/leds/led-class.c                  |  9 +++++++++
 drivers/leds/led-core.c                   | 16 +++++++++++++++-
 include/linux/leds.h                      | 12 ++++++++++++
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 491cdee..0af5191 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -1,12 +1,16 @@
 What:		/sys/class/leds/<led>/brightness
-Date:		March 2006
-KernelVersion:	2.6.17
+Date:		March 2006 (poll October 2016)
+KernelVersion:	2.6.17 (poll since 4.10)
 Contact:	Richard Purdie <rpurdie@rpsys.net>
 Description:
 		Set the brightness of the LED. Most LEDs don't
 		have hardware brightness support, so will just be turned on for
 		non-zero brightness settings. The value is between 0 and
 		/sys/class/leds/<led>/max_brightness.
+		The file supports poll() to detect brightness changes, in
+		some cases the hardware / firmware may change the brightness
+		autonomously, poll() should be woken up in this case too,
+		but not all drivers may support this.
 
 		Writing 0 to this file clears active trigger.
 
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 731e4eb..c8d2d67 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -204,6 +204,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));
 
+	led_cdev->brightness_kn = sysfs_get_dirent(led_cdev->dev->kobj.sd,
+						   "brightness");
+	if (!led_cdev->brightness_kn) {
+		dev_err(led_cdev->dev, "Error getting brightness kernfs_node\n");
+		device_unregister(led_cdev->dev);
+		return -ENODEV;
+	}
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	init_rwsem(&led_cdev->trigger_lock);
 #endif
@@ -255,6 +263,7 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
 	flush_work(&led_cdev->set_brightness_work);
 
+	sysfs_put(led_cdev->brightness_kn);
 	device_unregister(led_cdev->dev);
 
 	down_write(&leds_list_lock);
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 2d0c75a..af78279 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -33,16 +33,24 @@ static int __led_set_brightness(struct led_classdev *led_cdev,
 
 	led_cdev->brightness_set(led_cdev, value);
 
+	led_notify_brightness_change(led_cdev);
+
 	return 0;
 }
 
 static int __led_set_brightness_blocking(struct led_classdev *led_cdev,
 					 enum led_brightness value)
 {
+	int ret;
+
 	if (!led_cdev->brightness_set_blocking)
 		return -ENOTSUPP;
 
-	return led_cdev->brightness_set_blocking(led_cdev, value);
+	ret = led_cdev->brightness_set_blocking(led_cdev, value);
+	if (ret >= 0)
+		led_notify_brightness_change(led_cdev);
+
+	return ret;
 }
 
 static void led_timer_function(unsigned long data)
@@ -308,6 +316,12 @@ int led_update_brightness(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_update_brightness);
 
+void led_notify_brightness_change(struct led_classdev *led_cdev)
+{
+	sysfs_notify_dirent(led_cdev->brightness_kn);
+}
+EXPORT_SYMBOL_GPL(led_notify_brightness_change);
+
 /* Caller must ensure led_cdev->led_access held */
 void led_sysfs_disable(struct led_classdev *led_cdev)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 52993de..f034c2d 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>
@@ -93,6 +94,8 @@ struct led_classdev {
 
 	struct work_struct	set_brightness_work;
 
+	struct kernfs_node	*brightness_kn;
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	/* Protects the trigger data below */
 	struct rw_semaphore	 trigger_lock;
@@ -192,6 +195,15 @@ extern int led_set_brightness_sync(struct led_classdev *led_cdev,
 extern int led_update_brightness(struct led_classdev *led_cdev);
 
 /**
+ * led_notify_brightness_change - Notify userspace of brightness changes
+ * @led_cdev: the LED to do the notify on
+ *
+ * Let any users waiting for POLL_PRI on the led's brightness sysfs
+ * atrribute know that the brightness has been changed.
+ */
+extern void led_notify_brightness_change(struct led_classdev *led_cdev);
+
+/**
  * led_sysfs_disable - disable LED sysfs interface
  * @led_cdev: the LED to set
  *
-- 
2.9.3

[PATCH v4 2/4] platform: x86: thinkpad: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Make thinkpad_acpi call led_notify_brightness_change on the kbd_led led_classdev
registered by thinkpad_acpi when the kbd backlight brightness changes.

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
---
 drivers/platform/x86/thinkpad_acpi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index b65ce75..5dcd7d8b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -162,6 +162,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_THINKLIGHT		= 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 */
@@ -5167,6 +5168,8 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 		return rc;
 	}
 
+	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
+				      TP_ACPI_HKEY_THNKLGHT_MASK);
 	return 0;
 }
 
@@ -9114,6 +9117,8 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 			volume_alsa_notify_change();
 		}
 	}
+	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_THINKLIGHT)
+		led_notify_brightness_change(&tpacpi_led_kbdlight.led_classdev);
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
-- 
2.9.3

[PATCH v4 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
---
 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..b8e01cc 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 v4 4/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Make dell-wmi call led_notify_brightness_change on the kbd_led led_classdev
registered by dell-laptop when the kbd backlight brightness changes.

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
---
 drivers/platform/x86/dell-laptop.c | 28 +++++++++++++++++++++++++++-
 drivers/platform/x86/dell-wmi.c    | 14 +++++++++-----
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 2c2f02b..c9cd1d5 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1942,6 +1942,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;
@@ -1953,7 +1955,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,
@@ -1970,6 +1976,23 @@ 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_notify_brightness_change(&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;
@@ -2013,6 +2036,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;
 
@@ -2064,6 +2089,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 da2fe18..8e67061 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -287,11 +287,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;
@@ -319,6 +319,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 v4 1/4] leds: core: Add support for poll()ing the sysfs brightness attr for changes.

[Jacek Anaszewski]

Hi Hans,

I've tested it with userspace test application and it seems to work
well both with hardware leds-aat1290 driver and drivers/leds/uleds.c.

Applied to the for-next branch of linux-leds.git.

Thanks,
Jacek Anaszewski

On 11/01/2016 02:37 PM, Hans de Goede wrote:
> Add support for userspace using poll() for POLL_PRI on the sysfs brightness
> attr to watch for brightness changes.
>
> This commit adds a led_notify_brightness_change helper function for
> waking up any poll() waiters; and calls this after any brightness changes
> (after any led_classdev->brightness_set[_blocking] calls have completed).
>
> In some use-cases led hardware may autonomously change its brightness,
> e.g. the keyboard backlight used on some laptops is controlled by a
> hardwired (firmware handled) hotkey. led_notify_brightness_change is
> exported for use by drivers which can detect such autonomous changes.
>
> This commit also updates the Documentation/ABI/testing/sysfs-class-led
> documentation to document that userspace may now poll on the brightness
> attribute.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Wakeup / notify userspace on any brightness changes, not just on
>  autonomous changes done by the hw
> Changes in v3:
> -Rebase on linux-leds/for-next
> Changes in v4:
> -No Changes
> ---
>  Documentation/ABI/testing/sysfs-class-led |  8 ++++++--
>  drivers/leds/led-class.c                  |  9 +++++++++
>  drivers/leds/led-core.c                   | 16 +++++++++++++++-
>  include/linux/leds.h                      | 12 ++++++++++++
>  4 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 491cdee..0af5191 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -1,12 +1,16 @@
>  What:		/sys/class/leds/<led>/brightness
> -Date:		March 2006
> -KernelVersion:	2.6.17
> +Date:		March 2006 (poll October 2016)
> +KernelVersion:	2.6.17 (poll since 4.10)
>  Contact:	Richard Purdie <rpurdie@rpsys.net>
>  Description:
>  		Set the brightness of the LED. Most LEDs don't
>  		have hardware brightness support, so will just be turned on for
>  		non-zero brightness settings. The value is between 0 and
>  		/sys/class/leds/<led>/max_brightness.
> +		The file supports poll() to detect brightness changes, in
> +		some cases the hardware / firmware may change the brightness
> +		autonomously, poll() should be woken up in this case too,
> +		but not all drivers may support this.
>
>  		Writing 0 to this file clears active trigger.
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 731e4eb..c8d2d67 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -204,6 +204,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));
>
> +	led_cdev->brightness_kn = sysfs_get_dirent(led_cdev->dev->kobj.sd,
> +						   "brightness");
> +	if (!led_cdev->brightness_kn) {
> +		dev_err(led_cdev->dev, "Error getting brightness kernfs_node\n");
> +		device_unregister(led_cdev->dev);
> +		return -ENODEV;
> +	}
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	init_rwsem(&led_cdev->trigger_lock);
>  #endif
> @@ -255,6 +263,7 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>
>  	flush_work(&led_cdev->set_brightness_work);
>
> +	sysfs_put(led_cdev->brightness_kn);
>  	device_unregister(led_cdev->dev);
>
>  	down_write(&leds_list_lock);
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 2d0c75a..af78279 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -33,16 +33,24 @@ static int __led_set_brightness(struct led_classdev *led_cdev,
>
>  	led_cdev->brightness_set(led_cdev, value);
>
> +	led_notify_brightness_change(led_cdev);
> +
>  	return 0;
>  }
>
>  static int __led_set_brightness_blocking(struct led_classdev *led_cdev,
>  					 enum led_brightness value)
>  {
> +	int ret;
> +
>  	if (!led_cdev->brightness_set_blocking)
>  		return -ENOTSUPP;
>
> -	return led_cdev->brightness_set_blocking(led_cdev, value);
> +	ret = led_cdev->brightness_set_blocking(led_cdev, value);
> +	if (ret >= 0)
> +		led_notify_brightness_change(led_cdev);
> +
> +	return ret;
>  }
>
>  static void led_timer_function(unsigned long data)
> @@ -308,6 +316,12 @@ int led_update_brightness(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_update_brightness);
>
> +void led_notify_brightness_change(struct led_classdev *led_cdev)
> +{
> +	sysfs_notify_dirent(led_cdev->brightness_kn);
> +}
> +EXPORT_SYMBOL_GPL(led_notify_brightness_change);
> +
>  /* Caller must ensure led_cdev->led_access held */
>  void led_sysfs_disable(struct led_classdev *led_cdev)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 52993de..f034c2d 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>
> @@ -93,6 +94,8 @@ struct led_classdev {
>
>  	struct work_struct	set_brightness_work;
>
> +	struct kernfs_node	*brightness_kn;
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	/* Protects the trigger data below */
>  	struct rw_semaphore	 trigger_lock;
> @@ -192,6 +195,15 @@ extern int led_set_brightness_sync(struct led_classdev *led_cdev,
>  extern int led_update_brightness(struct led_classdev *led_cdev);
>
>  /**
> + * led_notify_brightness_change - Notify userspace of brightness changes
> + * @led_cdev: the LED to do the notify on
> + *
> + * Let any users waiting for POLL_PRI on the led's brightness sysfs
> + * atrribute know that the brightness has been changed.
> + */
> +extern void led_notify_brightness_change(struct led_classdev *led_cdev);
> +
> +/**
>   * led_sysfs_disable - disable LED sysfs interface
>   * @led_cdev: the LED to set
>   *
>

Re: [PATCH v4 1/4] leds: core: Add support for poll()ing the sysfs brightness attr for changes.

[Hans de Goede]

Hi,

On 08-11-16 12:52, Jacek Anaszewski wrote:
> Hi Hans,
>
> I've tested it with userspace test application and it seems to work
> well both with hardware leds-aat1290 driver and drivers/leds/uleds.c.
>
> Applied to the for-next branch of linux-leds.git.

Great, thank you. Can you also add a stable branch / tag with this
commit which the platform/x86 maintainer can merge into his tree,
so that he can merge the platform/x86 driver changes which depend
up on the new led_notify_brightness_change() function ?

Regards,

Hans



>
> Thanks,
> Jacek Anaszewski
>
> On 11/01/2016 02:37 PM, Hans de Goede wrote:
>> Add support for userspace using poll() for POLL_PRI on the sysfs brightness
>> attr to watch for brightness changes.
>>
>> This commit adds a led_notify_brightness_change helper function for
>> waking up any poll() waiters; and calls this after any brightness changes
>> (after any led_classdev->brightness_set[_blocking] calls have completed).
>>
>> In some use-cases led hardware may autonomously change its brightness,
>> e.g. the keyboard backlight used on some laptops is controlled by a
>> hardwired (firmware handled) hotkey. led_notify_brightness_change is
>> exported for use by drivers which can detect such autonomous changes.
>>
>> This commit also updates the Documentation/ABI/testing/sysfs-class-led
>> documentation to document that userspace may now poll on the brightness
>> attribute.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v2:
>> -Wakeup / notify userspace on any brightness changes, not just on
>>  autonomous changes done by the hw
>> Changes in v3:
>> -Rebase on linux-leds/for-next
>> Changes in v4:
>> -No Changes
>> ---
>>  Documentation/ABI/testing/sysfs-class-led |  8 ++++++--
>>  drivers/leds/led-class.c                  |  9 +++++++++
>>  drivers/leds/led-core.c                   | 16 +++++++++++++++-
>>  include/linux/leds.h                      | 12 ++++++++++++
>>  4 files changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
>> index 491cdee..0af5191 100644
>> --- a/Documentation/ABI/testing/sysfs-class-led
>> +++ b/Documentation/ABI/testing/sysfs-class-led
>> @@ -1,12 +1,16 @@
>>  What:        /sys/class/leds/<led>/brightness
>> -Date:        March 2006
>> -KernelVersion:    2.6.17
>> +Date:        March 2006 (poll October 2016)
>> +KernelVersion:    2.6.17 (poll since 4.10)
>>  Contact:    Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
>>  Description:
>>          Set the brightness of the LED. Most LEDs don't
>>          have hardware brightness support, so will just be turned on for
>>          non-zero brightness settings. The value is between 0 and
>>          /sys/class/leds/<led>/max_brightness.
>> +        The file supports poll() to detect brightness changes, in
>> +        some cases the hardware / firmware may change the brightness
>> +        autonomously, poll() should be woken up in this case too,
>> +        but not all drivers may support this.
>>
>>          Writing 0 to this file clears active trigger.
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 731e4eb..c8d2d67 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -204,6 +204,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));
>>
>> +    led_cdev->brightness_kn = sysfs_get_dirent(led_cdev->dev->kobj.sd,
>> +                           "brightness");
>> +    if (!led_cdev->brightness_kn) {
>> +        dev_err(led_cdev->dev, "Error getting brightness kernfs_node\n");
>> +        device_unregister(led_cdev->dev);
>> +        return -ENODEV;
>> +    }
>> +
>>  #ifdef CONFIG_LEDS_TRIGGERS
>>      init_rwsem(&led_cdev->trigger_lock);
>>  #endif
>> @@ -255,6 +263,7 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>>
>>      flush_work(&led_cdev->set_brightness_work);
>>
>> +    sysfs_put(led_cdev->brightness_kn);
>>      device_unregister(led_cdev->dev);
>>
>>      down_write(&leds_list_lock);
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 2d0c75a..af78279 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -33,16 +33,24 @@ static int __led_set_brightness(struct led_classdev *led_cdev,
>>
>>      led_cdev->brightness_set(led_cdev, value);
>>
>> +    led_notify_brightness_change(led_cdev);
>> +
>>      return 0;
>>  }
>>
>>  static int __led_set_brightness_blocking(struct led_classdev *led_cdev,
>>                       enum led_brightness value)
>>  {
>> +    int ret;
>> +
>>      if (!led_cdev->brightness_set_blocking)
>>          return -ENOTSUPP;
>>
>> -    return led_cdev->brightness_set_blocking(led_cdev, value);
>> +    ret = led_cdev->brightness_set_blocking(led_cdev, value);
>> +    if (ret >= 0)
>> +        led_notify_brightness_change(led_cdev);
>> +
>> +    return ret;
>>  }
>>
>>  static void led_timer_function(unsigned long data)
>> @@ -308,6 +316,12 @@ int led_update_brightness(struct led_classdev *led_cdev)
>>  }
>>  EXPORT_SYMBOL_GPL(led_update_brightness);
>>
>> +void led_notify_brightness_change(struct led_classdev *led_cdev)
>> +{
>> +    sysfs_notify_dirent(led_cdev->brightness_kn);
>> +}
>> +EXPORT_SYMBOL_GPL(led_notify_brightness_change);
>> +
>>  /* Caller must ensure led_cdev->led_access held */
>>  void led_sysfs_disable(struct led_classdev *led_cdev)
>>  {
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 52993de..f034c2d 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>
>> @@ -93,6 +94,8 @@ struct led_classdev {
>>
>>      struct work_struct    set_brightness_work;
>>
>> +    struct kernfs_node    *brightness_kn;
>> +
>>  #ifdef CONFIG_LEDS_TRIGGERS
>>      /* Protects the trigger data below */
>>      struct rw_semaphore     trigger_lock;
>> @@ -192,6 +195,15 @@ extern int led_set_brightness_sync(struct led_classdev *led_cdev,
>>  extern int led_update_brightness(struct led_classdev *led_cdev);
>>
>>  /**
>> + * led_notify_brightness_change - Notify userspace of brightness changes
>> + * @led_cdev: the LED to do the notify on
>> + *
>> + * Let any users waiting for POLL_PRI on the led's brightness sysfs
>> + * atrribute know that the brightness has been changed.
>> + */
>> +extern void led_notify_brightness_change(struct led_classdev *led_cdev);
>> +
>> +/**
>>   * led_sysfs_disable - disable LED sysfs interface
>>   * @led_cdev: the LED to set
>>   *
>>
>

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH v4 1/4] leds: core: Add support for poll()ing the sysfs brightness attr for changes.

[Jacek Anaszewski]

On 11/08/2016 01:31 PM, Hans de Goede wrote:
> Hi,
>
> On 08-11-16 12:52, Jacek Anaszewski wrote:
>> Hi Hans,
>>
>> I've tested it with userspace test application and it seems to work
>> well both with hardware leds-aat1290 driver and drivers/leds/uleds.c.
>>
>> Applied to the for-next branch of linux-leds.git.
>
> Great, thank you. Can you also add a stable branch / tag with this
> commit which the platform/x86 maintainer can merge into his tree,
> so that he can merge the platform/x86 driver changes which depend
> up on the new led_notify_brightness_change() function ?

Sure. I've created stable led_brightness_pollpri branch on

git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git

Best regards,
Jacek Anaszewski

>> On 11/01/2016 02:37 PM, Hans de Goede wrote:
>>> Add support for userspace using poll() for POLL_PRI on the sysfs
>>> brightness
>>> attr to watch for brightness changes.
>>>
>>> This commit adds a led_notify_brightness_change helper function for
>>> waking up any poll() waiters; and calls this after any brightness
>>> changes
>>> (after any led_classdev->brightness_set[_blocking] calls have
>>> completed).
>>>
>>> In some use-cases led hardware may autonomously change its brightness,
>>> e.g. the keyboard backlight used on some laptops is controlled by a
>>> hardwired (firmware handled) hotkey. led_notify_brightness_change is
>>> exported for use by drivers which can detect such autonomous changes.
>>>
>>> This commit also updates the Documentation/ABI/testing/sysfs-class-led
>>> documentation to document that userspace may now poll on the brightness
>>> attribute.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Wakeup / notify userspace on any brightness changes, not just on
>>>  autonomous changes done by the hw
>>> Changes in v3:
>>> -Rebase on linux-leds/for-next
>>> Changes in v4:
>>> -No Changes
>>> ---
>>>  Documentation/ABI/testing/sysfs-class-led |  8 ++++++--
>>>  drivers/leds/led-class.c                  |  9 +++++++++
>>>  drivers/leds/led-core.c                   | 16 +++++++++++++++-
>>>  include/linux/leds.h                      | 12 ++++++++++++
>>>  4 files changed, 42 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>> b/Documentation/ABI/testing/sysfs-class-led
>>> index 491cdee..0af5191 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -1,12 +1,16 @@
>>>  What:        /sys/class/leds/<led>/brightness
>>> -Date:        March 2006
>>> -KernelVersion:    2.6.17
>>> +Date:        March 2006 (poll October 2016)
>>> +KernelVersion:    2.6.17 (poll since 4.10)
>>>  Contact:    Richard Purdie <rpurdie@rpsys.net>
>>>  Description:
>>>          Set the brightness of the LED. Most LEDs don't
>>>          have hardware brightness support, so will just be turned on for
>>>          non-zero brightness settings. The value is between 0 and
>>>          /sys/class/leds/<led>/max_brightness.
>>> +        The file supports poll() to detect brightness changes, in
>>> +        some cases the hardware / firmware may change the brightness
>>> +        autonomously, poll() should be woken up in this case too,
>>> +        but not all drivers may support this.
>>>
>>>          Writing 0 to this file clears active trigger.
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 731e4eb..c8d2d67 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -204,6 +204,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));
>>>
>>> +    led_cdev->brightness_kn = sysfs_get_dirent(led_cdev->dev->kobj.sd,
>>> +                           "brightness");
>>> +    if (!led_cdev->brightness_kn) {
>>> +        dev_err(led_cdev->dev, "Error getting brightness
>>> kernfs_node\n");
>>> +        device_unregister(led_cdev->dev);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>>  #ifdef CONFIG_LEDS_TRIGGERS
>>>      init_rwsem(&led_cdev->trigger_lock);
>>>  #endif
>>> @@ -255,6 +263,7 @@ void led_classdev_unregister(struct led_classdev
>>> *led_cdev)
>>>
>>>      flush_work(&led_cdev->set_brightness_work);
>>>
>>> +    sysfs_put(led_cdev->brightness_kn);
>>>      device_unregister(led_cdev->dev);
>>>
>>>      down_write(&leds_list_lock);
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index 2d0c75a..af78279 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -33,16 +33,24 @@ static int __led_set_brightness(struct
>>> led_classdev *led_cdev,
>>>
>>>      led_cdev->brightness_set(led_cdev, value);
>>>
>>> +    led_notify_brightness_change(led_cdev);
>>> +
>>>      return 0;
>>>  }
>>>
>>>  static int __led_set_brightness_blocking(struct led_classdev *led_cdev,
>>>                       enum led_brightness value)
>>>  {
>>> +    int ret;
>>> +
>>>      if (!led_cdev->brightness_set_blocking)
>>>          return -ENOTSUPP;
>>>
>>> -    return led_cdev->brightness_set_blocking(led_cdev, value);
>>> +    ret = led_cdev->brightness_set_blocking(led_cdev, value);
>>> +    if (ret >= 0)
>>> +        led_notify_brightness_change(led_cdev);
>>> +
>>> +    return ret;
>>>  }
>>>
>>>  static void led_timer_function(unsigned long data)
>>> @@ -308,6 +316,12 @@ int led_update_brightness(struct led_classdev
>>> *led_cdev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(led_update_brightness);
>>>
>>> +void led_notify_brightness_change(struct led_classdev *led_cdev)
>>> +{
>>> +    sysfs_notify_dirent(led_cdev->brightness_kn);
>>> +}
>>> +EXPORT_SYMBOL_GPL(led_notify_brightness_change);
>>> +
>>>  /* Caller must ensure led_cdev->led_access held */
>>>  void led_sysfs_disable(struct led_classdev *led_cdev)
>>>  {
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index 52993de..f034c2d 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>
>>> @@ -93,6 +94,8 @@ struct led_classdev {
>>>
>>>      struct work_struct    set_brightness_work;
>>>
>>> +    struct kernfs_node    *brightness_kn;
>>> +
>>>  #ifdef CONFIG_LEDS_TRIGGERS
>>>      /* Protects the trigger data below */
>>>      struct rw_semaphore     trigger_lock;
>>> @@ -192,6 +195,15 @@ extern int led_set_brightness_sync(struct
>>> led_classdev *led_cdev,
>>>  extern int led_update_brightness(struct led_classdev *led_cdev);
>>>
>>>  /**
>>> + * led_notify_brightness_change - Notify userspace of brightness
>>> changes
>>> + * @led_cdev: the LED to do the notify on
>>> + *
>>> + * Let any users waiting for POLL_PRI on the led's brightness sysfs
>>> + * atrribute know that the brightness has been changed.
>>> + */
>>> +extern void led_notify_brightness_change(struct led_classdev
>>> *led_cdev);
>>> +
>>> +/**
>>>   * led_sysfs_disable - disable LED sysfs interface
>>>   * @led_cdev: the LED to set
>>>   *
>>>
>>
>
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 1/4] leds: core: Add support for poll()ing the sysfs brightness attr for changes.

[Hans de Goede]

Hi,

On 08-11-16 14:08, Jacek Anaszewski wrote:
> On 11/08/2016 01:31 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 08-11-16 12:52, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
>>> I've tested it with userspace test application and it seems to work
>>> well both with hardware leds-aat1290 driver and drivers/leds/uleds.c.
>>>
>>> Applied to the for-next branch of linux-leds.git.
>>
>> Great, thank you. Can you also add a stable branch / tag with this
>> commit which the platform/x86 maintainer can merge into his tree,
>> so that he can merge the platform/x86 driver changes which depend
>> up on the new led_notify_brightness_change() function ?
>
> Sure. I've created stable led_brightness_pollpri branch on
>
> git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git

Thank you.

Darren, can you merge this tag and patches 2-4 of this series please ?

Regards,

Hans

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2133 bytes --]

On Tuesday 01 November 2016 14:37:46 Hans de Goede wrote:
> Make thinkpad_acpi call led_notify_brightness_change on the kbd_led
> led_classdev registered by thinkpad_acpi when the kbd backlight
> brightness changes.
> 
> 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
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c index b65ce75..5dcd7d8b
> 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -162,6 +162,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_THINKLIGHT		= 0x1012, /* Thinklight/kbd backlight */

My question remains. Is this for Thinklight or keyboard backlight? 
Because Thinklinght has led device "tpacpi_led_thinklight" and keyboard 
backlight has led device "tpacpi_led_kbdlight".

>  	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 */
> @@ -5167,6 +5168,8 @@ static int __init kbdlight_init(struct
> ibm_init_struct *iibm) return rc;
>  	}
> 
> +	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
> +				      TP_ACPI_HKEY_THNKLGHT_MASK);
>  	return 0;
>  }
> 
> @@ -9114,6 +9117,8 @@ static void tpacpi_driver_event(const unsigned
> int hkey_event) volume_alsa_notify_change();
>  		}
>  	}
> +	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_THINKLIGHT)
> +		led_notify_brightness_change(&tpacpi_led_kbdlight.led_classdev);

This looks incorrect. You are trying to inform tpacpi_led_kbdlight when 
tpacpi_led_thinklight change led status?

>  }
> 
>  static void hotkey_driver_event(const unsigned int scancode)

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 565 bytes --]

On Tuesday 01 November 2016 14:37:47 Hans de Goede wrote:
> @@ -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,
> +};

Should not be upper case? Looks like that Documentation/CodingStyle also 
suggest it:

Names of macros defining constants and labels in enums are capitalized.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Hans de Goede]

Hi,

On 11-11-16 15:12, Pali Rohár wrote:
> On Tuesday 01 November 2016 14:37:46 Hans de Goede wrote:
>> Make thinkpad_acpi call led_notify_brightness_change on the kbd_led
>> led_classdev registered by thinkpad_acpi when the kbd backlight
>> brightness changes.
>>
>> 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
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c index b65ce75..5dcd7d8b
>> 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -162,6 +162,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_THINKLIGHT		= 0x1012, /* Thinklight/kbd backlight */
>
> My question remains. Is this for Thinklight or keyboard backlight?
> Because Thinklinght has led device "tpacpi_led_thinklight" and keyboard
> backlight has led device "tpacpi_led_kbdlight".

I would say both, this matches with the pre-existing
TP_ACPI_HKEY_THNKLGHT_MASK (they have a 1:1 mapping),
keep in mind that there are no thinkpads with both
a thinklight and a backlit keyboard, as those both
serve the same purpose so it looks like the re-used
the scancode.

I've tried this code on both a W541 and a P50 and both
emit TP_ACPI_HOTKEYSCAN_FNPAGEUP for the backlight
keyboard hotkey (fn + space), I'm naming the event for
this TP_HKEY_EV_THINKLIGHT for consistency with the
existing TP_ACPI_HKEY_THNKLGHT_MASK which corresponds
to TP_ACPI_HOTKEYSCAN_FNPAGEUP.

>>  	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 */
>> @@ -5167,6 +5168,8 @@ static int __init kbdlight_init(struct
>> ibm_init_struct *iibm) return rc;
>>  	}
>>
>> +	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
>> +				      TP_ACPI_HKEY_THNKLGHT_MASK);
>>  	return 0;
>>  }
>>
>> @@ -9114,6 +9117,8 @@ static void tpacpi_driver_event(const unsigned
>> int hkey_event) volume_alsa_notify_change();
>>  		}
>>  	}
>> +	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_THINKLIGHT)
>> +		led_notify_brightness_change(&tpacpi_led_kbdlight.led_classdev);
>
> This looks incorrect. You are trying to inform tpacpi_led_kbdlight when
> tpacpi_led_thinklight change led status?

No as said the scancode / event is shared, notice the check for
tp_features.kbdlight, so the notify will only happen on laptops which
actually have a backlit keyboard.

Regards,

Hans


>
>>  }
>>
>>  static void hotkey_driver_event(const unsigned int scancode)
>

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

[Hans de Goede]

Hi,

On 11-11-16 15:17, Pali Rohár wrote:
> On Tuesday 01 November 2016 14:37:47 Hans de Goede wrote:
>> @@ -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,
>> +};
>
> Should not be upper case? Looks like that Documentation/CodingStyle also
> suggest it:
>
> Names of macros defining constants and labels in enums are capitalized.

Ok, I will send a v5 once we agree on how to do the thinkpad bits.

Regards,

Hans

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 4204 bytes --]

On Friday 11 November 2016 15:33:48 Hans de Goede wrote:
> Hi,
> 
> On 11-11-16 15:12, Pali Rohár wrote:
> > On Tuesday 01 November 2016 14:37:46 Hans de Goede wrote:
> >> Make thinkpad_acpi call led_notify_brightness_change on the
> >> kbd_led led_classdev registered by thinkpad_acpi when the kbd
> >> backlight brightness changes.
> >> 
> >> 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
> >> ---
> >> 
> >>  drivers/platform/x86/thinkpad_acpi.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> >> b/drivers/platform/x86/thinkpad_acpi.c index b65ce75..5dcd7d8b
> >> 100644
> >> --- a/drivers/platform/x86/thinkpad_acpi.c
> >> +++ b/drivers/platform/x86/thinkpad_acpi.c
> >> @@ -162,6 +162,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_THINKLIGHT		= 0x1012, /* Thinklight/kbd backlight 
*/
> > 
> > My question remains. Is this for Thinklight or keyboard backlight?
> > Because Thinklinght has led device "tpacpi_led_thinklight" and
> > keyboard backlight has led device "tpacpi_led_kbdlight".
> 
> I would say both, this matches with the pre-existing
> TP_ACPI_HKEY_THNKLGHT_MASK (they have a 1:1 mapping),
> keep in mind that there are no thinkpads with both
> a thinklight and a backlit keyboard

That is not truth. X230 is in variant with both Thinklight (light above 
display) and keyboard backlight. Plus in BIOS it is possible to 
configure if you want to enable only Thinkligh, only keyboard backlight 
or both. Probably T430 and other 30s models can be found with both. But 
I do not own those modules...

> as those both
> serve the same purpose so it looks like the re-used
> the scancode.

So I could guess that it reports event when one of those leds are 
changed...

> I've tried this code on both a W541 and a P50 and both
> emit TP_ACPI_HOTKEYSCAN_FNPAGEUP for the backlight
> keyboard hotkey (fn + space), I'm naming the event for
> this TP_HKEY_EV_THINKLIGHT for consistency with the
> existing TP_ACPI_HKEY_THNKLGHT_MASK which corresponds
> to TP_ACPI_HOTKEYSCAN_FNPAGEUP.
> 
> >>  	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 */
> >> 
> >> @@ -5167,6 +5168,8 @@ static int __init kbdlight_init(struct
> >> ibm_init_struct *iibm) return rc;
> >> 
> >>  	}
> >> 
> >> +	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
> >> +				      TP_ACPI_HKEY_THNKLGHT_MASK);
> >> 
> >>  	return 0;
> >>  
> >>  }
> >> 
> >> @@ -9114,6 +9117,8 @@ static void tpacpi_driver_event(const
> >> unsigned int hkey_event) volume_alsa_notify_change();
> >> 
> >>  		}
> >>  	
> >>  	}
> >> 
> >> +	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_THINKLIGHT)
> >> +	
> >> led_notify_brightness_change(&tpacpi_led_kbdlight.led_classdev)
> >> ;
> > 
> > This looks incorrect. You are trying to inform tpacpi_led_kbdlight
> > when tpacpi_led_thinklight change led status?
> 
> No as said the scancode / event is shared, notice the check for
> tp_features.kbdlight, so the notify will only happen on laptops which
> actually have a backlit keyboard.

If that event is send when either Thinklight or keyb-backlight is 
changed, should not we inform both led devices about level change? Do 
you have a chance to test it on notebook with Thinklight?

And funny question is what would happen on those machines which have 
both Thinklight and keyboard backlight?

Anyway, I would suggest different name, not TP_HKEY_EV_THINKLIGHT as it 
is not thinklight-only and could be misleading in keyboard backlight 
context...

> Regards,
> 
> Hans
> 
> >>  }
> >>  
> >>  static void hotkey_driver_event(const unsigned int scancode)

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Kevin Locke]

On Fri, 2016-11-11 at 15:33 +0100, Hans de Goede wrote:
> On 11-11-16 15:12, Pali Rohár wrote:
>> My question remains. Is this for Thinklight or keyboard backlight?
>> Because Thinklinght has led device "tpacpi_led_thinklight" and keyboard
>> backlight has led device "tpacpi_led_kbdlight".
> 
> I would say both, this matches with the pre-existing
> TP_ACPI_HKEY_THNKLGHT_MASK (they have a 1:1 mapping),
> keep in mind that there are no thinkpads with both
> a thinklight and a backlit keyboard, as those both
> serve the same purpose so it looks like the re-used
> the scancode.

That's not entirely correct.  The ThinkPad T430 has both a ThinkLight
and a keyboard backlight.  Pressing Fn-Space toggles between 4 states:

Both Off -> Backlight Low -> Backlight High -> ThinkLight On (BL Off)

I tested out your patch and observed the following behavior (printing
brightness initially and on POLLPRI):

# Initial state with both lights off.
tpacpi::kbd_backlight/brightness: 0
tpacpi::thinklight/brightness: 0
# Press Fn-Space.  KBD Backlight comes on low.
tpacpi::kbd_backlight/brightness: 1
# Press Fn-Space.  KBD Backlight brightens to high.
tpacpi::kbd_backlight/brightness: 2
# Press Fn-Space.  KBD Backlight turns off.  ThinkLight turns on.
tpacpi::kbd_backlight/brightness: 0
# Press Fn-Space.  ThinkLight turns off.
tpacpi::kbd_backlight/brightness: 0

It works, but the behavior is not quite what I would hope for.  There
are no poll events for tpacpi::thinklight/brightness and when the
ThinkLight turns off it triggers an unnecessary
tpacpi::kbd_backlight/brightness poll event.

If there is anything else I can do to assist, let me know.

Thanks for working on this,
Kevin

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [ibm-acpi-devel] [PATCH v4 2/4] platform: x86: thinkpad: Call led_notify_brightness_change on kbd brightness change

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2399 bytes --]

On Friday 11 November 2016 19:40:51 Kevin Locke wrote:
> On Fri, 2016-11-11 at 15:33 +0100, Hans de Goede wrote:
> > On 11-11-16 15:12, Pali Rohár wrote:
> >> My question remains. Is this for Thinklight or keyboard backlight?
> >> Because Thinklinght has led device "tpacpi_led_thinklight" and
> >> keyboard backlight has led device "tpacpi_led_kbdlight".
> > 
> > I would say both, this matches with the pre-existing
> > TP_ACPI_HKEY_THNKLGHT_MASK (they have a 1:1 mapping),
> > keep in mind that there are no thinkpads with both
> > a thinklight and a backlit keyboard, as those both
> > serve the same purpose so it looks like the re-used
> > the scancode.
> 
> That's not entirely correct.  The ThinkPad T430 has both a ThinkLight
> and a keyboard backlight.  Pressing Fn-Space toggles between 4
> states:
> 
> Both Off -> Backlight Low -> Backlight High -> ThinkLight On (BL Off)
> 
> I tested out your patch and observed the following behavior (printing
> brightness initially and on POLLPRI):
> 
> # Initial state with both lights off.
> tpacpi::kbd_backlight/brightness: 0
> tpacpi::thinklight/brightness: 0
> # Press Fn-Space.  KBD Backlight comes on low.
> tpacpi::kbd_backlight/brightness: 1
> # Press Fn-Space.  KBD Backlight brightens to high.
> tpacpi::kbd_backlight/brightness: 2
> # Press Fn-Space.  KBD Backlight turns off.  ThinkLight turns on.
> tpacpi::kbd_backlight/brightness: 0
> # Press Fn-Space.  ThinkLight turns off.
> tpacpi::kbd_backlight/brightness: 0
> 
> It works, but the behavior is not quite what I would hope for.  There
> are no poll events for tpacpi::thinklight/brightness

Yes, this is what I already pointed... That we should send event also 
for tpacpi::thinklight.

> and when the
> ThinkLight turns off it triggers an unnecessary
> tpacpi::kbd_backlight/brightness poll event.

It is problem? Or are forced to send event only if brightness level is 
really changed? I think that bigger problem is when brightness changes, 
but we do not send event.

Should not be event treated as "hey, brightness level was probably 
changed, read file to get current status"?

> If there is anything else I can do to assist, let me know.

Great, this is really useful to see somebody who can test patches on 
those machines with both Thinklight and keyboard backlight...

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [ibm-acpi-devel] [PATCH v4 2/4] platform: x86: thinkpad: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

HI,

On 12-11-16 12:52, Pali Rohár wrote:
> On Friday 11 November 2016 19:40:51 Kevin Locke wrote:
>> On Fri, 2016-11-11 at 15:33 +0100, Hans de Goede wrote:
>>> On 11-11-16 15:12, Pali Rohár wrote:
>>>> My question remains. Is this for Thinklight or keyboard backlight?
>>>> Because Thinklinght has led device "tpacpi_led_thinklight" and
>>>> keyboard backlight has led device "tpacpi_led_kbdlight".
>>>
>>> I would say both, this matches with the pre-existing
>>> TP_ACPI_HKEY_THNKLGHT_MASK (they have a 1:1 mapping),
>>> keep in mind that there are no thinkpads with both
>>> a thinklight and a backlit keyboard, as those both
>>> serve the same purpose so it looks like the re-used
>>> the scancode.
>>
>> That's not entirely correct.  The ThinkPad T430 has both a ThinkLight
>> and a keyboard backlight.  Pressing Fn-Space toggles between 4
>> states:
>>
>> Both Off -> Backlight Low -> Backlight High -> ThinkLight On (BL Off)
>>
>> I tested out your patch and observed the following behavior (printing
>> brightness initially and on POLLPRI):
>>
>> # Initial state with both lights off.
>> tpacpi::kbd_backlight/brightness: 0
>> tpacpi::thinklight/brightness: 0
>> # Press Fn-Space.  KBD Backlight comes on low.
>> tpacpi::kbd_backlight/brightness: 1
>> # Press Fn-Space.  KBD Backlight brightens to high.
>> tpacpi::kbd_backlight/brightness: 2
>> # Press Fn-Space.  KBD Backlight turns off.  ThinkLight turns on.
>> tpacpi::kbd_backlight/brightness: 0
>> # Press Fn-Space.  ThinkLight turns off.
>> tpacpi::kbd_backlight/brightness: 0
>>
>> It works, but the behavior is not quite what I would hope for.  There
>> are no poll events for tpacpi::thinklight/brightness
>
> Yes, this is what I already pointed... That we should send event also
> for tpacpi::thinklight.

Agreed.

>> and when the
>> ThinkLight turns off it triggers an unnecessary
>> tpacpi::kbd_backlight/brightness poll event.
>
> It is problem? Or are forced to send event only if brightness level is
> really changed? I think that bigger problem is when brightness changes,
> but we do not send event.
>
> Should not be event treated as "hey, brightness level was probably
> changed, read file to get current status"?

That is what I was thinking too, waking up userspace poll()
waiters while nothing has changed is not 100% ideal, but
is not a big deal (esp. given the frequency with which this
will happen).

I will do a new version renaming the events / mask to indicate
that they apply to both the thinklight and the keyboard backlight;
and to also notify the thinklight led_classdev of changes
(if it is present).

Note that the led-class changes this relies on are still being
discussed, I will do a new version once that is sorted out.

>> If there is anything else I can do to assist, let me know.
>
> Great, this is really useful to see somebody who can test patches on
> those machines with both Thinklight and keyboard backlight...

+1 thank you for testing and for the feedback.

Regards,

Hans

>