[PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

[PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Hans de Goede]

In some cases it may be desirable for userspace to be notified when
a trigger event happens. This commit adds support for a poll-able
current_brightness trigger specific sysfs attribute which triggers
may register:

What:		/sys/class/leds/<led>/current_brightness
Date:		November 2016
KernelVersion:	4.10
Description:
	Triggers which support it may register a current_brightness
	file. This file supports poll() to detect when the trigger
	modifies the brightness of the LED.
	Reading this file will always return the current brightness
	of the LED.
	Writing this file sets the current brightness of the LED,
	without influencing the trigger.

This commit adds 3 functions triggers which want to support this can use:

void led_trigger_add_current_brightness(struct led_classdev *cdev);
void led_trigger_remove_current_brightness(struct led_classdev *cdev);
void led_trigger_notify_current_brightness_change(struct led_classdev *);

The add / remove functions are to be used as, or called from the triggers
activate / deactivate callbacks and when an event happens the trigger can
call the notify function to wake-up any poll() waiters.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-This is a new patch in v5 of this patch-set (replacing earlier attempts
 at similar functionality)
---
 Documentation/ABI/testing/sysfs-class-led | 15 +++++-
 drivers/leds/led-triggers.c               | 81 +++++++++++++++++++++++++++++++
 drivers/leds/leds.h                       |  3 ++
 include/linux/leds.h                      |  3 ++
 4 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 491cdee..b8cb74d 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -33,7 +33,8 @@ Description:
 		You can change triggers in a similar manner to the way an IO
 		scheduler is chosen. Trigger specific parameters can appear in
 		/sys/class/leds/<led> once a given trigger is selected. For
-		their documentation see sysfs-class-led-trigger-*.
+		their documentation see sysfs-class-led-trigger-*. Also see
+		the trigger specific current_brightness file described below.
 
 What:		/sys/class/leds/<led>/inverted
 Date:		January 2011
@@ -44,3 +45,15 @@ Description:
 		gpio and backlight triggers. In case of the backlight trigger,
 		it is useful when driving a LED which is intended to indicate
 		a device in a standby like state.
+
+What:		/sys/class/leds/<led>/current_brightness
+Date:		November 2016
+KernelVersion:	4.10
+Description:
+		Triggers which support it may register a current_brightness
+		file. This file supports poll() to detect when the trigger
+		modifies the brightness of the LED.
+		Reading this file will always return the current brightness
+		of the LED.
+		Writing this file sets the current brightness of the LED,
+		without influencing the trigger.
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b..d2ed9c2 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -102,6 +102,87 @@ ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
 }
 EXPORT_SYMBOL_GPL(led_trigger_show);
 
+static ssize_t current_brightness_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	led_update_brightness(led_cdev);
+
+	return sprintf(buf, "%u\n", led_cdev->brightness);
+}
+
+static ssize_t current_brightness_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_access);
+
+	if (led_sysfs_is_disabled(led_cdev)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		goto unlock;
+
+	/* _nosleep version so as to not stop sw blinking */
+	led_set_brightness_nosleep(led_cdev, state);
+
+	/* Let any listeners know the brighness changed */
+	if (led_cdev->current_brightness_kn)
+		sysfs_notify_dirent(led_cdev->current_brightness_kn);
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+
+static DEVICE_ATTR_RW(current_brightness);
+
+void led_trigger_add_current_brightness(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	ret = device_create_file(led_cdev->dev, &dev_attr_current_brightness);
+	if (ret) {
+		dev_err(led_cdev->dev, "Error creating current_brightness\n");
+		return;
+	}
+
+	led_cdev->current_brightness_kn =
+		sysfs_get_dirent(led_cdev->dev->kobj.sd, "current_brightness");
+	if (!led_cdev->current_brightness_kn)
+		dev_err(led_cdev->dev, "Error getting current_brightness kn\n");
+}
+EXPORT_SYMBOL_GPL(led_trigger_add_current_brightness);
+
+void led_trigger_remove_current_brightness(struct led_classdev *led_cdev)
+{
+	sysfs_put(led_cdev->current_brightness_kn);
+	led_cdev->current_brightness_kn = NULL;
+	device_remove_file(led_cdev->dev, &dev_attr_current_brightness);
+}
+EXPORT_SYMBOL_GPL(led_trigger_remove_current_brightness);
+
+void led_trigger_notify_current_brightness_change(struct led_trigger *trig)
+{
+	struct led_classdev *led_cdev;
+
+	read_lock(&trig->leddev_list_lock);
+	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+		if (led_cdev->current_brightness_kn)
+			sysfs_notify_dirent(led_cdev->current_brightness_kn);
+	}
+	read_unlock(&trig->leddev_list_lock);
+}
+EXPORT_SYMBOL_GPL(led_trigger_notify_current_brightness_change);
+
 /* Caller must ensure led_cdev->trigger_lock held */
 void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 {
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 7d38e6b..3d06ee6 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -27,6 +27,9 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 				enum led_brightness value);
 void led_set_brightness_nosleep(struct led_classdev *led_cdev,
 				enum led_brightness value);
+void led_trigger_add_current_brightness(struct led_classdev *cdev);
+void led_trigger_remove_current_brightness(struct led_classdev *cdev);
+void led_trigger_notify_current_brightness_change(struct led_trigger *trig);
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 569cb53..d3eb992 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>
@@ -108,6 +109,8 @@ struct led_classdev {
 	void			*trigger_data;
 	/* true if activated - deactivate routine uses it to do cleanup */
 	bool			activated;
+	/* For triggers with current_brightness sysfs attribute */
+	struct kernfs_node	*current_brightness_kn;
 #endif
 
 	/* Ensures consistent access to the LED Flash Class device */
-- 
2.9.3

[PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Add a trigger to control keyboard backlight LED devices. Note that in
some cases the keyboard backlight control is hardwired (taken care of
in firmware outside of the kernels control), in that case this triggers
main purpose is to allow userspace to monitor these changes.

The ledtrig_kbd_backlight function has a set_brightness parameter to
differentiate between full backlight control through the trigger
(set_brightness set to true) or change notification only (false).

Note the Kconfig option for this is a bool because the code is so
small that it is not worth the overhead of being a module.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-This is a new patch in v5 of this patch-set (replacing earlier attempts
 at similar functionality)
---
 drivers/leds/trigger/Kconfig                 | 10 ++++++++
 drivers/leds/trigger/Makefile                |  1 +
 drivers/leds/trigger/ledtrig-kbd-backlight.c | 38 ++++++++++++++++++++++++++++
 include/linux/leds.h                         |  8 ++++++
 4 files changed, 57 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-kbd-backlight.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 3f9ddb9..350e2c7 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -67,6 +67,16 @@ config LEDS_TRIGGER_BACKLIGHT
 
 	  If unsure, say N.
 
+config LEDS_TRIGGER_KBD_BACKLIGHT
+	bool "LED keyboard backlight Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This trigger can control keyboard backlight LED devices,
+	  it also allows user-space to monitor keyboard backlight brightness
+	  changes done through e.g. hotkeys on some laptops.
+
+	  If unsure, say Y.
+
 config LEDS_TRIGGER_CPU
 	bool "LED CPU Trigger"
 	depends on LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index a72c43c..be6b249 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_LEDS_TRIGGER_DISK)		+= ledtrig-disk.o
 obj-$(CONFIG_LEDS_TRIGGER_MTD)		+= ledtrig-mtd.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
+obj-$(CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT) += ledtrig-kbd-backlight.o
 obj-$(CONFIG_LEDS_TRIGGER_GPIO)		+= ledtrig-gpio.o
 obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
diff --git a/drivers/leds/trigger/ledtrig-kbd-backlight.c b/drivers/leds/trigger/ledtrig-kbd-backlight.c
new file mode 100644
index 0000000..353ee92
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-kbd-backlight.c
@@ -0,0 +1,38 @@
+/*
+ * LED Trigger for keyboard backlight control
+ *
+ * Copyright 2016, Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/leds.h>
+#include "../leds.h"
+
+static struct led_trigger kbd_backlight_trigger = {
+	.name     = "kbd-backlight",
+	.activate = led_trigger_add_current_brightness,
+	.deactivate = led_trigger_remove_current_brightness,
+};
+
+void ledtrig_kbd_backlight(bool set_brightness, enum led_brightness brightness)
+{
+	if (set_brightness)
+		led_trigger_event(&kbd_backlight_trigger, brightness);
+
+	led_trigger_notify_current_brightness_change(&kbd_backlight_trigger);
+}
+EXPORT_SYMBOL_GPL(ledtrig_kbd_backlight);
+
+static int __init kbd_backlight_trig_init(void)
+{
+	return led_trigger_register(&kbd_backlight_trigger);
+}
+device_initcall(kbd_backlight_trig_init);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d3eb992..870b8c2 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -353,6 +353,14 @@ static inline void ledtrig_flash_ctrl(bool on) {}
 static inline void ledtrig_torch_ctrl(bool on) {}
 #endif
 
+#ifdef CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT
+extern void ledtrig_kbd_backlight(bool set_brightness,
+				  enum led_brightness brightness);
+#else
+static inline void ledtrig_kbd_backlight(bool set_brightness,
+					 enum led_brightness brightness) {}
+#endif
+
 /*
  * Generic LED platform data for describing LED names and default triggers.
  */
-- 
2.9.3

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi Hans,

Thanks for the patch.

I think we need less generic trigger name.
With present name we pretend that all kbd-backlight controllers
can change LED brightness autonomously.

How about kbd-backlight-pollable ?

Best regards,
Jacek Anaszewski

On 11/17/2016 11:24 PM, Hans de Goede wrote:
> Add a trigger to control keyboard backlight LED devices. Note that in
> some cases the keyboard backlight control is hardwired (taken care of
> in firmware outside of the kernels control), in that case this triggers
> main purpose is to allow userspace to monitor these changes.
>
> The ledtrig_kbd_backlight function has a set_brightness parameter to
> differentiate between full backlight control through the trigger
> (set_brightness set to true) or change notification only (false).
>
> Note the Kconfig option for this is a bool because the code is so
> small that it is not worth the overhead of being a module.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> -This is a new patch in v5 of this patch-set (replacing earlier attempts
>  at similar functionality)
> ---
>  drivers/leds/trigger/Kconfig                 | 10 ++++++++
>  drivers/leds/trigger/Makefile                |  1 +
>  drivers/leds/trigger/ledtrig-kbd-backlight.c | 38 ++++++++++++++++++++++++++++
>  include/linux/leds.h                         |  8 ++++++
>  4 files changed, 57 insertions(+)
>  create mode 100644 drivers/leds/trigger/ledtrig-kbd-backlight.c
>
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 3f9ddb9..350e2c7 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -67,6 +67,16 @@ config LEDS_TRIGGER_BACKLIGHT
>
>  	  If unsure, say N.
>
> +config LEDS_TRIGGER_KBD_BACKLIGHT
> +	bool "LED keyboard backlight Trigger"
> +	depends on LEDS_TRIGGERS
> +	help
> +	  This trigger can control keyboard backlight LED devices,
> +	  it also allows user-space to monitor keyboard backlight brightness
> +	  changes done through e.g. hotkeys on some laptops.
> +
> +	  If unsure, say Y.
> +
>  config LEDS_TRIGGER_CPU
>  	bool "LED CPU Trigger"
>  	depends on LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index a72c43c..be6b249 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_LEDS_TRIGGER_DISK)		+= ledtrig-disk.o
>  obj-$(CONFIG_LEDS_TRIGGER_MTD)		+= ledtrig-mtd.o
>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
> +obj-$(CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT) += ledtrig-kbd-backlight.o
>  obj-$(CONFIG_LEDS_TRIGGER_GPIO)		+= ledtrig-gpio.o
>  obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
>  obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
> diff --git a/drivers/leds/trigger/ledtrig-kbd-backlight.c b/drivers/leds/trigger/ledtrig-kbd-backlight.c
> new file mode 100644
> index 0000000..353ee92
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-kbd-backlight.c
> @@ -0,0 +1,38 @@
> +/*
> + * LED Trigger for keyboard backlight control
> + *
> + * Copyright 2016, Hans de Goede <hdegoede@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/leds.h>
> +#include "../leds.h"
> +
> +static struct led_trigger kbd_backlight_trigger = {
> +	.name     = "kbd-backlight",
> +	.activate = led_trigger_add_current_brightness,
> +	.deactivate = led_trigger_remove_current_brightness,
> +};
> +
> +void ledtrig_kbd_backlight(bool set_brightness, enum led_brightness brightness)
> +{
> +	if (set_brightness)
> +		led_trigger_event(&kbd_backlight_trigger, brightness);
> +
> +	led_trigger_notify_current_brightness_change(&kbd_backlight_trigger);
> +}
> +EXPORT_SYMBOL_GPL(ledtrig_kbd_backlight);
> +
> +static int __init kbd_backlight_trig_init(void)
> +{
> +	return led_trigger_register(&kbd_backlight_trigger);
> +}
> +device_initcall(kbd_backlight_trig_init);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index d3eb992..870b8c2 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -353,6 +353,14 @@ static inline void ledtrig_flash_ctrl(bool on) {}
>  static inline void ledtrig_torch_ctrl(bool on) {}
>  #endif
>
> +#ifdef CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT
> +extern void ledtrig_kbd_backlight(bool set_brightness,
> +				  enum led_brightness brightness);
> +#else
> +static inline void ledtrig_kbd_backlight(bool set_brightness,
> +					 enum led_brightness brightness) {}
> +#endif
> +
>  /*
>   * Generic LED platform data for describing LED names and default triggers.
>   */
>

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 18-11-16 09:55, Jacek Anaszewski wrote:
> Hi Hans,
>
> Thanks for the patch.
>
> I think we need less generic trigger name.
> With present name we pretend that all kbd-backlight controllers
> can change LED brightness autonomously.
>
> How about kbd-backlight-pollable ?

This is a trigger to control kbd-backlights, in the
current use-case the brightness change is already done
by the firmware, hence the set_brightness argument to
ledtrig_kbd_backlight(), so that we can avoid setting
it again.

But I can see future cases where we do want to have some
event (e.g. a wmi hotkey event on a laptop where the firmware
does not do the adjustment automatically) which does
lead to actually updating the brightness.

So I decided to go with a generic kbd-backlight trigger,
which in the future can also be used to directly control
kbd-backlight brightness; and not just to make ot
poll-able.

Regards,

Hans



>
> Best regards,
> Jacek Anaszewski
>
> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>> Add a trigger to control keyboard backlight LED devices. Note that in
>> some cases the keyboard backlight control is hardwired (taken care of
>> in firmware outside of the kernels control), in that case this triggers
>> main purpose is to allow userspace to monitor these changes.
>>
>> The ledtrig_kbd_backlight function has a set_brightness parameter to
>> differentiate between full backlight control through the trigger
>> (set_brightness set to true) or change notification only (false).
>>
>> Note the Kconfig option for this is a bool because the code is so
>> small that it is not worth the overhead of being a module.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v5:
>> -This is a new patch in v5 of this patch-set (replacing earlier attempts
>>  at similar functionality)
>> ---
>>  drivers/leds/trigger/Kconfig                 | 10 ++++++++
>>  drivers/leds/trigger/Makefile                |  1 +
>>  drivers/leds/trigger/ledtrig-kbd-backlight.c | 38 ++++++++++++++++++++++++++++
>>  include/linux/leds.h                         |  8 ++++++
>>  4 files changed, 57 insertions(+)
>>  create mode 100644 drivers/leds/trigger/ledtrig-kbd-backlight.c
>>
>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>> index 3f9ddb9..350e2c7 100644
>> --- a/drivers/leds/trigger/Kconfig
>> +++ b/drivers/leds/trigger/Kconfig
>> @@ -67,6 +67,16 @@ config LEDS_TRIGGER_BACKLIGHT
>>
>>        If unsure, say N.
>>
>> +config LEDS_TRIGGER_KBD_BACKLIGHT
>> +    bool "LED keyboard backlight Trigger"
>> +    depends on LEDS_TRIGGERS
>> +    help
>> +      This trigger can control keyboard backlight LED devices,
>> +      it also allows user-space to monitor keyboard backlight brightness
>> +      changes done through e.g. hotkeys on some laptops.
>> +
>> +      If unsure, say Y.
>> +
>>  config LEDS_TRIGGER_CPU
>>      bool "LED CPU Trigger"
>>      depends on LEDS_TRIGGERS
>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>> index a72c43c..be6b249 100644
>> --- a/drivers/leds/trigger/Makefile
>> +++ b/drivers/leds/trigger/Makefile
>> @@ -4,6 +4,7 @@ obj-$(CONFIG_LEDS_TRIGGER_DISK)        += ledtrig-disk.o
>>  obj-$(CONFIG_LEDS_TRIGGER_MTD)        += ledtrig-mtd.o
>>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)    += ledtrig-heartbeat.o
>>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)    += ledtrig-backlight.o
>> +obj-$(CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT) += ledtrig-kbd-backlight.o
>>  obj-$(CONFIG_LEDS_TRIGGER_GPIO)        += ledtrig-gpio.o
>>  obj-$(CONFIG_LEDS_TRIGGER_CPU)        += ledtrig-cpu.o
>>  obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)    += ledtrig-default-on.o
>> diff --git a/drivers/leds/trigger/ledtrig-kbd-backlight.c b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>> new file mode 100644
>> index 0000000..353ee92
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * LED Trigger for keyboard backlight control
>> + *
>> + * Copyright 2016, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/device.h>
>> +#include <linux/leds.h>
>> +#include "../leds.h"
>> +
>> +static struct led_trigger kbd_backlight_trigger = {
>> +    .name     = "kbd-backlight",
>> +    .activate = led_trigger_add_current_brightness,
>> +    .deactivate = led_trigger_remove_current_brightness,
>> +};
>> +
>> +void ledtrig_kbd_backlight(bool set_brightness, enum led_brightness brightness)
>> +{
>> +    if (set_brightness)
>> +        led_trigger_event(&kbd_backlight_trigger, brightness);
>> +
>> +    led_trigger_notify_current_brightness_change(&kbd_backlight_trigger);
>> +}
>> +EXPORT_SYMBOL_GPL(ledtrig_kbd_backlight);
>> +
>> +static int __init kbd_backlight_trig_init(void)
>> +{
>> +    return led_trigger_register(&kbd_backlight_trigger);
>> +}
>> +device_initcall(kbd_backlight_trig_init);
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index d3eb992..870b8c2 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -353,6 +353,14 @@ static inline void ledtrig_flash_ctrl(bool on) {}
>>  static inline void ledtrig_torch_ctrl(bool on) {}
>>  #endif
>>
>> +#ifdef CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT
>> +extern void ledtrig_kbd_backlight(bool set_brightness,
>> +                  enum led_brightness brightness);
>> +#else
>> +static inline void ledtrig_kbd_backlight(bool set_brightness,
>> +                     enum led_brightness brightness) {}
>> +#endif
>> +
>>  /*
>>   * Generic LED platform data for describing LED names and default triggers.
>>   */
>>
>
>

------------------------------------------------------------------------------

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/18/2016 10:07 AM, Hans de Goede wrote:
> Hi,
>
> On 18-11-16 09:55, Jacek Anaszewski wrote:
>> Hi Hans,
>>
>> Thanks for the patch.
>>
>> I think we need less generic trigger name.
>> With present name we pretend that all kbd-backlight controllers
>> can change LED brightness autonomously.
>>
>> How about kbd-backlight-pollable ?
>
> This is a trigger to control kbd-backlights, in the
> current use-case the brightness change is already done
> by the firmware, hence the set_brightness argument to
> ledtrig_kbd_backlight(), so that we can avoid setting
> it again.
>
> But I can see future cases where we do want to have some
> event (e.g. a wmi hotkey event on a laptop where the firmware
> does not do the adjustment automatically) which does
> lead to actually updating the brightness.
>
> So I decided to go with a generic kbd-backlight trigger,
> which in the future can also be used to directly control
> kbd-backlight brightness; and not just to make ot
> poll-able.

I thought that kbd-backlight stands for "keyboard backlight",
that's why I assessed it is too generic. It seems however to be
the other way round - if kbd-backlight means that this is
a trigger only for use with dell-laptop keyboard driver
(I see kbd namespacing prefix in the driver functions) than it is
not generic at all.

Current LED subsystem triggers are generic - e.g. disk, mtd,
backlight (it registers on video fb notifications).
Driver specific trigger should be implemented inside the driver.

Last but not least - generic keyboard backlight trigger can't assume
that all devices of this type can adjust backlight brightness.

Best Regards,
Jacek Anaszewski

>> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>>> Add a trigger to control keyboard backlight LED devices. Note that in
>>> some cases the keyboard backlight control is hardwired (taken care of
>>> in firmware outside of the kernels control), in that case this triggers
>>> main purpose is to allow userspace to monitor these changes.
>>>
>>> The ledtrig_kbd_backlight function has a set_brightness parameter to
>>> differentiate between full backlight control through the trigger
>>> (set_brightness set to true) or change notification only (false).
>>>
>>> Note the Kconfig option for this is a bool because the code is so
>>> small that it is not worth the overhead of being a module.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v5:
>>> -This is a new patch in v5 of this patch-set (replacing earlier attempts
>>>  at similar functionality)
>>> ---
>>>  drivers/leds/trigger/Kconfig                 | 10 ++++++++
>>>  drivers/leds/trigger/Makefile                |  1 +
>>>  drivers/leds/trigger/ledtrig-kbd-backlight.c | 38
>>> ++++++++++++++++++++++++++++
>>>  include/linux/leds.h                         |  8 ++++++
>>>  4 files changed, 57 insertions(+)
>>>  create mode 100644 drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>
>>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>>> index 3f9ddb9..350e2c7 100644
>>> --- a/drivers/leds/trigger/Kconfig
>>> +++ b/drivers/leds/trigger/Kconfig
>>> @@ -67,6 +67,16 @@ config LEDS_TRIGGER_BACKLIGHT
>>>
>>>        If unsure, say N.
>>>
>>> +config LEDS_TRIGGER_KBD_BACKLIGHT
>>> +    bool "LED keyboard backlight Trigger"
>>> +    depends on LEDS_TRIGGERS
>>> +    help
>>> +      This trigger can control keyboard backlight LED devices,
>>> +      it also allows user-space to monitor keyboard backlight
>>> brightness
>>> +      changes done through e.g. hotkeys on some laptops.
>>> +
>>> +      If unsure, say Y.
>>> +
>>>  config LEDS_TRIGGER_CPU
>>>      bool "LED CPU Trigger"
>>>      depends on LEDS_TRIGGERS
>>> diff --git a/drivers/leds/trigger/Makefile
>>> b/drivers/leds/trigger/Makefile
>>> index a72c43c..be6b249 100644
>>> --- a/drivers/leds/trigger/Makefile
>>> +++ b/drivers/leds/trigger/Makefile
>>> @@ -4,6 +4,7 @@ obj-$(CONFIG_LEDS_TRIGGER_DISK)        += ledtrig-disk.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_MTD)        += ledtrig-mtd.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)    += ledtrig-heartbeat.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)    += ledtrig-backlight.o
>>> +obj-$(CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT) += ledtrig-kbd-backlight.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_GPIO)        += ledtrig-gpio.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_CPU)        += ledtrig-cpu.o
>>>  obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)    += ledtrig-default-on.o
>>> diff --git a/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>> b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>> new file mode 100644
>>> index 0000000..353ee92
>>> --- /dev/null
>>> +++ b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * LED Trigger for keyboard backlight control
>>> + *
>>> + * Copyright 2016, Hans de Goede <hdegoede@redhat.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/init.h>
>>> +#include <linux/device.h>
>>> +#include <linux/leds.h>
>>> +#include "../leds.h"
>>> +
>>> +static struct led_trigger kbd_backlight_trigger = {
>>> +    .name     = "kbd-backlight",
>>> +    .activate = led_trigger_add_current_brightness,
>>> +    .deactivate = led_trigger_remove_current_brightness,
>>> +};
>>> +
>>> +void ledtrig_kbd_backlight(bool set_brightness, enum led_brightness
>>> brightness)
>>> +{
>>> +    if (set_brightness)
>>> +        led_trigger_event(&kbd_backlight_trigger, brightness);
>>> +
>>> +
>>> led_trigger_notify_current_brightness_change(&kbd_backlight_trigger);
>>> +}
>>> +EXPORT_SYMBOL_GPL(ledtrig_kbd_backlight);
>>> +
>>> +static int __init kbd_backlight_trig_init(void)
>>> +{
>>> +    return led_trigger_register(&kbd_backlight_trigger);
>>> +}
>>> +device_initcall(kbd_backlight_trig_init);
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index d3eb992..870b8c2 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -353,6 +353,14 @@ static inline void ledtrig_flash_ctrl(bool on) {}
>>>  static inline void ledtrig_torch_ctrl(bool on) {}
>>>  #endif
>>>
>>> +#ifdef CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT
>>> +extern void ledtrig_kbd_backlight(bool set_brightness,
>>> +                  enum led_brightness brightness);
>>> +#else
>>> +static inline void ledtrig_kbd_backlight(bool set_brightness,
>>> +                     enum led_brightness brightness) {}
>>> +#endif
>>> +
>>>  /*
>>>   * Generic LED platform data for describing LED names and default
>>> triggers.
>>>   */
>>>
>>
>>
>
>
>

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

HI,

On 18-11-16 17:03, Jacek Anaszewski wrote:
> Hi,
>
> On 11/18/2016 10:07 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 18-11-16 09:55, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
>>> Thanks for the patch.
>>>
>>> I think we need less generic trigger name.
>>> With present name we pretend that all kbd-backlight controllers
>>> can change LED brightness autonomously.
>>>
>>> How about kbd-backlight-pollable ?
>>
>> This is a trigger to control kbd-backlights, in the
>> current use-case the brightness change is already done
>> by the firmware, hence the set_brightness argument to
>> ledtrig_kbd_backlight(), so that we can avoid setting
>> it again.
>>
>> But I can see future cases where we do want to have some
>> event (e.g. a wmi hotkey event on a laptop where the firmware
>> does not do the adjustment automatically) which does
>> lead to actually updating the brightness.
>>
>> So I decided to go with a generic kbd-backlight trigger,
>> which in the future can also be used to directly control
>> kbd-backlight brightness; and not just to make ot
>> poll-able.
>
> I thought that kbd-backlight stands for "keyboard backlight",

It does.

> that's why I assessed it is too generic.

The whole purpose of the trigger as implemented is to be
generic, as it seems senseless to implement a one off
trigger for just the dell / thinkpad case.

> It seems however to be
> the other way round - if kbd-backlight means that this is
> a trigger only for use with dell-laptop keyboard driver
> (I see kbd namespacing prefix in the driver functions) than it is
> not generic at all.

The trigger as implemented is generic, if you think
otherwise, please let me know which part is not generic
according to you.

>
> Current LED subsystem triggers are generic - e.g. disk, mtd,
> backlight (it registers on video fb notifications).

Right and I tried to follow that model, the trigger as
implemented is generic. Any event which wishes to trigger
a keyboard backlight brightness change can call the added
ledtrig_kbd_backlight() function, just like any mtd
event can call the mtd trigger, etc.

If you think anything about this trigger is not generic,
please explain which bits are not generic according to
you.

Regards,

Hans



> Driver specific trigger should be implemented inside the driver.
>
> Last but not least - generic keyboard backlight trigger can't assume
> that all devices of this type can adjust backlight brightness.


>
> Best Regards,
> Jacek Anaszewski
>
>>> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>>>> Add a trigger to control keyboard backlight LED devices. Note that in
>>>> some cases the keyboard backlight control is hardwired (taken care of
>>>> in firmware outside of the kernels control), in that case this triggers
>>>> main purpose is to allow userspace to monitor these changes.
>>>>
>>>> The ledtrig_kbd_backlight function has a set_brightness parameter to
>>>> differentiate between full backlight control through the trigger
>>>> (set_brightness set to true) or change notification only (false).
>>>>
>>>> Note the Kconfig option for this is a bool because the code is so
>>>> small that it is not worth the overhead of being a module.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v5:
>>>> -This is a new patch in v5 of this patch-set (replacing earlier attempts
>>>>  at similar functionality)
>>>> ---
>>>>  drivers/leds/trigger/Kconfig                 | 10 ++++++++
>>>>  drivers/leds/trigger/Makefile                |  1 +
>>>>  drivers/leds/trigger/ledtrig-kbd-backlight.c | 38
>>>> ++++++++++++++++++++++++++++
>>>>  include/linux/leds.h                         |  8 ++++++
>>>>  4 files changed, 57 insertions(+)
>>>>  create mode 100644 drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>>
>>>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>>>> index 3f9ddb9..350e2c7 100644
>>>> --- a/drivers/leds/trigger/Kconfig
>>>> +++ b/drivers/leds/trigger/Kconfig
>>>> @@ -67,6 +67,16 @@ config LEDS_TRIGGER_BACKLIGHT
>>>>
>>>>        If unsure, say N.
>>>>
>>>> +config LEDS_TRIGGER_KBD_BACKLIGHT
>>>> +    bool "LED keyboard backlight Trigger"
>>>> +    depends on LEDS_TRIGGERS
>>>> +    help
>>>> +      This trigger can control keyboard backlight LED devices,
>>>> +      it also allows user-space to monitor keyboard backlight
>>>> brightness
>>>> +      changes done through e.g. hotkeys on some laptops.
>>>> +
>>>> +      If unsure, say Y.
>>>> +
>>>>  config LEDS_TRIGGER_CPU
>>>>      bool "LED CPU Trigger"
>>>>      depends on LEDS_TRIGGERS
>>>> diff --git a/drivers/leds/trigger/Makefile
>>>> b/drivers/leds/trigger/Makefile
>>>> index a72c43c..be6b249 100644
>>>> --- a/drivers/leds/trigger/Makefile
>>>> +++ b/drivers/leds/trigger/Makefile
>>>> @@ -4,6 +4,7 @@ obj-$(CONFIG_LEDS_TRIGGER_DISK)        += ledtrig-disk.o
>>>>  obj-$(CONFIG_LEDS_TRIGGER_MTD)        += ledtrig-mtd.o
>>>>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)    += ledtrig-heartbeat.o
>>>>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)    += ledtrig-backlight.o
>>>> +obj-$(CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT) += ledtrig-kbd-backlight.o
>>>>  obj-$(CONFIG_LEDS_TRIGGER_GPIO)        += ledtrig-gpio.o
>>>>  obj-$(CONFIG_LEDS_TRIGGER_CPU)        += ledtrig-cpu.o
>>>>  obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)    += ledtrig-default-on.o
>>>> diff --git a/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>> b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>> new file mode 100644
>>>> index 0000000..353ee92
>>>> --- /dev/null
>>>> +++ b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> + * LED Trigger for keyboard backlight control
>>>> + *
>>>> + * Copyright 2016, Hans de Goede <hdegoede@redhat.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/leds.h>
>>>> +#include "../leds.h"
>>>> +
>>>> +static struct led_trigger kbd_backlight_trigger = {
>>>> +    .name     = "kbd-backlight",
>>>> +    .activate = led_trigger_add_current_brightness,
>>>> +    .deactivate = led_trigger_remove_current_brightness,
>>>> +};
>>>> +
>>>> +void ledtrig_kbd_backlight(bool set_brightness, enum led_brightness
>>>> brightness)
>>>> +{
>>>> +    if (set_brightness)
>>>> +        led_trigger_event(&kbd_backlight_trigger, brightness);
>>>> +
>>>> +
>>>> led_trigger_notify_current_brightness_change(&kbd_backlight_trigger);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(ledtrig_kbd_backlight);
>>>> +
>>>> +static int __init kbd_backlight_trig_init(void)
>>>> +{
>>>> +    return led_trigger_register(&kbd_backlight_trigger);
>>>> +}
>>>> +device_initcall(kbd_backlight_trig_init);
>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>> index d3eb992..870b8c2 100644
>>>> --- a/include/linux/leds.h
>>>> +++ b/include/linux/leds.h
>>>> @@ -353,6 +353,14 @@ static inline void ledtrig_flash_ctrl(bool on) {}
>>>>  static inline void ledtrig_torch_ctrl(bool on) {}
>>>>  #endif
>>>>
>>>> +#ifdef CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT
>>>> +extern void ledtrig_kbd_backlight(bool set_brightness,
>>>> +                  enum led_brightness brightness);
>>>> +#else
>>>> +static inline void ledtrig_kbd_backlight(bool set_brightness,
>>>> +                     enum led_brightness brightness) {}
>>>> +#endif
>>>> +
>>>>  /*
>>>>   * Generic LED platform data for describing LED names and default
>>>> triggers.
>>>>   */
>>>>
>>>
>>>
>>
>>
>>
>

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/18/2016 07:47 PM, Hans de Goede wrote:
> HI,
>
> On 18-11-16 17:03, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 11/18/2016 10:07 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 18-11-16 09:55, Jacek Anaszewski wrote:
>>>> Hi Hans,
>>>>
>>>> Thanks for the patch.
>>>>
>>>> I think we need less generic trigger name.
>>>> With present name we pretend that all kbd-backlight controllers
>>>> can change LED brightness autonomously.
>>>>
>>>> How about kbd-backlight-pollable ?
>>>
>>> This is a trigger to control kbd-backlights, in the
>>> current use-case the brightness change is already done
>>> by the firmware, hence the set_brightness argument to
>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>> it again.
>>>
>>> But I can see future cases where we do want to have some
>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>> does not do the adjustment automatically) which does
>>> lead to actually updating the brightness.
>>>
>>> So I decided to go with a generic kbd-backlight trigger,
>>> which in the future can also be used to directly control
>>> kbd-backlight brightness; and not just to make ot
>>> poll-able.
>>
>> I thought that kbd-backlight stands for "keyboard backlight",
>
> It does.
>
>> that's why I assessed it is too generic.
>
> The whole purpose of the trigger as implemented is to be
> generic, as it seems senseless to implement a one off
> trigger for just the dell / thinkpad case.
>
>> It seems however to be
>> the other way round - if kbd-backlight means that this is
>> a trigger only for use with dell-laptop keyboard driver
>> (I see kbd namespacing prefix in the driver functions) than it is
>> not generic at all.
>
> The trigger as implemented is generic, if you think
> otherwise, please let me know which part is not generic
> according to you.

I think I was too meticulous here. In the end of the previous
message I mentioned that we cannot guarantee that all keyboard
backlight controllers can adjust brightness autonomously. Nonetheless,
for the ones that cannot do that it would make no sense to have
a trigger. In view of that this trigger is generic enough.

I'll wait for Pavel's opinion before merging the patch set
as he was also involved in this whole thread.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

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

On Saturday 19 November 2016 16:44:09 Jacek Anaszewski wrote:
> Hi,
> 
> On 11/18/2016 07:47 PM, Hans de Goede wrote:
> > HI,
> > 
> > On 18-11-16 17:03, Jacek Anaszewski wrote:
> >> Hi,
> >> 
> >> On 11/18/2016 10:07 AM, Hans de Goede wrote:
> >>> Hi,
> >>> 
> >>> On 18-11-16 09:55, Jacek Anaszewski wrote:
> >>>> Hi Hans,
> >>>> 
> >>>> Thanks for the patch.
> >>>> 
> >>>> I think we need less generic trigger name.
> >>>> With present name we pretend that all kbd-backlight controllers
> >>>> can change LED brightness autonomously.
> >>>> 
> >>>> How about kbd-backlight-pollable ?
> >>> 
> >>> This is a trigger to control kbd-backlights, in the
> >>> current use-case the brightness change is already done
> >>> by the firmware, hence the set_brightness argument to
> >>> ledtrig_kbd_backlight(), so that we can avoid setting
> >>> it again.
> >>> 
> >>> But I can see future cases where we do want to have some
> >>> event (e.g. a wmi hotkey event on a laptop where the firmware
> >>> does not do the adjustment automatically) which does
> >>> lead to actually updating the brightness.
> >>> 
> >>> So I decided to go with a generic kbd-backlight trigger,
> >>> which in the future can also be used to directly control
> >>> kbd-backlight brightness; and not just to make ot
> >>> poll-able.
> >> 
> >> I thought that kbd-backlight stands for "keyboard backlight",
> > 
> > It does.
> > 
> >> that's why I assessed it is too generic.
> > 
> > The whole purpose of the trigger as implemented is to be
> > generic, as it seems senseless to implement a one off
> > trigger for just the dell / thinkpad case.
> > 
> >> It seems however to be
> >> the other way round - if kbd-backlight means that this is
> >> a trigger only for use with dell-laptop keyboard driver
> >> (I see kbd namespacing prefix in the driver functions) than it is
> >> not generic at all.
> > 
> > The trigger as implemented is generic, if you think
> > otherwise, please let me know which part is not generic
> > according to you.
> 
> I think I was too meticulous here. In the end of the previous
> message I mentioned that we cannot guarantee that all keyboard
> backlight controllers can adjust brightness autonomously.
> Nonetheless, for the ones that cannot do that it would make no sense
> to have a trigger. In view of that this trigger is generic enough.
> 
> I'll wait for Pavel's opinion before merging the patch set
> as he was also involved in this whole thread.

Do you mean me? Or Pavel Machek (CCed) who was involved in LEDs for input devices?

As pointed in other email, we do not know if HW really controls keyboard backlight,
so adding "fake" trigger on machines without HW control is not a good idea.

-- 
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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> > >>>> Thanks for the patch.
> > >>>> 
> > >>>> I think we need less generic trigger name.
> > >>>> With present name we pretend that all kbd-backlight controllers
> > >>>> can change LED brightness autonomously.
> > >>>> 
> > >>>> How about kbd-backlight-pollable ?
> > >>> 
> > >>> This is a trigger to control kbd-backlights, in the
> > >>> current use-case the brightness change is already done
> > >>> by the firmware, hence the set_brightness argument to
> > >>> ledtrig_kbd_backlight(), so that we can avoid setting
> > >>> it again.
> > >>> 
> > >>> But I can see future cases where we do want to have some
> > >>> event (e.g. a wmi hotkey event on a laptop where the firmware
> > >>> does not do the adjustment automatically) which does
> > >>> lead to actually updating the brightness.
> > >>> 
> > >>> So I decided to go with a generic kbd-backlight trigger,
> > >>> which in the future can also be used to directly control
> > >>> kbd-backlight brightness; and not just to make ot
> > >>> poll-able.
> > >> 
> > >> I thought that kbd-backlight stands for "keyboard backlight",
> > > 
> > > It does.
> > > 
> > >> that's why I assessed it is too generic.
> > > 
> > > The whole purpose of the trigger as implemented is to be
> > > generic, as it seems senseless to implement a one off
> > > trigger for just the dell / thinkpad case.
> > > 
> > >> It seems however to be
> > >> the other way round - if kbd-backlight means that this is
> > >> a trigger only for use with dell-laptop keyboard driver
> > >> (I see kbd namespacing prefix in the driver functions) than it is
> > >> not generic at all.
> > > 
> > > The trigger as implemented is generic, if you think
> > > otherwise, please let me know which part is not generic
> > > according to you.
> > 
> > I think I was too meticulous here. In the end of the previous
> > message I mentioned that we cannot guarantee that all keyboard
> > backlight controllers can adjust brightness autonomously.
> > Nonetheless, for the ones that cannot do that it would make no sense
> > to have a trigger. In view of that this trigger is generic enough.
> > 
> > I'll wait for Pavel's opinion before merging the patch set
> > as he was also involved in this whole thread.

If we have a keyboard backlight that may be changed automatically, I'd
go for trigger. If we know for sure that hardware will not change
brightnes "on its own", I'd not put a trigger there (and polling makes
no sense). If we don't know... I guess I'd go for trigger.

We can do various white/blacklists if we really want to..

> As pointed in other email, we do not know if HW really controls keyboard backlight,
> so adding "fake" trigger on machines without HW control is not a good idea.

