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

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

[Hans de Goede]

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

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

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

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

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

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

[PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Hans de Goede]

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

This commits add 3 generic functions:
dell_smbios_register_notifier()
dell_smbios_unregister_notifier()
dell_smbios_call_notifier()

It currently only defines 1 action:
dell_smbios_kbd_backlight_brightness_changed

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h | 11 +++++++++++
 2 files changed, 31 insertions(+)

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

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Pali Rohár]

On Sunday 23 October 2016 21:46:50 Hans de Goede wrote:
> There are several cases where events handled in one of the dell-* drivers
> need to be propagated to another dell-* driver.
> 
> This commits add 3 generic functions:
> dell_smbios_register_notifier()
> dell_smbios_unregister_notifier()
> dell_smbios_call_notifier()
> 
> It currently only defines 1 action:
> dell_smbios_kbd_backlight_brightness_changed
> 
> Which is intended to propagate kbd_backlight_brightness_changed wmi
> events from dell-wmi to dell-laptop (which contains the actual kbd
> backlight driver).
> 
> This is only somewhat dell smbios related, both dell-wmi and dell-laptop
> use smbios functions and I do not want to put the notifier head in
> either driver, as that will make the 2 drivers depend on each other.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h | 11 +++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..8b96bdf 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_find_token);
>  
> +static ATOMIC_NOTIFIER_HEAD(dell_smbios_chain_head);
> +
> +int dell_smbios_register_notifier(struct notifier_block *nb)
> +{
> +	return atomic_notifier_chain_register(&dell_smbios_chain_head, nb);
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_register_notifier);
> +
> +int dell_smbios_unregister_notifier(struct notifier_block *nb)
> +{
> +	return atomic_notifier_chain_unregister(&dell_smbios_chain_head, nb);
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_unregister_notifier);
> +
> +void dell_smbios_call_notifier(unsigned long action, void *data)
> +{
> +	atomic_notifier_call_chain(&dell_smbios_chain_head, action, data);
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_call_notifier);
> +
>  static void __init parse_da_table(const struct dmi_header *dm)
>  {
>  	/* Final token is a terminator, so we don't want to copy it */
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index ec7d40a..e91f13f 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -16,6 +16,8 @@
>  #ifndef _DELL_SMBIOS_H_
>  #define _DELL_SMBIOS_H_
>  
> +struct notifier_block;
> +
>  /* This structure will be modified by the firmware when we enter
>   * system management mode, hence the volatiles */
>  
> @@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
>  void dell_smbios_send_request(int class, int select);
>  
>  struct calling_interface_token *dell_smbios_find_token(int tokenid);
> +
> +enum dell_smbios_notifier_actions {
> +	dell_smbios_kbd_backlight_brightness_changed,
> +};
> +
> +int dell_smbios_register_notifier(struct notifier_block *nb);
> +int dell_smbios_unregister_notifier(struct notifier_block *nb);
> +void dell_smbios_call_notifier(unsigned long action, void *data);
> +
>  #endif

That is better but technically speaking those notifiers do not have
nothing common with dell-smbios :-( Driver dell-smbios.ko provides API
for sending SMBIOS requests via SMM. Maybe move those functions into
separate module (and built statically into kernel)? Just speculation, do
not have something useful in my mind.

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

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Hans de Goede]

Hi,

On 24-10-16 15:31, Pali Rohár wrote:
> On Sunday 23 October 2016 21:46:50 Hans de Goede wrote:
>> There are several cases where events handled in one of the dell-* drivers
>> need to be propagated to another dell-* driver.
>>
>> This commits add 3 generic functions:
>> dell_smbios_register_notifier()
>> dell_smbios_unregister_notifier()
>> dell_smbios_call_notifier()
>>
>> It currently only defines 1 action:
>> dell_smbios_kbd_backlight_brightness_changed
>>
>> Which is intended to propagate kbd_backlight_brightness_changed wmi
>> events from dell-wmi to dell-laptop (which contains the actual kbd
>> backlight driver).
>>
>> This is only somewhat dell smbios related, both dell-wmi and dell-laptop
>> use smbios functions and I do not want to put the notifier head in
>> either driver, as that will make the 2 drivers depend on each other.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>  drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
>>  drivers/platform/x86/dell-smbios.h | 11 +++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
>> index d2412ab..8b96bdf 100644
>> --- a/drivers/platform/x86/dell-smbios.c
>> +++ b/drivers/platform/x86/dell-smbios.c
>> @@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
>>  }
>>  EXPORT_SYMBOL_GPL(dell_smbios_find_token);
>>
>> +static ATOMIC_NOTIFIER_HEAD(dell_smbios_chain_head);
>> +
>> +int dell_smbios_register_notifier(struct notifier_block *nb)
>> +{
>> +	return atomic_notifier_chain_register(&dell_smbios_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(dell_smbios_register_notifier);
>> +
>> +int dell_smbios_unregister_notifier(struct notifier_block *nb)
>> +{
>> +	return atomic_notifier_chain_unregister(&dell_smbios_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(dell_smbios_unregister_notifier);
>> +
>> +void dell_smbios_call_notifier(unsigned long action, void *data)
>> +{
>> +	atomic_notifier_call_chain(&dell_smbios_chain_head, action, data);
>> +}
>> +EXPORT_SYMBOL_GPL(dell_smbios_call_notifier);
>> +
>>  static void __init parse_da_table(const struct dmi_header *dm)
>>  {
>>  	/* Final token is a terminator, so we don't want to copy it */
>> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
>> index ec7d40a..e91f13f 100644
>> --- a/drivers/platform/x86/dell-smbios.h
>> +++ b/drivers/platform/x86/dell-smbios.h
>> @@ -16,6 +16,8 @@
>>  #ifndef _DELL_SMBIOS_H_
>>  #define _DELL_SMBIOS_H_
>>
>> +struct notifier_block;
>> +
>>  /* This structure will be modified by the firmware when we enter
>>   * system management mode, hence the volatiles */
>>
>> @@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
>>  void dell_smbios_send_request(int class, int select);
>>
>>  struct calling_interface_token *dell_smbios_find_token(int tokenid);
>> +
>> +enum dell_smbios_notifier_actions {
>> +	dell_smbios_kbd_backlight_brightness_changed,
>> +};
>> +
>> +int dell_smbios_register_notifier(struct notifier_block *nb);
>> +int dell_smbios_unregister_notifier(struct notifier_block *nb);
>> +void dell_smbios_call_notifier(unsigned long action, void *data);
>> +
>>  #endif
>
> That is better but technically speaking those notifiers do not have
> nothing common with dell-smbios :-(

Well WMI events get enabled via a SMBIOS call, and dell-laptop uses
SMBIOS exclusively, so it seems to fit. Basically this is a case of
we have to put this somewhere and dell-smbios is the best fit IMHO.

Regards,

Hans

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Pali Rohár]

On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
> Well WMI events get enabled via a SMBIOS call,

This is truth only for few laptops and only for one WMI event.
Everything else is automatically enabled, no call is needed to issue.

> and dell-laptop uses SMBIOS exclusively

IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
dell-laptop.

> so it seems to fit. Basically this is a case of
> we have to put this somewhere and dell-smbios is the best fit IMHO.

Agree, we need to put it somewhere...

Basically we need to solve problem how (currently) 3 kernel modules can
communicate. Does not kernel support such "bus/event" mechanism for
this?

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

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Hans de Goede]

Hi,

On 24-10-16 15:43, Pali Rohár wrote:
> On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
>> Well WMI events get enabled via a SMBIOS call,
>
> This is truth only for few laptops and only for one WMI event.
> Everything else is automatically enabled, no call is needed to issue.
>
>> and dell-laptop uses SMBIOS exclusively
>
> IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
> dell-laptop.
>
>> so it seems to fit. Basically this is a case of
>> we have to put this somewhere and dell-smbios is the best fit IMHO.
>
> Agree, we need to put it somewhere...
>
> Basically we need to solve problem how (currently) 3 kernel modules can
> communicate. Does not kernel support such "bus/event" mechanism for
> this?

Yes it does, that is exactly what notifiers are for, but we need to
declare the bus somewhere. I still believe dell-smbios is the best
place.

Regards,

Hans

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Pali Rohár]

On Monday 24 October 2016 15:45:02 Hans de Goede wrote:
> Hi,
> 
> On 24-10-16 15:43, Pali Rohár wrote:
> >On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
> >>Well WMI events get enabled via a SMBIOS call,
> >
> >This is truth only for few laptops and only for one WMI event.
> >Everything else is automatically enabled, no call is needed to issue.
> >
> >>and dell-laptop uses SMBIOS exclusively
> >
> >IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
> >dell-laptop.
> >
> >>so it seems to fit. Basically this is a case of
> >>we have to put this somewhere and dell-smbios is the best fit IMHO.
> >
> >Agree, we need to put it somewhere...
> >
> >Basically we need to solve problem how (currently) 3 kernel modules can
> >communicate. Does not kernel support such "bus/event" mechanism for
> >this?
> 
> Yes it does, that is exactly what notifiers are for, but we need to
> declare the bus somewhere. I still believe dell-smbios is the best
> place.

But dell_smbios_register_notifier() name is totally confusing. It does
not register any notifier for SMBIOS. Nor it have nothing common with
SMBIOS API.

Also there is absolutely no need that dell-rbtn.ko needs to depends on
dell-smbios.ko. dell-rbtn.ko is ACPI driver which does not use any of
SMBIOS API.

Right now I'm not saying what is the best place for that notifier (as I
still do not have ideal candidate). I'm just saying that notifier is not
part of SMBIOS API and therefore dell-smbios.ko is not right place for
it.

So currently we have these different APIs for dell notebook drivers:
* ACPI (used in dell-rbtn.ko and dell-smo8800.ko)
* WMI (used in dell-wmi.ko, dell-wmi-aio.ko, dell-led.ko)
* SMBIOS (used in dell-laptop.ko, dell-wmi.ko and dell-led.ko)
* some other platform code (used in dell-laptop.ko)

And now notifier is needed for drivers:
* dell-laptop.ko
* dell-wmi.ko
* dell-rbtn.ko

And if I look at above two sets, none of above drivers is good candidate
for central notifier functions... Maybe we should really introduce new
separate file where will central dell notifier live?

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

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Hans de Goede]

HI,

On 27-10-16 12:32, Pali Rohár wrote:
> On Monday 24 October 2016 15:45:02 Hans de Goede wrote:
>> Hi,
>>
>> On 24-10-16 15:43, Pali Rohár wrote:
>>> On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
>>>> Well WMI events get enabled via a SMBIOS call,
>>>
>>> This is truth only for few laptops and only for one WMI event.
>>> Everything else is automatically enabled, no call is needed to issue.
>>>
>>>> and dell-laptop uses SMBIOS exclusively
>>>
>>> IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
>>> dell-laptop.
>>>
>>>> so it seems to fit. Basically this is a case of
>>>> we have to put this somewhere and dell-smbios is the best fit IMHO.
>>>
>>> Agree, we need to put it somewhere...
>>>
>>> Basically we need to solve problem how (currently) 3 kernel modules can
>>> communicate. Does not kernel support such "bus/event" mechanism for
>>> this?
>>
>> Yes it does, that is exactly what notifiers are for, but we need to
>> declare the bus somewhere. I still believe dell-smbios is the best
>> place.
>
> But dell_smbios_register_notifier() name is totally confusing. It does
> not register any notifier for SMBIOS. Nor it have nothing common with
> SMBIOS API.
>
> Also there is absolutely no need that dell-rbtn.ko needs to depends on
> dell-smbios.ko. dell-rbtn.ko is ACPI driver which does not use any of
> SMBIOS API.
>
> Right now I'm not saying what is the best place for that notifier (as I
> still do not have ideal candidate). I'm just saying that notifier is not
> part of SMBIOS API and therefore dell-smbios.ko is not right place for
> it.
>
> So currently we have these different APIs for dell notebook drivers:
> * ACPI (used in dell-rbtn.ko and dell-smo8800.ko)
> * WMI (used in dell-wmi.ko, dell-wmi-aio.ko, dell-led.ko)
> * SMBIOS (used in dell-laptop.ko, dell-wmi.ko and dell-led.ko)
> * some other platform code (used in dell-laptop.ko)
>
> And now notifier is needed for drivers:
> * dell-laptop.ko
> * dell-wmi.ko
> * dell-rbtn.ko
>
> And if I look at above two sets, none of above drivers is good candidate
> for central notifier functions... Maybe we should really introduce new
> separate file where will central dell notifier live?

