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

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

[Hans de Goede]

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

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

It currently only defines 1 action:
DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
Changes in v3:
-No changes
Changes in v4:
-Rename functions from dell_smbios_*_notifier to dell_laptop_*_notifier
Changes in v5:
-Dropped due to new led-trigger based approach making this unnecessary
Changes in v6:
-Re-introduced as led-trigger based approach got nacked
Changes in v8:
-Change from atomic to blocking notifier as some of the called notifier
 callbacks may take a mutex
---
 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..0a57234 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 BLOCKING_NOTIFIER_HEAD(dell_laptop_chain_head);
+
+int dell_laptop_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&dell_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_register_notifier);
+
+int dell_laptop_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&dell_laptop_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_unregister_notifier);
+
+void dell_laptop_call_notifier(unsigned long action, void *data)
+{
+	blocking_notifier_call_chain(&dell_laptop_chain_head, action, data);
+}
+EXPORT_SYMBOL_GPL(dell_laptop_call_notifier);
+
 static void __init parse_da_table(const struct dmi_header *dm)
 {
 	/* Final token is a terminator, so we don't want to copy it */
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..45cbc22 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -16,6 +16,8 @@
 #ifndef _DELL_SMBIOS_H_
 #define _DELL_SMBIOS_H_
 
+struct notifier_block;
+
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
 
@@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
 void dell_smbios_send_request(int class, int select);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
+
+enum dell_laptop_notifier_actions {
+	DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED,
+};
+
+int dell_laptop_register_notifier(struct notifier_block *nb);
+int dell_laptop_unregister_notifier(struct notifier_block *nb);
+void dell_laptop_call_notifier(unsigned long action, void *data);
+
 #endif
-- 
2.9.3

[PATCH v9 2/4] platform/x86/dell-laptop: Refactor kbd_led_triggers_store()

[Hans de Goede]

Return -EINVAL immediately on invalid input, rather then doing
the straight path in an if block and returning -EINVAL at the end
of the function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
Changes in v8:
-New patch in v8 of this patch-set
---
 drivers/platform/x86/dell-laptop.c | 63 +++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f57dd28..4de7aa0 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1666,38 +1666,39 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 		}
 	}
 
-	if (trigger_bit != -1) {
-		new_state = state;
-		if (trigger[0] == '+')
-			new_state.triggers |= BIT(trigger_bit);
-		else {
-			new_state.triggers &= ~BIT(trigger_bit);
-			/* NOTE: trackstick bit (2) must be disabled when
-			 *       disabling touchpad bit (1), otherwise touchpad
-			 *       bit (1) will not be disabled */
-			if (trigger_bit == 1)
-				new_state.triggers &= ~BIT(2);
-		}
-		if ((kbd_info.triggers & new_state.triggers) !=
-		    new_state.triggers)
-			return -EINVAL;
-		if (new_state.triggers && !triggers_enabled) {
-			new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
-			kbd_set_level(&new_state, kbd_previous_level);
-		} else if (new_state.triggers == 0) {
-			kbd_set_level(&new_state, 0);
-		}
-		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-			return -EINVAL;
-		ret = kbd_set_state_safe(&new_state, &state);
-		if (ret)
-			return ret;
-		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
-			kbd_previous_mode_bit = new_state.mode_bit;
-		return count;
-	}
+	if (trigger_bit == -1)
+		return -EINVAL;
 