Well, if we know that hardware will not change the brightness on its
own, yes, I'd avoid the trigger. If we don't know (as is common on
ACPI machines, I'd keep the trigger).

Best regards,

     	    	   	      	       	       	  	     	   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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

HI,

On 20-11-16 17:21, Pavel Machek wrote:
> Hi!
>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> I think we need less generic trigger name.
>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>> can change LED brightness autonomously.
>>>>>>>
>>>>>>> How about kbd-backlight-pollable ?
>>>>>>
>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>> current use-case the brightness change is already done
>>>>>> by the firmware, hence the set_brightness argument to
>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>> it again.
>>>>>>
>>>>>> But I can see future cases where we do want to have some
>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>> does not do the adjustment automatically) which does
>>>>>> lead to actually updating the brightness.
>>>>>>
>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>> which in the future can also be used to directly control
>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>> poll-able.
>>>>>
>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>
>>>> It does.
>>>>
>>>>> that's why I assessed it is too generic.
>>>>
>>>> The whole purpose of the trigger as implemented is to be
>>>> generic, as it seems senseless to implement a one off
>>>> trigger for just the dell / thinkpad case.
>>>>
>>>>> It seems however to be
>>>>> the other way round - if kbd-backlight means that this is
>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>> not generic at all.
>>>>
>>>> The trigger as implemented is generic, if you think
>>>> otherwise, please let me know which part is not generic
>>>> according to you.
>>>
>>> I think I was too meticulous here. In the end of the previous
>>> message I mentioned that we cannot guarantee that all keyboard
>>> backlight controllers can adjust brightness autonomously.
>>> Nonetheless, for the ones that cannot do that it would make no sense
>>> to have a trigger. In view of that this trigger is generic enough.
>>>
>>> I'll wait for Pavel's opinion before merging the patch set
>>> as he was also involved in this whole thread.
>
> If we have a keyboard backlight that may be changed automatically, I'd
> go for trigger. If we know for sure that hardware will not change
> brightnes "on its own", I'd not put a trigger there (and polling makes
> no sense). If we don't know... I guess I'd go for trigger.
>
> We can do various white/blacklists if we really want to..
>
>> As pointed in other email, we do not know if HW really controls keyboard backlight,
>> so adding "fake" trigger on machines without HW control is not a good idea.
>
> Well, if we know that hardware will not change the brightness on its
> own, yes, I'd avoid the trigger. If we don't know (as is common on
> ACPI machines, I'd keep the trigger).

Right, this is taken care of by the default_trigger field of the
led_classdev, we only set the "kbd-backlight" default_trigger on
(kbd-backlight) LED devices where we know we have a hotkey directly
controlling the brightness.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

[-- Attachment #1.1: Type: Text/Plain, Size: 3932 bytes --]

On Sunday 20 November 2016 19:48:02 Hans de Goede wrote:
> HI,
> 
> On 20-11-16 17:21, Pavel Machek wrote:
> > Hi!
> > 
> >>>>>>> Thanks for the patch.
> >>>>>>> 
> >>>>>>> I think we need less generic trigger name.
> >>>>>>> With present name we pretend that all kbd-backlight
> >>>>>>> controllers can change LED brightness autonomously.
> >>>>>>> 
> >>>>>>> How about kbd-backlight-pollable ?
> >>>>>> 
> >>>>>> This is a trigger to control kbd-backlights, in the
> >>>>>> current use-case the brightness change is already done
> >>>>>> by the firmware, hence the set_brightness argument to
> >>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
> >>>>>> it again.
> >>>>>> 
> >>>>>> But I can see future cases where we do want to have some
> >>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
> >>>>>> does not do the adjustment automatically) which does
> >>>>>> lead to actually updating the brightness.
> >>>>>> 
> >>>>>> So I decided to go with a generic kbd-backlight trigger,
> >>>>>> which in the future can also be used to directly control
> >>>>>> kbd-backlight brightness; and not just to make ot
> >>>>>> poll-able.
> >>>>> 
> >>>>> I thought that kbd-backlight stands for "keyboard backlight",
> >>>> 
> >>>> It does.
> >>>> 
> >>>>> that's why I assessed it is too generic.
> >>>> 
> >>>> The whole purpose of the trigger as implemented is to be
> >>>> generic, as it seems senseless to implement a one off
> >>>> trigger for just the dell / thinkpad case.
> >>>> 
> >>>>> It seems however to be
> >>>>> the other way round - if kbd-backlight means that this is
> >>>>> a trigger only for use with dell-laptop keyboard driver
> >>>>> (I see kbd namespacing prefix in the driver functions) than it
> >>>>> is not generic at all.
> >>>> 
> >>>> The trigger as implemented is generic, if you think
> >>>> otherwise, please let me know which part is not generic
> >>>> according to you.
> >>> 
> >>> I think I was too meticulous here. In the end of the previous
> >>> message I mentioned that we cannot guarantee that all keyboard
> >>> backlight controllers can adjust brightness autonomously.
> >>> Nonetheless, for the ones that cannot do that it would make no
> >>> sense to have a trigger. In view of that this trigger is generic
> >>> enough.
> >>> 
> >>> I'll wait for Pavel's opinion before merging the patch set
> >>> as he was also involved in this whole thread.
> > 
> > If we have a keyboard backlight that may be changed automatically,
> > I'd go for trigger. If we know for sure that hardware will not
> > change brightnes "on its own", I'd not put a trigger there (and
> > polling makes no sense). If we don't know... I guess I'd go for
> > trigger.
> > 
> > We can do various white/blacklists if we really want to..
> > 
> >> As pointed in other email, we do not know if HW really controls
> >> keyboard backlight, so adding "fake" trigger on machines without
> >> HW control is not a good idea.
> > 
> > Well, if we know that hardware will not change the brightness on
> > its own, yes, I'd avoid the trigger. If we don't know (as is
> > common on ACPI machines, I'd keep the trigger).
> 
> Right, this is taken care of by the default_trigger field of the
> led_classdev, we only set the "kbd-backlight" default_trigger on
> (kbd-backlight) LED devices where we know we have a hotkey directly
> controlling the brightness.
> 
> Regards,
> 
> Hans

Hm... now I'm thinking... should not be there a generic readonly led 
trigger named "managed by hw" (or similar name)? Only LED drivers (not 
userspace) could set it and when set it tells that hw could on its own 
change level of LED. Not keyboard backlight specific, but generic for 
led subsystem.

It not this what is needed for keyboard backlight LED drivers?

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/20/2016 05:21 PM, Pavel Machek wrote:
> Hi!
>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> I think we need less generic trigger name.
>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>> can change LED brightness autonomously.
>>>>>>>
>>>>>>> How about kbd-backlight-pollable ?
>>>>>>
>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>> current use-case the brightness change is already done
>>>>>> by the firmware, hence the set_brightness argument to
>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>> it again.
>>>>>>
>>>>>> But I can see future cases where we do want to have some
>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>> does not do the adjustment automatically) which does
>>>>>> lead to actually updating the brightness.
>>>>>>
>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>> which in the future can also be used to directly control
>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>> poll-able.
>>>>>
>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>
>>>> It does.
>>>>
>>>>> that's why I assessed it is too generic.
>>>>
>>>> The whole purpose of the trigger as implemented is to be
>>>> generic, as it seems senseless to implement a one off
>>>> trigger for just the dell / thinkpad case.
>>>>
>>>>> It seems however to be
>>>>> the other way round - if kbd-backlight means that this is
>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>> not generic at all.
>>>>
>>>> The trigger as implemented is generic, if you think
>>>> otherwise, please let me know which part is not generic
>>>> according to you.
>>>
>>> I think I was too meticulous here. In the end of the previous
>>> message I mentioned that we cannot guarantee that all keyboard
>>> backlight controllers can adjust brightness autonomously.
>>> Nonetheless, for the ones that cannot do that it would make no sense
>>> to have a trigger. In view of that this trigger is generic enough.
>>>
>>> I'll wait for Pavel's opinion before merging the patch set
>>> as he was also involved in this whole thread.
>
> If we have a keyboard backlight that may be changed automatically, I'd
> go for trigger. If we know for sure that hardware will not change
> brightnes "on its own", I'd not put a trigger there (and polling makes
> no sense). If we don't know... I guess I'd go for trigger.
>
> We can do various white/blacklists if we really want to..
>
>> As pointed in other email, we do not know if HW really controls keyboard backlight,
>> so adding "fake" trigger on machines without HW control is not a good idea.
>
> Well, if we know that hardware will not change the brightness on its
> own, yes, I'd avoid the trigger. If we don't know (as is common on
> ACPI machines, I'd keep the trigger).

I'd drop the trigger approach due to the mess it can make in peoples'
minds due to the fact that LED class device handles trigger events
generated by itself.

I have an impression that we're trying to abuse trigger mechanism,
e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(),
which actually prevents setting brightness, and its only task is
to generate brightness change notification.

I'd add a file hw_brightness_change or async_brightness or something
similar and make it only readable/pollable. current_brightness is
ambiguous and questionable.

This is quite specific hardware feature so it needs specific handling
and a separate sysfs file. We could add polling on brightness and
apply some event filtering as proposed by Pali, by it could result
in losing crucial brightness changes in some use cases.

Therefore a separate file for this specific feature is needed.
There still remains objection raised by Hans related to polling
a sysfs file to detect changes on the other sysfs file, but in either
case we need to make some workaround, be it circular trigger or
inconsistent polling. The advantage of the latter is that it explicitly
advertises additional LED class device feature.

The file and corresponding op should be compiled only when turned on in
kernel config, and LED class devices which need that feature should
select in the kernel config.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 21-11-16 11:24, Jacek Anaszewski wrote:
> Hi,
>
> On 11/20/2016 05:21 PM, Pavel Machek wrote:
>> Hi!
>>
>>>>>>>> Thanks for the patch.
>>>>>>>>
>>>>>>>> I think we need less generic trigger name.
>>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>>> can change LED brightness autonomously.
>>>>>>>>
>>>>>>>> How about kbd-backlight-pollable ?
>>>>>>>
>>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>>> current use-case the brightness change is already done
>>>>>>> by the firmware, hence the set_brightness argument to
>>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>>> it again.
>>>>>>>
>>>>>>> But I can see future cases where we do want to have some
>>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>>> does not do the adjustment automatically) which does
>>>>>>> lead to actually updating the brightness.
>>>>>>>
>>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>>> which in the future can also be used to directly control
>>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>>> poll-able.
>>>>>>
>>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>>
>>>>> It does.
>>>>>
>>>>>> that's why I assessed it is too generic.
>>>>>
>>>>> The whole purpose of the trigger as implemented is to be
>>>>> generic, as it seems senseless to implement a one off
>>>>> trigger for just the dell / thinkpad case.
>>>>>
>>>>>> It seems however to be
>>>>>> the other way round - if kbd-backlight means that this is
>>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>>> not generic at all.
>>>>>
>>>>> The trigger as implemented is generic, if you think
>>>>> otherwise, please let me know which part is not generic
>>>>> according to you.
>>>>
>>>> I think I was too meticulous here. In the end of the previous
>>>> message I mentioned that we cannot guarantee that all keyboard
>>>> backlight controllers can adjust brightness autonomously.
>>>> Nonetheless, for the ones that cannot do that it would make no sense
>>>> to have a trigger. In view of that this trigger is generic enough.
>>>>
>>>> I'll wait for Pavel's opinion before merging the patch set
>>>> as he was also involved in this whole thread.
>>
>> If we have a keyboard backlight that may be changed automatically, I'd
>> go for trigger. If we know for sure that hardware will not change
>> brightnes "on its own", I'd not put a trigger there (and polling makes
>> no sense). If we don't know... I guess I'd go for trigger.
>>
>> We can do various white/blacklists if we really want to..
>>
>>> As pointed in other email, we do not know if HW really controls keyboard backlight,
>>> so adding "fake" trigger on machines without HW control is not a good idea.
>>
>> Well, if we know that hardware will not change the brightness on its
>> own, yes, I'd avoid the trigger. If we don't know (as is common on
>> ACPI machines, I'd keep the trigger).
>
> I'd drop the trigger approach due to the mess it can make in peoples'
> minds due to the fact that LED class device handles trigger events
> generated by itself.

That is actually not true. I believe that Pavel Machek was entirely
right that we should model this as a trigger. Take e.g. Dell laptops,
there are 2 different drivers there on for the Dell smbios ACPI
interface, which is the one registering the led_classdev to query /
control the kbd backlight and then there is a completely separate
driver for receiving WMI events, the dell-wmi driver (both can be
build and insmod-ed completely separate from one another), which
actually gets events that the brightness was changed by the hotkey.

Before the trigger approach we had a custom dell_laptop notifier
chain in a dell-common module to propagate events from one to
the other, with the trigger approach this hack is all gone.

> I have an impression that we're trying to abuse trigger mechanism,
> e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(),
> which actually prevents setting brightness, and its only task is
> to generate brightness change notification.

In the dell / thinkpad case yes, but maybe we will get another
laptop where we do actually need to actually update the brightness
ourselves on some event, in that case we will also want to notify
userspace.

> I'd add a file hw_brightness_change or async_brightness or something
> similar and make it only readable/pollable. current_brightness is
> ambiguous and questionable.
>
> This is quite specific hardware feature so it needs specific handling
> and a separate sysfs file. We could add polling on brightness and
> apply some event filtering as proposed by Pali, by it could result
> in losing crucial brightness changes in some use cases.
>
> Therefore a separate file for this specific feature is needed.
> There still remains objection raised by Hans related to polling
> a sysfs file to detect changes on the other sysfs file, but in either
> case we need to make some workaround, be it circular trigger or
> inconsistent polling. The advantage of the latter is that it explicitly
> advertises additional LED class device feature.
>
> The file and corresponding op should be compiled only when turned on in
> kernel config, and LED class devices which need that feature should
> select in the kernel config.

I do not understand why you're changing your mind on this.

The current_brightness approach with a trigger works well and is
generic, other triggers (which don't fire too often) can easily
also add current_brightness to allow userspace to monitor for changes,
which is why I added shared current_brightness code to led-triggers.c,
so that this code can be re-used.

Pavel Machek is completely right that this should be modeled as
a trigger and I think that doing some one-off special sysfs
file called hw_brightness_change for this case is bad design when
we've a better generic solution.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/21/2016 11:42 AM, Hans de Goede wrote:
> Hi,
>
> On 21-11-16 11:24, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 11/20/2016 05:21 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>>> Thanks for the patch.
>>>>>>>>>
>>>>>>>>> I think we need less generic trigger name.
>>>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>>>> can change LED brightness autonomously.
>>>>>>>>>
>>>>>>>>> How about kbd-backlight-pollable ?
>>>>>>>>
>>>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>>>> current use-case the brightness change is already done
>>>>>>>> by the firmware, hence the set_brightness argument to
>>>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>>>> it again.
>>>>>>>>
>>>>>>>> But I can see future cases where we do want to have some
>>>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>>>> does not do the adjustment automatically) which does
>>>>>>>> lead to actually updating the brightness.
>>>>>>>>
>>>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>>>> which in the future can also be used to directly control
>>>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>>>> poll-able.
>>>>>>>
>>>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>>>
>>>>>> It does.
>>>>>>
>>>>>>> that's why I assessed it is too generic.
>>>>>>
>>>>>> The whole purpose of the trigger as implemented is to be
>>>>>> generic, as it seems senseless to implement a one off
>>>>>> trigger for just the dell / thinkpad case.
>>>>>>
>>>>>>> It seems however to be
>>>>>>> the other way round - if kbd-backlight means that this is
>>>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>>>> not generic at all.
>>>>>>
>>>>>> The trigger as implemented is generic, if you think
>>>>>> otherwise, please let me know which part is not generic
>>>>>> according to you.
>>>>>
>>>>> I think I was too meticulous here. In the end of the previous
>>>>> message I mentioned that we cannot guarantee that all keyboard
>>>>> backlight controllers can adjust brightness autonomously.
>>>>> Nonetheless, for the ones that cannot do that it would make no sense
>>>>> to have a trigger. In view of that this trigger is generic enough.
>>>>>
>>>>> I'll wait for Pavel's opinion before merging the patch set
>>>>> as he was also involved in this whole thread.
>>>
>>> If we have a keyboard backlight that may be changed automatically, I'd
>>> go for trigger. If we know for sure that hardware will not change
>>> brightnes "on its own", I'd not put a trigger there (and polling makes
>>> no sense). If we don't know... I guess I'd go for trigger.
>>>
>>> We can do various white/blacklists if we really want to..
>>>
>>>> As pointed in other email, we do not know if HW really controls
>>>> keyboard backlight,
>>>> so adding "fake" trigger on machines without HW control is not a
>>>> good idea.
>>>
>>> Well, if we know that hardware will not change the brightness on its
>>> own, yes, I'd avoid the trigger. If we don't know (as is common on
>>> ACPI machines, I'd keep the trigger).
>>
>> I'd drop the trigger approach due to the mess it can make in peoples'
>> minds due to the fact that LED class device handles trigger events
>> generated by itself.
>
> That is actually not true. I believe that Pavel Machek was entirely
> right that we should model this as a trigger. Take e.g. Dell laptops,
> there are 2 different drivers there on for the Dell smbios ACPI
> interface, which is the one registering the led_classdev to query /
> control the kbd backlight and then there is a completely separate
> driver for receiving WMI events, the dell-wmi driver (both can be
> build and insmod-ed completely separate from one another), which
> actually gets events that the brightness was changed by the hotkey.
>
> Before the trigger approach we had a custom dell_laptop notifier
> chain in a dell-common module to propagate events from one to
> the other, with the trigger approach this hack is all gone.

Well, in this particular case it turned out to be beneficial.

>> I have an impression that we're trying to abuse trigger mechanism,
>> e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(),
>> which actually prevents setting brightness, and its only task is
>> to generate brightness change notification.
>
> In the dell / thinkpad case yes, but maybe we will get another
> laptop where we do actually need to actually update the brightness
> ourselves on some event, in that case we will also want to notify
> userspace.
>
>> I'd add a file hw_brightness_change or async_brightness or something
>> similar and make it only readable/pollable. current_brightness is
>> ambiguous and questionable.
>>
>> This is quite specific hardware feature so it needs specific handling
>> and a separate sysfs file. We could add polling on brightness and
>> apply some event filtering as proposed by Pali, by it could result
>> in losing crucial brightness changes in some use cases.
>>
>> Therefore a separate file for this specific feature is needed.
>> There still remains objection raised by Hans related to polling
>> a sysfs file to detect changes on the other sysfs file, but in either
>> case we need to make some workaround, be it circular trigger or
>> inconsistent polling. The advantage of the latter is that it explicitly
>> advertises additional LED class device feature.
>>
>> The file and corresponding op should be compiled only when turned on in
>> kernel config, and LED class devices which need that feature should
>> select in the kernel config.
>
> I do not understand why you're changing your mind on this.
>
> The current_brightness approach with a trigger works well and is
> generic, other triggers (which don't fire too often) can easily
> also add current_brightness to allow userspace to monitor for changes,

LED class drivers could also easily add a hw_brightness_change attr.

> which is why I added shared current_brightness code to led-triggers.c,
> so that this code can be re-used.

I don't like the name current_brightness. It is ambiguous in the
relation to existing brightness file.

> Pavel Machek is completely right that this should be modeled as
> a trigger and I think that doing some one-off special sysfs
> file called hw_brightness_change for this case is bad design when
> we've a better generic solution.

Let's sum up pros and cons of both approaches:

hw_brightness_change/async_brightness:

Pros:
- explicit declaration of an additional LED class device feature
   in the sysfs
- ability to receive hw brightness change notifications even
   when having other trigger active
Cons:
- polling one file to detect changes on the other (not entirely
   true as the polled file will also report updated brightness)

kbd-backlight trigger:

Pros:
- simplification of specific driver design
Cons:
- lack of information if given LED class device supports POLLPRI events
- impossible to apply other trigger while polling
- circular trigger event path


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 21-11-16 12:24, Jacek Anaszewski wrote:
> On 11/21/2016 11:42 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 21-11-16 11:24, Jacek Anaszewski wrote:
>>> Hi,
>>>
>>> On 11/20/2016 05:21 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>>>>>> Thanks for the patch.
>>>>>>>>>>
>>>>>>>>>> I think we need less generic trigger name.
>>>>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>>>>> can change LED brightness autonomously.
>>>>>>>>>>
>>>>>>>>>> How about kbd-backlight-pollable ?
>>>>>>>>>
>>>>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>>>>> current use-case the brightness change is already done
>>>>>>>>> by the firmware, hence the set_brightness argument to
>>>>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>>>>> it again.
>>>>>>>>>
>>>>>>>>> But I can see future cases where we do want to have some
>>>>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>>>>> does not do the adjustment automatically) which does
>>>>>>>>> lead to actually updating the brightness.
>>>>>>>>>
>>>>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>>>>> which in the future can also be used to directly control
>>>>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>>>>> poll-able.
>>>>>>>>
>>>>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>>>>
>>>>>>> It does.
>>>>>>>
>>>>>>>> that's why I assessed it is too generic.
>>>>>>>
>>>>>>> The whole purpose of the trigger as implemented is to be
>>>>>>> generic, as it seems senseless to implement a one off
>>>>>>> trigger for just the dell / thinkpad case.
>>>>>>>
>>>>>>>> It seems however to be
>>>>>>>> the other way round - if kbd-backlight means that this is
>>>>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>>>>> not generic at all.
>>>>>>>
>>>>>>> The trigger as implemented is generic, if you think
>>>>>>> otherwise, please let me know which part is not generic
>>>>>>> according to you.
>>>>>>
>>>>>> I think I was too meticulous here. In the end of the previous
>>>>>> message I mentioned that we cannot guarantee that all keyboard
>>>>>> backlight controllers can adjust brightness autonomously.
>>>>>> Nonetheless, for the ones that cannot do that it would make no sense
>>>>>> to have a trigger. In view of that this trigger is generic enough.
>>>>>>
>>>>>> I'll wait for Pavel's opinion before merging the patch set
>>>>>> as he was also involved in this whole thread.
>>>>
>>>> If we have a keyboard backlight that may be changed automatically, I'd
>>>> go for trigger. If we know for sure that hardware will not change
>>>> brightnes "on its own", I'd not put a trigger there (and polling makes
>>>> no sense). If we don't know... I guess I'd go for trigger.
>>>>
>>>> We can do various white/blacklists if we really want to..
>>>>
>>>>> As pointed in other email, we do not know if HW really controls
>>>>> keyboard backlight,
>>>>> so adding "fake" trigger on machines without HW control is not a
>>>>> good idea.
>>>>
>>>> Well, if we know that hardware will not change the brightness on its
>>>> own, yes, I'd avoid the trigger. If we don't know (as is common on
>>>> ACPI machines, I'd keep the trigger).
>>>
>>> I'd drop the trigger approach due to the mess it can make in peoples'
>>> minds due to the fact that LED class device handles trigger events
>>> generated by itself.
>>
>> That is actually not true. I believe that Pavel Machek was entirely
>> right that we should model this as a trigger. Take e.g. Dell laptops,
>> there are 2 different drivers there on for the Dell smbios ACPI
>> interface, which is the one registering the led_classdev to query /
>> control the kbd backlight and then there is a completely separate
>> driver for receiving WMI events, the dell-wmi driver (both can be
>> build and insmod-ed completely separate from one another), which
>> actually gets events that the brightness was changed by the hotkey.
>>
>> Before the trigger approach we had a custom dell_laptop notifier
>> chain in a dell-common module to propagate events from one to
>> the other, with the trigger approach this hack is all gone.
>
> Well, in this particular case it turned out to be beneficial.
>
>>> I have an impression that we're trying to abuse trigger mechanism,
>>> e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(),
>>> which actually prevents setting brightness, and its only task is
>>> to generate brightness change notification.
>>
>> In the dell / thinkpad case yes, but maybe we will get another
>> laptop where we do actually need to actually update the brightness
>> ourselves on some event, in that case we will also want to notify
>> userspace.
>>
>>> I'd add a file hw_brightness_change or async_brightness or something
>>> similar and make it only readable/pollable. current_brightness is
>>> ambiguous and questionable.
>>>
>>> This is quite specific hardware feature so it needs specific handling
>>> and a separate sysfs file. We could add polling on brightness and
>>> apply some event filtering as proposed by Pali, by it could result
>>> in losing crucial brightness changes in some use cases.
>>>
>>> Therefore a separate file for this specific feature is needed.
>>> There still remains objection raised by Hans related to polling
>>> a sysfs file to detect changes on the other sysfs file, but in either
>>> case we need to make some workaround, be it circular trigger or
>>> inconsistent polling. The advantage of the latter is that it explicitly
>>> advertises additional LED class device feature.
>>>
>>> The file and corresponding op should be compiled only when turned on in
>>> kernel config, and LED class devices which need that feature should
>>> select in the kernel config.
>>
>> I do not understand why you're changing your mind on this.
>>
>> The current_brightness approach with a trigger works well and is
>> generic, other triggers (which don't fire too often) can easily
>> also add current_brightness to allow userspace to monitor for changes,
>
> LED class drivers could also easily add a hw_brightness_change attr.
>
>> which is why I added shared current_brightness code to led-triggers.c,
>> so that this code can be re-used.
>
> I don't like the name current_brightness. It is ambiguous in the
> relation to existing brightness file.
>
>> Pavel Machek is completely right that this should be modeled as
>> a trigger and I think that doing some one-off special sysfs
>> file called hw_brightness_change for this case is bad design when
>> we've a better generic solution.
>
> Let's sum up pros and cons of both approaches:
>
> hw_brightness_change/async_brightness:
>
> Pros:
> - explicit declaration of an additional LED class device feature
>   in the sysfs
> - ability to receive hw brightness change notifications even
>   when having other trigger active
> Cons:
> - polling one file to detect changes on the other (not entirely
>   true as the polled file will also report updated brightness)
>
> kbd-backlight trigger:
>
> Pros:
> - simplification of specific driver design
> Cons:
> - lack of information if given LED class device supports POLLPRI events
> - impossible to apply other trigger while polling

That is not true, current_brightness presence implies POLL_PRI support.

> - circular trigger event path

That is not true either.

Anyways I'm fine with using a new (optional) hw_brightness_change sysfs
file which is poll-able, but, BUT can you please make a decision and then
stick with it ? All this lets do it this way, I spend time writing and
testing a patch and then you changing your mind is becoming very tiresome.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/21/2016 12:56 PM, Hans de Goede wrote:
> Hi,
>
> On 21-11-16 12:24, Jacek Anaszewski wrote:
>> On 11/21/2016 11:42 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 21-11-16 11:24, Jacek Anaszewski wrote:
>>>> Hi,
>>>>
>>>> On 11/20/2016 05:21 PM, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>>>>>>> Thanks for the patch.
>>>>>>>>>>>
>>>>>>>>>>> I think we need less generic trigger name.
>>>>>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>>>>>> can change LED brightness autonomously.
>>>>>>>>>>>
>>>>>>>>>>> How about kbd-backlight-pollable ?
>>>>>>>>>>
>>>>>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>>>>>> current use-case the brightness change is already done
>>>>>>>>>> by the firmware, hence the set_brightness argument to
>>>>>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>>>>>> it again.
>>>>>>>>>>
>>>>>>>>>> But I can see future cases where we do want to have some
>>>>>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>>>>>> does not do the adjustment automatically) which does
>>>>>>>>>> lead to actually updating the brightness.
>>>>>>>>>>
>>>>>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>>>>>> which in the future can also be used to directly control
>>>>>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>>>>>> poll-able.
>>>>>>>>>
>>>>>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>>>>>
>>>>>>>> It does.
>>>>>>>>
>>>>>>>>> that's why I assessed it is too generic.
>>>>>>>>
>>>>>>>> The whole purpose of the trigger as implemented is to be
>>>>>>>> generic, as it seems senseless to implement a one off
>>>>>>>> trigger for just the dell / thinkpad case.
>>>>>>>>
>>>>>>>>> It seems however to be
>>>>>>>>> the other way round - if kbd-backlight means that this is
>>>>>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>>>>>> not generic at all.
>>>>>>>>
>>>>>>>> The trigger as implemented is generic, if you think
>>>>>>>> otherwise, please let me know which part is not generic
>>>>>>>> according to you.
>>>>>>>
>>>>>>> I think I was too meticulous here. In the end of the previous
>>>>>>> message I mentioned that we cannot guarantee that all keyboard
>>>>>>> backlight controllers can adjust brightness autonomously.
>>>>>>> Nonetheless, for the ones that cannot do that it would make no sense
>>>>>>> to have a trigger. In view of that this trigger is generic enough.
>>>>>>>
>>>>>>> I'll wait for Pavel's opinion before merging the patch set
>>>>>>> as he was also involved in this whole thread.
>>>>>
>>>>> If we have a keyboard backlight that may be changed automatically, I'd
>>>>> go for trigger. If we know for sure that hardware will not change
>>>>> brightnes "on its own", I'd not put a trigger there (and polling makes
>>>>> no sense). If we don't know... I guess I'd go for trigger.
>>>>>
>>>>> We can do various white/blacklists if we really want to..
>>>>>
>>>>>> As pointed in other email, we do not know if HW really controls
>>>>>> keyboard backlight,
>>>>>> so adding "fake" trigger on machines without HW control is not a
>>>>>> good idea.
>>>>>
>>>>> Well, if we know that hardware will not change the brightness on its
>>>>> own, yes, I'd avoid the trigger. If we don't know (as is common on
>>>>> ACPI machines, I'd keep the trigger).
>>>>
>>>> I'd drop the trigger approach due to the mess it can make in peoples'
>>>> minds due to the fact that LED class device handles trigger events
>>>> generated by itself.
>>>
>>> That is actually not true. I believe that Pavel Machek was entirely
>>> right that we should model this as a trigger. Take e.g. Dell laptops,
>>> there are 2 different drivers there on for the Dell smbios ACPI
>>> interface, which is the one registering the led_classdev to query /
>>> control the kbd backlight and then there is a completely separate
>>> driver for receiving WMI events, the dell-wmi driver (both can be
>>> build and insmod-ed completely separate from one another), which
>>> actually gets events that the brightness was changed by the hotkey.
>>>
>>> Before the trigger approach we had a custom dell_laptop notifier
>>> chain in a dell-common module to propagate events from one to
>>> the other, with the trigger approach this hack is all gone.
>>
>> Well, in this particular case it turned out to be beneficial.
>>
>>>> I have an impression that we're trying to abuse trigger mechanism,
>>>> e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(),
>>>> which actually prevents setting brightness, and its only task is
>>>> to generate brightness change notification.
>>>
>>> In the dell / thinkpad case yes, but maybe we will get another
>>> laptop where we do actually need to actually update the brightness
>>> ourselves on some event, in that case we will also want to notify
>>> userspace.
>>>
>>>> I'd add a file hw_brightness_change or async_brightness or something
>>>> similar and make it only readable/pollable. current_brightness is
>>>> ambiguous and questionable.
>>>>
>>>> This is quite specific hardware feature so it needs specific handling
>>>> and a separate sysfs file. We could add polling on brightness and
>>>> apply some event filtering as proposed by Pali, by it could result
>>>> in losing crucial brightness changes in some use cases.
>>>>
>>>> Therefore a separate file for this specific feature is needed.
>>>> There still remains objection raised by Hans related to polling
>>>> a sysfs file to detect changes on the other sysfs file, but in either
>>>> case we need to make some workaround, be it circular trigger or
>>>> inconsistent polling. The advantage of the latter is that it explicitly
>>>> advertises additional LED class device feature.
>>>>
>>>> The file and corresponding op should be compiled only when turned on in
>>>> kernel config, and LED class devices which need that feature should
>>>> select in the kernel config.
>>>
>>> I do not understand why you're changing your mind on this.
>>>
>>> The current_brightness approach with a trigger works well and is
>>> generic, other triggers (which don't fire too often) can easily
>>> also add current_brightness to allow userspace to monitor for changes,
>>
>> LED class drivers could also easily add a hw_brightness_change attr.
>>
>>> which is why I added shared current_brightness code to led-triggers.c,
>>> so that this code can be re-used.
>>
>> I don't like the name current_brightness. It is ambiguous in the
>> relation to existing brightness file.
>>
>>> Pavel Machek is completely right that this should be modeled as
>>> a trigger and I think that doing some one-off special sysfs
>>> file called hw_brightness_change for this case is bad design when
>>> we've a better generic solution.
>>
>> Let's sum up pros and cons of both approaches:
>>
>> hw_brightness_change/async_brightness:
>>
>> Pros:
>> - explicit declaration of an additional LED class device feature
>>   in the sysfs
>> - ability to receive hw brightness change notifications even
>>   when having other trigger active
>> Cons:
>> - polling one file to detect changes on the other (not entirely
>>   true as the polled file will also report updated brightness)
>>
>> kbd-backlight trigger:
>>
>> Pros:
>> - simplification of specific driver design
>> Cons:
>> - lack of information if given LED class device supports POLLPRI events
>> - impossible to apply other trigger while polling
>
> That is not true, current_brightness presence implies POLL_PRI support.

Right, it should be:
- lack of information if given LED class device can generate hardware
   triggered POLLPRI events

>
>> - circular trigger event path
>
> That is not true either.

Not true only if set_brightness argument is false.

> Anyways I'm fine with using a new (optional) hw_brightness_change sysfs
> file which is poll-able, but, BUT can you please make a decision and then
> stick with it ? All this lets do it this way, I spend time writing and
> testing a patch and then you changing your mind is becoming very tiresome.

Kernel development relies on lazy consensus, so it is always possible
that someone will add something to the discussion that changes the way
how the newly proposed feature is being assessed. Until it is merged
to mainline it is always possible that someone will point out some
crucial problem. Once an API is added to kernel it is carved in stone
so we have to be very careful, as the case of existing brightness file
shows.

Let's wait until every involved part agrees (Pavel, Pali).
I'm open to hear strong arguments against my approach.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

On Monday 21 November 2016 14:29:00 Jacek Anaszewski wrote:
> Let's wait until every involved part agrees (Pavel, Pali).

Ok, I read that discussion on linux-leds ML and finally understand
motivation and results.

Personally I still do not like current approach and big problem what I
see is that I was not able to understand *why* introduction of
current_brightness is needed and how userspace application should use
it, before I read whole that linux-leds discussion.

For people who already understand situation it is probably OK, but when
I first time saw this patch series I just said WTF and description in
Documentation files nor in commit messages did not help me.

I would suggest to properly document *current* behaviour of LED sysfs
files in Documentation/ABI before doing any decision how to solve
current problem.

Without correct documentation how sysfs LED interface behave in
different situations (trigger is active; zero is written to brightness;
brightness is read when blinking is active; etc) it is really hard to
discuss about those problems. As many people (me included) first looked
at those documentation files and think that info written here is correct
and handle everything...

Adding trigger for LED devices which belongs to keyboard backlight has
only disadvantage: You cannot set other kernel trigger to control level
of keyboard backlight.

I can imagine that trigger "key pressed" or "mouse moved" can be really
useful for controlling keyboard backlight level. But that new keyboard
backlight trigger just disallow it.

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

Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Glenn Golden]

Pali Rohár <pali.rohar@gmail.com> [2016-11-22 15:58:25 +0100]:
> On Monday 21 November 2016 14:29:00 Jacek Anaszewski wrote:
> > Let's wait until every involved part agrees (Pavel, Pali).
> 
> Ok, I read that discussion on linux-leds ML and finally understand
> motivation and results.
> 
> Personally I still do not like current approach and big problem what I
> see is that I was not able to understand *why* introduction of
> current_brightness is needed and how userspace application should use
> it, before I read whole that linux-leds discussion.
> 
> For people who already understand situation it is probably OK, but when
> I first time saw this patch series I just said WTF and description in
> Documentation files nor in commit messages did not help me.
> 
> I would suggest to properly document *current* behaviour of LED sysfs
> files in Documentation/ABI before doing any decision how to solve
> current problem.
> 
> Without correct documentation how sysfs LED interface behave in
> different situations (trigger is active; zero is written to brightness;
> brightness is read when blinking is active; etc) it is really hard to
> discuss about those problems. As many people (me included) first looked
> at those documentation files and think that info written here is correct
> and handle everything...
> 
 
+1 on the above sentiment, sense of frustration well expressed.

I went thru the same sequence of wtf-ing as Pali describes before coming
away with any level of real understanding.  IMO, this is a great example
of an issue which is 90% doc vs. 10% technical.

Hearing statements made earlier such as "the variable name doesn't matter
much" (approximate paraphrase) makes me worry, and must respectfully
disagree.  I am not involved at all in development or patches on this,
just a lowly Joe User type, and as such, can tell you that out here in
the cheap seats, the naming really does matter.

Please respect views of Pali as stated above and concentrate on documentation.

As an aside from complaining though: Major tip o' the hat to all the
motivated and devoted devs on this ACPI work throughout many years;
it's really very much appreciated, despite the occasional whining (as
mine above).

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi Pali, Glenn,