We could put this in a dell-common or some such. My main reason for
going with dell-smbios is that dell-smbios is a "library" module,
loading it does not do anything other then make symbols available
for use by other modules. So using it to store the notifier is safe
(no side effects even if e.g. only dell-rbtn gets loaded).

Given that we already have dell-smbios as dell library functions
module, I think that adding a dell-common is a bit overkill.

I can rename the notifier, maybe use dell_laptop*notifier as names,
since dell-laptop is the main consumer of the notifications ?

Regards,

Hans

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Pali Rohár]

On Thursday 27 October 2016 14:45:20 Hans de Goede wrote:
> HI,
> 
> On 27-10-16 12:32, Pali Rohár wrote:
> >On Monday 24 October 2016 15:45:02 Hans de Goede wrote:
> >>Hi,
> >>
> >>On 24-10-16 15:43, Pali Rohár wrote:
> >>>On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
> >>>>Well WMI events get enabled via a SMBIOS call,
> >>>
> >>>This is truth only for few laptops and only for one WMI event.
> >>>Everything else is automatically enabled, no call is needed to issue.
> >>>
> >>>>and dell-laptop uses SMBIOS exclusively
> >>>
> >>>IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
> >>>dell-laptop.
> >>>
> >>>>so it seems to fit. Basically this is a case of
> >>>>we have to put this somewhere and dell-smbios is the best fit IMHO.
> >>>
> >>>Agree, we need to put it somewhere...
> >>>
> >>>Basically we need to solve problem how (currently) 3 kernel modules can
> >>>communicate. Does not kernel support such "bus/event" mechanism for
> >>>this?
> >>
> >>Yes it does, that is exactly what notifiers are for, but we need to
> >>declare the bus somewhere. I still believe dell-smbios is the best
> >>place.
> >
> >But dell_smbios_register_notifier() name is totally confusing. It does
> >not register any notifier for SMBIOS. Nor it have nothing common with
> >SMBIOS API.
> >
> >Also there is absolutely no need that dell-rbtn.ko needs to depends on
> >dell-smbios.ko. dell-rbtn.ko is ACPI driver which does not use any of
> >SMBIOS API.
> >
> >Right now I'm not saying what is the best place for that notifier (as I
> >still do not have ideal candidate). I'm just saying that notifier is not
> >part of SMBIOS API and therefore dell-smbios.ko is not right place for
> >it.
> >
> >So currently we have these different APIs for dell notebook drivers:
> >* ACPI (used in dell-rbtn.ko and dell-smo8800.ko)
> >* WMI (used in dell-wmi.ko, dell-wmi-aio.ko, dell-led.ko)
> >* SMBIOS (used in dell-laptop.ko, dell-wmi.ko and dell-led.ko)
> >* some other platform code (used in dell-laptop.ko)
> >
> >And now notifier is needed for drivers:
> >* dell-laptop.ko
> >* dell-wmi.ko
> >* dell-rbtn.ko
> >
> >And if I look at above two sets, none of above drivers is good candidate
> >for central notifier functions... Maybe we should really introduce new
> >separate file where will central dell notifier live?
> 
> We could put this in a dell-common or some such. My main reason for
> going with dell-smbios is that dell-smbios is a "library" module,
> loading it does not do anything other then make symbols available
> for use by other modules. So using it to store the notifier is safe
> (no side effects even if e.g. only dell-rbtn gets loaded).