-	return -EINVAL;
+	new_state = state;
+	if (trigger[0] == '+')
+		new_state.triggers |= BIT(trigger_bit);
+	else {
+		new_state.triggers &= ~BIT(trigger_bit);
+		/*
+		 * NOTE: trackstick bit (2) must be disabled when
+		 *       disabling touchpad bit (1), otherwise touchpad
+		 *       bit (1) will not be disabled
+		 */
+		if (trigger_bit == 1)
+			new_state.triggers &= ~BIT(2);
+	}
+	if ((kbd_info.triggers & new_state.triggers) !=
+	    new_state.triggers)
+		return -EINVAL;
+	if (new_state.triggers && !triggers_enabled) {
+		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+		kbd_set_level(&new_state, kbd_previous_level);
+	} else if (new_state.triggers == 0) {
+		kbd_set_level(&new_state, 0);
+	}
+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+		return -EINVAL;
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
+		kbd_previous_mode_bit = new_state.mode_bit;
+	return count;
 }
 
 static ssize_t kbd_led_triggers_show(struct device *dev,
-- 
2.9.3

[PATCH v9 3/4] platform/x86/dell-laptop: Protect kbd_state against races

[Hans de Goede]

The kbd led code has multiple entry points each of which modifies the
kbd_state by reading it, modifying a copy, writing the copy and on
error setting the modified copy writing back the original state.

This is racy, so add a mutex protection the read-modify-write cycle
on each of the entry points.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
Changes in v8:
-New patch in v8 of this patch-set
---
 drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
 1 file changed, 76 insertions(+), 36 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 4de7aa0..da185fb 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1139,6 +1139,7 @@ static u8 kbd_previous_level;
 static u8 kbd_previous_mode_bit;
 
 static bool kbd_led_present;
+static DEFINE_MUTEX(kbd_led_mutex);
 
 /*
  * NOTE: there are three ways to set the keyboard backlight level.
@@ -1568,9 +1569,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 		}
 	}
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
 	new_state = state;
 	new_state.timeout_value = value;
@@ -1578,9 +1581,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_timeout_show(struct device *dev,
@@ -1640,9 +1646,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 	if (trigger[0] != '+' && trigger[0] != '-')
 		return -EINVAL;
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
 	if (kbd_triggers_supported)
 		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
@@ -1656,18 +1664,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
 				continue;
 			if (trigger[0] == '+' &&
-			    triggers_enabled && (state.triggers & BIT(i)))
-				return count;
+			    triggers_enabled && (state.triggers & BIT(i))) {
+				ret = count;
+				goto out;
+			}
 			if (trigger[0] == '-' &&
-			    (!triggers_enabled || !(state.triggers & BIT(i))))
-				return count;
+			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
+				ret = count;
+				goto out;
+			}
 			trigger_bit = i;
 			break;
 		}
 	}
 
-	if (trigger_bit == -1)
-		return -EINVAL;
+	if (trigger_bit == -1) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	new_state = state;
 	if (trigger[0] == '+')
@@ -1683,22 +1697,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
 			new_state.triggers &= ~BIT(2);
 	}
 	if ((kbd_info.triggers & new_state.triggers) !=
-	    new_state.triggers)
-		return -EINVAL;
+	    new_state.triggers) {
+		ret = -EINVAL;
+		goto out;
+	}
 	if (new_state.triggers && !triggers_enabled) {
 		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
 		kbd_set_level(&new_state, kbd_previous_level);
 	} else if (new_state.triggers == 0) {
 		kbd_set_level(&new_state, 0);
 	}
-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-		return -EINVAL;
+	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
+		ret = -EINVAL;
+		goto out;
+	}
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
 		kbd_previous_mode_bit = new_state.mode_bit;
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_triggers_show(struct device *dev,
@@ -1755,12 +1776,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
-	if (enable == kbd_is_als_mode_bit(state.mode_bit))
-		return count;
+	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
+		ret = count;
+		goto out;
+	}
 
 	new_state = state;
 
@@ -1780,15 +1805,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
 			new_state.mode_bit = KBD_MODE_BIT_ON;
 		}
 	}
-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-		return -EINVAL;
+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 	kbd_previous_mode_bit = new_state.mode_bit;
 
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_als_enabled_show(struct device *dev,
@@ -1823,18 +1853,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&kbd_led_mutex);
+
 	ret = kbd_get_state(&state);
 	if (ret)
-		return ret;
+		goto out;
 
 	new_state = state;
 	new_state.als_setting = setting;
 
 	ret = kbd_set_state_safe(&new_state, &state);
 	if (ret)
-		return ret;
+		goto out;
 
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static ssize_t kbd_led_als_setting_show(struct device *dev,
@@ -1919,27 +1954,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 	u16 num;
 	int ret;
 
+	mutex_lock(&kbd_led_mutex);
+
 	if (kbd_get_max_level()) {
 		ret = kbd_get_state(&state);
 		if (ret)
-			return ret;
+			goto out;
 		new_state = state;
 		ret = kbd_set_level(&new_state, value);
 		if (ret)
-			return ret;
-		return kbd_set_state_safe(&new_state, &state);
-	}
-
-	if (kbd_get_valid_token_counts()) {
+			goto out;
+		ret = kbd_set_state_safe(&new_state, &state);
+	} else if (kbd_get_valid_token_counts()) {
 		for (num = kbd_token_bits; num != 0 && value > 0; --value)
 			num &= num - 1; /* clear the first bit set */
 		if (num == 0)
-			return 0;
-		return kbd_set_token_bit(ffs(num) - 1);
+			ret = 0;
+		else
+			ret = kbd_set_token_bit(ffs(num) - 1);
+	} else {
+		pr_warn("Keyboard brightness level control not supported\n");
+		ret = -ENXIO;
 	}
 