Thanks for your feedback.

On 11/22/2016 03:58 PM, Pali Rohár wrote:
> On Monday 21 November 2016 14:29:00 Jacek Anaszewski wrote:
>> Let's wait until every involved part agrees (Pavel, Pali).
>
> Ok, I read that discussion on linux-leds ML and finally understand
> motivation and results.
>
> Personally I still do not like current approach and big problem what I
> see is that I was not able to understand *why* introduction of
> current_brightness is needed and how userspace application should use
> it, before I read whole that linux-leds discussion.
>
> For people who already understand situation it is probably OK, but when
> I first time saw this patch series I just said WTF and description in
> Documentation files nor in commit messages did not help me.
>
> I would suggest to properly document *current* behaviour of LED sysfs
> files in Documentation/ABI before doing any decision how to solve
> current problem.
>
> Without correct documentation how sysfs LED interface behave in
> different situations (trigger is active; zero is written to brightness;
> brightness is read when blinking is active; etc) it is really hard to
> discuss about those problems. As many people (me included) first looked
> at those documentation files and think that info written here is correct
> and handle everything...
>
> Adding trigger for LED devices which belongs to keyboard backlight has
> only disadvantage: You cannot set other kernel trigger to control level
> of keyboard backlight.
>
> I can imagine that trigger "key pressed" or "mouse moved" can be really
> useful for controlling keyboard backlight level. But that new keyboard
> backlight trigger just disallow it.
>

On 11/22/2016 04:20 PM, Glenn Golden wrote:
 > +1 on the above sentiment, sense of frustration well expressed.

So we have more voices against the trigger approach.

I would also appreciate your opinion on the other solution to the
problem of notifying brightness changes originating from hardware,
i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
and reading brightness.

Do you find the file name informative enough or maybe you'd like to
propose other name, or even entirely different approach?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
> I would also appreciate your opinion on the other solution to the
> problem of notifying brightness changes originating from hardware,
> i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
> and reading brightness.

Another idea:

If no trigger is active then led subsystem will invoke POLLPRI on
"brightness" sysfs file.

And if there is active trigger then only trigger code could invoke
POLLPRI on "brightness" file.

This could solve problem with high CPU load and power usage when e.g.
cpu trigger is active (and cpu trigger will not implement any POLLPRI).

Do not know if this is really enough for your situation, it is just and
another idea.

But first please update documentation in ABI/testing to match current
situation. That is really needed.

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

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi Pali,

On 24-11-16 10:15, Pali Rohár wrote:
> On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
>> I would also appreciate your opinion on the other solution to the
>> problem of notifying brightness changes originating from hardware,
>> i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
>> and reading brightness.
>
> Another idea:
>
> If no trigger is active then led subsystem will invoke POLLPRI on
> "brightness" sysfs file.
>
> And if there is active trigger then only trigger code could invoke
> POLLPRI on "brightness" file.
>
> This could solve problem with high CPU load and power usage when e.g.
> cpu trigger is active (and cpu trigger will not implement any POLLPRI).
>
> Do not know if this is really enough for your situation, it is just and
> another idea.
>
> But first please update documentation in ABI/testing to match current
> situation. That is really needed.

Patches are welcome ...     :)

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/24/2016 10:15 AM, Pali Rohár wrote:
> On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
>> I would also appreciate your opinion on the other solution to the
>> problem of notifying brightness changes originating from hardware,
>> i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
>> and reading brightness.
>
> Another idea:
>
> If no trigger is active then led subsystem will invoke POLLPRI on
> "brightness" sysfs file.
>
> And if there is active trigger then only trigger code could invoke
> POLLPRI on "brightness" file.
>
> This could solve problem with high CPU load and power usage when e.g.
> cpu trigger is active (and cpu trigger will not implement any POLLPRI).
>
> Do not know if this is really enough for your situation, it is just and
> another idea.

This way we would be losing POLLPRI events when trigger is active,
whereas it would be useful to have ones in some use cases.

> But first please update documentation in ABI/testing to match current
> situation. That is really needed.
>

I suppose that you're thinking about behaviour on brightness file
reading? Is there anything else you'd like to have clarified in the doc?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

On Thursday 24 November 2016 15:21:36 Jacek Anaszewski wrote:
> On 11/24/2016 10:15 AM, Pali Rohár wrote:
> >On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
> >>I would also appreciate your opinion on the other solution to the
> >>problem of notifying brightness changes originating from hardware,
> >>i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
> >>and reading brightness.
> >
> >Another idea:
> >
> >If no trigger is active then led subsystem will invoke POLLPRI on
> >"brightness" sysfs file.
> >
> >And if there is active trigger then only trigger code could invoke
> >POLLPRI on "brightness" file.
> >
> >This could solve problem with high CPU load and power usage when e.g.
> >cpu trigger is active (and cpu trigger will not implement any POLLPRI).
> >
> >Do not know if this is really enough for your situation, it is just and
> >another idea.
> 
> This way we would be losing POLLPRI events when trigger is active,
> whereas it would be useful to have ones in some use cases.

In case it makes sense, trigger can implement that POLLPRI event. E.g.
for CPU trigger it probably does not make sense (or at least send
POLLPRI event lot of times per second).

> >But first please update documentation in ABI/testing to match current
> >situation. That is really needed.
> >
> 
> I suppose that you're thinking about behaviour on brightness file
> reading? Is there anything else you'd like to have clarified in the doc?

Yes, how triggers interact with brightness file, what happen when you
write 0 on active trigger, what happen when you read brightness file
with active trigger / without trigger.

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

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/24/2016 03:26 PM, Pali Rohár wrote:
> On Thursday 24 November 2016 15:21:36 Jacek Anaszewski wrote:
>> On 11/24/2016 10:15 AM, Pali Rohár wrote:
>>> On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
>>>> I would also appreciate your opinion on the other solution to the
>>>> problem of notifying brightness changes originating from hardware,
>>>> i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
>>>> and reading brightness.
>>>
>>> Another idea:
>>>
>>> If no trigger is active then led subsystem will invoke POLLPRI on
>>> "brightness" sysfs file.
>>>
>>> And if there is active trigger then only trigger code could invoke
>>> POLLPRI on "brightness" file.
>>>
>>> This could solve problem with high CPU load and power usage when e.g.
>>> cpu trigger is active (and cpu trigger will not implement any POLLPRI).
>>>
>>> Do not know if this is really enough for your situation, it is just and
>>> another idea.
>>
>> This way we would be losing POLLPRI events when trigger is active,
>> whereas it would be useful to have ones in some use cases.
>
> In case it makes sense, trigger can implement that POLLPRI event. E.g.
> for CPU trigger it probably does not make sense (or at least send
> POLLPRI event lot of times per second).

I'd like to have uniform semantics of this across all triggers.
The more exceptions we make the more chances we will miss something.
We would have to modify all triggers in drivers/led/triggers, but
there are also in-driver triggers scattered outside LED subsystem
which would also need the update. Let's not do any risky
modifications affecting LED Trigger core clients.

Since it has been reported that POLLPRI notifications on brightness
file can lead to increased power consumption, and having my above
statement I don't think that it is a good idea to use brightness
file for this.

So, let's focus on the hw_brightness_change_ro I've already mentioned.
I added "ro" postfix to make it clear that it is read only.

Four words in the sysfs file name make it somehow noisy,
so maybe someone will be able to come up with a better name.

>
>>> But first please update documentation in ABI/testing to match current
>>> situation. That is really needed.
>>>
>>
>> I suppose that you're thinking about behaviour on brightness file
>> reading? Is there anything else you'd like to have clarified in the doc?
>
> Yes, how triggers interact with brightness file, what happen when you
> write 0 on active trigger,

There is already a patch in linux-next adding the following:


+		Writing 0 to this file clears active trigger.
+
+		Writing non-zero to this file while trigger is active changes the
+		top brightness trigger is going to use.
+

> what happen when you read brightness file
> with active trigger / without trigger.
>

Yes, this needs to be covered too.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

On Thursday 24 November 2016 16:32:06 Jacek Anaszewski wrote:
> Since it has been reported that POLLPRI notifications on brightness
> file can lead to increased power consumption, and having my above
> statement I don't think that it is a good idea to use brightness
> file for this.

How is brightness file different from others that it cannot issue
POLLPRI notification?

I understood that problem is there in case that LED level is changed too
many times per second (like by CPU trigger).

If this is not that problem can you describe real issue, why we cannot
use POLLPRI for brightness file?

> >Yes, how triggers interact with brightness file, what happen when you
> >write 0 on active trigger,
> 
> There is already a patch in linux-next adding the following:
> 
> 
> +		Writing 0 to this file clears active trigger.
> +
> +		Writing non-zero to this file while trigger is active changes the
> +		top brightness trigger is going to use.
> +

Great!

> >what happen when you read brightness file
> >with active trigger / without trigger.
> >
> 
> Yes, this needs to be covered too.

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

------------------------------------------------------------------------------
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/24/2016 04:36 PM, Pali Rohár wrote:
> On Thursday 24 November 2016 16:32:06 Jacek Anaszewski wrote:
>> Since it has been reported that POLLPRI notifications on brightness
>> file can lead to increased power consumption, and having my above
>> statement I don't think that it is a good idea to use brightness
>> file for this.
>
> How is brightness file different from others that it cannot issue
> POLLPRI notification?
>
> I understood that problem is there in case that LED level is changed too
> many times per second (like by CPU trigger).
>
> If this is not that problem can you describe real issue, why we cannot
> use POLLPRI for brightness file?

It would be inconsistent not to notify all brightness changes on
brightness file. We should notify all of them or none.

>>> Yes, how triggers interact with brightness file, what happen when you
>>> write 0 on active trigger,
>>
>> There is already a patch in linux-next adding the following:
>>
>>
>> +		Writing 0 to this file clears active trigger.
>> +
>> +		Writing non-zero to this file while trigger is active changes the
>> +		top brightness trigger is going to use.
>> +
>
> Great!
>
>>> what happen when you read brightness file
>>> with active trigger / without trigger.
>>>
>>
>> Yes, this needs to be covered too.
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

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

On Thursday 24 November 2016 17:21:19 Jacek Anaszewski wrote:
> On 11/24/2016 04:36 PM, Pali Rohár wrote:
> > On Thursday 24 November 2016 16:32:06 Jacek Anaszewski wrote:
> >> Since it has been reported that POLLPRI notifications on
> >> brightness file can lead to increased power consumption, and
> >> having my above statement I don't think that it is a good idea to
> >> use brightness file for this.
> > 
> > How is brightness file different from others that it cannot issue
> > POLLPRI notification?
> > 
> > I understood that problem is there in case that LED level is
> > changed too many times per second (like by CPU trigger).
> > 
> > If this is not that problem can you describe real issue, why we
> > cannot use POLLPRI for brightness file?
> 
> It would be inconsistent not to notify all brightness changes on
> brightness file. We should notify all of them or none.

I understood that we cannot notify about changes done by CPU trigger due 
to high power usage... Or not?

-- 
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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/24/2016 05:51 PM, Pali Rohár wrote:
> On Thursday 24 November 2016 17:21:19 Jacek Anaszewski wrote:
>> On 11/24/2016 04:36 PM, Pali Rohár wrote:
>>> On Thursday 24 November 2016 16:32:06 Jacek Anaszewski wrote:
>>>> Since it has been reported that POLLPRI notifications on
>>>> brightness file can lead to increased power consumption, and
>>>> having my above statement I don't think that it is a good idea to
>>>> use brightness file for this.
>>>
>>> How is brightness file different from others that it cannot issue
>>> POLLPRI notification?
>>>
>>> I understood that problem is there in case that LED level is
>>> changed too many times per second (like by CPU trigger).
>>>
>>> If this is not that problem can you describe real issue, why we
>>> cannot use POLLPRI for brightness file?
>>
>> It would be inconsistent not to notify all brightness changes on
>> brightness file. We should notify all of them or none.
>
> I understood that we cannot notify about changes done by CPU trigger due
> to high power usage... Or not?

Exactly.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

[-- Attachment #1.1: Type: Text/Plain, Size: 707 bytes --]

On Thursday 24 November 2016 22:35:52 Jacek Anaszewski wrote:
> > I understood that we cannot notify about changes done by CPU
> > trigger due to high power usage... Or not?
> 
> Exactly.

So in this case exporting any new sysfs file (or using existing) which 
report POLLPRI events for LED devices with active trigger is not 
accepted.

This means that trigger itself is problematic and trigger code should 
tell to led subsystem if it should send POLLPRI events to userspace or 
not. E.g. cpu trigger should tell led subsystem that events must not be 
sent and e.g. rfkill trigger could tell that events can be sent...

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

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

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/24/2016 10:45 PM, Pali Rohár wrote:
> On Thursday 24 November 2016 22:35:52 Jacek Anaszewski wrote:
>>> I understood that we cannot notify about changes done by CPU
>>> trigger due to high power usage... Or not?
>>
>> Exactly.
>
> So in this case exporting any new sysfs file (or using existing) which
> report POLLPRI events for LED devices with active trigger is not
> accepted.

Why so? The new feature would be made optional so existing users
wouldn't be affected, unless they enabled it in the kernel
config.

> This means that trigger itself is problematic and trigger code should
> tell to led subsystem if it should send POLLPRI events to userspace or
> not. E.g. cpu trigger should tell led subsystem that events must not be
> sent and e.g. rfkill trigger could tell that events can be sent...

It would entail changes in all triggers, I'd like to avoid for the
reasons I provided in one of the previous messages.

I've just looked one more time at POLLPRI description in poll(2)
documentation:

"There is urgent data to read (e.g., out-of-band data on TCP socket;
pseudoterminal master in packet mode has seen state change in slave)."