I understand your motivation.

> Given that we already have dell-smbios as dell library functions
> module, I think that adding a dell-common is a bit overkill.

New module is probably really overkill... But cannot we link add those
notifier functions statically? So it would not be new module.

> I can rename the notifier, maybe use dell_laptop*notifier as names,
> since dell-laptop is the main consumer of the notifications ?

Yes, this is name is better!

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

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Hans de Goede]

Hi,

On 27-10-16 14:51, Pali Rohár wrote:
> On Thursday 27 October 2016 14:45:20 Hans de Goede wrote:
>> HI,
>>
>> On 27-10-16 12:32, Pali Rohár wrote:
>>> On Monday 24 October 2016 15:45:02 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 24-10-16 15:43, Pali Rohár wrote:
>>>>> On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
>>>>>> Well WMI events get enabled via a SMBIOS call,
>>>>>
>>>>> This is truth only for few laptops and only for one WMI event.
>>>>> Everything else is automatically enabled, no call is needed to issue.
>>>>>
>>>>>> and dell-laptop uses SMBIOS exclusively
>>>>>
>>>>> IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
>>>>> dell-laptop.
>>>>>
>>>>>> so it seems to fit. Basically this is a case of
>>>>>> we have to put this somewhere and dell-smbios is the best fit IMHO.
>>>>>
>>>>> Agree, we need to put it somewhere...
>>>>>
>>>>> Basically we need to solve problem how (currently) 3 kernel modules can
>>>>> communicate. Does not kernel support such "bus/event" mechanism for
>>>>> this?
>>>>
>>>> Yes it does, that is exactly what notifiers are for, but we need to
>>>> declare the bus somewhere. I still believe dell-smbios is the best
>>>> place.
>>>
>>> But dell_smbios_register_notifier() name is totally confusing. It does
>>> not register any notifier for SMBIOS. Nor it have nothing common with
>>> SMBIOS API.
>>>
>>> Also there is absolutely no need that dell-rbtn.ko needs to depends on
>>> dell-smbios.ko. dell-rbtn.ko is ACPI driver which does not use any of
>>> SMBIOS API.
>>>
>>> Right now I'm not saying what is the best place for that notifier (as I
>>> still do not have ideal candidate). I'm just saying that notifier is not
>>> part of SMBIOS API and therefore dell-smbios.ko is not right place for
>>> it.
>>>
>>> So currently we have these different APIs for dell notebook drivers:
>>> * ACPI (used in dell-rbtn.ko and dell-smo8800.ko)
>>> * WMI (used in dell-wmi.ko, dell-wmi-aio.ko, dell-led.ko)
>>> * SMBIOS (used in dell-laptop.ko, dell-wmi.ko and dell-led.ko)
>>> * some other platform code (used in dell-laptop.ko)
>>>
>>> And now notifier is needed for drivers:
>>> * dell-laptop.ko
>>> * dell-wmi.ko
>>> * dell-rbtn.ko
>>>
>>> And if I look at above two sets, none of above drivers is good candidate
>>> for central notifier functions... Maybe we should really introduce new
>>> separate file where will central dell notifier live?
>>
>> We could put this in a dell-common or some such. My main reason for
>> going with dell-smbios is that dell-smbios is a "library" module,
>> loading it does not do anything other then make symbols available
>> for use by other modules. So using it to store the notifier is safe
>> (no side effects even if e.g. only dell-rbtn gets loaded).
>
> I understand your motivation.
>
>> Given that we already have dell-smbios as dell library functions
>> module, I think that adding a dell-common is a bit overkill.
>
> New module is probably really overkill... But cannot we link add those
> notifier functions statically? So it would not be new module.