-	pr_warn("Keyboard brightness level control not supported\n");
-	return -ENXIO;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
 }
 
 static struct led_classdev kbd_led = {
-- 
2.9.3

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

[Hans de Goede]

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Changes in v2:
-Use the new dell_smbios*notify functionality
Changes in v3:
-Simplify the if condition for calling led_notify_brightness_change
Changes in v4:
-Adjust for dell_smbios_*_notifier to dell_laptop_*_notifier rename
Changes in v5:
-Switch to new led-trigger based API for notifying userspace about
 keyboard backlight brightness changes.
Changes in v6:
-Switch to new brightness_hw_changed LED class API
Changes in v8:
-Cache last set brightness value and only call
 led_classdev_notify_brightness_hw_changed if the brightness has changed,
 this is necessary because the WMI events get triggered when we set the
 brightness ourselves too
Changes in v9:
-Only listen to type 0x0010 kbd-led events in dell-wmi.c, these only trigger
 on hotkey presses
-Drop the new / previous brightness comparison from dell-laptop.c now that
 we only get events on hotkey presses it is no longer necessary
---
 drivers/platform/x86/dell-laptop.c | 32 +++++++++++++++++++++++++++++++-
 drivers/platform/x86/dell-wmi.c    |  4 ++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index da185fb..1cd258b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1984,6 +1984,7 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
 
 static struct led_classdev kbd_led = {
 	.name           = "dell::kbd_backlight",
+	.flags		= LED_BRIGHT_HW_CHANGED,
 	.brightness_set_blocking = kbd_led_level_set,
 	.brightness_get = kbd_led_level_get,
 	.groups         = kbd_led_groups,
@@ -1991,6 +1992,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;
@@ -2002,7 +2005,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,
@@ -2019,6 +2026,26 @@ static void kbd_led_exit(void)
 	led_classdev_unregister(&kbd_led);
 }
 
+static int dell_laptop_notifier_call(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	switch (action) {
+	case DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED:
+		if (!kbd_led_present)
+			break;
+
+		led_classdev_notify_brightness_hw_changed(&kbd_led,
+						kbd_led_level_get(&kbd_led));
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dell_laptop_notifier = {
+	.notifier_call = dell_laptop_notifier_call,
+};
+
 static int __init dell_init(void)
 {
 	struct calling_interface_buffer *buffer;
@@ -2062,6 +2089,8 @@ static int __init dell_init(void)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
 				    &dell_debugfs_fops);
 
+	dell_laptop_register_notifier(&dell_laptop_notifier);
+
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
 
@@ -2113,6 +2142,7 @@ static int __init dell_init(void)
 
 static void __exit dell_exit(void)
 {
+	dell_laptop_unregister_notifier(&dell_laptop_notifier);
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 75e6370..751f8c4 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -329,6 +329,10 @@ static void dell_wmi_process_key(int type, int code)
 	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
+	if (key->keycode == KEY_KBDILLUMTOGGLE)
+		dell_laptop_call_notifier(
+			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
+
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
 }
 
-- 
2.9.3

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

[Darren Hart]

On Thu, Mar 16, 2017 at 11:55:35AM +0100, Hans de Goede wrote:
> Make dell-wmi notify on hotkey kbd brightness changes, listen for this
> in dell-laptop and call led_classdev_notify_brightness_hw_changed.
> 
> This will allow userspace to monitor (poll) for brightness changes on
> these LEDs caused by the hotkey.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thanks Hans, this appears to be consistent with the conclusion on v8 between you
and Pali. While I don't care for the cross-driver-dependency, that's
pre-existing and not something I have a solution for. So this looks good to me,
pending Pali's final review.

Pali, I know you have had some reservations reading through the v8 discussion. I
believe Hans has addressed each of those sufficiently for the purposes of this
patch set. As a follow-on effort, I'd like to discuss the future of libsmbios
with the Dell folks and see if we can't phase it out.

Hans, a couple of nits on this patch. To keep the subject under 80, I used:

platform/x86: dell-*: Call new led hw_changed API on kbd brightness change

since you used the full led function name in the commit message anyway.

I presume the changelog was intended to go after the --- and you didn't want it
going into the commit itself, so I've removed it.

I'll await Pali's final Reviewed-by before pushing to testing, but you can see
my minor tweaks listed above in the dell branch:

git://git.infradead.org/linux-platform-drivers-x86.git dell

http://git.infradead.org/linux-platform-drivers-x86.git/log/refs/heads/dell

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

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

[Hans de Goede]

Hi,

On 17-03-17 23:33, Darren Hart wrote:
> On Thu, Mar 16, 2017 at 11:55:35AM +0100, Hans de Goede wrote:
>> Make dell-wmi notify on hotkey kbd brightness changes, listen for this
>> in dell-laptop and call led_classdev_notify_brightness_hw_changed.
>>
>> This will allow userspace to monitor (poll) for brightness changes on
>> these LEDs caused by the hotkey.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Thanks Hans, this appears to be consistent with the conclusion on v8 between you
> and Pali. While I don't care for the cross-driver-dependency, that's
> pre-existing and not something I have a solution for. So this looks good to me,
> pending Pali's final review.
>
> Pali, I know you have had some reservations reading through the v8 discussion. I
> believe Hans has addressed each of those sufficiently for the purposes of this
> patch set. As a follow-on effort, I'd like to discuss the future of libsmbios
> with the Dell folks and see if we can't phase it out.
>
> Hans, a couple of nits on this patch. To keep the subject under 80, I used:
>
> platform/x86: dell-*: Call new led hw_changed API on kbd brightness change
>
> since you used the full led function name in the commit message anyway.

Ok.

> I presume the changelog was intended to go after the --- and you didn't want it
> going into the commit itself, so I've removed it.

Ah yes, my bad.

> I'll await Pali's final Reviewed-by before pushing to testing, but you can see
> my minor tweaks listed above in the dell branch:
>
> git://git.infradead.org/linux-platform-drivers-x86.git dell
>
> http://git.infradead.org/linux-platform-drivers-x86.git/log/refs/heads/dell

Looks good to me, thank you.

Regards,

Hans

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

[Pali Rohár]

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

On Thursday 16 March 2017 11:55:35 Hans de Goede wrote:
> Changes in v9:
> -Only listen to type 0x0010 kbd-led events in dell-wmi.c, these only
> trigger on hotkey presses
> -Drop the new / previous brightness comparison from dell-laptop.c now
> that we only get events on hotkey presses it is no longer necessary
> ---

Hi! I'm really not sure if this change is correct there.

Now you are only listening for keypress "change kbd backlight", but some 
dell machines could change keyboard backlight also in other different 
situations, like attaching AC adapter. I guess (but I'm not sure) this 
probably does not send keypress event. 

-- 
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 v9 4/4] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Hans de Goede]

Hi,

On 19-03-17 16:10, Pali Rohár wrote:
> On Thursday 16 March 2017 11:55:35 Hans de Goede wrote:
>> Changes in v9:
>> -Only listen to type 0x0010 kbd-led events in dell-wmi.c, these only
>> trigger on hotkey presses
>> -Drop the new / previous brightness comparison from dell-laptop.c now
>> that we only get events on hotkey presses it is no longer necessary
>> ---
>
> Hi! I'm really not sure if this change is correct there.
>
> Now you are only listening for keypress "change kbd backlight", but some
> dell machines could change keyboard backlight also in other different
> situations, like attaching AC adapter. I guess (but I'm not sure) this
> probably does not send keypress event.

I'm not aware of any Dells doing such a thing, if we encounter any then
we can deal with that if and when that happens.

Regards,

Hans

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

[Pali Rohár]

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

On Sunday 19 March 2017 19:06:19 Hans de Goede wrote:
> Hi,
> 
> On 19-03-17 16:10, Pali Rohár wrote:
> > On Thursday 16 March 2017 11:55:35 Hans de Goede wrote:
> >> Changes in v9:
> >> -Only listen to type 0x0010 kbd-led events in dell-wmi.c, these
> >> only trigger on hotkey presses
> >> -Drop the new / previous brightness comparison from dell-laptop.c
> >> now that we only get events on hotkey presses it is no longer
> >> necessary ---
> > 
> > Hi! I'm really not sure if this change is correct there.
> > 
> > Now you are only listening for keypress "change kbd backlight", but
> > some dell machines could change keyboard backlight also in other
> > different situations, like attaching AC adapter. I guess (but I'm
> > not sure) this probably does not send keypress event.
> 
> I'm not aware of any Dells doing such a thing, if we encounter any
> then we can deal with that if and when that happens.

Ok, go ahead. We can fix issues when appear later.

-- 
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 v9 4/4] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change

[Darren Hart]

On Sun, Mar 19, 2017 at 07:11:43PM +0100, Pali Rohár wrote:
> On Sunday 19 March 2017 19:06:19 Hans de Goede wrote:
> > Hi,
> > 
> > On 19-03-17 16:10, Pali Rohár wrote:
> > > On Thursday 16 March 2017 11:55:35 Hans de Goede wrote:
> > >> Changes in v9:
> > >> -Only listen to type 0x0010 kbd-led events in dell-wmi.c, these
> > >> only trigger on hotkey presses
> > >> -Drop the new / previous brightness comparison from dell-laptop.c
> > >> now that we only get events on hotkey presses it is no longer
> > >> necessary ---
> > > 
> > > Hi! I'm really not sure if this change is correct there.
> > > 
> > > Now you are only listening for keypress "change kbd backlight", but
> > > some dell machines could change keyboard backlight also in other
> > > different situations, like attaching AC adapter. I guess (but I'm
> > > not sure) this probably does not send keypress event.
> > 
> > I'm not aware of any Dells doing such a thing, if we encounter any
> > then we can deal with that if and when that happens.
> 
> Ok, go ahead. We can fix issues when appear later.

These are queued to testing.

Pali, I can only add a tag from you if you provide it in email. If you want your
reviewed by on these, please include that in your replies.

-- 
Darren Hart
VMware Open Source Technology Center

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

[Pali Rohár]

On Wednesday 22 March 2017 09:59:04 Darren Hart wrote:
> On Sun, Mar 19, 2017 at 07:11:43PM +0100, Pali Rohár wrote:
> > On Sunday 19 March 2017 19:06:19 Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 19-03-17 16:10, Pali Rohár wrote:
> > > > On Thursday 16 March 2017 11:55:35 Hans de Goede wrote:
> > > >> Changes in v9:
> > > >> -Only listen to type 0x0010 kbd-led events in dell-wmi.c, these
> > > >> only trigger on hotkey presses
> > > >> -Drop the new / previous brightness comparison from dell-laptop.c
> > > >> now that we only get events on hotkey presses it is no longer
> > > >> necessary ---
> > > > 
> > > > Hi! I'm really not sure if this change is correct there.
> > > > 
> > > > Now you are only listening for keypress "change kbd backlight", but
> > > > some dell machines could change keyboard backlight also in other
> > > > different situations, like attaching AC adapter. I guess (but I'm
> > > > not sure) this probably does not send keypress event.
> > > 
> > > I'm not aware of any Dells doing such a thing, if we encounter any
> > > then we can deal with that if and when that happens.
> > 
> > Ok, go ahead. We can fix issues when appear later.
> 
> These are queued to testing.
> 
> Pali, I can only add a tag from you if you provide it in email. If you want your
> reviewed by on these, please include that in your replies.

Sorry, add my Acked-by.

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

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

[Darren Hart]

On Sun, Mar 19, 2017 at 07:06:19PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 19-03-17 16:10, Pali Rohár wrote:
> > On Thursday 16 March 2017 11:55:35 Hans de Goede wrote:
> > > Changes in v9:
> > > -Only listen to type 0x0010 kbd-led events in dell-wmi.c, these only
> > > trigger on hotkey presses
> > > -Drop the new / previous brightness comparison from dell-laptop.c now
> > > that we only get events on hotkey presses it is no longer necessary
> > > ---
> > 
> > Hi! I'm really not sure if this change is correct there.
> > 
> > Now you are only listening for keypress "change kbd backlight", but some
> > dell machines could change keyboard backlight also in other different
> > situations, like attaching AC adapter. I guess (but I'm not sure) this
> > probably does not send keypress event.
> 
> I'm not aware of any Dells doing such a thing, if we encounter any then
> we can deal with that if and when that happens.

In general, let's always focus on what we know and can test. Hypotheticals with
these drivers will trap us in endless loops of discussions that ultimately just
prevent us from moving forward.

-- 
Darren Hart
VMware Open Source Technology Center