I don't think that brightness changes originating from triggers fall
into this category, whereas the ones originating from hardware do.

In view of the above we could report hw brightness changes with POLLPRI
on brightness file, but unfortunately we can't because it is impossible
to guarantee that readout of brightness file will return the brightness
the POLLPRI was meant to notify about.

That's why a separate read only file seems to be the only proper
solution. Moreover, the file should return the brightness from the time
of last POLLPRI.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> In view of the above we could report hw brightness changes with POLLPRI
> on brightness file, but unfortunately we can't because it is impossible
> to guarantee that readout of brightness file will return the brightness
> the POLLPRI was meant to notify about.

Agreed here.

> That's why a separate read only file seems to be the only proper
> solution.

Yes please. And lets make self-changing leds into a trigger, as
proposed, and as Hans' patch should be already doing.

> Moreover, the file should return the brightness from the time
> of last POLLPRI.

Not sure I agree here. Normally, kernel returns current state for
variables, does not track "old" state.

Best regards,
									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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/25/2016 11:01 AM, Pavel Machek wrote:
> Hi!
>
>> In view of the above we could report hw brightness changes with POLLPRI
>> on brightness file, but unfortunately we can't because it is impossible
>> to guarantee that readout of brightness file will return the brightness
>> the POLLPRI was meant to notify about.
>
> Agreed here.
>
>> That's why a separate read only file seems to be the only proper
>> solution.
>
> Yes please. And lets make self-changing leds into a trigger, as
> proposed, and as Hans' patch should be already doing.

We can set one trigger at a time. In this case it will be impossible
to have hw brightness change notifications while other trigger is
active.

>> Moreover, the file should return the brightness from the time
>> of last POLLPRI.
>
> Not sure I agree here. Normally, kernel returns current state for
> variables, does not track "old" state.

It would report current state. The file called hw_brightness_change
should report last brightness change originating from hardware.
The most recent hw brightness change is still current state from
this perspective. It will be valid until next hw brightness change
occurs.

Current state does not mean current brightness in this case.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1927 bytes --]

Hi!

> >>In view of the above we could report hw brightness changes with POLLPRI
> >>on brightness file, but unfortunately we can't because it is impossible
> >>to guarantee that readout of brightness file will return the brightness
> >>the POLLPRI was meant to notify about.
> >
> >Agreed here.
> >
> >>That's why a separate read only file seems to be the only proper
> >>solution.
> >
> >Yes please. And lets make self-changing leds into a trigger, as
> >proposed, and as Hans' patch should be already doing.
> 
> We can set one trigger at a time. In this case it will be impossible
> to have hw brightness change notifications while other trigger is
> active.

And that is a _good_ thing. We don't want to deal with "echo heartbeat
> kbd_backlight_trigger" and then asking for hardware brightness
changes.

Lets keep it simple. Yes, monitoring backlight state while hardware
updates it is useful. But doing the monitor when some kind of blinking
from the kernel is active is just a unneccessary complexity...

> >>Moreover, the file should return the brightness from the time
> >>of last POLLPRI.
> >
> >Not sure I agree here. Normally, kernel returns current state for
> >variables, does not track "old" state.
> 
> It would report current state. The file called hw_brightness_change
> should report last brightness change originating from hardware.
> The most recent hw brightness change is still current state from
> this perspective. It will be valid until next hw brightness change
> occurs.
> 
> Current state does not mean current brightness in this case.

Well.. actually... I think this is a little bit over complex and
probably unneccessary. I'd let Hans implement whatever he thinks is
easiest.

Best regards,
									Pavel

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

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

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/25/2016 12:05 PM, Pavel Machek wrote:
> Hi!
>
>>>> In view of the above we could report hw brightness changes with POLLPRI
>>>> on brightness file, but unfortunately we can't because it is impossible
>>>> to guarantee that readout of brightness file will return the brightness
>>>> the POLLPRI was meant to notify about.
>>>
>>> Agreed here.
>>>
>>>> That's why a separate read only file seems to be the only proper
>>>> solution.
>>>
>>> Yes please. And lets make self-changing leds into a trigger, as
>>> proposed, and as Hans' patch should be already doing.
>>
>> We can set one trigger at a time. In this case it will be impossible
>> to have hw brightness change notifications while other trigger is
>> active.
>
> And that is a _good_ thing. We don't want to deal with "echo heartbeat
>> kbd_backlight_trigger" and then asking for hardware brightness
> changes.

I was far from proposing multi-trigger solution here.
Just wanted to highlight that with trigger approach there is
a trade-off problem.

> Lets keep it simple. Yes, monitoring backlight state while hardware
> updates it is useful. But doing the monitor when some kind of blinking
> from the kernel is active is just a unneccessary complexity...

Triggers are not limited to periodic blinking or reporting cpu
activity. There is also oneshot trigger that can be used e.g. when
user touches the screen, as Pali mentioned.

>>>> Moreover, the file should return the brightness from the time
>>>> of last POLLPRI.
>>>
>>> Not sure I agree here. Normally, kernel returns current state for
>>> variables, does not track "old" state.
>>
>> It would report current state. The file called hw_brightness_change
>> should report last brightness change originating from hardware.
>> The most recent hw brightness change is still current state from
>> this perspective. It will be valid until next hw brightness change
>> occurs.
>>
>> Current state does not mean current brightness in this case.
>
> Well.. actually... I think this is a little bit over complex and
> probably unneccessary. I'd let Hans implement whatever he thinks is
> easiest.

I'd say this is the trigger approach which is a bit convoluted.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1689 bytes --]

Hi!

> >Lets keep it simple. Yes, monitoring backlight state while hardware
> >updates it is useful. But doing the monitor when some kind of blinking
> >from the kernel is active is just a unneccessary complexity...
> 
> Triggers are not limited to periodic blinking or reporting cpu
> activity. There is also oneshot trigger that can be used e.g. when
> user touches the screen, as Pali mentioned.

Using oneshot trigger for this would be pretty strange. Notice that in
some cases (thinkpad battery led, for example) we either have firmware
controls the LED (but then software can't control it) or we have
software controlling the LED (but then we don't know what firmware
would put there). Maybe keyboard backlight can be controlled
"simultaneously" by both software and firmware, but there are
certainly LEDs that can't handle that, and IMO it would be nice to
have same interface.

> >Well.. actually... I think this is a little bit over complex and
> >probably unneccessary. I'd let Hans implement whatever he thinks is
> >easiest.
> 
> I'd say this is the trigger approach which is a bit convoluted.

In my eyes trigger approach is neccessary at least for some hardware,
and things it pretty clear: trigger on == LED changes without
userspace involvement. trigger off == userspace controls the LED.

So I'd do the trigger here. It is same way we can handle LEDs on
thinkpad. Yes, it means you won't be able to do oneshot trigger on
backlight. I don't think that's a huge problem.

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

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

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/25/2016 03:49 PM, Pavel Machek wrote:
> Hi!
>
>>> Lets keep it simple. Yes, monitoring backlight state while hardware
>>> updates it is useful. But doing the monitor when some kind of blinking
>> >from the kernel is active is just a unneccessary complexity...
>>
>> Triggers are not limited to periodic blinking or reporting cpu
>> activity. There is also oneshot trigger that can be used e.g. when
>> user touches the screen, as Pali mentioned.
>
> Using oneshot trigger for this would be pretty strange.

It was only an example to mention other than periodic triggers.
You could have a trigger that just turns the LED permanently on
after user touches the screen.

> Notice that in
> some cases (thinkpad battery led, for example) we either have firmware
> controls the LED (but then software can't control it) or we have
> software controlling the LED (but then we don't know what firmware
> would put there). Maybe keyboard backlight can be controlled
> "simultaneously" by both software and firmware, but there are
> certainly LEDs that can't handle that, and IMO it would be nice to
> have same interface.
>
>>> Well.. actually... I think this is a little bit over complex and
>>> probably unneccessary. I'd let Hans implement whatever he thinks is
>>> easiest.
>>
>> I'd say this is the trigger approach which is a bit convoluted.
>
> In my eyes trigger approach is neccessary at least for some hardware,
> and things it pretty clear: trigger on == LED changes without
> userspace involvement. trigger off == userspace controls the LED.

It is likely that it would break many existing users.

> So I'd do the trigger here. It is same way we can handle LEDs on
> thinkpad. Yes, it means you won't be able to do oneshot trigger on
> backlight. I don't think that's a huge problem.

There have been voices in this discussion claiming the opposite. :-)

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> >>Triggers are not limited to periodic blinking or reporting cpu
> >>activity. There is also oneshot trigger that can be used e.g. when
> >>user touches the screen, as Pali mentioned.
> >
> >Using oneshot trigger for this would be pretty strange.
> 
> It was only an example to mention other than periodic triggers.
> You could have a trigger that just turns the LED permanently on
> after user touches the screen.

Well.. triggers kind of assume they control the LED. They were not
prepared to deal with hardware changing the brightness behind their
back.

> >Notice that in
> >some cases (thinkpad battery led, for example) we either have firmware
> >controls the LED (but then software can't control it) or we have
> >software controlling the LED (but then we don't know what firmware
> >would put there). Maybe keyboard backlight can be controlled
> >"simultaneously" by both software and firmware, but there are
> >certainly LEDs that can't handle that, and IMO it would be nice to
> >have same interface.
> >
> >>>Well.. actually... I think this is a little bit over complex and
> >>>probably unneccessary. I'd let Hans implement whatever he thinks is
> >>>easiest.
> >>
> >>I'd say this is the trigger approach which is a bit convoluted.
> >
> >In my eyes trigger approach is neccessary at least for some hardware,
> >and things it pretty clear: trigger on == LED changes without
> >userspace involvement. trigger off == userspace controls the LED.
> 
> It is likely that it would break many existing users.

Can you elaborate on that?

I just tried with leds on thinkpad

root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
root@duo:/sys/class/leds/tpacpi::power# cat trigger
[none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
BAT0-charging-or-full BAT0-charging BAT0-full
BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
heartbeat
root@duo:/sys/class/leds/tpacpi::power#

I can control the LED from userspace, but then there's no way to put
the LED back to firmware control. That's just broken.

Do you have a proposal how to handle that?

> >So I'd do the trigger here. It is same way we can handle LEDs on
> >thinkpad. Yes, it means you won't be able to do oneshot trigger on
> >backlight. I don't think that's a huge problem.
> 
> There have been voices in this discussion claiming the opposite. :-)

Well, lets ignore those voices until the voices understand the current
design :-).

								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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/25/2016 09:40 PM, Pavel Machek wrote:
> Hi!
>
>>>> Triggers are not limited to periodic blinking or reporting cpu
>>>> activity. There is also oneshot trigger that can be used e.g. when
>>>> user touches the screen, as Pali mentioned.
>>>
>>> Using oneshot trigger for this would be pretty strange.
>>
>> It was only an example to mention other than periodic triggers.
>> You could have a trigger that just turns the LED permanently on
>> after user touches the screen.
>
> Well.. triggers kind of assume they control the LED. They were not
> prepared to deal with hardware changing the brightness behind their
> back.
>
>>> Notice that in
>>> some cases (thinkpad battery led, for example) we either have firmware
>>> controls the LED (but then software can't control it) or we have
>>> software controlling the LED (but then we don't know what firmware
>>> would put there). Maybe keyboard backlight can be controlled
>>> "simultaneously" by both software and firmware, but there are
>>> certainly LEDs that can't handle that, and IMO it would be nice to
>>> have same interface.
>>>
>>>>> Well.. actually... I think this is a little bit over complex and
>>>>> probably unneccessary. I'd let Hans implement whatever he thinks is
>>>>> easiest.
>>>>
>>>> I'd say this is the trigger approach which is a bit convoluted.
>>>
>>> In my eyes trigger approach is neccessary at least for some hardware,
>>> and things it pretty clear: trigger on == LED changes without
>>> userspace involvement. trigger off == userspace controls the LED.
>>
>> It is likely that it would break many existing users.
>
> Can you elaborate on that?

There might exist users that adjust LED brightness while having
active trigger. The best example is default-on trigger - it sets
brightness only on init, but remains active all the time. Whereas
this could be fixed, there is another case: think of changing blinking
brightness - it would be impossible.

> I just tried with leds on thinkpad
>
> root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
> root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
> root@duo:/sys/class/leds/tpacpi::power# cat trigger
> [none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
> kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
> kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
> BAT0-charging-or-full BAT0-charging BAT0-full
> BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
> phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
> heartbeat
> root@duo:/sys/class/leds/tpacpi::power#
>
> I can control the LED from userspace, but then there's no way to put
> the LED back to firmware control. That's just broken.
>
> Do you have a proposal how to handle that?

Isn't it under firmware control all the time?

>
>>> So I'd do the trigger here. It is same way we can handle LEDs on
>>> thinkpad. Yes, it means you won't be able to do oneshot trigger on
>>> backlight. I don't think that's a huge problem.
>>
>> There have been voices in this discussion claiming the opposite. :-)
>
> Well, lets ignore those voices until the voices understand the current
> design :-).

Sheer user is not interested in design, but in usability.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> >>>In my eyes trigger approach is neccessary at least for some hardware,
> >>>and things it pretty clear: trigger on == LED changes without
> >>>userspace involvement. trigger off == userspace controls the LED.
> >>
> >>It is likely that it would break many existing users.
> >
> >Can you elaborate on that?
> 
> There might exist users that adjust LED brightness while having
> active trigger. The best example is default-on trigger - it sets
> brightness only on init, but remains active all the time. Whereas
> this could be fixed, there is another case: think of changing blinking
> brightness - it would be impossible.

I don't see the breakage. For existing blinking and default-on
triggers, existing behaviour would be kept. Difference would be that
for hardware that changes triggers itself (like the keyboard light) we
would present it as a trigger. And you are right -- there would be
user visible changes there. You could not assign blinking, because
.. it already has a trigger. But I believe it is reasonable.

> >I just tried with leds on thinkpad
> >
> >root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
> >root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
> >root@duo:/sys/class/leds/tpacpi::power# cat trigger
> >[none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
> >kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
> >kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
> >BAT0-charging-or-full BAT0-charging BAT0-full
> >BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
> >phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
> >heartbeat
> >root@duo:/sys/class/leds/tpacpi::power#
> >
> >I can control the LED from userspace, but then there's no way to put
> >the LED back to firmware control. That's just broken.
> >
> >Do you have a proposal how to handle that?
> 
> Isn't it under firmware control all the time?

I'm not 100% sure in the thinkpad case. In the N900 case, we have LED
that displays (user_brightness || (user_enabled_indicator && cpu_activity)).

I believe clean way to do that would be a trigger.

> >>>So I'd do the trigger here. It is same way we can handle LEDs on
> >>>thinkpad. Yes, it means you won't be able to do oneshot trigger on
> >>>backlight. I don't think that's a huge problem.
> >>
> >>There have been voices in this discussion claiming the opposite. :-)
> >
> >Well, lets ignore those voices until the voices understand the current
> >design :-).
> 
> Sheer user is not interested in design, but in usability.

Well, end user is not expected to touch /sys files directly -- we
should create a script for that. /sys should be logical and map well
to what we do in kernel. Being "nice to use from shell" is
secondary...

								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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 12/21/2016 07:49 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> In my eyes trigger approach is neccessary at least for some hardware,
>>>>> and things it pretty clear: trigger on == LED changes without
>>>>> userspace involvement. trigger off == userspace controls the LED.
>>>>
>>>> It is likely that it would break many existing users.
>>>
>>> Can you elaborate on that?
>>
>> There might exist users that adjust LED brightness while having
>> active trigger. The best example is default-on trigger - it sets
>> brightness only on init, but remains active all the time. Whereas
>> this could be fixed, there is another case: think of changing blinking
>> brightness - it would be impossible.
> 
> I don't see the breakage. For existing blinking and default-on
> triggers, existing behaviour would be kept. Difference would be that
> for hardware that changes triggers itself (like the keyboard light) we
> would present it as a trigger. And you are right -- there would be
> user visible changes there. You could not assign blinking, because
> .. it already has a trigger. But I believe it is reasonable.

We've already received negative feedback regarding blocking
application of different trigger when hw brightness change notifications
are enabled. One solution to that is making the notifications
orthogonal to the trigger mechanism. The other possibility is to allow
for having more than one active trigger for a LED.

We could think of defining a special type of persistent trigger that
would be always enabled.

Best regards,
Jacek Anaszewski


>>> I just tried with leds on thinkpad
>>>
>>> root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
>>> root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
>>> root@duo:/sys/class/leds/tpacpi::power# cat trigger
>>> [none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
>>> kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
>>> kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
>>> BAT0-charging-or-full BAT0-charging BAT0-full
>>> BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
>>> phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
>>> heartbeat
>>> root@duo:/sys/class/leds/tpacpi::power#
>>>
>>> I can control the LED from userspace, but then there's no way to put
>>> the LED back to firmware control. That's just broken.
>>>
>>> Do you have a proposal how to handle that?
>>
>> Isn't it under firmware control all the time?
> 
> I'm not 100% sure in the thinkpad case. In the N900 case, we have LED
> that displays (user_brightness || (user_enabled_indicator && cpu_activity)).
> 
> I believe clean way to do that would be a trigger.
> 
>>>>> So I'd do the trigger here. It is same way we can handle LEDs on
>>>>> thinkpad. Yes, it means you won't be able to do oneshot trigger on
>>>>> backlight. I don't think that's a huge problem.
>>>>
>>>> There have been voices in this discussion claiming the opposite. :-)
>>>
>>> Well, lets ignore those voices until the voices understand the current
>>> design :-).
>>
>> Sheer user is not interested in design, but in usability.
> 
> Well, end user is not expected to touch /sys files directly -- we
> should create a script for that. /sys should be logical and map well
> to what we do in kernel. Being "nice to use from shell" is
> secondary...
> 
> 								Pavel
> 

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Darren Hart]

On Fri, Dec 23, 2016 at 10:55:34PM +0100, Jacek Anaszewski wrote:
> Hi,
> 
> On 12/21/2016 07:49 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>>>> In my eyes trigger approach is neccessary at least for some hardware,
> >>>>> and things it pretty clear: trigger on == LED changes without
> >>>>> userspace involvement. trigger off == userspace controls the LED.
> >>>>
> >>>> It is likely that it would break many existing users.
> >>>
> >>> Can you elaborate on that?
> >>
> >> There might exist users that adjust LED brightness while having
> >> active trigger. The best example is default-on trigger - it sets
> >> brightness only on init, but remains active all the time. Whereas
> >> this could be fixed, there is another case: think of changing blinking
> >> brightness - it would be impossible.
> > 
> > I don't see the breakage. For existing blinking and default-on
> > triggers, existing behaviour would be kept. Difference would be that
> > for hardware that changes triggers itself (like the keyboard light) we
> > would present it as a trigger. And you are right -- there would be
> > user visible changes there. You could not assign blinking, because
> > .. it already has a trigger. But I believe it is reasonable.
> 
> We've already received negative feedback regarding blocking
> application of different trigger when hw brightness change notifications
> are enabled. One solution to that is making the notifications
> orthogonal to the trigger mechanism. The other possibility is to allow
> for having more than one active trigger for a LED.
> 
> We could think of defining a special type of persistent trigger that
> would be always enabled.
> 
> Best regards,
> Jacek Anaszewski
> 
> 
> >>> I just tried with leds on thinkpad
> >>>
> >>> root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
> >>> root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
> >>> root@duo:/sys/class/leds/tpacpi::power# cat trigger
> >>> [none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
> >>> kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
> >>> kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
> >>> BAT0-charging-or-full BAT0-charging BAT0-full
> >>> BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
> >>> phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
> >>> heartbeat
> >>> root@duo:/sys/class/leds/tpacpi::power#
> >>>
> >>> I can control the LED from userspace, but then there's no way to put
> >>> the LED back to firmware control. That's just broken.
> >>>
> >>> Do you have a proposal how to handle that?
> >>
> >> Isn't it under firmware control all the time?
> > 
> > I'm not 100% sure in the thinkpad case. In the N900 case, we have LED
> > that displays (user_brightness || (user_enabled_indicator && cpu_activity)).
> > 
> > I believe clean way to do that would be a trigger.
> > 
> >>>>> So I'd do the trigger here. It is same way we can handle LEDs on
> >>>>> thinkpad. Yes, it means you won't be able to do oneshot trigger on
> >>>>> backlight. I don't think that's a huge problem.
> >>>>
> >>>> There have been voices in this discussion claiming the opposite. :-)
> >>>
> >>> Well, lets ignore those voices until the voices understand the current
> >>> design :-).
> >>
> >> Sheer user is not interested in design, but in usability.
> > 
> > Well, end user is not expected to touch /sys files directly -- we
> > should create a script for that. /sys should be logical and map well
> > to what we do in kernel. Being "nice to use from shell" is
> > secondary...
> > 
> > 								Pavel
> > 

This seems to have stalled out. From the driver/platform/x86 perspective, I
believe we're still waiting on the leds subsystem mechanism to be decided on.
Hans, you said you'd send a v6 once that was squared away I believe.

For now, I'm marking these as "changes requested" and archiving the patches.
Will revisit once the LEDs bits are sorted and v6 lands on the list.