No, we need to put the notifier_head somewhere, at which point
making the actual notifier functions inline static is not really
helpful.

>> I can rename the notifier, maybe use dell_laptop*notifier as names,
>> since dell-laptop is the main consumer of the notifications ?
>
> Yes, this is name is better!

Ok, I will change this for the next version.

Regards,

Hans

Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

[Pali Rohár]

On Thursday 27 October 2016 14:54:54 Hans de Goede wrote:
> Hi,
> 
> On 27-10-16 14:51, Pali Rohár wrote:
> >On Thursday 27 October 2016 14:45:20 Hans de Goede wrote:
> >>HI,
> >>
> >>On 27-10-16 12:32, Pali Rohár wrote:
> >>>On Monday 24 October 2016 15:45:02 Hans de Goede wrote:
> >>>>Hi,
> >>>>
> >>>>On 24-10-16 15:43, Pali Rohár wrote:
> >>>>>On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
> >>>>>>Well WMI events get enabled via a SMBIOS call,
> >>>>>
> >>>>>This is truth only for few laptops and only for one WMI event.
> >>>>>Everything else is automatically enabled, no call is needed to issue.
> >>>>>
> >>>>>>and dell-laptop uses SMBIOS exclusively
> >>>>>
> >>>>>IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
> >>>>>dell-laptop.
> >>>>>
> >>>>>>so it seems to fit. Basically this is a case of
> >>>>>>we have to put this somewhere and dell-smbios is the best fit IMHO.
> >>>>>
> >>>>>Agree, we need to put it somewhere...
> >>>>>
> >>>>>Basically we need to solve problem how (currently) 3 kernel modules can
> >>>>>communicate. Does not kernel support such "bus/event" mechanism for
> >>>>>this?
> >>>>
> >>>>Yes it does, that is exactly what notifiers are for, but we need to
> >>>>declare the bus somewhere. I still believe dell-smbios is the best
> >>>>place.
> >>>
> >>>But dell_smbios_register_notifier() name is totally confusing. It does
> >>>not register any notifier for SMBIOS. Nor it have nothing common with
> >>>SMBIOS API.
> >>>
> >>>Also there is absolutely no need that dell-rbtn.ko needs to depends on
> >>>dell-smbios.ko. dell-rbtn.ko is ACPI driver which does not use any of
> >>>SMBIOS API.
> >>>
> >>>Right now I'm not saying what is the best place for that notifier (as I
> >>>still do not have ideal candidate). I'm just saying that notifier is not
> >>>part of SMBIOS API and therefore dell-smbios.ko is not right place for
> >>>it.
> >>>
> >>>So currently we have these different APIs for dell notebook drivers:
> >>>* ACPI (used in dell-rbtn.ko and dell-smo8800.ko)
> >>>* WMI (used in dell-wmi.ko, dell-wmi-aio.ko, dell-led.ko)
> >>>* SMBIOS (used in dell-laptop.ko, dell-wmi.ko and dell-led.ko)
> >>>* some other platform code (used in dell-laptop.ko)
> >>>
> >>>And now notifier is needed for drivers:
> >>>* dell-laptop.ko
> >>>* dell-wmi.ko
> >>>* dell-rbtn.ko
> >>>
> >>>And if I look at above two sets, none of above drivers is good candidate
> >>>for central notifier functions... Maybe we should really introduce new
> >>>separate file where will central dell notifier live?
> >>
> >>We could put this in a dell-common or some such. My main reason for
> >>going with dell-smbios is that dell-smbios is a "library" module,
> >>loading it does not do anything other then make symbols available
> >>for use by other modules. So using it to store the notifier is safe
> >>(no side effects even if e.g. only dell-rbtn gets loaded).
> >
> >I understand your motivation.
> >
> >>Given that we already have dell-smbios as dell library functions
> >>module, I think that adding a dell-common is a bit overkill.
> >
> >New module is probably really overkill... But cannot we link add those
> >notifier functions statically? So it would not be new module.
> 
> No, we need to put the notifier_head somewhere, at which point
> making the actual notifier functions inline static is not really
> helpful.

I mean object file which will not be tristate, but only Y/N and selected
automatically when dell-laptop is Y or M. Not inline static functions.

> >>I can rename the notifier, maybe use dell_laptop*notifier as names,
> >>since dell-laptop is the main consumer of the notifications ?
> >
> >Yes, this is name is better!
> 
> Ok, I will change this for the next version.
> 
> Regards,
> 
> Hans

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