If I've missed some context some place, please point me at it.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 13-01-17 20:17, Darren Hart wrote:
> On Fri, Dec 23, 2016 at 10:55:34PM +0100, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 12/21/2016 07:49 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>> In my eyes trigger approach is neccessary at least for some hardware,
>>>>>>> and things it pretty clear: trigger on == LED changes without
>>>>>>> userspace involvement. trigger off == userspace controls the LED.
>>>>>>
>>>>>> It is likely that it would break many existing users.
>>>>>
>>>>> Can you elaborate on that?
>>>>
>>>> There might exist users that adjust LED brightness while having
>>>> active trigger. The best example is default-on trigger - it sets
>>>> brightness only on init, but remains active all the time. Whereas
>>>> this could be fixed, there is another case: think of changing blinking
>>>> brightness - it would be impossible.
>>>
>>> I don't see the breakage. For existing blinking and default-on
>>> triggers, existing behaviour would be kept. Difference would be that
>>> for hardware that changes triggers itself (like the keyboard light) we
>>> would present it as a trigger. And you are right -- there would be
>>> user visible changes there. You could not assign blinking, because
>>> .. it already has a trigger. But I believe it is reasonable.
>>
>> We've already received negative feedback regarding blocking
>> application of different trigger when hw brightness change notifications
>> are enabled. One solution to that is making the notifications
>> orthogonal to the trigger mechanism. The other possibility is to allow
>> for having more than one active trigger for a LED.
>>
>> We could think of defining a special type of persistent trigger that
>> would be always enabled.
>>
>> Best regards,
>> Jacek Anaszewski
>>
>>
>>>>> I just tried with leds on thinkpad
>>>>>
>>>>> root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness
>>>>> root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness
>>>>> root@duo:/sys/class/leds/tpacpi::power# cat trigger
>>>>> [none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock
>>>>> kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock
>>>>> kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online
>>>>> BAT0-charging-or-full BAT0-charging BAT0-full
>>>>> BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
>>>>> phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74
>>>>> heartbeat
>>>>> root@duo:/sys/class/leds/tpacpi::power#
>>>>>
>>>>> I can control the LED from userspace, but then there's no way to put
>>>>> the LED back to firmware control. That's just broken.
>>>>>
>>>>> Do you have a proposal how to handle that?
>>>>
>>>> Isn't it under firmware control all the time?
>>>
>>> I'm not 100% sure in the thinkpad case. In the N900 case, we have LED
>>> that displays (user_brightness || (user_enabled_indicator && cpu_activity)).
>>>
>>> I believe clean way to do that would be a trigger.
>>>
>>>>>>> So I'd do the trigger here. It is same way we can handle LEDs on
>>>>>>> thinkpad. Yes, it means you won't be able to do oneshot trigger on
>>>>>>> backlight. I don't think that's a huge problem.
>>>>>>
>>>>>> There have been voices in this discussion claiming the opposite. :-)
>>>>>
>>>>> Well, lets ignore those voices until the voices understand the current
>>>>> design :-).
>>>>
>>>> Sheer user is not interested in design, but in usability.
>>>
>>> Well, end user is not expected to touch /sys files directly -- we
>>> should create a script for that. /sys should be logical and map well
>>> to what we do in kernel. Being "nice to use from shell" is
>>> secondary...
>>>
>>> 								Pavel
>>>
>
> This seems to have stalled out. From the driver/platform/x86 perspective, I
> believe we're still waiting on the leds subsystem mechanism to be decided on.

Correct.

> Hans, you said you'd send a v6 once that was squared away I believe.

Yes, I need to brush these patches of and send a new version.

Do you want me to also send out a new version of the platform patches when
I send the next shot at the LED side of things, or shall I keep those
in my personal tree until the LED api is finalized ?

> For now, I'm marking these as "changes requested" and archiving the patches.
> Will revisit once the LEDs bits are sorted and v6 lands on the list.

Ack.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Henrique de Moraes Holschuh]

On Sun, 15 Jan 2017, Hans de Goede wrote:
> Do you want me to also send out a new version of the platform patches when
> I send the next shot at the LED side of things, or shall I keep those
> in my personal tree until the LED api is finalized ?

If you don't sent the entire patch set here, please keep me CC'd.  You
guys seem to have some misconceptions about how the ThinkPad LEDs work.

For one, they're LIFO, as in: whomever set their state last, wins.  And
you have several agents trying to command them: EC itself, SMBIOS (in
SMM mode), ACPI DSDT, thinkpad-acpi.   Most of the time, changes are
event-driven, so you _could_ try to have the last word on those which
all such events are visible...

Anyway, the fact is that you can never be really ensure they're at the
state you want.  It is a best-effort kind of thing.   Also, they're
controlled by EC firmware, not GPIO, so you don't know their physical
state (on/off/brightness level): you just know what high-level state the
EC is set to (on/off/blink/ramp) and that information is not always
available (the LEDs can be write-only, or worse, turn-on-only or
turn-off-only at the ACPI DSDT interface level, which is what we
typically use in thinkpad-acpi :p).

I.e: some thinkpad LEDs are better behaved than others, and more
controllable than others.  And this changes with firmware model/version.

-- 
  Henrique Holschuh

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 15-01-17 13:25, Henrique de Moraes Holschuh wrote:
> On Sun, 15 Jan 2017, Hans de Goede wrote:
>> Do you want me to also send out a new version of the platform patches when
>> I send the next shot at the LED side of things, or shall I keep those
>> in my personal tree until the LED api is finalized ?
>
> If you don't sent the entire patch set here, please keep me CC'd.  You
> guys seem to have some misconceptions about how the ThinkPad LEDs work.

Right, note the main focus of these patches is the keyboard backlight control,
not any other LEDs on the Thinkpads.

Eitherway I'll keep you Cc-ed.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> >> There might exist users that adjust LED brightness while having
> >> active trigger. The best example is default-on trigger - it sets
> >> brightness only on init, but remains active all the time. Whereas
> >> this could be fixed, there is another case: think of changing blinking
> >> brightness - it would be impossible.
> > 
> > I don't see the breakage. For existing blinking and default-on
> > triggers, existing behaviour would be kept. Difference would be that
> > for hardware that changes triggers itself (like the keyboard light) we
> > would present it as a trigger. And you are right -- there would be
> > user visible changes there. You could not assign blinking, because
> > .. it already has a trigger. But I believe it is reasonable.
> 
> We've already received negative feedback regarding blocking
> application of different trigger when hw brightness change
> notifications

Actually sometimes we'll get negative feedback for anything we do. I
still believe exposing the backlight case as a trigger is a right thing.

> are enabled. One solution to that is making the notifications
> orthogonal to the trigger mechanism. The other possibility is to allow
> for having more than one active trigger for a LED.
> 
> We could think of defining a special type of persistent trigger that
> would be always enabled.

Yes, that could be done. I'd not call it persistent trigger, but
with right name...

#1:

Apparently some LEDs change themselves, and we can't find how or
when. That's thinkpad battery LED, for example.

#2:

Then there are some LEDs that change themselves, and we can get
notifications. We should make it very clear that we'll not send
notifications kernel did itself.

So... for #1 and #2 something like

led/hardware_changes_brightness 

and for #2

led/poll_for_hardware_brightness_change

where poll() wakes the userspace up when the brightness changes, and
read() can get new brightness...?
								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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 01/24/2017 01:32 PM, Pavel Machek wrote:
> Hi!
> 
>>>> There might exist users that adjust LED brightness while having
>>>> active trigger. The best example is default-on trigger - it sets
>>>> brightness only on init, but remains active all the time. Whereas
>>>> this could be fixed, there is another case: think of changing blinking
>>>> brightness - it would be impossible.
>>>
>>> I don't see the breakage. For existing blinking and default-on
>>> triggers, existing behaviour would be kept. Difference would be that
>>> for hardware that changes triggers itself (like the keyboard light) we
>>> would present it as a trigger. And you are right -- there would be
>>> user visible changes there. You could not assign blinking, because
>>> .. it already has a trigger. But I believe it is reasonable.
>>
>> We've already received negative feedback regarding blocking
>> application of different trigger when hw brightness change
>> notifications
> 
> Actually sometimes we'll get negative feedback for anything we do. I
> still believe exposing the backlight case as a trigger is a right thing.
> 
>> are enabled. One solution to that is making the notifications
>> orthogonal to the trigger mechanism. The other possibility is to allow
>> for having more than one active trigger for a LED.
>>
>> We could think of defining a special type of persistent trigger that
>> would be always enabled.
> 
> Yes, that could be done. I'd not call it persistent trigger, but
> with right name...
> 
> #1:
> 
> Apparently some LEDs change themselves, and we can't find how or
> when. That's thinkpad battery LED, for example.
> 
> #2:
> 
> Then there are some LEDs that change themselves, and we can get
> notifications. We should make it very clear that we'll not send
> notifications kernel did itself.
> 
> So... for #1 and #2 something like
> 
> led/hardware_changes_brightness 
> 
> and for #2
> 
> led/poll_for_hardware_brightness_change
> 
> where poll() wakes the userspace up when the brightness changes, and
> read() can get new brightness...?

Well, it is almost the same as hw_brightness_change I proposed
earlier. Persistent triggers OTOH would be more generic and it would
fit for the trigger approach Hans already implemented.


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 01/24/2017 11:56 PM, Jacek Anaszewski wrote:
> Hi,
>
> On 01/24/2017 01:32 PM, Pavel Machek wrote:
>> Hi!
>>
>>>>> There might exist users that adjust LED brightness while having
>>>>> active trigger. The best example is default-on trigger - it sets
>>>>> brightness only on init, but remains active all the time. Whereas
>>>>> this could be fixed, there is another case: think of changing blinking
>>>>> brightness - it would be impossible.
>>>>
>>>> I don't see the breakage. For existing blinking and default-on
>>>> triggers, existing behaviour would be kept. Difference would be that
>>>> for hardware that changes triggers itself (like the keyboard light) we
>>>> would present it as a trigger. And you are right -- there would be
>>>> user visible changes there. You could not assign blinking, because
>>>> .. it already has a trigger. But I believe it is reasonable.
>>>
>>> We've already received negative feedback regarding blocking
>>> application of different trigger when hw brightness change
>>> notifications
>>
>> Actually sometimes we'll get negative feedback for anything we do. I
>> still believe exposing the backlight case as a trigger is a right thing.
>>
>>> are enabled. One solution to that is making the notifications
>>> orthogonal to the trigger mechanism. The other possibility is to allow
>>> for having more than one active trigger for a LED.
>>>
>>> We could think of defining a special type of persistent trigger that
>>> would be always enabled.
>>
>> Yes, that could be done. I'd not call it persistent trigger, but
>> with right name...
>>
>> #1:
>>
>> Apparently some LEDs change themselves, and we can't find how or
>> when. That's thinkpad battery LED, for example.
>>
>> #2:
>>
>> Then there are some LEDs that change themselves, and we can get
>> notifications. We should make it very clear that we'll not send
>> notifications kernel did itself.
>>
>> So... for #1 and #2 something like
>>
>> led/hardware_changes_brightness
>>
>> and for #2
>>
>> led/poll_for_hardware_brightness_change
>>
>> where poll() wakes the userspace up when the brightness changes, and
>> read() can get new brightness...?
>
> Well, it is almost the same as hw_brightness_change I proposed
> earlier. Persistent triggers OTOH would be more generic and it would
> fit for the trigger approach Hans already implemented.

I have spend most of my time yesterday implementing the brightness_hw_changed
solution. I've just finished testing this on my XPS 15, so I'm going to post
the patches now. It would be nice if we can move forward with that as agreed
upon.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 25-11-16 11:01, Pavel Machek wrote:
> Hi!
>
>> In view of the above we could report hw brightness changes with POLLPRI
>> on brightness file, but unfortunately we can't because it is impossible
>> to guarantee that readout of brightness file will return the brightness
>> the POLLPRI was meant to notify about.
>
> Agreed here.
>
>> That's why a separate read only file seems to be the only proper
>> solution.
>
> Yes please. And lets make self-changing leds into a trigger, as
> proposed, and as Hans' patch should be already doing.
>
>> Moreover, the file should return the brightness from the time
>> of last POLLPRI.
>
> Not sure I agree here. Normally, kernel returns current state for
> variables, does not track "old" state.

Agreed, storing the last state just unnecessarily complicates things.

So do we have a consensus on implementing a new hw_brightness_change
sysfs attribute now, which only some LEDs will have, can be polled
to detect changed done autonomously by the hardware and returns
the current / actual LED brightness when read ?

As for the modeling how the hotkey controls the LED as a trigger,
although I do like this from one pov, I can see Jacek's point that
this is confusing as there really is nothing to configure here,
where as normally a user could do "echo none > trigger" to break
the link. So I think that is best (cleanest /minimal non confusing
API) with just the hw_brightness_change sysfs-attribute and not
model this as a trigger.

That, or fall back to my latest patch-set as posted, I still like
that one the most.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

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

On Friday 25 November 2016 12:14:56 Hans de Goede wrote:
> Hi,
> 
> On 25-11-16 11:01, Pavel Machek wrote:
> > Hi!
> > 
> >> In view of the above we could report hw brightness changes with
> >> POLLPRI on brightness file, but unfortunately we can't because it
> >> is impossible to guarantee that readout of brightness file will
> >> return the brightness the POLLPRI was meant to notify about.
> > 
> > Agreed here.
> > 
> >> That's why a separate read only file seems to be the only proper
> >> solution.
> > 
> > Yes please. And lets make self-changing leds into a trigger, as
> > proposed, and as Hans' patch should be already doing.
> > 
> >> Moreover, the file should return the brightness from the time
> >> of last POLLPRI.
> > 
> > Not sure I agree here. Normally, kernel returns current state for
> > variables, does not track "old" state.
> 
> Agreed, storing the last state just unnecessarily complicates things.
> 
> So do we have a consensus on implementing a new hw_brightness_change
> sysfs attribute now, which only some LEDs will have, can be polled
> to detect changed done autonomously by the hardware and returns
> the current / actual LED brightness when read ?
> 
> As for the modeling how the hotkey controls the LED as a trigger,
> although I do like this from one pov, I can see Jacek's point that
> this is confusing as there really is nothing to configure here,
> where as normally a user could do "echo none > trigger" to break
> the link. So I think that is best (cleanest /minimal non confusing
> API) with just the hw_brightness_change sysfs-attribute and not
> model this as a trigger.

I can accept with this solution (no trigger, event on new sysfs file 
which returns current/actual brightness state, new sysfs file only for 
devices which can report brightness state).

But I'm not sure if it is really fixing that original problem with high 
power usage...

As wrote in some previous emails, consider "actual_brightness" sysfs 
name which is already used for this purpose by backlight subsystem -- 
because for consistency. backlight devices have: actual_brightness, 
brightness, max_brightness.

> That, or fall back to my latest patch-set as posted, I still like
> that one the most.
> 
> Regards,
> 
> Hans

-- 
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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> > As for the modeling how the hotkey controls the LED as a trigger,
> > although I do like this from one pov, I can see Jacek's point that
> > this is confusing as there really is nothing to configure here,
> > where as normally a user could do "echo none > trigger" to break
> > the link. So I think that is best (cleanest /minimal non confusing
> > API) with just the hw_brightness_change sysfs-attribute and not
> > model this as a trigger.
> 
> I can accept with this solution (no trigger, event on new sysfs file 
> which returns current/actual brightness state, new sysfs file only for 
> devices which can report brightness state).
> 
> But I'm not sure if it is really fixing that original problem with high 
> power usage...

Yes, it is fixing that problem.

-- 
(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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/25/2016 12:26 PM, Pali Rohár wrote:
> On Friday 25 November 2016 12:14:56 Hans de Goede wrote:
>> Hi,
>>
>> On 25-11-16 11:01, Pavel Machek wrote:
>>> Hi!
>>>
>>>> In view of the above we could report hw brightness changes with
>>>> POLLPRI on brightness file, but unfortunately we can't because it
>>>> is impossible to guarantee that readout of brightness file will
>>>> return the brightness the POLLPRI was meant to notify about.
>>>
>>> Agreed here.
>>>
>>>> That's why a separate read only file seems to be the only proper
>>>> solution.
>>>
>>> Yes please. And lets make self-changing leds into a trigger, as
>>> proposed, and as Hans' patch should be already doing.
>>>
>>>> Moreover, the file should return the brightness from the time
>>>> of last POLLPRI.
>>>
>>> Not sure I agree here. Normally, kernel returns current state for
>>> variables, does not track "old" state.
>>
>> Agreed, storing the last state just unnecessarily complicates things.
>>
>> So do we have a consensus on implementing a new hw_brightness_change
>> sysfs attribute now, which only some LEDs will have, can be polled
>> to detect changed done autonomously by the hardware and returns
>> the current / actual LED brightness when read ?
>>
>> As for the modeling how the hotkey controls the LED as a trigger,
>> although I do like this from one pov, I can see Jacek's point that
>> this is confusing as there really is nothing to configure here,
>> where as normally a user could do "echo none > trigger" to break
>> the link. So I think that is best (cleanest /minimal non confusing
>> API) with just the hw_brightness_change sysfs-attribute and not
>> model this as a trigger.
>
> I can accept with this solution (no trigger, event on new sysfs file
> which returns current/actual brightness state, new sysfs file only for
> devices which can report brightness state).
>
> But I'm not sure if it is really fixing that original problem with high
> power usage...

High power consumption occurred because in that solution every call
to __led_set_brightness{_blocking} triggered the POLLPRI.
In the discussed approach it will not be the case.

> As wrote in some previous emails, consider "actual_brightness" sysfs
> name which is already used for this purpose by backlight subsystem --
> because for consistency. backlight devices have: actual_brightness,
> brightness, max_brightness.

I don't like it very much but I could accept it eventually if
other people deem it accurate.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/25/2016 12:14 PM, Hans de Goede wrote:
> Hi,
>
> On 25-11-16 11:01, Pavel Machek wrote:
>> Hi!
>>
>>> In view of the above we could report hw brightness changes with POLLPRI
>>> on brightness file, but unfortunately we can't because it is impossible
>>> to guarantee that readout of brightness file will return the brightness
>>> the POLLPRI was meant to notify about.
>>
>> Agreed here.
>>
>>> That's why a separate read only file seems to be the only proper
>>> solution.
>>
>> Yes please. And lets make self-changing leds into a trigger, as
>> proposed, and as Hans' patch should be already doing.
>>
>>> Moreover, the file should return the brightness from the time
>>> of last POLLPRI.
>>
>> Not sure I agree here. Normally, kernel returns current state for
>> variables, does not track "old" state.
>
> Agreed, storing the last state just unnecessarily complicates things.

It is mandatory to secure a reliable readout of the brightness value
set by the hardware. Without it, we could end up reading brightness
set in the meantime either by trigger or by userspace. Knowing that
value can be useful in case hardware tries to lower the brightness
level set by the user for some reason (e.g. low battery voltage).

However, I'd tweak the file name a bit, to make it clear
what property the file represents - the brightness changed
by the hardware, effectively I'm proposing the property:

brightness_hw_changed

Best regards,
Jacek Anaszewski

>
> So do we have a consensus on implementing a new hw_brightness_change
> sysfs attribute now, which only some LEDs will have, can be polled
> to detect changed done autonomously by the hardware and returns
> the current / actual LED brightness when read ?
>
> As for the modeling how the hotkey controls the LED as a trigger,
> although I do like this from one pov, I can see Jacek's point that
> this is confusing as there really is nothing to configure here,
> where as normally a user could do "echo none > trigger" to break
> the link. So I think that is best (cleanest /minimal non confusing
> API) with just the hw_brightness_change sysfs-attribute and not
> model this as a trigger.
>
> That, or fall back to my latest patch-set as posted, I still like
> that one the most.
>
> Regards,
>
> Hans
>
>
>
>

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

On Thu 2016-11-24 16:36:51, Pali Rohár wrote:
> On Thursday 24 November 2016 16:32:06 Jacek Anaszewski wrote:
> > Since it has been reported that POLLPRI notifications on brightness
> > file can lead to increased power consumption, and having my above
> > statement I don't think that it is a good idea to use brightness
> > file for this.
> 
> How is brightness file different from others that it cannot issue
> POLLPRI notification?

Reading "brightness" may or may not give you current brightness of the
led, depending on hardware limitations -- and you can't know. (It may
give you maximum brightness of trigger)

Reading "hw_current_brightness" file (if present) will give you
current brightness. In addition, it is pollable.
								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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

On Thu 2016-11-24 10:15:25, Pali Rohár wrote:
> On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
> > I would also appreciate your opinion on the other solution to the
> > problem of notifying brightness changes originating from hardware,
> > i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
> > and reading brightness.
> 
> Another idea:
> 
> If no trigger is active then led subsystem will invoke POLLPRI on
> "brightness" sysfs file.
> 
> And if there is active trigger then only trigger code could invoke
> POLLPRI on "brightness" file.

That would mess up interface even more. Sorry.

> Do not know if this is really enough for your situation, it is just and
> another idea.

We don't need another ideas at the moment, unless they are better than
proposed solution.

Thanks,
									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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> >>As pointed in other email, we do not know if HW really controls keyboard backlight,