[PATCH v2 3/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell-laptop.c | 28 +++++++++++++++++++++++++++-
 drivers/platform/x86/dell-wmi.c    |  5 +++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 2c2f02b..5a96c25 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1942,6 +1942,8 @@ static struct led_classdev kbd_led = {
 
 static int __init kbd_led_init(struct device *dev)
 {
+	int ret;
+
 	kbd_init();
 	if (!kbd_led_present)
 		return -ENODEV;
@@ -1953,7 +1955,11 @@ static int __init kbd_led_init(struct device *dev)
 		if (kbd_led.max_brightness)
 			kbd_led.max_brightness--;
 	}
-	return led_classdev_register(dev, &kbd_led);
+	ret = led_classdev_register(dev, &kbd_led);
+	if (ret)
+		kbd_led_present = false;
+
+	return ret;
 }
 
 static void brightness_set_exit(struct led_classdev *led_cdev,
@@ -1970,6 +1976,23 @@ static void kbd_led_exit(void)
 	led_classdev_unregister(&kbd_led);
 }
 
+static int dell_laptop_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	switch (action) {
+	case dell_smbios_kbd_backlight_brightness_changed:
+		if (kbd_led_present)
+			led_notify_brightness_change(&kbd_led);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dell_laptop_notifier = {
+	.notifier_call = dell_laptop_notifier_call,
+};
+
 static int __init dell_init(void)
 {
 	struct calling_interface_buffer *buffer;
@@ -2013,6 +2036,8 @@ static int __init dell_init(void)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
 				    &dell_debugfs_fops);
 
+	dell_smbios_register_notifier(&dell_laptop_notifier);
+
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
 
@@ -2064,6 +2089,7 @@ static int __init dell_init(void)
 
 static void __exit dell_exit(void)
 {
+	dell_smbios_unregister_notifier(&dell_laptop_notifier);
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index da2fe18..f86e774 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code)
 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
+	if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea ||
+			 code == 0x02eb || code == 0x02ec || code == 0x02f6))
+		dell_smbios_call_notifier(
+			dell_smbios_kbd_backlight_brightness_changed, NULL);
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
2.9.3

Re: [PATCH v2 3/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Pali Rohár]

On Sunday 23 October 2016 21:46:51 Hans de Goede wrote:
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index da2fe18..f86e774 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code)
>  	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>  		return;
>  
> +	if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea ||
> +			 code == 0x02eb || code == 0x02ec || code == 0x02f6))
> +		dell_smbios_call_notifier(
> +			dell_smbios_kbd_backlight_brightness_changed, NULL);
> +
>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>  }
>  

This part of patch is ugly. Some random numbers are checked and then
notifier is called. We already have big table with explanation of those
events... It is not possible to extend it with some flag or somehow
other that value should be called via notifier?

Btw, personally I would use uppercase DELL_SMBIOS_KBD_... name from that
enum, but I do not know what is correct coding style here for kernel.

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

Re: [PATCH v2 3/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Hi,

On 24-10-16 15:34, Pali Rohár wrote:
> On Sunday 23 October 2016 21:46:51 Hans de Goede wrote:
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index da2fe18..f86e774 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code)
>>  	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>>  		return;
>>
>> +	if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea ||
>> +			 code == 0x02eb || code == 0x02ec || code == 0x02f6))
>> +		dell_smbios_call_notifier(
>> +			dell_smbios_kbd_backlight_brightness_changed, NULL);
>> +
>>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>>  }
>>
>
> This part of patch is ugly. Some random numbers are checked and then
> notifier is called. We already have big table with explanation of those
> events... It is not possible to extend it with some flag or somehow
> other that value should be called via notifier?

Nope, sparse_keymaps are a well defined API for, well, keymaps! The problem
really is this commit:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86?id=e075b3c898e4055ec62a1f0ed7f3b8e62814bfb6

Which mixes status-events and key-press events in one sparse-keymap,
which happens to work because so far all the status events are
using { KE_IGNORE, 0x...., { KEY_RESERVED } }, but now we want to
actually do something and that shows that the above commit really
is a bad idea (at least for the 0x0011 type events, if we (partially)
revert that, then the ugly if goes away and I can simply insert
the dell_smbios_call_notifier() above the break in the original
switch-case handling for 0x0011 type events.

So shall I revert the 0011 part of the mentioned commit?

That is actually what I had in an earlier (never posted) version
of the patch-set.

Regards,

Hans

Re: [PATCH v2 3/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Pali Rohár]

On Monday 24 October 2016 15:43:50 Hans de Goede wrote:
> Hi,
> 
> On 24-10-16 15:34, Pali Rohár wrote:
> >On Sunday 23 October 2016 21:46:51 Hans de Goede wrote:
> >>diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> >>index da2fe18..f86e774 100644
> >>--- a/drivers/platform/x86/dell-wmi.c
> >>+++ b/drivers/platform/x86/dell-wmi.c
> >>@@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code)
> >> 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
> >> 		return;
> >>
> >>+	if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea ||
> >>+			 code == 0x02eb || code == 0x02ec || code == 0x02f6))
> >>+		dell_smbios_call_notifier(
> >>+			dell_smbios_kbd_backlight_brightness_changed, NULL);
> >>+
> >> 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
> >> }
> >>
> >
> >This part of patch is ugly. Some random numbers are checked and then
> >notifier is called. We already have big table with explanation of those
> >events... It is not possible to extend it with some flag or somehow
> >other that value should be called via notifier?
> 
> Nope, sparse_keymaps are a well defined API for, well, keymaps! The problem
> really is this commit:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86?id=e075b3c898e4055ec62a1f0ed7f3b8e62814bfb6
> 
> Which mixes status-events and key-press events in one sparse-keymap,
> which happens to work because so far all the status events are
> using { KE_IGNORE, 0x...., { KEY_RESERVED } }, but now we want to
> actually do something and that shows that the above commit really
> is a bad idea (at least for the 0x0011 type events, if we (partially)
> revert that, then the ugly if goes away and I can simply insert
> the dell_smbios_call_notifier() above the break in the original
> switch-case handling for 0x0011 type events.
> 
> So shall I revert the 0011 part of the mentioned commit?

Does not help us, because keyboard backlight change event is also in
dell_wmi_keymap_type_0000 table.

Another idea: instead of struct key_entry create new structure which
reflect information which comes from dell's WMI:
u16 type (key, event or ignore)
u16 code (wmi code)
union { key, event } (linux keycode or enum notifier event)

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

Re: [PATCH v2 3/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Hans de Goede]

Hi,

On 24-10-16 15:51, Pali Rohár wrote:
> On Monday 24 October 2016 15:43:50 Hans de Goede wrote:
>> Hi,
>>
>> On 24-10-16 15:34, Pali Rohár wrote:
>>> On Sunday 23 October 2016 21:46:51 Hans de Goede wrote:
>>>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>>>> index da2fe18..f86e774 100644
>>>> --- a/drivers/platform/x86/dell-wmi.c
>>>> +++ b/drivers/platform/x86/dell-wmi.c
>>>> @@ -319,6 +319,11 @@ static void dell_wmi_process_key(int type, int code)
>>>> 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>>>> 		return;
>>>>
>>>> +	if (type == 0x0011 && (code == 0x01e1 || code == 0x02ea ||
>>>> +			 code == 0x02eb || code == 0x02ec || code == 0x02f6))
>>>> +		dell_smbios_call_notifier(
>>>> +			dell_smbios_kbd_backlight_brightness_changed, NULL);
>>>> +
>>>> 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>>>> }
>>>>
>>>
>>> This part of patch is ugly. Some random numbers are checked and then
>>> notifier is called. We already have big table with explanation of those
>>> events... It is not possible to extend it with some flag or somehow
>>> other that value should be called via notifier?
>>
>> Nope, sparse_keymaps are a well defined API for, well, keymaps! The problem
>> really is this commit:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86?id=e075b3c898e4055ec62a1f0ed7f3b8e62814bfb6
>>
>> Which mixes status-events and key-press events in one sparse-keymap,
>> which happens to work because so far all the status events are
>> using { KE_IGNORE, 0x...., { KEY_RESERVED } }, but now we want to
>> actually do something and that shows that the above commit really
>> is a bad idea (at least for the 0x0011 type events, if we (partially)
>> revert that, then the ugly if goes away and I can simply insert
>> the dell_smbios_call_notifier() above the break in the original
>> switch-case handling for 0x0011 type events.
>>
>> So shall I revert the 0011 part of the mentioned commit?
>
> Does not help us, because keyboard backlight change event is also in
> dell_wmi_keymap_type_0000 table.

Hmm, the 0000 table contains:

         /* Key code is followed by keyboard illumination level */
         { KE_IGNORE, 0xe00c, { KEY_KBDILLUMTOGGLE } },

We could use the same keycode in the 0011 table for kbd brightness events and then do:

	if (event->keycode == KEY_KBDILLUMTOGGLE)
		dell_smbios_kbd_backlight_brightness_changed, NULL);

I will send an updated version using this.

Regards,

Hans

Re: [PATCH v2 3/4] platform: x86: dell-*: Call led_notify_brightness_change on kbd brightness change

[Pali Rohár]

On Monday 24 October 2016 15:57:41 Hans de Goede wrote:
> Hmm, the 0000 table contains:
> 
>         /* Key code is followed by keyboard illumination level */
>         { KE_IGNORE, 0xe00c, { KEY_KBDILLUMTOGGLE } },
> 
> We could use the same keycode in the 0011 table for kbd brightness events and then do:
> 
> 	if (event->keycode == KEY_KBDILLUMTOGGLE)
> 		dell_smbios_kbd_backlight_brightness_changed, NULL);
> 
> I will send an updated version using this.

Yea, that is great!

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

[PATCH v2 4/4] platform: x86: dell-*: Simplify dell-rbtn integration with dell-laptop [untested]

[Hans de Goede]

Use dell_smbios*notifier for dell-laptop to listen to dell-rbtn slider
events, replace dell_rbtn_notifier_register() /
dell_rbtn_notifier_unregister() with a single dell_rbtn_has_rfkill() used
by dell-laptop to decide whether or not to use the i8042 filter and used
by dell-rbtn to auto-remove its rfkill interface when called.

This results in a nice cleanup, downside is that the rfkill interface
of dell-rbtn is not automatically re-enabled on rmmod dell-laptop, this
now requires rmmod + insmod of dell-rbtn. But people who do not want
dell-laptop for some reason will have it blacklisted anyways, so this
is not an issue and there is a work-around.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of my platform/x86/dell-* notifier set intended
 to show how dell_smbios*notifier can be used to improve the dell-rbtn
 integration too
---
 drivers/platform/x86/Kconfig       |  1 +
 drivers/platform/x86/dell-laptop.c | 53 +++++++-------------------------
 drivers/platform/x86/dell-rbtn.c   | 63 +++++++++-----------------------------
 drivers/platform/x86/dell-rbtn.h   |  5 +--
 drivers/platform/x86/dell-smbios.h |  1 +
 5 files changed, 29 insertions(+), 94 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 81b8dcc..30291e1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -158,6 +158,7 @@ config DELL_RBTN
 	depends on ACPI
 	depends on INPUT
 	depends on RFKILL
+	depends on DELL_SMBIOS
 	---help---
 	  Say Y here if you want to support Dell Airplane Mode Switch ACPI
 	  device on Dell laptops. Sometimes it has names: DELLABCE or DELRBTN.
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 5a96c25..3803da1 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -670,25 +670,12 @@ static bool dell_laptop_i8042_filter(unsigned char data, unsigned char str,
 	return false;
 }
 