> >>so adding "fake" trigger on machines without HW control is not a good idea.
> >
> >Well, if we know that hardware will not change the brightness on its
> >own, yes, I'd avoid the trigger. If we don't know (as is common on
> >ACPI machines, I'd keep the trigger).
> 
> I'd drop the trigger approach due to the mess it can make in peoples'
> minds due to the fact that LED class device handles trigger events
> generated by itself.

We can teach people. IMO the LED that changes itself is special, and
trigger explains that nicely to the userspace. Plus, it allows us to
keep this functionality out of the core. 

> I'd add a file hw_brightness_change or async_brightness or something
> similar and make it only readable/pollable. current_brightness is
> ambiguous and questionable.

Well, exact name is not too important...
									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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

Hi,

On 11/21/2016 12:41 PM, Pavel Machek wrote:
> Hi!
>
>>>> As pointed in other email, we do not know if HW really controls keyboard backlight,
>>>> so adding "fake" trigger on machines without HW control is not a good idea.
>>>
>>> Well, if we know that hardware will not change the brightness on its
>>> own, yes, I'd avoid the trigger. If we don't know (as is common on
>>> ACPI machines, I'd keep the trigger).
>>
>> I'd drop the trigger approach due to the mess it can make in peoples'
>> minds due to the fact that LED class device handles trigger events
>> generated by itself.
>
> We can teach people. IMO the LED that changes itself is special, and
> trigger explains that nicely to the userspace. Plus, it allows us to
> keep this functionality out of the core.

Please refer to the downsides of this appraoch:

- lack of information if given LED class device supports hw
   generated POLLPRI events
- impossible to apply other trigger while polling
- circular trigger event path (if set_brightness parameter of
   ledtrig_kbd_backlight() is true)

>> I'd add a file hw_brightness_change or async_brightness or something
>> similar and make it only readable/pollable. current_brightness is
>> ambiguous and questionable.
>
> Well, exact name is not too important...

The name should clearly explain the file purpose. I bet that we would
see many questions once the file appeared in the mainline.
Also, I'm afraid that I wouldn't be able to explain this name in
few simple words, without daunting the listener, or even triggering
the discussion on brightness shortcomings we've already gone through.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

Hi!

> >>>>As pointed in other email, we do not know if HW really controls keyboard backlight,
> >>>>so adding "fake" trigger on machines without HW control is not a good idea.
> >>>
> >>>Well, if we know that hardware will not change the brightness on its
> >>>own, yes, I'd avoid the trigger. If we don't know (as is common on
> >>>ACPI machines, I'd keep the trigger).
> >>
> >>I'd drop the trigger approach due to the mess it can make in peoples'
> >>minds due to the fact that LED class device handles trigger events
> >>generated by itself.
> >
> >We can teach people. IMO the LED that changes itself is special, and
> >trigger explains that nicely to the userspace. Plus, it allows us to
> >keep this functionality out of the core.
> 
> Please refer to the downsides of this appraoch:
> 
> - lack of information if given LED class device supports hw
>   generated POLLPRI events

Userspace can plainly see that a trigger is active, and knows to
expect 

> - impossible to apply other trigger while polling

That's a good thing. We _don't_ want polling to be active when trigger
such as "CPU active" is active. We want userspace to monitor hardware
events but not software ones.

> >>I'd add a file hw_brightness_change or async_brightness or something
> >>similar and make it only readable/pollable. current_brightness is
> >>ambiguous and questionable.
> >
> >Well, exact name is not too important...
> 
> The name should clearly explain the file purpose. I bet that we would
> see many questions once the file appeared in the mainline.
> Also, I'm afraid that I wouldn't be able to explain this name in
> few simple words, without daunting the listener, or even triggering
> the discussion on brightness shortcomings we've already gone through.

Well, feel free to suggest non-confusing name.

Unfortunately, yes, we'll need to do some explaining, as existing
"brightness" behaviour is already pretty tricky / confusing /
counterintuitive.

Lets at least make sure new additions are clean and simple.

Thanks,
									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 v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski]

On 11/20/2016 04:05 PM, Pali Rohár wrote:
> On Saturday 19 November 2016 16:44:09 Jacek Anaszewski wrote:
>> Hi,
>>
>> On 11/18/2016 07:47 PM, Hans de Goede wrote:
>>> HI,
>>>
>>> On 18-11-16 17:03, Jacek Anaszewski wrote:
>>>> Hi,
>>>>
>>>> On 11/18/2016 10:07 AM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 18-11-16 09:55, Jacek Anaszewski wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> Thanks for the patch.
>>>>>>
>>>>>> I think we need less generic trigger name.
>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>> can change LED brightness autonomously.
>>>>>>
>>>>>> How about kbd-backlight-pollable ?
>>>>>
>>>>> This is a trigger to control kbd-backlights, in the
>>>>> current use-case the brightness change is already done
>>>>> by the firmware, hence the set_brightness argument to
>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>> it again.
>>>>>
>>>>> But I can see future cases where we do want to have some
>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>> does not do the adjustment automatically) which does
>>>>> lead to actually updating the brightness.
>>>>>
>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>> which in the future can also be used to directly control
>>>>> kbd-backlight brightness; and not just to make ot
>>>>> poll-able.
>>>>
>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>
>>> It does.
>>>
>>>> that's why I assessed it is too generic.
>>>
>>> The whole purpose of the trigger as implemented is to be
>>> generic, as it seems senseless to implement a one off
>>> trigger for just the dell / thinkpad case.
>>>
>>>> It seems however to be
>>>> the other way round - if kbd-backlight means that this is
>>>> a trigger only for use with dell-laptop keyboard driver
>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>> not generic at all.
>>>
>>> The trigger as implemented is generic, if you think
>>> otherwise, please let me know which part is not generic
>>> according to you.
>>
>> I think I was too meticulous here. In the end of the previous
>> message I mentioned that we cannot guarantee that all keyboard
>> backlight controllers can adjust brightness autonomously.
>> Nonetheless, for the ones that cannot do that it would make no sense
>> to have a trigger. In view of that this trigger is generic enough.
>>
>> I'll wait for Pavel's opinion before merging the patch set
>> as he was also involved in this whole thread.
>
> Do you mean me? Or Pavel Machek (CCed) who was involved in LEDs for input devices?

I meant Pavel Machek, I haven't known that Pali is an equivalent of
Pavel :-)
Your opinion is very much appreciated though, thanks.

> As pointed in other email, we do not know if HW really controls keyboard backlight,
> so adding "fake" trigger on machines without HW control is not a good idea.
>

Yes, I had also similar doubts, but got almost convinced due to
no objections. Now it becomes clear that we need to improve this
feature.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 21-11-16 09:35, Jacek Anaszewski wrote:
> On 11/20/2016 04:05 PM, Pali Rohár wrote:
>> On Saturday 19 November 2016 16:44:09 Jacek Anaszewski wrote:
>>> Hi,
>>>
>>> On 11/18/2016 07:47 PM, Hans de Goede wrote:
>>>> HI,
>>>>
>>>> On 18-11-16 17:03, Jacek Anaszewski wrote:
>>>>> Hi,
>>>>>
>>>>> On 11/18/2016 10:07 AM, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 18-11-16 09:55, Jacek Anaszewski wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> I think we need less generic trigger name.
>>>>>>> With present name we pretend that all kbd-backlight controllers
>>>>>>> can change LED brightness autonomously.
>>>>>>>
>>>>>>> How about kbd-backlight-pollable ?
>>>>>>
>>>>>> This is a trigger to control kbd-backlights, in the
>>>>>> current use-case the brightness change is already done
>>>>>> by the firmware, hence the set_brightness argument to
>>>>>> ledtrig_kbd_backlight(), so that we can avoid setting
>>>>>> it again.
>>>>>>
>>>>>> But I can see future cases where we do want to have some
>>>>>> event (e.g. a wmi hotkey event on a laptop where the firmware
>>>>>> does not do the adjustment automatically) which does
>>>>>> lead to actually updating the brightness.
>>>>>>
>>>>>> So I decided to go with a generic kbd-backlight trigger,
>>>>>> which in the future can also be used to directly control
>>>>>> kbd-backlight brightness; and not just to make ot
>>>>>> poll-able.
>>>>>
>>>>> I thought that kbd-backlight stands for "keyboard backlight",
>>>>
>>>> It does.
>>>>
>>>>> that's why I assessed it is too generic.
>>>>
>>>> The whole purpose of the trigger as implemented is to be
>>>> generic, as it seems senseless to implement a one off
>>>> trigger for just the dell / thinkpad case.
>>>>
>>>>> It seems however to be
>>>>> the other way round - if kbd-backlight means that this is
>>>>> a trigger only for use with dell-laptop keyboard driver
>>>>> (I see kbd namespacing prefix in the driver functions) than it is
>>>>> not generic at all.
>>>>
>>>> The trigger as implemented is generic, if you think
>>>> otherwise, please let me know which part is not generic
>>>> according to you.
>>>
>>> I think I was too meticulous here. In the end of the previous
>>> message I mentioned that we cannot guarantee that all keyboard
>>> backlight controllers can adjust brightness autonomously.
>>> Nonetheless, for the ones that cannot do that it would make no sense
>>> to have a trigger. In view of that this trigger is generic enough.
>>>
>>> I'll wait for Pavel's opinion before merging the patch set
>>> as he was also involved in this whole thread.
>>
>> Do you mean me? Or Pavel Machek (CCed) who was involved in LEDs for input devices?
>
> I meant Pavel Machek, I haven't known that Pali is an equivalent of
> Pavel :-)
> Your opinion is very much appreciated though, thanks.
>
>> As pointed in other email, we do not know if HW really controls keyboard backlight,
>> so adding "fake" trigger on machines without HW control is not a good idea.
>>
>
> Yes, I had also similar doubts, but got almost convinced due to
> no objections. Now it becomes clear that we need to improve this
> feature.

But we are not adding such a fake trigger. We are only setting up the
kbd-backlight trigger on systems where there actually is hw-control.

Sure someone can do echo "kbd-backlight" > trigger to enable it,
but the same is true for e.g. "mtd" or "nand-disk" on systems
without an mtd-device / a nand-disk, and the result is the same,
the LED gets coupled to the trigger, but nothing ever triggers
the trigger.

I really believe that we have the right design now (I was skeptical
about the trigger approach at first, but it has turned out really
well) and unless Pavel Machek has any objections I would really like
patches 1-2 of this series merged.

Regards,

Hans


p.s.

Pali, I'm sorry that you don't like the LED side design, but there
has been a long discussion about this (which you apparently missed)
and this really is the best way forward.

Have you looked at what the new design means for the platform/x86
patches ? Gone is the ugly dell_laptop_notifier as the event
forwarding between dell-wmi and dell-laptop is now handles by
the led-trigger subsys.

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pali Rohár]

On Monday 21 November 2016 10:31:33 Hans de Goede wrote:
> Pali, I'm sorry that you don't like the LED side design, but there
> has been a long discussion about this (which you apparently missed)
> and this really is the best way forward.

Yea, I thought that I should have missed something as I was not able to
find all needed information about it in my mailbox.

Is that discussion somewhere logged/available? That could help to
describe and understand all problems hidden behind.

> Have you looked at what the new design means for the platform/x86
> patches ? Gone is the ugly dell_laptop_notifier as the event
> forwarding between dell-wmi and dell-laptop is now handles by
> the led-trigger subsys.

Yes, I saw that ugly dell_lpatop notifier is not there. If there is some
more information about decision and pro/coins about this approach I
would like to read it before.

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

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Hans de Goede]

Hi,

On 21-11-16 11:12, Pali Rohár wrote:
> On Monday 21 November 2016 10:31:33 Hans de Goede wrote:
>> Pali, I'm sorry that you don't like the LED side design, but there
>> has been a long discussion about this (which you apparently missed)
>> and this really is the best way forward.
>
> Yea, I thought that I should have missed something as I was not able to
> find all needed information about it in my mailbox.
>
> Is that discussion somewhere logged/available? That could help to
> describe and understand all problems hidden behind.
>
>> Have you looked at what the new design means for the platform/x86
>> patches ? Gone is the ugly dell_laptop_notifier as the event
>> forwarding between dell-wmi and dell-laptop is now handles by
>> the led-trigger subsys.
>
> Yes, I saw that ugly dell_lpatop notifier is not there. If there is some
> more information about decision and pro/coins about this approach I
> would like to read it before.

It is discussed in this thread:

https://www.spinics.net/lists/linux-leds/msg07049.html

Unfortunately the starter of the thread dropped the Cc: platform-driver-x86@vger.kernel.org
the original patch that thread is about had.

Regards,

Hans

Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Pavel Machek]

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

On Mon 2016-11-21 10:31:33, Hans de Goede wrote:
> Hi,
> 
> On 21-11-16 09:35, Jacek Anaszewski wrote:
> >On 11/20/2016 04:05 PM, Pali Rohár wrote:
> >>On Saturday 19 November 2016 16:44:09 Jacek Anaszewski wrote:
> >>>Hi,
> >>>
> >>>On 11/18/2016 07:47 PM, Hans de Goede wrote:
> >>>>HI,
> >>>>
> >>>>On 18-11-16 17:03, Jacek Anaszewski wrote:
> >>>>>Hi,
> >>>>>
> >>>>>On 11/18/2016 10:07 AM, Hans de Goede wrote:
> >>>>>>Hi,
> >>>>>>
> >>>>>>On 18-11-16 09:55, Jacek Anaszewski wrote:
> >>>>>>>Hi Hans,
> >>>>>>>
> >>>>>>>Thanks for the patch.
> >>>>>>>
> >>>>>>>I think we need less generic trigger name.
> >>>>>>>With present name we pretend that all kbd-backlight controllers
> >>>>>>>can change LED brightness autonomously.
> >>>>>>>
> >>>>>>>How about kbd-backlight-pollable ?
> >>>>>>
> >>>>>>This is a trigger to control kbd-backlights, in the
> >>>>>>current use-case the brightness change is already done
> >>>>>>by the firmware, hence the set_brightness argument to
> >>>>>>ledtrig_kbd_backlight(), so that we can avoid setting
> >>>>>>it again.
> >>>>>>
> >>>>>>But I can see future cases where we do want to have some
> >>>>>>event (e.g. a wmi hotkey event on a laptop where the firmware
> >>>>>>does not do the adjustment automatically) which does
> >>>>>>lead to actually updating the brightness.
> >>>>>>
> >>>>>>So I decided to go with a generic kbd-backlight trigger,
> >>>>>>which in the future can also be used to directly control
> >>>>>>kbd-backlight brightness; and not just to make ot
> >>>>>>poll-able.
> >>>>>
> >>>>>I thought that kbd-backlight stands for "keyboard backlight",
> >>>>
> >>>>It does.
> >>>>
> >>>>>that's why I assessed it is too generic.
> >>>>
> >>>>The whole purpose of the trigger as implemented is to be
> >>>>generic, as it seems senseless to implement a one off
> >>>>trigger for just the dell / thinkpad case.
> >>>>
> >>>>>It seems however to be
> >>>>>the other way round - if kbd-backlight means that this is
> >>>>>a trigger only for use with dell-laptop keyboard driver
> >>>>>(I see kbd namespacing prefix in the driver functions) than it is
> >>>>>not generic at all.
> >>>>
> >>>>The trigger as implemented is generic, if you think
> >>>>otherwise, please let me know which part is not generic
> >>>>according to you.
> >>>
> >>>I think I was too meticulous here. In the end of the previous
> >>>message I mentioned that we cannot guarantee that all keyboard
> >>>backlight controllers can adjust brightness autonomously.
> >>>Nonetheless, for the ones that cannot do that it would make no sense
> >>>to have a trigger. In view of that this trigger is generic enough.
> >>>
> >>>I'll wait for Pavel's opinion before merging the patch set
> >>>as he was also involved in this whole thread.
> >>
> >>Do you mean me? Or Pavel Machek (CCed) who was involved in LEDs for input devices?
> >
> >I meant Pavel Machek, I haven't known that Pali is an equivalent of
> >Pavel :-)
> >Your opinion is very much appreciated though, thanks.
> >
> >>As pointed in other email, we do not know if HW really controls keyboard backlight,
> >>so adding "fake" trigger on machines without HW control is not a good idea.
> >>
> >
> >Yes, I had also similar doubts, but got almost convinced due to
> >no objections. Now it becomes clear that we need to improve this
> >feature.
> 
> But we are not adding such a fake trigger. We are only setting up the
> kbd-backlight trigger on systems where there actually is hw-control.
> 
> Sure someone can do echo "kbd-backlight" > trigger to enable it,
> but the same is true for e.g. "mtd" or "nand-disk" on systems
> without an mtd-device / a nand-disk, and the result is the same,
> the LED gets coupled to the trigger, but nothing ever triggers
> the trigger.

We can do that, yes.

Alternatively, we could do some magic so that kbd-backlight trigger is
not available for other leds, and that _only_ kbd-backlight trigger is
available for leds that are always controlled by the hardware. But we
can do that in future patch...

> I really believe that we have the right design now (I was skeptical
> about the trigger approach at first, but it has turned out really
> well) and unless Pavel Machek has any objections I would really like
> patches 1-2 of this series merged.

I certainly like the interface from thedescription.

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

[PATCH v5 3/6] leds: triggers: Add support for read-only triggers

[Hans de Goede]

In some cases an LED is controlled through a hardwired (taken care of
in firmware outside of the kernels control) trigger.