-static int (*dell_rbtn_notifier_register_func)(struct notifier_block *);
-static int (*dell_rbtn_notifier_unregister_func)(struct notifier_block *);
-
-static int dell_laptop_rbtn_notifier_call(struct notifier_block *nb,
-					  unsigned long action, void *data)
-{
-	schedule_delayed_work(&dell_rfkill_work, 0);
-	return NOTIFY_OK;
-}
-
-static struct notifier_block dell_laptop_rbtn_notifier = {
-	.notifier_call = dell_laptop_rbtn_notifier_call,
-};
-
 static int __init dell_setup_rfkill(void)
 {
 	struct calling_interface_buffer *buffer;
 	int status, ret, whitelisted;
 	const char *product;
+	int (*dell_rbtn_has_rfkill_func)(void);
 
 	/*
 	 * rfkill support causes trouble on various models, mostly Inspirons.
@@ -773,29 +760,13 @@ static int __init dell_setup_rfkill(void)
 	 *
 	 * To prevent duplicate rfkill devices which control and do same thing,
 	 * dell-rbtn driver will automatically remove its own rfkill devices
-	 * once function dell_rbtn_notifier_register() is called.
+	 * once function dell_rbtn_has_rfkill() is called.
 	 */
 
-	dell_rbtn_notifier_register_func =
-		symbol_request(dell_rbtn_notifier_register);
-	if (dell_rbtn_notifier_register_func) {
-		dell_rbtn_notifier_unregister_func =
-			symbol_request(dell_rbtn_notifier_unregister);
-		if (!dell_rbtn_notifier_unregister_func) {
-			symbol_put(dell_rbtn_notifier_register);
-			dell_rbtn_notifier_register_func = NULL;
-		}
-	}
-
-	if (dell_rbtn_notifier_register_func) {
-		ret = dell_rbtn_notifier_register_func(
-			&dell_laptop_rbtn_notifier);
-		symbol_put(dell_rbtn_notifier_register);
-		dell_rbtn_notifier_register_func = NULL;
-		if (ret != 0) {
-			symbol_put(dell_rbtn_notifier_unregister);
-			dell_rbtn_notifier_unregister_func = NULL;
-		}
+	dell_rbtn_has_rfkill_func = symbol_request(dell_rbtn_has_rfkill);
+	if (dell_rbtn_has_rfkill_func) {
+		ret = dell_rbtn_has_rfkill();
+		symbol_put(dell_rbtn_has_rfkill);
 	} else {
 		pr_info("Symbols from dell-rbtn acpi driver are not available\n");
 		ret = -ENODEV;
@@ -835,13 +806,7 @@ static int __init dell_setup_rfkill(void)
 
 static void dell_cleanup_rfkill(void)
 {
-	if (dell_rbtn_notifier_unregister_func) {
-		dell_rbtn_notifier_unregister_func(&dell_laptop_rbtn_notifier);
-		symbol_put(dell_rbtn_notifier_unregister);
-		dell_rbtn_notifier_unregister_func = NULL;
-	} else {
-		i8042_remove_filter(dell_laptop_i8042_filter);
-	}
+	i8042_remove_filter(dell_laptop_i8042_filter);
 	cancel_delayed_work_sync(&dell_rfkill_work);
 	if (wifi_rfkill) {
 		rfkill_unregister(wifi_rfkill);
@@ -1984,6 +1949,10 @@ static int dell_laptop_notifier_call(struct notifier_block *nb,
 		if (kbd_led_present)
 			led_notify_brightness_change(&kbd_led);
 		break;
+	case dell_smbios_rbtn_slider:
+		if (wifi_rfkill || bluetooth_rfkill || wwan_rfkill)
+			schedule_delayed_work(&dell_rfkill_work, 0);
+		break;
 	}
 
 	return NOTIFY_OK;
diff --git a/drivers/platform/x86/dell-rbtn.c b/drivers/platform/x86/dell-rbtn.c
index dcd9f40..b6c03d4 100644
--- a/drivers/platform/x86/dell-rbtn.c
+++ b/drivers/platform/x86/dell-rbtn.c
@@ -17,6 +17,8 @@
 #include <linux/acpi.h>
 #include <linux/rfkill.h>
 #include <linux/input.h>
+#include "dell-smbios.h"
+#include "dell-rbtn.h"
 
 enum rbtn_type {
 	RBTN_UNKNOWN,
@@ -295,12 +297,11 @@ static struct acpi_driver rbtn_driver = {
 
 
 /*
- * notifier export functions
+ * Functions to coordinate registering only 1 rfkill with dell-laptop
  */
 
 static bool auto_remove_rfkill = true;
-
-static ATOMIC_NOTIFIER_HEAD(rbtn_chain_head);
+static bool init_rfkill = true;
 
 static int rbtn_inc_count(struct device *dev, void *data)
 {
@@ -314,26 +315,14 @@ static int rbtn_inc_count(struct device *dev, void *data)
 	return 0;
 }
 
-static int rbtn_switch_dev(struct device *dev, void *data)
+static int rbtn_disable_rfkill(struct device *dev, void *data)
 {
-	struct acpi_device *device = to_acpi_device(dev);
-	struct rbtn_data *rbtn_data = device->driver_data;
-	bool enable = data;
-
-	if (rbtn_data->type != RBTN_SLIDER)
-		return 0;
-
-	if (enable)
-		rbtn_rfkill_init(device);
-	else
-		rbtn_rfkill_exit(device);
-
+	rbtn_rfkill_exit(to_acpi_device(dev));
 	return 0;
 }
 
-int dell_rbtn_notifier_register(struct notifier_block *nb)
+int dell_rbtn_has_rfkill(void)
 {
-	bool first;
 	int count;
 	int ret;
 
@@ -343,35 +332,15 @@ int dell_rbtn_notifier_register(struct notifier_block *nb)
 	if (ret || count == 0)
 		return -ENODEV;
 
-	first = !rbtn_chain_head.head;
-
-	ret = atomic_notifier_chain_register(&rbtn_chain_head, nb);
-	if (ret != 0)
-		return ret;
-
-	if (auto_remove_rfkill && first)
-		ret = driver_for_each_device(&rbtn_driver.drv, NULL,
-					     (void *)false, rbtn_switch_dev);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(dell_rbtn_notifier_register);
-
-int dell_rbtn_notifier_unregister(struct notifier_block *nb)
-{
-	int ret;
-
-	ret = atomic_notifier_chain_unregister(&rbtn_chain_head, nb);
-	if (ret != 0)
-		return ret;
-
-	if (auto_remove_rfkill && !rbtn_chain_head.head)
-		ret = driver_for_each_device(&rbtn_driver.drv, NULL,
-					     (void *)true, rbtn_switch_dev);
+	if (auto_remove_rfkill) {
+		init_rfkill = false;
+		ret = driver_for_each_device(&rbtn_driver.drv, NULL, NULL,
+					     rbtn_disable_rfkill);
+	}
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(dell_rbtn_notifier_unregister);
+EXPORT_SYMBOL_GPL(dell_rbtn_has_rfkill);
 
 
 /*
@@ -408,9 +377,7 @@ static int rbtn_add(struct acpi_device *device)
 		ret = rbtn_input_init(rbtn_data);
 		break;
 	case RBTN_SLIDER:
-		if (auto_remove_rfkill && rbtn_chain_head.head)
-			ret = 0;
-		else
+		if (init_rfkill)
 			ret = rbtn_rfkill_init(device);
 		break;
 	default:
@@ -467,7 +434,7 @@ static void rbtn_notify(struct acpi_device *device, u32 event)
 		break;
 	case RBTN_SLIDER:
 		rbtn_rfkill_event(device);
-		atomic_notifier_call_chain(&rbtn_chain_head, event, device);
+		dell_smbios_call_notifier(dell_smbios_rbtn_slider, NULL);
 		break;
 	default:
 		break;
diff --git a/drivers/platform/x86/dell-rbtn.h b/drivers/platform/x86/dell-rbtn.h
index c59cc6b..97578e8 100644
--- a/drivers/platform/x86/dell-rbtn.h
+++ b/drivers/platform/x86/dell-rbtn.h
@@ -16,9 +16,6 @@
 #ifndef _DELL_RBTN_H_
 #define _DELL_RBTN_H_
 
-struct notifier_block;
-
-int dell_rbtn_notifier_register(struct notifier_block *nb);
-int dell_rbtn_notifier_unregister(struct notifier_block *nb);
+int dell_rbtn_has_rfkill(void);
 
 #endif
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index e91f13f..92817a1 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -48,6 +48,7 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid);
 
 enum dell_smbios_notifier_actions {
 	dell_smbios_kbd_backlight_brightness_changed,
+	dell_smbios_rbtn_slider,
 };
 
 int dell_smbios_register_notifier(struct notifier_block *nb);
-- 
2.9.3

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

[Jacek Anaszewski]

Hi Hans,

Thanks for the update.
Could you please rebase your patches onto recent linux-leds.git,
for-next branch?

Also it would be convenient to have sample application for polling
brightness changes in tools/leds.

Thanks,
Jacek Anaszewski

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

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

[Hans de Goede]

Hi,

On 24-10-16 22:43, Jacek Anaszewski wrote:
> Hi Hans,
>
> Thanks for the update.
> Could you please rebase your patches onto recent linux-leds.git,
> for-next branch?

Done, I will send a v3 soon (today or tomorrow).

> Also it would be convenient to have sample application for polling
> brightness changes in tools/leds.

I'm sorry but I do not have time to create such a sample application.

Regards,

Hans


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

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

[Jacek Anaszewski]

Hi,

On 10/26/2016 05:18 PM, Hans de Goede wrote:
> Hi,
>
> On 24-10-16 22:43, Jacek Anaszewski wrote:
>> Hi Hans,
>>
>> Thanks for the update.
>> Could you please rebase your patches onto recent linux-leds.git,
>> for-next branch?
>
> Done, I will send a v3 soon (today or tomorrow).
>
>> Also it would be convenient to have sample application for polling
>> brightness changes in tools/leds.
>
> I'm sorry but I do not have time to create such a sample application.

You must have tested your solution somehow. Just polish the code a bit,
add a Makefile and that's it. Compare tools/leds/uledmon.c - it doesn't
need to be anything big.

If you're adding a feature that interacts with userspace, then I'd like
to have a sample code that shows how to use it. Just for a reference.

-- 
Best regards,
Jacek Anaszewski

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

[Hans de Goede]

Hi,

On 27-10-16 08:58, Jacek Anaszewski wrote:
> Hi,
>
> On 10/26/2016 05:18 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 24-10-16 22:43, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
>>> Thanks for the update.
>>> Could you please rebase your patches onto recent linux-leds.git,
>>> for-next branch?
>>
>> Done, I will send a v3 soon (today or tomorrow).
>>
>>> Also it would be convenient to have sample application for polling
>>> brightness changes in tools/leds.
>>
>> I'm sorry but I do not have time to create such a sample application.
>
> You must have tested your solution somehow.

Yes, as is made clear by the other patches in this set, I'm using
it with keyboard backlight control led interfaces. Which in userspace
are handled by upower which provides a dbus interface which is used
by gnome-settings-daemon.

> Just polish the code a bit,
> add a Makefile and that's it. Compare tools/leds/uledmon.c - it doesn't
> need to be anything big.

upower uses gio which is really not suitable for this, so I would need
to create something from scratch.

Regards,

Hans


>
> If you're adding a feature that interacts with userspace, then I'd like
> to have a sample code that shows how to use it. Just for a reference.
>

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

[Jacek Anaszewski]

On 10/27/2016 09:33 AM, Hans de Goede wrote:
> Hi,
>
> On 27-10-16 08:58, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 10/26/2016 05:18 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 24-10-16 22:43, Jacek Anaszewski wrote:
>>>> Hi Hans,
>>>>
>>>> Thanks for the update.
>>>> Could you please rebase your patches onto recent linux-leds.git,
>>>> for-next branch?
>>>
>>> Done, I will send a v3 soon (today or tomorrow).
>>>
>>>> Also it would be convenient to have sample application for polling
>>>> brightness changes in tools/leds.
>>>
>>> I'm sorry but I do not have time to create such a sample application.
>>
>> You must have tested your solution somehow.
>
> Yes, as is made clear by the other patches in this set, I'm using
> it with keyboard backlight control led interfaces. Which in userspace
> are handled by upower which provides a dbus interface which is used
> by gnome-settings-daemon.
>
>> Just polish the code a bit,
>> add a Makefile and that's it. Compare tools/leds/uledmon.c - it doesn't
>> need to be anything big.
>
> upower uses gio which is really not suitable for this, so I would need
> to create something from scratch.

Okay, so I'll have to do it myself, when I'll find a moment.

-- 
Best regards,
Jacek Anaszewski