Add an LED_TRIGGER_READ_ONLY flag for this and disallow user-space
changing the trigger when this flag is set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-This is a new patch in v5 of this patch-set
---
 drivers/leds/led-class.c    | 2 +-
 drivers/leds/led-triggers.c | 5 +++++
 include/linux/leds.h        | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 326ee6e..56f32cc 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -54,7 +54,7 @@ static ssize_t brightness_store(struct device *dev,
 	if (ret)
 		goto unlock;
 
-	if (state == LED_OFF)
+	if (state == LED_OFF && !(led_cdev->flags & LED_TRIGGER_READ_ONLY))
 		led_trigger_remove(led_cdev);
 	led_set_brightness(led_cdev, state);
 
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index d2ed9c2..9669104 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -37,6 +37,11 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
 	struct led_trigger *trig;
 	int ret = count;
 
+	if (led_cdev->flags & LED_TRIGGER_READ_ONLY) {
+		dev_err(led_cdev->dev, "Error this led triggers is hardwired and cannot be changed\n");
+		return -EINVAL;
+	}
+
 	mutex_lock(&led_cdev->led_access);
 
 	if (led_sysfs_is_disabled(led_cdev)) {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 870b8c2..e076b74 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -47,6 +47,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_TRIGGER_READ_ONLY   (1 << 21)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
-- 
2.9.3

Re: [PATCH v5 3/6] leds: triggers: Add support for read-only triggers

[Jacek Anaszewski]

Hi Hans,

Thanks for the new patch set.

On 11/17/2016 11:24 PM, Hans de Goede wrote:
> In some cases an LED is controlled through a hardwired (taken care of
> in firmware outside of the kernels control) trigger.
>
> Add an LED_TRIGGER_READ_ONLY flag for this and disallow user-space
> changing the trigger when this flag is set.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> -This is a new patch in v5 of this patch-set
> ---
>  drivers/leds/led-class.c    | 2 +-
>  drivers/leds/led-triggers.c | 5 +++++
>  include/linux/leds.h        | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 326ee6e..56f32cc 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -54,7 +54,7 @@ static ssize_t brightness_store(struct device *dev,
>  	if (ret)
>  		goto unlock;
>
> -	if (state == LED_OFF)
> +	if (state == LED_OFF && !(led_cdev->flags & LED_TRIGGER_READ_ONLY))
>  		led_trigger_remove(led_cdev);
>  	led_set_brightness(led_cdev, state);
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index d2ed9c2..9669104 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -37,6 +37,11 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>  	struct led_trigger *trig;
>  	int ret = count;
>
> +	if (led_cdev->flags & LED_TRIGGER_READ_ONLY) {
> +		dev_err(led_cdev->dev, "Error this led triggers is hardwired and cannot be changed\n");
> +		return -EINVAL;

It means that after a trigger is once assigned to a LED class device
it will be impossible to deactivate it. Why not leaving it to the
user's decision whether they want to have hw brightness changes
notifications? This way we disable the possibility to set different
trigger like e.g. timer, after this one is set, which is not
a non-realistic scenario.

Generally it is quite odd to add a functionality that once
set is latched. If one will set such a trigger by mistake, then
system restart will be required to reset this (unless the driver
is built as a module).

> +	}
> +
>  	mutex_lock(&led_cdev->led_access);
>
>  	if (led_sysfs_is_disabled(led_cdev)) {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 870b8c2..e076b74 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -47,6 +47,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_TRIGGER_READ_ONLY   (1 << 21)
>
>  	/* set_brightness_work / blink_timer flags, atomic, private. */
>  	unsigned long		work_flags;
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 3/6] leds: triggers: Add support for read-only triggers

[Hans de Goede]

Hi,

On 18-11-16 09:52, Jacek Anaszewski wrote:
> Hi Hans,
>
> Thanks for the new patch set.
>
> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>> In some cases an LED is controlled through a hardwired (taken care of
>> in firmware outside of the kernels control) trigger.
>>
>> Add an LED_TRIGGER_READ_ONLY flag for this and disallow user-space
>> changing the trigger when this flag is set.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v5:
>> -This is a new patch in v5 of this patch-set
>> ---
>>  drivers/leds/led-class.c    | 2 +-
>>  drivers/leds/led-triggers.c | 5 +++++
>>  include/linux/leds.h        | 1 +
>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 326ee6e..56f32cc 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -54,7 +54,7 @@ static ssize_t brightness_store(struct device *dev,
>>      if (ret)
>>          goto unlock;
>>
>> -    if (state == LED_OFF)
>> +    if (state == LED_OFF && !(led_cdev->flags & LED_TRIGGER_READ_ONLY))
>>          led_trigger_remove(led_cdev);
>>      led_set_brightness(led_cdev, state);
>>
>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>> index d2ed9c2..9669104 100644
>> --- a/drivers/leds/led-triggers.c
>> +++ b/drivers/leds/led-triggers.c
>> @@ -37,6 +37,11 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>>      struct led_trigger *trig;
>>      int ret = count;
>>
>> +    if (led_cdev->flags & LED_TRIGGER_READ_ONLY) {
>> +        dev_err(led_cdev->dev, "Error this led triggers is hardwired and cannot be changed\n");
>> +        return -EINVAL;
>
> It means that after a trigger is once assigned to a LED class device
> it will be impossible to deactivate it.

No this flag is not set by the trigger code, it is set by the LED
driver itself, to indicate there is a hardwired trigger.

> Why not leaving it to the
> user's decision whether they want to have hw brightness changes
> notifications?

The user cannot disable the hotkey -> keyboard-backlight-led link
and the trigger represents this link.

More over, if we allow changing the trigger, then writing 0
to the brightness attribute will remove the trigger and make
the device no longer poll-able, and writing 0 is exactly what
the systemd-backlight service does when restoring backlight
settings on boot and the kbd-backlight was off at the last
shutdown.

This again shows how poorly thought out the old "brightness"
file API is, all systemd-backlight want to do is set the
brightness to 0, but as a side effect it will also unlink the
trigger, because writing 0 has 2 effects for one system call.
Anyways this is not something we can fix.


> This way we disable the possibility to set different
> trigger like e.g. timer, after this one is set, which is not
> a non-realistic scenario.

Then we would have 2 triggers active, as the hotkey trigger
is part of the firmware of the laptop and is never going away.

> Generally it is quite odd to add a functionality that once
> set is latched. If one will set such a trigger by mistake, then
> system restart will be required to reset this (unless the driver
> is built as a module).

This is not under user-control, the is controlled by the
LED driver by setting the flag before registering the LED and
this is on purpose, because the trigger is hardwired.

TL;DR:

1) We've decided to model the hotkey -> kbd-backlight control link as a trigger
2) This is hardwired in the laptop's firmware therefor the trigger cannot
    be changed
3) Thus we need support for read-only triggers

Regards,

Hans



>
>> +    }
>> +
>>      mutex_lock(&led_cdev->led_access);
>>
>>      if (led_sysfs_is_disabled(led_cdev)) {
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 870b8c2..e076b74 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -47,6 +47,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_TRIGGER_READ_ONLY   (1 << 21)
>>
>>      /* set_brightness_work / blink_timer flags, atomic, private. */
>>      unsigned long        work_flags;
>>
>
>

Re: [PATCH v5 3/6] leds: triggers: Add support for read-only triggers

[Jacek Anaszewski]

Hi,

On 11/18/2016 10:04 AM, Hans de Goede wrote:
> Hi,
>
> On 18-11-16 09:52, Jacek Anaszewski wrote:
>> Hi Hans,
>>
>> Thanks for the new patch set.
>>
>> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>>> In some cases an LED is controlled through a hardwired (taken care of
>>> in firmware outside of the kernels control) trigger.
>>>
>>> Add an LED_TRIGGER_READ_ONLY flag for this and disallow user-space
>>> changing the trigger when this flag is set.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v5:
>>> -This is a new patch in v5 of this patch-set
>>> ---
>>>  drivers/leds/led-class.c    | 2 +-
>>>  drivers/leds/led-triggers.c | 5 +++++
>>>  include/linux/leds.h        | 1 +
>>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 326ee6e..56f32cc 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -54,7 +54,7 @@ static ssize_t brightness_store(struct device *dev,
>>>      if (ret)
>>>          goto unlock;
>>>
>>> -    if (state == LED_OFF)
>>> +    if (state == LED_OFF && !(led_cdev->flags & LED_TRIGGER_READ_ONLY))
>>>          led_trigger_remove(led_cdev);
>>>      led_set_brightness(led_cdev, state);
>>>
>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>> index d2ed9c2..9669104 100644
>>> --- a/drivers/leds/led-triggers.c
>>> +++ b/drivers/leds/led-triggers.c
>>> @@ -37,6 +37,11 @@ ssize_t led_trigger_store(struct device *dev,
>>> struct device_attribute *attr,
>>>      struct led_trigger *trig;
>>>      int ret = count;
>>>
>>> +    if (led_cdev->flags & LED_TRIGGER_READ_ONLY) {
>>> +        dev_err(led_cdev->dev, "Error this led triggers is hardwired
>>> and cannot be changed\n");
>>> +        return -EINVAL;
>>
>> It means that after a trigger is once assigned to a LED class device
>> it will be impossible to deactivate it.
>
> No this flag is not set by the trigger code, it is set by the LED
> driver itself, to indicate there is a hardwired trigger.
>
>> Why not leaving it to the
>> user's decision whether they want to have hw brightness changes
>> notifications?
>
> The user cannot disable the hotkey -> keyboard-backlight-led link
> and the trigger represents this link.
>
> More over, if we allow changing the trigger, then writing 0
> to the brightness attribute will remove the trigger and make
> the device no longer poll-able, and writing 0 is exactly what
> the systemd-backlight service does when restoring backlight
> settings on boot and the kbd-backlight was off at the last
> shutdown.
>
> This again shows how poorly thought out the old "brightness"
> file API is, all systemd-backlight want to do is set the
> brightness to 0, but as a side effect it will also unlink the
> trigger, because writing 0 has 2 effects for one system call.
> Anyways this is not something we can fix.

So we will need to fix that. Maybe we should make it configurable.
E.g. a sysfs file could define whether writing 0 to brightness file
clears a trigger.

Trigger can already be disabled by writing "none" to triggers file.

>> This way we disable the possibility to set different
>> trigger like e.g. timer, after this one is set, which is not
>> a non-realistic scenario.
>
> Then we would have 2 triggers active, as the hotkey trigger
> is part of the firmware of the laptop and is never going away.

Having this type of hardware trigger reflected in the LED class
device configuration all the time is not something critical I think.
More important is leaving a possibility of applying other existing
sources of kernel events to the LED controller.

>> Generally it is quite odd to add a functionality that once
>> set is latched. If one will set such a trigger by mistake, then
>> system restart will be required to reset this (unless the driver
>> is built as a module).
>
> This is not under user-control, the is controlled by the
> LED driver by setting the flag before registering the LED and
> this is on purpose, because the trigger is hardwired.
>
> TL;DR:
>
> 1) We've decided to model the hotkey -> kbd-backlight control link as a
> trigger
> 2) This is hardwired in the laptop's firmware therefor the trigger cannot
>    be changed
> 3) Thus we need support for read-only triggers
>
> Regards,
>
> Hans
>
>
>
>>
>>> +    }
>>> +
>>>      mutex_lock(&led_cdev->led_access);
>>>
>>>      if (led_sysfs_is_disabled(led_cdev)) {
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index 870b8c2..e076b74 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -47,6 +47,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_TRIGGER_READ_ONLY   (1 << 21)
>>>
>>>      /* set_brightness_work / blink_timer flags, atomic, private. */
>>>      unsigned long        work_flags;
>>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 3/6] leds: triggers: Add support for read-only triggers

[Hans de Goede]

Hi,

On 18-11-16 11:49, Jacek Anaszewski wrote:
> Hi,
>
> On 11/18/2016 10:04 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 18-11-16 09:52, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
>>> Thanks for the new patch set.
>>>
>>> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>>>> In some cases an LED is controlled through a hardwired (taken care of
>>>> in firmware outside of the kernels control) trigger.
>>>>
>>>> Add an LED_TRIGGER_READ_ONLY flag for this and disallow user-space
>>>> changing the trigger when this flag is set.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v5:
>>>> -This is a new patch in v5 of this patch-set
>>>> ---
>>>>  drivers/leds/led-class.c    | 2 +-
>>>>  drivers/leds/led-triggers.c | 5 +++++
>>>>  include/linux/leds.h        | 1 +
>>>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>>> index 326ee6e..56f32cc 100644
>>>> --- a/drivers/leds/led-class.c
>>>> +++ b/drivers/leds/led-class.c
>>>> @@ -54,7 +54,7 @@ static ssize_t brightness_store(struct device *dev,
>>>>      if (ret)
>>>>          goto unlock;
>>>>
>>>> -    if (state == LED_OFF)
>>>> +    if (state == LED_OFF && !(led_cdev->flags & LED_TRIGGER_READ_ONLY))
>>>>          led_trigger_remove(led_cdev);
>>>>      led_set_brightness(led_cdev, state);
>>>>
>>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>>> index d2ed9c2..9669104 100644
>>>> --- a/drivers/leds/led-triggers.c
>>>> +++ b/drivers/leds/led-triggers.c
>>>> @@ -37,6 +37,11 @@ ssize_t led_trigger_store(struct device *dev,
>>>> struct device_attribute *attr,
>>>>      struct led_trigger *trig;
>>>>      int ret = count;
>>>>
>>>> +    if (led_cdev->flags & LED_TRIGGER_READ_ONLY) {
>>>> +        dev_err(led_cdev->dev, "Error this led triggers is hardwired
>>>> and cannot be changed\n");
>>>> +        return -EINVAL;
>>>
>>> It means that after a trigger is once assigned to a LED class device
>>> it will be impossible to deactivate it.
>>
>> No this flag is not set by the trigger code, it is set by the LED
>> driver itself, to indicate there is a hardwired trigger.
>>
>>> Why not leaving it to the
>>> user's decision whether they want to have hw brightness changes
>>> notifications?
>>
>> The user cannot disable the hotkey -> keyboard-backlight-led link
>> and the trigger represents this link.
>>
>> More over, if we allow changing the trigger, then writing 0
>> to the brightness attribute will remove the trigger and make
>> the device no longer poll-able, and writing 0 is exactly what
>> the systemd-backlight service does when restoring backlight
>> settings on boot and the kbd-backlight was off at the last
>> shutdown.
>>
>> This again shows how poorly thought out the old "brightness"
>> file API is, all systemd-backlight want to do is set the
>> brightness to 0, but as a side effect it will also unlink the
>> trigger, because writing 0 has 2 effects for one system call.
>> Anyways this is not something we can fix.
>
> So we will need to fix that. Maybe we should make it configurable.
> E.g. a sysfs file could define whether writing 0 to brightness file
> clears a trigger.

Hmm, I don't like adding a sysfs file which changes behavior of
the "brightness" file, I think that we can work around this,
see below.

> Trigger can already be disabled by writing "none" to triggers file.
>
>>> This way we disable the possibility to set different
>>> trigger like e.g. timer, after this one is set, which is not
>>> a non-realistic scenario.
>>
>> Then we would have 2 triggers active, as the hotkey trigger
>> is part of the firmware of the laptop and is never going away.
>
> Having this type of hardware trigger reflected in the LED class
> device configuration all the time is not something critical I think.
> More important is leaving a possibility of applying other existing
> sources of kernel events to the LED controller.

Ok, I've created a systemd-backlight patch to prefer
current_brightness when present, avoiding the problem of
systemd-backlight removing the kbd-backlight trigger on boot
when it is restoring a brightness setting of 0.

That fixes the immediate problem, without needing to break /
hack the old "brightness" ABI.

So if you want feel free to drop this patch from the series
and just apply patch 1 and 2, Once merged I'll send a v6 of
the platform driver patches to match the dropping of this
patch.

Regards,

Hans




>
>>> Generally it is quite odd to add a functionality that once
>>> set is latched. If one will set such a trigger by mistake, then
>>> system restart will be required to reset this (unless the driver
>>> is built as a module).
>>
>> This is not under user-control, the is controlled by the
>> LED driver by setting the flag before registering the LED and
>> this is on purpose, because the trigger is hardwired.
>>
>> TL;DR:
>>
>> 1) We've decided to model the hotkey -> kbd-backlight control link as a
>> trigger
>> 2) This is hardwired in the laptop's firmware therefor the trigger cannot
>>    be changed
>> 3) Thus we need support for read-only triggers
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>
>>>> +    }
>>>> +
>>>>      mutex_lock(&led_cdev->led_access);
>>>>
>>>>      if (led_sysfs_is_disabled(led_cdev)) {
>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>> index 870b8c2..e076b74 100644
>>>> --- a/include/linux/leds.h
>>>> +++ b/include/linux/leds.h
>>>> @@ -47,6 +47,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_TRIGGER_READ_ONLY   (1 << 21)
>>>>
>>>>      /* set_brightness_work / blink_timer flags, atomic, private. */
>>>>      unsigned long        work_flags;
>>>>
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
>

[PATCH v5 4/6] platform: x86: thinkpad: Call led kbd_backlight trigger on kbd brightness change

[Hans de Goede]

Set the default-trigger of the thinklight and keyboard-backlight LEDs
to "kbd-backlight" and call ledtrig_kbd_backlight() when the hotkey
for controlling the thinklight and/or the keyboard-backlight gets pressed.

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

Note this also sets the LED_TRIGGER_READ_ONLY flag since the hotkey
is hardwired to control the brightness.

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

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index b65ce75..066ad20 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_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 */
@@ -1950,7 +1951,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,
@@ -2320,7 +2321,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);
 	}
@@ -5138,8 +5139,10 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 	.led_classdev = {
 		.name		= "tpacpi::kbd_backlight",
 		.max_brightness	= 2,
+		.flags		= LED_TRIGGER_READ_ONLY,
 		.brightness_set	= &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
+		.default_trigger = "kbd-backlight",
 	}
 };
 
@@ -5167,6 +5170,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;
 }
 
@@ -5332,8 +5337,10 @@ static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
 static struct tpacpi_led_classdev tpacpi_led_thinklight = {
 	.led_classdev = {
 		.name		= "tpacpi::thinklight",
+		.flags		= LED_TRIGGER_READ_ONLY,
 		.brightness_set	= &light_sysfs_set,
 		.brightness_get	= &light_sysfs_get,
+		.default_trigger = "kbd-backlight",
 	}
 };
 
@@ -5372,11 +5379,12 @@ static int __init light_init(struct ibm_init_struct *iibm)
 	if (rc < 0) {
 		tp_features.light = 0;
 		tp_features.light_status = 0;
-	} else  {
-		rc = 0;
+		return rc;
 	}
 
-	return rc;
+	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask |
+				      TP_ACPI_HKEY_KBD_LIGHT_MASK);
+	return 0;
 }
 
 static void light_exit(void)
@@ -9114,6 +9122,10 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 			volume_alsa_notify_change();
 		}
 	}
+	if (hkey_event == TP_HKEY_EV_KBD_LIGHT) {
+		/* set_brightness = false, already handled by firmware */
+		ledtrig_kbd_backlight(false, 0);
+	}
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
-- 
2.9.3

[PATCH v5 5/6] platform: x86: dell-laptop: Set keyboard backlight led device default trigger

[Hans de Goede]

Set kbd_led.default_trigger to "kbd-backlight", this will allow user
space to be notified of brightness changes through
ledtrig_kbd_backlight().

Note this also sets the LED_TRIGGER_READ_ONLY flag since the brightness
is hardwired to be controlled by the kbd-backlight hotkey.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-This is a new patch in v5 of this patch-set
---
 drivers/platform/x86/dell-laptop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 14392a0..6c0f047 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1937,9 +1937,11 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 
 static struct led_classdev kbd_led = {
 	.name           = "dell::kbd_backlight",
+	.flags		= LED_TRIGGER_READ_ONLY,
 	.brightness_set_blocking = kbd_led_level_set,
 	.brightness_get = kbd_led_level_get,
 	.groups         = kbd_led_groups,
+	.default_trigger = "kbd-backlight",
 };
 
 static int __init kbd_led_init(struct device *dev)
-- 
2.9.3

[PATCH v5 6/6] platform: x86: dell-wmi: Call led kbd_backlight trigger on kbd brightness change

[Hans de Goede]

Make dell-wmi call ledtrig_kbd_backlight() when the keyboard-backlight
brightness changes.

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.
-This also drops the need for the custom dell_laptop_*_notifier code
---
 drivers/platform/x86/dell-wmi.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index da2fe18..64b5035 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -36,6 +36,7 @@
 #include <linux/acpi.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
+#include <linux/leds.h>
 #include <acpi/video.h>
 #include "dell-smbios.h"
 
@@ -287,11 +288,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 +320,11 @@ 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) {
+		/* set_brightness = false, already handled by firmware */
+		ledtrig_kbd_backlight(false, 0);
+	}
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
2.9.3

Re: [PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Pali Rohár]

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

On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> In some cases it may be desirable for userspace to be notified when
> a trigger event happens. This commit adds support for a poll-able
> current_brightness trigger specific sysfs attribute which triggers
> may register:
> 
> What:		/sys/class/leds/<led>/current_brightness
> Date:		November 2016
> KernelVersion:	4.10
> Description:
> 	Triggers which support it may register a current_brightness
> 	file. This file supports poll() to detect when the trigger
> 	modifies the brightness of the LED.
> 	Reading this file will always return the current brightness
> 	of the LED.
> 	Writing this file sets the current brightness of the LED,
> 	without influencing the trigger.

Personally I do not like this new sysfs attribute...

Now when somebody look at /sys/class/leds/<something>/, the first thing 
which say would be:

"What the hell, why there are two files (brightness and 
current_brightness) for changing LED level? And which should I use?"

If I understood correctly we need to handle two things:

1) Provide poll() for userspace when LED level is changed (either by HW
   or other user call)

2) Deal with fact that on _some_ hardware, special key is hardwired to
   change LED level

So why for 1) we cannot use existing sysfs file "brightness"? I do not 
see any problem with it.

And for 2) we even do not know on which machines is such hardwired 
feature enabled. Yes, on _some_ (not *all*) Dell machines there is Fn 
key combination which changes level of one LED device. But kernel does 
not know if hardware on which is running is doing such thing or not. 
Some machines do not have to have key for such action and we do not know 
it.

And what about situation when somebody wants to configure e.g. mouse 
movement (or keypress) trigger to enable/disable LED device (which 
belongs to keyboard brightness)? In this case user explicitly know that 
his Fn+Space change level of LED device, so can be careful to not press 
it. With your read-only trigger you basically disable such (I think 
useful) feature.

-- 
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 v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Hans de Goede]

Hi,

On 20-11-16 15:59, Pali Rohár wrote:
> On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
>> In some cases it may be desirable for userspace to be notified when
>> a trigger event happens. This commit adds support for a poll-able
>> current_brightness trigger specific sysfs attribute which triggers
>> may register:
>>
>> What:		/sys/class/leds/<led>/current_brightness
>> Date:		November 2016
>> KernelVersion:	4.10
>> Description:
>> 	Triggers which support it may register a current_brightness
>> 	file. This file supports poll() to detect when the trigger
>> 	modifies the brightness of the LED.
>> 	Reading this file will always return the current brightness
>> 	of the LED.
>> 	Writing this file sets the current brightness of the LED,
>> 	without influencing the trigger.
>
> Personally I do not like this new sysfs attribute...
>
> Now when somebody look at /sys/class/leds/<something>/, the first thing
> which say would be:
>
> "What the hell, why there are two files (brightness and
> current_brightness) for changing LED level? And which should I use?"
>
> If I understood correctly we need to handle two things:
>
> 1) Provide poll() for userspace when LED level is changed (either by HW
>    or other user call)
 >
> 2) Deal with fact that on _some_ hardware, special key is hardwired to
>    change LED level
>
> So why for 1) we cannot use existing sysfs file "brightness"? I do not
> see any problem with it.

That was our first attempt at this, but because the brightness may also
be changed by triggers / blink-timers, we need to wakeup poll() in those
cases too (anything else would be inconsistent) and doing such a wakeup
in that case has turned out to cause too much overhead in some cases
(even if userspace is not listening), specifically the idle power uses
on some systems got multiplied by a factor of 5 or more.

So this approach was rejected.

> And for 2) we even do not know on which machines is such hardwired
> feature enabled. Yes, on _some_ (not *all*) Dell machines there is Fn
> key combination which changes level of one LED device. But kernel does
> not know if hardware on which is running is doing such thing or not.
> Some machines do not have to have key for such action and we do not know
> it.
>
> And what about situation when somebody wants to configure e.g. mouse
> movement (or keypress) trigger to enable/disable LED device (which
> belongs to keyboard brightness)? In this case user explicitly know that
> his Fn+Space change level of LED device, so can be careful to not press
> it. With your read-only trigger you basically disable such (I think
> useful) feature.

This has already been discussed and the third patch in the set, which is
the one making the trigger read-only has been dropped.

Regards,

Hans

Re: [PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Pali Rohár]

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

On Sunday 20 November 2016 19:45:40 Hans de Goede wrote:
> Hi,
> 
> On 20-11-16 15:59, Pali Rohár wrote:
> > On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> >> In some cases it may be desirable for userspace to be notified
> >> when a trigger event happens. This commit adds support for a
> >> poll-able current_brightness trigger specific sysfs attribute
> >> which triggers may register:
> >> 
> >> What:		/sys/class/leds/<led>/current_brightness
> >> Date:		November 2016
> >> KernelVersion:	4.10
> >> 
> >> Description:
> >> 	Triggers which support it may register a current_brightness
> >> 	file. This file supports poll() to detect when the trigger
> >> 	modifies the brightness of the LED.
> >> 	Reading this file will always return the current brightness
> >> 	of the LED.
> >> 	Writing this file sets the current brightness of the LED,
> >> 	without influencing the trigger.
> > 
> > Personally I do not like this new sysfs attribute...
> > 
> > Now when somebody look at /sys/class/leds/<something>/, the first
> > thing which say would be:
> > 
> > "What the hell, why there are two files (brightness and
> > current_brightness) for changing LED level? And which should I
> > use?"
> > 
> > If I understood correctly we need to handle two things:
> > 
> > 1) Provide poll() for userspace when LED level is changed (either
> > by HW
> > 
> >    or other user call)
> > 
> > 2) Deal with fact that on _some_ hardware, special key is hardwired
> > to
> > 
> >    change LED level
> > 
> > So why for 1) we cannot use existing sysfs file "brightness"? I do
> > not see any problem with it.
> 
> That was our first attempt at this, but because the brightness may
> also be changed by triggers / blink-timers, we need to wakeup poll()
> in those cases too (anything else would be inconsistent) and doing
> such a wakeup in that case has turned out to cause too much overhead
> in some cases (even if userspace is not listening), specifically the
> idle power uses on some systems got multiplied by a factor of 5 or
> more.
> 
> So this approach was rejected.

But approach with exporting new sysfs file with name current_brightness 
with existence of old brightness sysfs file is not good and in my 
opinion it is even worst as current situation (= without poll support).

What happen in next 5 years? Somebody point that sysfs file "brightness" 
and sysfs file "current_brightness" is still not good and invent 
"really_current_brightness" sysfs with new logic? No this is really not 
a way...

I understand that extending current "brightness" sysfs is complicated, 
but it is not a reason to not doing it and inventing new crippling sysfs 
file which duplicate existing one (with some modifications).

Anyway, I cannot believe that power usage is increased by factor 5 with 
exporting poll support. If we are going to change brightness level 
(either by trigger or timer) we still need to do wakeup.

And we can do some optimizations, e.g. do not send poll event when 
nobody is listening or postpone event so we do not send too many events 
in 1s interval (or choose longer interval).

Polling support is not in mainline kernel yet, so we can change its 
behaviour without breaking ABI. And we can define (if it help us!) that 
evens can be send to userspace with some delay (e.g. 1-3s).

-- 
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 v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Pavel Machek]

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

Hi!

> > > If I understood correctly we need to handle two things:
> > > 
> > > 1) Provide poll() for userspace when LED level is changed (either
> > > by HW
> > > 
> > >    or other user call)
> > > 
> > > 2) Deal with fact that on _some_ hardware, special key is hardwired
> > > to
> > > 
> > >    change LED level
> > > 
> > > So why for 1) we cannot use existing sysfs file "brightness"? I do
> > > not see any problem with it.
> > 
> > That was our first attempt at this, but because the brightness may
> > also be changed by triggers / blink-timers, we need to wakeup poll()
> > in those cases too (anything else would be inconsistent) and doing
> > such a wakeup in that case has turned out to cause too much overhead
> > in some cases (even if userspace is not listening), specifically the
> > idle power uses on some systems got multiplied by a factor of 5 or
> > more.
> > 
> > So this approach was rejected.
> 
> But approach with exporting new sysfs file with name current_brightness 
> with existence of old brightness sysfs file is not good and in my 
> opinion it is even worst as current situation (= without poll
> support).

It is neccessary, see the current discussion.

> What happen in next 5 years? Somebody point that sysfs file "brightness" 

current_brightness will only appear on machines and in situations,
where we, well, can report current brightness. When you read
"brightness" file, you don't know if you are getting current
brightness or not, and it can't be changed easily.

Please go through the discussion. This design was chosen for a reason.

Thanks,
									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 v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Pali Rohár]

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

On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> In some cases it may be desirable for userspace to be notified when
> a trigger event happens. This commit adds support for a poll-able
> current_brightness trigger specific sysfs attribute which triggers
> may register:
> 
> What:		/sys/class/leds/<led>/current_brightness

What about name actual_brightness? That name is already used by 
/sys/class/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: [PATCH v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Pavel Machek]

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

On Mon 2016-11-21 00:07:35, Pali Rohár wrote:
> On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> > In some cases it may be desirable for userspace to be notified when
> > a trigger event happens. This commit adds support for a poll-able
> > current_brightness trigger specific sysfs attribute which triggers
> > may register:
> > 
> > What:		/sys/class/leds/<led>/current_brightness
> 
> What about name actual_brightness? That name is already used by 
> /sys/class/backlight/...

Is that a problem?
									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 v5 1/6] leds: triggers: Add current_brightness trigger parameter

[Pali Rohár]

On Monday 21 November 2016 00:48:42 Pavel Machek wrote:
> On Mon 2016-11-21 00:07:35, Pali Rohár wrote:
> > On Thursday 17 November 2016 23:24:36 Hans de Goede wrote:
> > > In some cases it may be desirable for userspace to be notified when
> > > a trigger event happens. This commit adds support for a poll-able
> > > current_brightness trigger specific sysfs attribute which triggers
> > > may register:
> > > 
> > > What:		/sys/class/leds/<led>/current_brightness
> > 
> > What about name actual_brightness? That name is already used by 
> > /sys/class/backlight/...
> 
> Is that a problem?

Is not better to have same name?

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