ibm-acpi-devel — ibm-acpi Linux driver development

[ibm-acpi-devel] [PATCH v2 10/24] platform/x86: thinkpad_acpi: Move hotkey_user_mask check to tpacpi_input_send_key()

[Hans de G. <hde...@re...>]

Move hotkey_user_mask check to tpacpi_input_send_key(), this is
a preparation patch for further refactoring.

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ba440213ae49..05c1a562f6a1 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2256,6 +2256,10 @@ static void tpacpi_input_send_key(const unsigned int scancode)
 {
 	const unsigned int keycode = hotkey_keycode_map[scancode];
 
+	if (scancode < TP_ACPI_HOTKEYSCAN_ADAPTIVE_START &&
+	    !(hotkey_user_mask & (1 << scancode)))
+		return;
+
 	if (keycode != KEY_RESERVED) {
 		mutex_lock(&tpacpi_inputdev_send_mutex);
 
@@ -2275,8 +2279,7 @@ static void tpacpi_input_send_key(const unsigned int scancode)
 static void tpacpi_input_send_key_masked(const unsigned int scancode)
 {
 	hotkey_driver_event(scancode);
-	if (hotkey_user_mask & (1 << scancode))
-		tpacpi_input_send_key(scancode);
+	tpacpi_input_send_key(scancode);
 }
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Hans de G. <hde...@re...>]

Factor out the adaptive kbd non hotkey event handling into
adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().

This groups all the handling of hotkey events which do not emit
a key press event together in tpacpi_driver_event().

This also drops the returning of false as known-event value when
adaptive_keyboard_get_mode() / adaptive_keyboard_set_mode() fail.
These functions already log an error on failure, returning false just
leads to an extra messgae being logged about the hkey event being
unknown, which is wrong as the event is not unknown.

Reviewed-by: Mark Pearson <mpe...@sq...>
Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 86 +++++++++++++++-------------
 1 file changed, 46 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0bbc462d604c..f4d7f3c25a4a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3677,51 +3677,51 @@ static int adaptive_keyboard_get_next_mode(int mode)
 	return adaptive_keyboard_modes[i];
 }
 
+static void adaptive_keyboard_change_row(void)
+{
+	int mode;
+
+	if (adaptive_keyboard_mode_is_saved) {
+		mode = adaptive_keyboard_prev_mode;
+		adaptive_keyboard_mode_is_saved = false;
+	} else {
+		mode = adaptive_keyboard_get_mode();
+		if (mode < 0)
+			return;
+		mode = adaptive_keyboard_get_next_mode(mode);
+	}
+
+	adaptive_keyboard_set_mode(mode);
+}
+
+static void adaptive_keyboard_s_quickview_row(void)
+{
+	int mode;
+
+	mode = adaptive_keyboard_get_mode();
+	if (mode < 0)
+		return;
+
+	adaptive_keyboard_prev_mode = mode;
+	adaptive_keyboard_mode_is_saved = true;
+
+	adaptive_keyboard_set_mode(FUNCTION_MODE);
+}
+
 static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
 {
-	int current_mode = 0;
-	int new_mode = 0;
-
-	switch (hkey) {
-	case TP_HKEY_EV_DFR_CHANGE_ROW:
-		if (adaptive_keyboard_mode_is_saved) {
-			new_mode = adaptive_keyboard_prev_mode;
-			adaptive_keyboard_mode_is_saved = false;
-		} else {
-			current_mode = adaptive_keyboard_get_mode();
-			if (current_mode < 0)
-				return false;
-			new_mode = adaptive_keyboard_get_next_mode(
-					current_mode);
-		}
-
-		if (adaptive_keyboard_set_mode(new_mode) < 0)
-			return false;
-
+	if (tpacpi_driver_event(hkey))
 		return true;
 
-	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
-		current_mode = adaptive_keyboard_get_mode();
-		if (current_mode < 0)
-			return false;
-
-		adaptive_keyboard_prev_mode = current_mode;
-		adaptive_keyboard_mode_is_saved = true;
-
-		if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
-			return false;
-		return true;
-
-	default:
-		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
-		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
-			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
-			return false;
-		}
-		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
-				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
-		return true;
+	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
+	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
+		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
+		return false;
 	}
+
+	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
+			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
+	return true;
 }
 
 static bool hotkey_notify_extended_hotkey(const u32 hkey)
@@ -11117,6 +11117,12 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 		}
 		/* Key events are suppressed by default hotkey_user_mask */
 		return false;
+	case TP_HKEY_EV_DFR_CHANGE_ROW:
+		adaptive_keyboard_change_row();
+		return true;
+	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
+		adaptive_keyboard_s_quickview_row();
+		return true;
 	case TP_HKEY_EV_THM_CSM_COMPLETED:
 		lapsensor_refresh();
 		/* If we are already accessing DYTC then skip dytc update */
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 07/24] platform/x86: thinkpad_acpi: Make tpacpi_driver_event() return if it handled the event

[Hans de G. <hde...@re...>]

tpacpi_driver_event() already only responds to hkey events which it knows
about. Make it return a bool and return true when it has handled the event.

This avoids the need to list TP_HKEY_EV_foo values to which it responds
both in its caller and in the function itself.

Instead callers can now call it unconditionally and check the return value.

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 115 ++++++++++++++-------------
 1 file changed, 61 insertions(+), 54 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index c009885c8820..0bbc462d604c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1918,7 +1918,7 @@ static u32 hotkey_acpi_mask;		/* events enabled in firmware */
 
 static u16 *hotkey_keycode_map;
 
-static void tpacpi_driver_event(const unsigned int hkey_event);
+static bool tpacpi_driver_event(const unsigned int hkey_event);
 static void hotkey_driver_event(const unsigned int scancode);
 static void hotkey_poll_setup(const bool may_warn);
 
@@ -3726,13 +3726,8 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
 
 static bool hotkey_notify_extended_hotkey(const u32 hkey)
 {
-	switch (hkey) {
-	case TP_HKEY_EV_PRIVACYGUARD_TOGGLE:
-	case TP_HKEY_EV_AMT_TOGGLE:
-	case TP_HKEY_EV_PROFILE_TOGGLE:
-		tpacpi_driver_event(hkey);
+	if (tpacpi_driver_event(hkey))
 		return true;
-	}
 
 	if (hkey >= TP_HKEY_EV_EXTENDED_KEY_START &&
 	    hkey <= TP_HKEY_EV_EXTENDED_KEY_END) {
@@ -11081,72 +11076,84 @@ static struct platform_driver tpacpi_hwmon_pdriver = {
  * HKEY event callout for other subdrivers go here
  * (yes, it is ugly, but it is quick, safe, and gets the job done
  */
-static void tpacpi_driver_event(const unsigned int hkey_event)
+static bool tpacpi_driver_event(const unsigned int hkey_event)
 {
-	if (ibm_backlight_device) {
-		switch (hkey_event) {
-		case TP_HKEY_EV_BRGHT_UP:
-		case TP_HKEY_EV_BRGHT_DOWN:
+	switch (hkey_event) {
+	case TP_HKEY_EV_BRGHT_UP:
+	case TP_HKEY_EV_BRGHT_DOWN:
+		if (ibm_backlight_device)
 			tpacpi_brightness_notify_change();
-		}
-	}
-	if (alsa_card) {
-		switch (hkey_event) {
-		case TP_HKEY_EV_VOL_UP:
-		case TP_HKEY_EV_VOL_DOWN:
-		case TP_HKEY_EV_VOL_MUTE:
-			volume_alsa_notify_change();
-		}
-	}
-	if (tp_features.kbdlight && hkey_event == TP_HKEY_EV_KBD_LIGHT) {
-		enum led_brightness brightness;
-
-		mutex_lock(&kbdlight_mutex);
-
 		/*
-		 * Check the brightness actually changed, setting the brightness
-		 * through kbdlight_set_level() also triggers this event.
+		 * Key press events are suppressed by default hotkey_user_mask
+		 * and should still be reported if explicitly requested.
 		 */
-		brightness = kbdlight_sysfs_get(NULL);
-		if (kbdlight_brightness != brightness) {
-			kbdlight_brightness = brightness;
-			led_classdev_notify_brightness_hw_changed(
-				&tpacpi_led_kbdlight.led_classdev, brightness);
+		return false;
+	case TP_HKEY_EV_VOL_UP:
+	case TP_HKEY_EV_VOL_DOWN:
+	case TP_HKEY_EV_VOL_MUTE:
+		if (alsa_card)
+			volume_alsa_notify_change();
+
+		/* Key events are suppressed by default hotkey_user_mask */
+		return false;
+	case TP_HKEY_EV_KBD_LIGHT:
+		if (tp_features.kbdlight) {
+			enum led_brightness brightness;
+
+			mutex_lock(&kbdlight_mutex);
+
+			/*
+			 * Check the brightness actually changed, setting the brightness
+			 * through kbdlight_set_level() also triggers this event.
+			 */
+			brightness = kbdlight_sysfs_get(NULL);
+			if (kbdlight_brightness != brightness) {
+				kbdlight_brightness = brightness;
+				led_classdev_notify_brightness_hw_changed(
+					&tpacpi_led_kbdlight.led_classdev, brightness);
+			}
+
+			mutex_unlock(&kbdlight_mutex);
 		}
-
-		mutex_unlock(&kbdlight_mutex);
-	}
-
-	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
+		/* Key events are suppressed by default hotkey_user_mask */
+		return false;
+	case TP_HKEY_EV_THM_CSM_COMPLETED:
 		lapsensor_refresh();
 		/* If we are already accessing DYTC then skip dytc update */
 		if (!atomic_add_unless(&dytc_ignore_event, -1, 0))
 			dytc_profile_refresh();
-	}
 
-	if (lcdshadow_dev && hkey_event == TP_HKEY_EV_PRIVACYGUARD_TOGGLE) {
-		enum drm_privacy_screen_status old_hw_state;
-		bool changed;
+		return true;
+	case TP_HKEY_EV_PRIVACYGUARD_TOGGLE:
+		if (lcdshadow_dev) {
+			enum drm_privacy_screen_status old_hw_state;
+			bool changed;
 
-		mutex_lock(&lcdshadow_dev->lock);
-		old_hw_state = lcdshadow_dev->hw_state;
-		lcdshadow_get_hw_state(lcdshadow_dev);
-		changed = lcdshadow_dev->hw_state != old_hw_state;
-		mutex_unlock(&lcdshadow_dev->lock);
+			mutex_lock(&lcdshadow_dev->lock);
+			old_hw_state = lcdshadow_dev->hw_state;
+			lcdshadow_get_hw_state(lcdshadow_dev);
+			changed = lcdshadow_dev->hw_state != old_hw_state;
+			mutex_unlock(&lcdshadow_dev->lock);
 
-		if (changed)
-			drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
-	}
-	if (hkey_event == TP_HKEY_EV_AMT_TOGGLE) {
+			if (changed)
+				drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
+		}
+		return true;
+	case TP_HKEY_EV_AMT_TOGGLE:
 		/* If we're enabling AMT we need to force balanced mode */
 		if (!dytc_amt_active)
 			/* This will also set AMT mode enabled */
 			dytc_profile_set(NULL, PLATFORM_PROFILE_BALANCED);
 		else
 			dytc_control_amt(!dytc_amt_active);
-	}
-	if (hkey_event == TP_HKEY_EV_PROFILE_TOGGLE)
+
+		return true;
+	case TP_HKEY_EV_PROFILE_TOGGLE:
 		platform_profile_cycle();
+		return true;
+	}
+
+	return false;
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 13/24] platform/x86: thinkpad_acpi: Move hkey > scancode mapping to tpacpi_input_send_key()

[Hans de G. <hde...@re...>]

Move the mapping of hkey events to scancodes to tpacpi_input_send_key(),
this results in a nice cleanup and prepares things for adding sparse-keymap
support.

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 81 +++++++++-------------------
 1 file changed, 24 insertions(+), 57 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 593093884cc5..08419dede995 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2250,15 +2250,28 @@ static void tpacpi_input_send_tabletsw(void)
 	}
 }
 
-/* Do NOT call without validating scancode first */
-static void tpacpi_input_send_key(const unsigned int scancode)
+static bool tpacpi_input_send_key(const u32 hkey)
 {
-	const unsigned int keycode = hotkey_keycode_map[scancode];
+	unsigned int keycode, scancode;
 
-	if (scancode < TP_ACPI_HOTKEYSCAN_ADAPTIVE_START &&
-	    !(hotkey_user_mask & (1 << scancode)))
-		return;
+	if (hkey >= TP_HKEY_EV_ORIG_KEY_START &&
+	    hkey <= TP_HKEY_EV_ORIG_KEY_END) {
+		scancode = hkey - TP_HKEY_EV_ORIG_KEY_START;
+		if (!(hotkey_user_mask & (1 << scancode)))
+			return true; /* Not reported but still a known code */
+	} else if (hkey >= TP_HKEY_EV_ADAPTIVE_KEY_START &&
+		   hkey <= TP_HKEY_EV_ADAPTIVE_KEY_END) {
+		scancode = hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
+			   TP_ACPI_HOTKEYSCAN_ADAPTIVE_START;
+	} else if (hkey >= TP_HKEY_EV_EXTENDED_KEY_START &&
+		   hkey <= TP_HKEY_EV_EXTENDED_KEY_END) {
+		scancode = hkey - TP_HKEY_EV_EXTENDED_KEY_START +
+			   TP_ACPI_HOTKEYSCAN_EXTENDED_START;
+	} else {
+		return false;
+	}
 
+	keycode = hotkey_keycode_map[scancode];
 	if (keycode != KEY_RESERVED) {
 		mutex_lock(&tpacpi_inputdev_send_mutex);
 
@@ -2272,6 +2285,8 @@ static void tpacpi_input_send_key(const unsigned int scancode)
 
 		mutex_unlock(&tpacpi_inputdev_send_mutex);
 	}
+
+	return true;
 }
 
 #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
@@ -2281,7 +2296,7 @@ static struct tp_acpi_drv_struct ibm_hotkey_acpidriver;
 static void tpacpi_hotkey_send_key(unsigned int scancode)
 {
 	tpacpi_driver_event(TP_HKEY_EV_ORIG_KEY_START + scancode);
-	tpacpi_input_send_key(scancode);
+	tpacpi_input_send_key(TP_HKEY_EV_ORIG_KEY_START + scancode);
 }
 
 static void hotkey_read_nvram(struct tp_nvram_state *n, const u32 m)
@@ -3704,42 +3719,15 @@ static void adaptive_keyboard_s_quickview_row(void)
 	adaptive_keyboard_set_mode(FUNCTION_MODE);
 }
 
-static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
-{
-	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
-	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
-		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
-		return false;
-	}
-
-	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
-			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
-	return true;
-}
-
-static bool hotkey_notify_extended_hotkey(const u32 hkey)
-{
-	if (hkey >= TP_HKEY_EV_EXTENDED_KEY_START &&
-	    hkey <= TP_HKEY_EV_EXTENDED_KEY_END) {
-		unsigned int scancode = hkey - TP_HKEY_EV_EXTENDED_KEY_START +
-					TP_ACPI_HOTKEYSCAN_EXTENDED_START;
-		tpacpi_input_send_key(scancode);
-		return true;
-	}
-
-	return false;
-}
-
 /* 0x1000-0x1FFF: key presses */
 static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 {
-	unsigned int scancode = hkey - TP_HKEY_EV_ORIG_KEY_START;
-
 	/* Never send ACPI netlink events for original hotkeys (hkey: 0x1001 - 0x1020) */
 	if (hkey >= TP_HKEY_EV_ORIG_KEY_START && hkey <= TP_HKEY_EV_ORIG_KEY_END) {
 		*send_acpi_ev = false;
 
 		/* Original hotkeys may be polled from NVRAM instead */
+		unsigned int scancode = hkey - TP_HKEY_EV_ORIG_KEY_START;
 		if (hotkey_source_mask & (1 << scancode))
 			return true;
 	}
@@ -3747,28 +3735,7 @@ static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 	if (tpacpi_driver_event(hkey))
 		return true;
 
-	/*
-	 * Original events are in the 0x10XX range, the adaptive keyboard
-	 * found in 2014 X1 Carbon emits events are of 0x11XX. In 2017
-	 * models, additional keys are emitted through 0x13XX.
-	 */
-	switch ((hkey >> 8) & 0xf) {
-	case 0:
-		if (hkey >= TP_HKEY_EV_ORIG_KEY_START &&
-		    hkey <= TP_HKEY_EV_ORIG_KEY_END) {
-			tpacpi_input_send_key(scancode);
-			return true;
-		}
-		break;
-
-	case 1:
-		return adaptive_keyboard_hotkey_notify_hotkey(hkey);
-
-	case 3:
-		return hotkey_notify_extended_hotkey(hkey);
-	}
-
-	return false;
+	return tpacpi_input_send_key(hkey);
 }
 
 /* 0x2000-0x2FFF: Wakeup reason */
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 09/24] platform/x86: thinkpad_acpi: Move special original hotkeys handling out of switch-case

[Hans de G. <hde...@re...>]

Move the special handling (send_acpi_ev = false, hotkey_source_mask check)
for original hotkeys out of the switch-case in hotkey_notify_hotkey().
This is a preparation patch for further refactoring.

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index f4d7f3c25a4a..ba440213ae49 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3745,6 +3745,15 @@ static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 {
 	unsigned int scancode = hkey - TP_HKEY_EV_ORIG_KEY_START;
 
+	/* Never send ACPI netlink events for original hotkeys (hkey: 0x1001 - 0x1020) */
+	if (hkey >= TP_HKEY_EV_ORIG_KEY_START && hkey <= TP_HKEY_EV_ORIG_KEY_END) {
+		*send_acpi_ev = false;
+
+		/* Original hotkeys may be polled from NVRAM instead */
+		if (hotkey_source_mask & (1 << scancode))
+			return true;
+	}
+
 	/*
 	 * Original events are in the 0x10XX range, the adaptive keyboard
 	 * found in 2014 X1 Carbon emits events are of 0x11XX. In 2017
@@ -3754,10 +3763,7 @@ static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 	case 0:
 		if (hkey >= TP_HKEY_EV_ORIG_KEY_START &&
 		    hkey <= TP_HKEY_EV_ORIG_KEY_END) {
-			if (!(hotkey_source_mask & (1 << scancode)))
-				tpacpi_input_send_key_masked(scancode);
-
-			*send_acpi_ev = false;
+			tpacpi_input_send_key_masked(scancode);
 			return true;
 		}
 		break;
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 03/24] platform/x86: thinkpad_acpi: Drop setting send_/ignore_acpi_ev defaults twice

[Hans de G. <hde...@re...>]

send_acpi_ev and ignore_acpi_ev are already initialized to true and false
respectively by hotkey_notify() before calling the various helpers. Drop
the needless re-initialization from the helpers.

Reviewed-by: Ilpo Järvinen <ilp...@li...>
Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index fc5681808c3b..007223fded30 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3754,14 +3754,12 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey)
 	return false;
 }
 
+/* 0x1000-0x1FFF: key presses */
 static bool hotkey_notify_hotkey(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
 {
-	/* 0x1000-0x1FFF: key presses */
 	unsigned int scancode = hkey & 0xfff;
-	*send_acpi_ev = true;
-	*ignore_acpi_ev = false;
 
 	/*
 	 * Original events are in the 0x10XX range, the adaptive keyboard
@@ -3794,14 +3792,11 @@ static bool hotkey_notify_hotkey(const u32 hkey,
 	return false;
 }
 
+/* 0x2000-0x2FFF: Wakeup reason */
 static bool hotkey_notify_wakeup(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
 {
-	/* 0x2000-0x2FFF: Wakeup reason */
-	*send_acpi_ev = true;
-	*ignore_acpi_ev = false;
-
 	switch (hkey) {
 	case TP_HKEY_EV_WKUP_S3_UNDOCK: /* suspend, undock */
 	case TP_HKEY_EV_WKUP_S4_UNDOCK: /* hibernation, undock */
@@ -3834,14 +3829,11 @@ static bool hotkey_notify_wakeup(const u32 hkey,
 	return true;
 }
 
+/* 0x4000-0x4FFF: dock-related events */
 static bool hotkey_notify_dockevent(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
 {
-	/* 0x4000-0x4FFF: dock-related events */
-	*send_acpi_ev = true;
-	*ignore_acpi_ev = false;
-
 	switch (hkey) {
 	case TP_HKEY_EV_UNDOCK_ACK:
 		/* ACPI undock operation completed after wakeup */
@@ -3879,14 +3871,11 @@ static bool hotkey_notify_dockevent(const u32 hkey,
 	}
 }
 
+/* 0x5000-0x5FFF: human interface helpers */
 static bool hotkey_notify_usrevent(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
 {
-	/* 0x5000-0x5FFF: human interface helpers */
-	*send_acpi_ev = true;
-	*ignore_acpi_ev = false;
-
 	switch (hkey) {
 	case TP_HKEY_EV_PEN_INSERTED:  /* X61t: tablet pen inserted into bay */
 	case TP_HKEY_EV_PEN_REMOVED:   /* X61t: tablet pen removed from bay */
@@ -3914,14 +3903,11 @@ static bool hotkey_notify_usrevent(const u32 hkey,
 static void thermal_dump_all_sensors(void);
 static void palmsensor_refresh(void);
 
+/* 0x6000-0x6FFF: thermal alarms/notices and keyboard events */
 static bool hotkey_notify_6xxx(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
 {
-	/* 0x6000-0x6FFF: thermal alarms/notices and keyboard events */
-	*send_acpi_ev = true;
-	*ignore_acpi_ev = false;
-
 	switch (hkey) {
 	case TP_HKEY_EV_THM_TABLE_CHANGED:
 		pr_debug("EC reports: Thermal Table has changed\n");
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 06/24] platform/x86: thinkpad_acpi: Do hkey to scancode translation later

[Hans de G. <hde...@re...>]

Modify hotkey_notify_hotkey() and it helpers to mostly directly operate
on hkey codes (TP_HKEY_EV_* returned by "MHKP") instead of on the 0 -
TPACPI_HOTKEY_MAP_LEN scancodes used for scancode -> keycode translation.

Keeping things in the hkey format as long a possible is a bit cleaner and
this patch prepares things for moving to sparse-keymaps.

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 71 ++++++++++++++--------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 126e39367924..c009885c8820 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -157,15 +157,30 @@ enum {
 
 /* HKEY events */
 enum tpacpi_hkey_event_t {
-	/* Hotkey-related */
-	TP_HKEY_EV_HOTKEY_BASE		= 0x1001, /* first hotkey (FN+F1) */
+	/* Original hotkeys */
+	TP_HKEY_EV_ORIG_KEY_START	= 0x1001, /* First hotkey (FN+F1) */
 	TP_HKEY_EV_BRGHT_UP		= 0x1010, /* Brightness up */
 	TP_HKEY_EV_BRGHT_DOWN		= 0x1011, /* Brightness down */
 	TP_HKEY_EV_KBD_LIGHT		= 0x1012, /* Thinklight/kbd backlight */
 	TP_HKEY_EV_VOL_UP		= 0x1015, /* Volume up or unmute */
 	TP_HKEY_EV_VOL_DOWN		= 0x1016, /* Volume down or unmute */
 	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
+	TP_HKEY_EV_ORIG_KEY_END		= 0x1020, /* Last original hotkey code */
+
+	/* Adaptive keyboard (2014 X1 Carbon) */
+	TP_HKEY_EV_DFR_CHANGE_ROW	= 0x1101, /* Change adaptive kbd Fn row mode */
+	TP_HKEY_EV_DFR_S_QUICKVIEW_ROW	= 0x1102, /* Set adap. kbd Fn row to function mode */
+	TP_HKEY_EV_ADAPTIVE_KEY_START	= 0x1103, /* First hotkey code on adaptive kbd */
+	TP_HKEY_EV_ADAPTIVE_KEY_END	= 0x1116, /* Last hotkey code on adaptive kbd */
+
+	/* Extended hotkey events in 2017+ models */
+	TP_HKEY_EV_EXTENDED_KEY_START	= 0x1300, /* First extended hotkey code */
 	TP_HKEY_EV_PRIVACYGUARD_TOGGLE	= 0x130f, /* Toggle priv.guard on/off */
+	TP_HKEY_EV_EXTENDED_KEY_END	= 0x1319, /* Last extended hotkey code using
+						   * hkey -> scancode translation for
+						   * compat. Later codes are entered
+						   * directly in the sparse-keymap.
+						   */
 	TP_HKEY_EV_AMT_TOGGLE		= 0x131a, /* Toggle AMT on/off */
 	TP_HKEY_EV_PROFILE_TOGGLE	= 0x131f, /* Toggle platform profile */
 
@@ -1752,7 +1767,7 @@ enum {	/* hot key scan codes (derived from ACPI DSDT) */
 	TP_ACPI_HOTKEYSCAN_UNK8,
 
 	/* Adaptive keyboard keycodes */
-	TP_ACPI_HOTKEYSCAN_ADAPTIVE_START,
+	TP_ACPI_HOTKEYSCAN_ADAPTIVE_START, /* 32 / 0x20 */
 	TP_ACPI_HOTKEYSCAN_MUTE2        = TP_ACPI_HOTKEYSCAN_ADAPTIVE_START,
 	TP_ACPI_HOTKEYSCAN_BRIGHTNESS_ZERO,
 	TP_ACPI_HOTKEYSCAN_CLIPPING_TOOL,
@@ -1775,7 +1790,7 @@ enum {	/* hot key scan codes (derived from ACPI DSDT) */
 	TP_ACPI_HOTKEYSCAN_ROTATE_DISPLAY,
 
 	/* Lenovo extended keymap, starting at 0x1300 */
-	TP_ACPI_HOTKEYSCAN_EXTENDED_START,
+	TP_ACPI_HOTKEYSCAN_EXTENDED_START, /* 52 / 0x34 */
 	/* first new observed key (star, favorites) is 0x1311 */
 	TP_ACPI_HOTKEYSCAN_STAR = 69,
 	TP_ACPI_HOTKEYSCAN_CLIPPING_TOOL2,
@@ -3612,10 +3627,6 @@ static const int adaptive_keyboard_modes[] = {
 	FUNCTION_MODE
 };
 
-#define DFR_CHANGE_ROW			0x101
-#define DFR_SHOW_QUICKVIEW_ROW		0x102
-#define FIRST_ADAPTIVE_KEY		0x103
-
 /* press Fn key a while second, it will switch to Function Mode. Then
  * release Fn key, previous mode be restored.
  */
@@ -3666,13 +3677,13 @@ static int adaptive_keyboard_get_next_mode(int mode)
 	return adaptive_keyboard_modes[i];
 }
 
-static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
+static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
 {
 	int current_mode = 0;
 	int new_mode = 0;
 
-	switch (scancode) {
-	case DFR_CHANGE_ROW:
+	switch (hkey) {
+	case TP_HKEY_EV_DFR_CHANGE_ROW:
 		if (adaptive_keyboard_mode_is_saved) {
 			new_mode = adaptive_keyboard_prev_mode;
 			adaptive_keyboard_mode_is_saved = false;
@@ -3689,7 +3700,7 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
 
 		return true;
 
-	case DFR_SHOW_QUICKVIEW_ROW:
+	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
 		current_mode = adaptive_keyboard_get_mode();
 		if (current_mode < 0)
 			return false;
@@ -3702,15 +3713,12 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
 		return true;
 
 	default:
-		if (scancode < FIRST_ADAPTIVE_KEY ||
-		    scancode >= FIRST_ADAPTIVE_KEY +
-		    TP_ACPI_HOTKEYSCAN_EXTENDED_START -
-		    TP_ACPI_HOTKEYSCAN_ADAPTIVE_START) {
-			pr_info("Unhandled adaptive keyboard key: 0x%x\n",
-				scancode);
+		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
+		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
+			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
 			return false;
 		}
-		tpacpi_input_send_key(scancode - FIRST_ADAPTIVE_KEY +
+		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
 				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
 		return true;
 	}
@@ -3718,8 +3726,6 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
 
 static bool hotkey_notify_extended_hotkey(const u32 hkey)
 {
-	unsigned int scancode;
-
 	switch (hkey) {
 	case TP_HKEY_EV_PRIVACYGUARD_TOGGLE:
 	case TP_HKEY_EV_AMT_TOGGLE:
@@ -3728,13 +3734,10 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey)
 		return true;
 	}
 
-	/* Extended keycodes start at 0x300 and our offset into the map
-	 * TP_ACPI_HOTKEYSCAN_EXTENDED_START. The calculated scancode
-	 * will be positive, but might not be in the correct range.
-	 */
-	scancode = (hkey & 0xfff) - (0x300 - TP_ACPI_HOTKEYSCAN_EXTENDED_START);
-	if (scancode >= TP_ACPI_HOTKEYSCAN_EXTENDED_START &&
-	    scancode < TPACPI_HOTKEY_MAP_LEN) {
+	if (hkey >= TP_HKEY_EV_EXTENDED_KEY_START &&
+	    hkey <= TP_HKEY_EV_EXTENDED_KEY_END) {
+		unsigned int scancode = hkey - TP_HKEY_EV_EXTENDED_KEY_START +
+					TP_ACPI_HOTKEYSCAN_EXTENDED_START;
 		tpacpi_input_send_key(scancode);
 		return true;
 	}
@@ -3745,7 +3748,7 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey)
 /* 0x1000-0x1FFF: key presses */
 static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 {
-	unsigned int scancode = hkey & 0xfff;
+	unsigned int scancode = hkey - TP_HKEY_EV_ORIG_KEY_START;
 
 	/*
 	 * Original events are in the 0x10XX range, the adaptive keyboard
@@ -3754,10 +3757,8 @@ static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 	 */
 	switch ((hkey >> 8) & 0xf) {
 	case 0:
-		if (scancode > 0 &&
-		    scancode <= TP_ACPI_HOTKEYSCAN_ADAPTIVE_START) {
-			/* HKEY event 0x1001 is scancode 0x00 */
-			scancode--;
+		if (hkey >= TP_HKEY_EV_ORIG_KEY_START &&
+		    hkey <= TP_HKEY_EV_ORIG_KEY_END) {
 			if (!(hotkey_source_mask & (1 << scancode)))
 				tpacpi_input_send_key_masked(scancode);
 
@@ -3767,7 +3768,7 @@ static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 		break;
 
 	case 1:
-		return adaptive_keyboard_hotkey_notify_hotkey(scancode);
+		return adaptive_keyboard_hotkey_notify_hotkey(hkey);
 
 	case 3:
 		return hotkey_notify_extended_hotkey(hkey);
@@ -11150,7 +11151,7 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 
 static void hotkey_driver_event(const unsigned int scancode)
 {
-	tpacpi_driver_event(TP_HKEY_EV_HOTKEY_BASE + scancode);
+	tpacpi_driver_event(TP_HKEY_EV_ORIG_KEY_START + scancode);
 }
 
 /* --------------------------------------------------------------------- */
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 02/24] platform/x86: thinkpad_acpi: Provide hotkey_poll_stop_sync() dummy

[Hans de G. <hde...@re...>]

Provide a hotkey_poll_stop_sync() dummy implementation when
CONFIG_THINKPAD_ACPI_HOTKEY_POLL, so that the #ifdef-ery around
hotkey_poll_stop_sync() can be removed from hotkey_exit().

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ba4df8f68c2a..fc5681808c3b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2575,6 +2575,9 @@ static void hotkey_poll_setup_safe(const bool __unused)
 {
 }
 
+static void hotkey_poll_stop_sync(void)
+{
+}
 #endif /* CONFIG_THINKPAD_ACPI_HOTKEY_POLL */
 
 static int hotkey_inputdev_open(struct input_dev *dev)
@@ -3045,9 +3048,7 @@ static void tpacpi_send_radiosw_update(void)
 static void hotkey_exit(void)
 {
 	mutex_lock(&hotkey_mutex);
-#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
 	hotkey_poll_stop_sync();
-#endif
 	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
 		   "restoring original HKEY status and mask\n");
 	/* yes, there is a bitwise or below, we want the
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 01/24] platform/x86: thinkpad_acpi: Take hotkey_mutex during hotkey_exit()

[Hans de G. <hde...@re...>]

hotkey_exit() already takes the mutex around the hotkey_poll_stop_sync()
call, but not around the other calls.

commit 38831eaf7d4c ("platform/x86: thinkpad_acpi: use lockdep
annotations") has added lockdep_assert_held() checks to various hotkey
functions.

These lockdep_assert_held() checks fail causing WARN() backtraces in
dmesg due to missing locking in hotkey_exit(), fix this.

Fixes: 38831eaf7d4c ("platform/x86: thinkpad_acpi: use lockdep annotations")
Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 771aaa7ae4cf..ba4df8f68c2a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3044,10 +3044,9 @@ static void tpacpi_send_radiosw_update(void)
 
 static void hotkey_exit(void)
 {
-#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
 	mutex_lock(&hotkey_mutex);
+#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
 	hotkey_poll_stop_sync();
-	mutex_unlock(&hotkey_mutex);
 #endif
 	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
 		   "restoring original HKEY status and mask\n");
@@ -3057,6 +3056,8 @@ static void hotkey_exit(void)
 	      hotkey_mask_set(hotkey_orig_mask)) |
 	     hotkey_status_set(false)) != 0)
 		pr_err("failed to restore hot key mask to BIOS defaults\n");
+
+	mutex_unlock(&hotkey_mutex);
 }
 
 static void __init hotkey_unmap(const unsigned int scancode)
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 04/24] platform/x86: thinkpad_acpi: Drop ignore_acpi_ev

[Hans de G. <hde...@re...>]

Setting ignore_acpi_ev to true has the same result as setting
send_acpi_ev to false, so there is no need to have both.

Drop ignore_acpi_ev.

Tested-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 56 +++++++++-------------------
 1 file changed, 17 insertions(+), 39 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 007223fded30..bb6b880a5b50 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3755,9 +3755,7 @@ static bool hotkey_notify_extended_hotkey(const u32 hkey)
 }
 
 /* 0x1000-0x1FFF: key presses */
-static bool hotkey_notify_hotkey(const u32 hkey,
-				 bool *send_acpi_ev,
-				 bool *ignore_acpi_ev)
+static bool hotkey_notify_hotkey(const u32 hkey, bool *send_acpi_ev)
 {
 	unsigned int scancode = hkey & 0xfff;
 
@@ -3772,12 +3770,10 @@ static bool hotkey_notify_hotkey(const u32 hkey,
 		    scancode <= TP_ACPI_HOTKEYSCAN_ADAPTIVE_START) {
 			/* HKEY event 0x1001 is scancode 0x00 */
 			scancode--;
-			if (!(hotkey_source_mask & (1 << scancode))) {
+			if (!(hotkey_source_mask & (1 << scancode)))
 				tpacpi_input_send_key_masked(scancode);
-				*send_acpi_ev = false;
-			} else {
-				*ignore_acpi_ev = true;
-			}
+
+			*send_acpi_ev = false;
 			return true;
 		}
 		break;
@@ -3793,21 +3789,19 @@ static bool hotkey_notify_hotkey(const u32 hkey,
 }
 
 /* 0x2000-0x2FFF: Wakeup reason */
-static bool hotkey_notify_wakeup(const u32 hkey,
-				 bool *send_acpi_ev,
-				 bool *ignore_acpi_ev)
+static bool hotkey_notify_wakeup(const u32 hkey, bool *send_acpi_ev)
 {
 	switch (hkey) {
 	case TP_HKEY_EV_WKUP_S3_UNDOCK: /* suspend, undock */
 	case TP_HKEY_EV_WKUP_S4_UNDOCK: /* hibernation, undock */
 		hotkey_wakeup_reason = TP_ACPI_WAKEUP_UNDOCK;
-		*ignore_acpi_ev = true;
+		*send_acpi_ev = false;
 		break;
 
 	case TP_HKEY_EV_WKUP_S3_BAYEJ: /* suspend, bay eject */
 	case TP_HKEY_EV_WKUP_S4_BAYEJ: /* hibernation, bay eject */
 		hotkey_wakeup_reason = TP_ACPI_WAKEUP_BAYEJ;
-		*ignore_acpi_ev = true;
+		*send_acpi_ev = false;
 		break;
 
 	case TP_HKEY_EV_WKUP_S3_BATLOW: /* Battery on critical low level/S3 */
@@ -3830,9 +3824,7 @@ static bool hotkey_notify_wakeup(const u32 hkey,
 }
 
 /* 0x4000-0x4FFF: dock-related events */
-static bool hotkey_notify_dockevent(const u32 hkey,
-				 bool *send_acpi_ev,
-				 bool *ignore_acpi_ev)
+static bool hotkey_notify_dockevent(const u32 hkey, bool *send_acpi_ev)
 {
 	switch (hkey) {
 	case TP_HKEY_EV_UNDOCK_ACK:
@@ -3863,7 +3855,6 @@ static bool hotkey_notify_dockevent(const u32 hkey,
 	case TP_HKEY_EV_KBD_COVER_ATTACH:
 	case TP_HKEY_EV_KBD_COVER_DETACH:
 		*send_acpi_ev = false;
-		*ignore_acpi_ev = true;
 		return true;
 
 	default:
@@ -3872,9 +3863,7 @@ static bool hotkey_notify_dockevent(const u32 hkey,
 }
 
 /* 0x5000-0x5FFF: human interface helpers */
-static bool hotkey_notify_usrevent(const u32 hkey,
-				 bool *send_acpi_ev,
-				 bool *ignore_acpi_ev)
+static bool hotkey_notify_usrevent(const u32 hkey, bool *send_acpi_ev)
 {
 	switch (hkey) {
 	case TP_HKEY_EV_PEN_INSERTED:  /* X61t: tablet pen inserted into bay */
@@ -3892,7 +3881,7 @@ static bool hotkey_notify_usrevent(const u32 hkey,
 	case TP_HKEY_EV_LID_OPEN:	/* Lid opened */
 	case TP_HKEY_EV_BRGHT_CHANGED:	/* brightness changed */
 		/* do not propagate these events */
-		*ignore_acpi_ev = true;
+		*send_acpi_ev = false;
 		return true;
 
 	default:
@@ -3904,9 +3893,7 @@ static void thermal_dump_all_sensors(void);
 static void palmsensor_refresh(void);
 
 /* 0x6000-0x6FFF: thermal alarms/notices and keyboard events */
-static bool hotkey_notify_6xxx(const u32 hkey,
-				 bool *send_acpi_ev,
-				 bool *ignore_acpi_ev)
+static bool hotkey_notify_6xxx(const u32 hkey, bool *send_acpi_ev)
 {
 	switch (hkey) {
 	case TP_HKEY_EV_THM_TABLE_CHANGED:
@@ -3953,14 +3940,12 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		/* key press events, we just ignore them as long as the EC
 		 * is still reporting them in the normal keyboard stream */
 		*send_acpi_ev = false;
-		*ignore_acpi_ev = true;
 		return true;
 
 	case TP_HKEY_EV_KEY_FN_ESC:
 		/* Get the media key status to force the status LED to update */
 		acpi_evalf(hkey_handle, NULL, "GMKS", "v");
 		*send_acpi_ev = false;
-		*ignore_acpi_ev = true;
 		return true;
 
 	case TP_HKEY_EV_TABLET_CHANGED:
@@ -3988,7 +3973,6 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 {
 	u32 hkey;
 	bool send_acpi_ev;
-	bool ignore_acpi_ev;
 	bool known_ev;
 
 	if (event != 0x80) {
@@ -4013,18 +3997,15 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 		}
 
 		send_acpi_ev = true;
-		ignore_acpi_ev = false;
 
 		switch (hkey >> 12) {
 		case 1:
 			/* 0x1000-0x1FFF: key presses */
-			known_ev = hotkey_notify_hotkey(hkey, &send_acpi_ev,
-						 &ignore_acpi_ev);
+			known_ev = hotkey_notify_hotkey(hkey, &send_acpi_ev);
 			break;
 		case 2:
 			/* 0x2000-0x2FFF: Wakeup reason */
-			known_ev = hotkey_notify_wakeup(hkey, &send_acpi_ev,
-						 &ignore_acpi_ev);
+			known_ev = hotkey_notify_wakeup(hkey, &send_acpi_ev);
 			break;
 		case 3:
 			/* 0x3000-0x3FFF: bay-related wakeups */
@@ -4045,19 +4026,16 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 			break;
 		case 4:
 			/* 0x4000-0x4FFF: dock-related events */
-			known_ev = hotkey_notify_dockevent(hkey, &send_acpi_ev,
-						&ignore_acpi_ev);
+			known_ev = hotkey_notify_dockevent(hkey, &send_acpi_ev);
 			break;
 		case 5:
 			/* 0x5000-0x5FFF: human interface helpers */
-			known_ev = hotkey_notify_usrevent(hkey, &send_acpi_ev,
-						 &ignore_acpi_ev);
+			known_ev = hotkey_notify_usrevent(hkey, &send_acpi_ev);
 			break;
 		case 6:
 			/* 0x6000-0x6FFF: thermal alarms/notices and
 			 *                keyboard events */
-			known_ev = hotkey_notify_6xxx(hkey, &send_acpi_ev,
-						 &ignore_acpi_ev);
+			known_ev = hotkey_notify_6xxx(hkey, &send_acpi_ev);
 			break;
 		case 7:
 			/* 0x7000-0x7FFF: misc */
@@ -4079,7 +4057,7 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 		}
 
 		/* netlink events */
-		if (!ignore_acpi_ev && send_acpi_ev) {
+		if (send_acpi_ev) {
 			acpi_bus_generate_netlink_event(
 					ibm->acpi->device->pnp.device_class,
 					dev_name(&ibm->acpi->device->dev),
-- 
2.44.0

[ibm-acpi-devel] [PATCH v2 00/24] platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys

[Hans de G. <hde...@re...>]

Hi All,

Here is v2 of my patch-series to refactor thinkpad_acpi's hotkey handling
and to add support for some new hotkeys on new models.

Changes in v2:
- Some small code style tweaks in response to reviews
- Add various Reviewed-by and Tested-by tags

Relevant parts of v1 cover-letter:

I have been very careful to not introduce any changes wrt support for
the original ThinkPad models / hotkeys which use the hotkey_*_mask
related code.

I've also done my best to avoid any *significant* functional change but
there are still some functional changes, which should all not impact
userspace compatibility:

1. Adaptive keyboard special keys will now also send EV_MSC events with
   the scancode, just like all the other hotkeys.

2. Rely on the input core for KEY_RESERVED suppression. This means that
   keys marked as KEY_RESERVED will still send EV_MSC evdev events when
   pressed (which helps users with mapping them to non reserved KEY_FOO
   values if desired).

3. Align the keycodes for volume up/down/mute and brightness up/down with
   those set by userspace through udev upstream's hwdb. Note these are all
   for keys which are suppressed by hotkey_reserved_mask by default.
   So this is only a functional change for users who override the default
   hotkey-mask *and* who do not have udev's default hwdb installed.

4. Suppress ACPI netlink event generation for unknown 0x1xxx hkey events to
   avoid userspace starting to rely on the netlink events for new hotkeys
   before these have been added to the keymap, only to have the netlink
   events get disabled by the adding of the new hotkeys to the keymap.

   This should not cause a behavior change for existing models since all
   currently known 0x1xxx events have a mapping.

Here is a quick breakdown of the patches in this series:

Patch 1 - 2: Fix a small locking issue on rmmod the only problem here
   really is a lockdep warning, so I plan to route these fixes through
   for-next together with the rest to keep things simple.

Patch 3 - 14: Do a bunch of cleanups and refactoring

Patch 15: Implements functional change no 4. I really kinda want to just
   completely disable ACPI netlink event generation when also sending evdev
   events instead of reporting these twice. But for the 0x11xx / 0x13xx
   hkey events the kernel has send ACPI netlink events for years now. So
   this disables ACPI netlink events for any new hotkeys going forward.

Patch 16 - 18: Refactor / cleanup reserved key handling

Patch 19: Actually move to sparse-keymaps

Patch 20: Update the keymap for 2 adaptive kbd Fn row keys

Patch 21 - 24: Mark's original patches adding support for the new Fn + N
   key combo and for trackpoint doubletap slightly reworked to use
   the new sparse-keymap.

Regards,

Hans


Hans de Goede (20):
  platform/x86: thinkpad_acpi: Take hotkey_mutex during hotkey_exit()
  platform/x86: thinkpad_acpi: Provide hotkey_poll_stop_sync() dummy
  platform/x86: thinkpad_acpi: Drop setting send_/ignore_acpi_ev
    defaults twice
  platform/x86: thinkpad_acpi: Drop ignore_acpi_ev
  platform/x86: thinkpad_acpi: Use tpacpi_input_send_key() in adaptive
    kbd code
  platform/x86: thinkpad_acpi: Do hkey to scancode translation later
  platform/x86: thinkpad_acpi: Make tpacpi_driver_event() return if it
    handled the event
  platform/x86: thinkpad_acpi: Move adaptive kbd event handling to
    tpacpi_driver_event()
  platform/x86: thinkpad_acpi: Move special original hotkeys handling
    out of switch-case
  platform/x86: thinkpad_acpi: Move hotkey_user_mask check to
    tpacpi_input_send_key()
  platform/x86: thinkpad_acpi: Always call tpacpi_driver_event() for
    hotkeys
  platform/x86: thinkpad_acpi: Drop tpacpi_input_send_key_masked() and
    hotkey_driver_event()
  platform/x86: thinkpad_acpi: Move hkey > scancode mapping to
    tpacpi_input_send_key()
  platform/x86: thinkpad_acpi: Move tpacpi_driver_event() call to
    tpacpi_input_send_key()
  platform/x86: thinkpad_acpi: Do not send ACPI netlink events for
    unknown hotkeys
  platform/x86: thinkpad_acpi: Change hotkey_reserved_mask
    initialization
  platform/x86: thinkpad_acpi: Use correct keycodes for volume and
    brightness keys
  platform/x86: thinkpad_acpi: Drop KEY_RESERVED special handling
  platform/x86: thinkpad_acpi: Switch to using sparse-keymap helpers
  platform/x86: thinkpad_acpi: Add mappings for adaptive kbd
    clipping-tool and cloud keys

Mark Pearson (4):
  platform/x86: thinkpad_acpi: Simplify known_ev handling
  platform/x86: thinkpad_acpi: Support for trackpoint doubletap
  platform/x86: thinkpad_acpi: Support for system debug info hotkey
  platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint
    doubletap

 drivers/platform/x86/thinkpad_acpi.c | 854 +++++++++++----------------
 1 file changed, 353 insertions(+), 501 deletions(-)

-- 
2.44.0

Re: [ibm-acpi-devel] [PATCH 17/24] platform/x86: thinkpad_acpi: Use correct keycodes for volume and brightness keys

[Hans de G. <hde...@re...>]

Hi,

On 4/21/24 9:11 PM, Andy Shevchenko wrote:
> On Sun, Apr 21, 2024 at 6:45 PM Hans de Goede <hde...@re...> wrote:
>>
>> Change the default keymap to report the correct keycodes for the volume and
>> brightness keys. Reporting key events for these is already filtered out by
>> the hotkey_reserved_mask which masks these keys out of hotkey_user_mask at
>> initialization time, so there is no need to also map them to KEY_RESERVED.
>>
>> This avoids users, who want these to be reported, having to also remap
>> the keycodes on top of overriding hotkey_user_mask to report these
>> and Linux userspace has already been overridding the KEY_RESERVED mappings
> 
> overriding

Ack, fixed for v2.

> 
>> with the correct keycodes through udev/hwdb/60-keyboard.hwdb for years now.
>>
>> Also drop hotkey_unmap() it was only used to dynamically map the brightness
>> keys to KEY_RESERVED and after removing that it has no remaining users.
> 
> ...
> 
>> +               /* brightness: firmware always reacts to them.
>> +                * Suppressed by default through hotkey_reserved_mask.
>> +                */
> 
>> +               /* Thinklight: firmware always react to it.
>> +                * Suppressed by default through hotkey_reserved_mask.
>> +                */
> 
>>                 /* Volume: firmware always react to it and reprograms
>>                  * the built-in *extra* mixer.  Never map it to control
>> +                * another mixer by default.
>> +                * Suppressed by default through hotkey_reserved_mask.
>> +                */
> 
> Hmm... While at it, can we rectify the block comments to follow the
> standard style?
> (I meant those which you are touching here.)

Ack, but these get moved around in:

[PATCH 19/24] platform/x86: thinkpad_acpi: Switch to using sparse-keymap helpers

So to save my self some rebasing pain I will fix up the block comment style
there instead in v2 of the series :)

Regards,

Hans

Re: [ibm-acpi-devel] [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Hans de G. <hde...@re...>]

Hi,

On 4/22/24 10:29 AM, Ilpo Järvinen wrote:
> On Sun, 21 Apr 2024, Hans de Goede wrote:
> 
>> Factor out the adaptive kbd non hotkey event handling into
>> adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
>> helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
>> TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().
>>
>> This groups all the handling of hotkey events which do not emit
>> a key press event together in tpacpi_driver_event().
>>
>> This is a preparation patch for moving to sparse-keymaps.
>>
>> Signed-off-by: Hans de Goede <hde...@re...>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++-------------
>>  1 file changed, 45 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 0bbc462d604c..e8d30f4af126 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -3677,51 +3677,50 @@ static int adaptive_keyboard_get_next_mode(int mode)
>>  	return adaptive_keyboard_modes[i];
>>  }
>>  
>> +static void adaptive_keyboard_change_row(void)
>> +{
>> +	int mode;
>> +
>> +	if (adaptive_keyboard_mode_is_saved) {
>> +		mode = adaptive_keyboard_prev_mode;
>> +		adaptive_keyboard_mode_is_saved = false;
>> +	} else {
>> +		mode = adaptive_keyboard_get_mode();
>> +		if (mode < 0)
>> +			return;
>> +		mode = adaptive_keyboard_get_next_mode(mode);
>> +	}
>> +
>> +	adaptive_keyboard_set_mode(mode);
>> +}
>> +
>> +static void adaptive_keyboard_s_quickview_row(void)
>> +{
>> +	int mode = adaptive_keyboard_get_mode();
>> +
>> +	if (mode < 0)
> 
> Please try to keep call and it's error handling together, it costs one 
> line but takes less effort to understand:
> 
> 	int mode;
> 
> 	mode = adaptive_keyboard_get_mode();
> 	if (mode < 0)

Ack, I've changed this for v2 following your suggestion.

Regards,

Hans




> 
>> +		return;
>> +
>> +	adaptive_keyboard_prev_mode = mode;
>> +	adaptive_keyboard_mode_is_saved = true;
>> +
>> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
>> +}
> 

Re: [ibm-acpi-devel] [PATCH 03/24] platform/x86: thinkpad_acpi: Drop setting send_/ignore_acpi_ev defaults twice

[Hans de G. <hde...@re...>]

Hi,

On 4/22/24 10:07 AM, Ilpo Järvinen wrote:
> On Sun, 21 Apr 2024, Hans de Goede wrote:
> 
>> send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by
> 
> Wording here is odd (but I'm not native so could be I just don't 
> understand what "true resp. false" is supposed to mean/fit into the 
> general structure of this sentence). I could nonetheless guess the 
> general meaning of the sentence despite that, but you might want to 
> consider rewording it into something that is easier to understand.

Ok, I have changed this to:

"""
send_acpi_ev and ignore_acpi_ev are already initialized to true and false
respectively by hotkey_notify() before calling the various helpers. Drop
the needless re-initialization from the helpers.
"""

now.

> 
> The code change is fine,
> 
> Reviewed-by: Ilpo Järvinen <ilp...@li...>

Thank you.

Regards,

Hans

Re: [ibm-acpi-devel] [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Hans de G. <hde...@re...>]

Hi Mark,

On 4/23/24 2:15 PM, Mark Pearson wrote:
> Hi Hans
> 
> On Tue, Apr 23, 2024, at 4:35 AM, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 4/22/24 9:27 PM, Mark Pearson wrote:
>>> Hi Hans,
>>>
>>> On Sun, Apr 21, 2024, at 11:45 AM, Hans de Goede wrote:
>>>> Factor out the adaptive kbd non hotkey event handling into
>>>> adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
>>>> helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
>>>> TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().
>>>>
>>>> This groups all the handling of hotkey events which do not emit
>>>> a key press event together in tpacpi_driver_event().
>>>>
>>>> This is a preparation patch for moving to sparse-keymaps.
>>>>
>>>> Signed-off-by: Hans de Goede <hde...@re...>
>>>> ---
>>>>  drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++-------------
>>>>  1 file changed, 45 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>>>> b/drivers/platform/x86/thinkpad_acpi.c
>>>> index 0bbc462d604c..e8d30f4af126 100644
>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>> @@ -3677,51 +3677,50 @@ static int adaptive_keyboard_get_next_mode(int 
>>>> mode)
>>>>  	return adaptive_keyboard_modes[i];
>>>>  }
>>>>
>>>> +static void adaptive_keyboard_change_row(void)
>>>> +{
>>>> +	int mode;
>>>> +
>>>> +	if (adaptive_keyboard_mode_is_saved) {
>>>> +		mode = adaptive_keyboard_prev_mode;
>>>> +		adaptive_keyboard_mode_is_saved = false;
>>>> +	} else {
>>>> +		mode = adaptive_keyboard_get_mode();
>>>> +		if (mode < 0)
>>>> +			return;
>>>> +		mode = adaptive_keyboard_get_next_mode(mode);
>>>> +	}
>>>> +
>>>> +	adaptive_keyboard_set_mode(mode);
>>>> +}
>>>> +
>>>> +static void adaptive_keyboard_s_quickview_row(void)
>>>> +{
>>>> +	int mode = adaptive_keyboard_get_mode();
>>>> +
>>>> +	if (mode < 0)
>>>> +		return;
>>>> +
>>>> +	adaptive_keyboard_prev_mode = mode;
>>>> +	adaptive_keyboard_mode_is_saved = true;
>>>> +
>>>> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
>>>> +}
>>>> +
>>>>  static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
>>>>  {
>>>> -	int current_mode = 0;
>>>> -	int new_mode = 0;
>>>> -
>>>> -	switch (hkey) {
>>>> -	case TP_HKEY_EV_DFR_CHANGE_ROW:
>>>> -		if (adaptive_keyboard_mode_is_saved) {
>>>> -			new_mode = adaptive_keyboard_prev_mode;
>>>> -			adaptive_keyboard_mode_is_saved = false;
>>>> -		} else {
>>>> -			current_mode = adaptive_keyboard_get_mode();
>>>> -			if (current_mode < 0)
>>>> -				return false;
>>>> -			new_mode = adaptive_keyboard_get_next_mode(
>>>> -					current_mode);
>>>> -		}
>>>> -
>>>> -		if (adaptive_keyboard_set_mode(new_mode) < 0)
>>>> -			return false;
>>>> -
>>>> +	if (tpacpi_driver_event(hkey))
>>>>  		return true;
>>>>
>>>> -	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>>>> -		current_mode = adaptive_keyboard_get_mode();
>>>> -		if (current_mode < 0)
>>>> -			return false;
>>>> -
>>>> -		adaptive_keyboard_prev_mode = current_mode;
>>>> -		adaptive_keyboard_mode_is_saved = true;
>>>> -
>>>> -		if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
>>>> -			return false;
>>>> -		return true;
>>>> -
>>>> -	default:
>>>> -		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>>>> -		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>>>> -			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>>>> -			return false;
>>>> -		}
>>>> -		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>>>> -				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>>>> -		return true;
>>>> +	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>>>> +	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>>>> +		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>>>> +		return false;
>>>>  	}
>>>> +
>>>> +	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>>>> +			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>>>> +	return true;
>>>>  }
>>>>
>>>>  static bool hotkey_notify_extended_hotkey(const u32 hkey)
>>>> @@ -11117,6 +11116,12 @@ static bool tpacpi_driver_event(const unsigned 
>>>> int hkey_event)
>>>>  		}
>>>>  		/* Key events are suppressed by default hotkey_user_mask */
>>>>  		return false;
>>>> +	case TP_HKEY_EV_DFR_CHANGE_ROW:
>>>> +		adaptive_keyboard_change_row();
>>>> +		return true;
>>>> +	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>>>> +		adaptive_keyboard_s_quickview_row();
>>>
>>> Was there a reason to get rid of error handling that was previously done with adaptive_keyboard_set_mode and is now not used here?
>>
>> The error handling consisted of returning false instead of true
>> (for known_ev), causing an unknown event message to get logged on
>> top of the error already logged by adaptive_keyboard_get_mode() /
>> adaptive_keyboard_set_mode().
>>
>> This second unknown event error is consfusin / not helpful so
>> I've dropped the "return false" on error behavior, note that
>> the new helpers do still return early if get_mode() fails just
>> as before.
>>
>> I'll add a note about this to the commit message for v2 of
>> the series.
>>
> Thanks for the explanation - makes sense.
> 
> Reviwed-by: Mark Pearson <mpe...@sq...>
> 
> As a note - I've been working my way thru the patches. Is it OK to send one Reviewed-by at the end for all the patches for which I had no questions, or is it better to ack each one individually?

One Reviewed-by for the series when you're done (with possible
exception for some patches you have outstanding remarks for)
is fine.

Regards,

Hans

Re: [ibm-acpi-devel] [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Mark P. <mpe...@sq...>]

Hi Hans

On Tue, Apr 23, 2024, at 4:35 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 4/22/24 9:27 PM, Mark Pearson wrote:
>> Hi Hans,
>> 
>> On Sun, Apr 21, 2024, at 11:45 AM, Hans de Goede wrote:
>>> Factor out the adaptive kbd non hotkey event handling into
>>> adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
>>> helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
>>> TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().
>>>
>>> This groups all the handling of hotkey events which do not emit
>>> a key press event together in tpacpi_driver_event().
>>>
>>> This is a preparation patch for moving to sparse-keymaps.
>>>
>>> Signed-off-by: Hans de Goede <hde...@re...>
>>> ---
>>>  drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++-------------
>>>  1 file changed, 45 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>>> b/drivers/platform/x86/thinkpad_acpi.c
>>> index 0bbc462d604c..e8d30f4af126 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -3677,51 +3677,50 @@ static int adaptive_keyboard_get_next_mode(int 
>>> mode)
>>>  	return adaptive_keyboard_modes[i];
>>>  }
>>>
>>> +static void adaptive_keyboard_change_row(void)
>>> +{
>>> +	int mode;
>>> +
>>> +	if (adaptive_keyboard_mode_is_saved) {
>>> +		mode = adaptive_keyboard_prev_mode;
>>> +		adaptive_keyboard_mode_is_saved = false;
>>> +	} else {
>>> +		mode = adaptive_keyboard_get_mode();
>>> +		if (mode < 0)
>>> +			return;
>>> +		mode = adaptive_keyboard_get_next_mode(mode);
>>> +	}
>>> +
>>> +	adaptive_keyboard_set_mode(mode);
>>> +}
>>> +
>>> +static void adaptive_keyboard_s_quickview_row(void)
>>> +{
>>> +	int mode = adaptive_keyboard_get_mode();
>>> +
>>> +	if (mode < 0)
>>> +		return;
>>> +
>>> +	adaptive_keyboard_prev_mode = mode;
>>> +	adaptive_keyboard_mode_is_saved = true;
>>> +
>>> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
>>> +}
>>> +
>>>  static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
>>>  {
>>> -	int current_mode = 0;
>>> -	int new_mode = 0;
>>> -
>>> -	switch (hkey) {
>>> -	case TP_HKEY_EV_DFR_CHANGE_ROW:
>>> -		if (adaptive_keyboard_mode_is_saved) {
>>> -			new_mode = adaptive_keyboard_prev_mode;
>>> -			adaptive_keyboard_mode_is_saved = false;
>>> -		} else {
>>> -			current_mode = adaptive_keyboard_get_mode();
>>> -			if (current_mode < 0)
>>> -				return false;
>>> -			new_mode = adaptive_keyboard_get_next_mode(
>>> -					current_mode);
>>> -		}
>>> -
>>> -		if (adaptive_keyboard_set_mode(new_mode) < 0)
>>> -			return false;
>>> -
>>> +	if (tpacpi_driver_event(hkey))
>>>  		return true;
>>>
>>> -	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>>> -		current_mode = adaptive_keyboard_get_mode();
>>> -		if (current_mode < 0)
>>> -			return false;
>>> -
>>> -		adaptive_keyboard_prev_mode = current_mode;
>>> -		adaptive_keyboard_mode_is_saved = true;
>>> -
>>> -		if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
>>> -			return false;
>>> -		return true;
>>> -
>>> -	default:
>>> -		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>>> -		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>>> -			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>>> -			return false;
>>> -		}
>>> -		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>>> -				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>>> -		return true;
>>> +	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>>> +	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>>> +		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>>> +		return false;
>>>  	}
>>> +
>>> +	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>>> +			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>>> +	return true;
>>>  }
>>>
>>>  static bool hotkey_notify_extended_hotkey(const u32 hkey)
>>> @@ -11117,6 +11116,12 @@ static bool tpacpi_driver_event(const unsigned 
>>> int hkey_event)
>>>  		}
>>>  		/* Key events are suppressed by default hotkey_user_mask */
>>>  		return false;
>>> +	case TP_HKEY_EV_DFR_CHANGE_ROW:
>>> +		adaptive_keyboard_change_row();
>>> +		return true;
>>> +	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>>> +		adaptive_keyboard_s_quickview_row();
>> 
>> Was there a reason to get rid of error handling that was previously done with adaptive_keyboard_set_mode and is now not used here?
>
> The error handling consisted of returning false instead of true
> (for known_ev), causing an unknown event message to get logged on
> top of the error already logged by adaptive_keyboard_get_mode() /
> adaptive_keyboard_set_mode().
>
> This second unknown event error is consfusin / not helpful so
> I've dropped the "return false" on error behavior, note that
> the new helpers do still return early if get_mode() fails just
> as before.
>
> I'll add a note about this to the commit message for v2 of
> the series.
>
Thanks for the explanation - makes sense.

Reviwed-by: Mark Pearson <mpe...@sq...>

As a note - I've been working my way thru the patches. Is it OK to send one Reviewed-by at the end for all the patches for which I had no questions, or is it better to ack each one individually?

Mark

Re: [ibm-acpi-devel] [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Hans de G. <hde...@re...>]

Hi Mark,

On 4/22/24 9:27 PM, Mark Pearson wrote:
> Hi Hans,
> 
> On Sun, Apr 21, 2024, at 11:45 AM, Hans de Goede wrote:
>> Factor out the adaptive kbd non hotkey event handling into
>> adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
>> helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
>> TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().
>>
>> This groups all the handling of hotkey events which do not emit
>> a key press event together in tpacpi_driver_event().
>>
>> This is a preparation patch for moving to sparse-keymaps.
>>
>> Signed-off-by: Hans de Goede <hde...@re...>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++-------------
>>  1 file changed, 45 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index 0bbc462d604c..e8d30f4af126 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -3677,51 +3677,50 @@ static int adaptive_keyboard_get_next_mode(int 
>> mode)
>>  	return adaptive_keyboard_modes[i];
>>  }
>>
>> +static void adaptive_keyboard_change_row(void)
>> +{
>> +	int mode;
>> +
>> +	if (adaptive_keyboard_mode_is_saved) {
>> +		mode = adaptive_keyboard_prev_mode;
>> +		adaptive_keyboard_mode_is_saved = false;
>> +	} else {
>> +		mode = adaptive_keyboard_get_mode();
>> +		if (mode < 0)
>> +			return;
>> +		mode = adaptive_keyboard_get_next_mode(mode);
>> +	}
>> +
>> +	adaptive_keyboard_set_mode(mode);
>> +}
>> +
>> +static void adaptive_keyboard_s_quickview_row(void)
>> +{
>> +	int mode = adaptive_keyboard_get_mode();
>> +
>> +	if (mode < 0)
>> +		return;
>> +
>> +	adaptive_keyboard_prev_mode = mode;
>> +	adaptive_keyboard_mode_is_saved = true;
>> +
>> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
>> +}
>> +
>>  static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
>>  {
>> -	int current_mode = 0;
>> -	int new_mode = 0;
>> -
>> -	switch (hkey) {
>> -	case TP_HKEY_EV_DFR_CHANGE_ROW:
>> -		if (adaptive_keyboard_mode_is_saved) {
>> -			new_mode = adaptive_keyboard_prev_mode;
>> -			adaptive_keyboard_mode_is_saved = false;
>> -		} else {
>> -			current_mode = adaptive_keyboard_get_mode();
>> -			if (current_mode < 0)
>> -				return false;
>> -			new_mode = adaptive_keyboard_get_next_mode(
>> -					current_mode);
>> -		}
>> -
>> -		if (adaptive_keyboard_set_mode(new_mode) < 0)
>> -			return false;
>> -
>> +	if (tpacpi_driver_event(hkey))
>>  		return true;
>>
>> -	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>> -		current_mode = adaptive_keyboard_get_mode();
>> -		if (current_mode < 0)
>> -			return false;
>> -
>> -		adaptive_keyboard_prev_mode = current_mode;
>> -		adaptive_keyboard_mode_is_saved = true;
>> -
>> -		if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
>> -			return false;
>> -		return true;
>> -
>> -	default:
>> -		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>> -		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>> -			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>> -			return false;
>> -		}
>> -		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>> -				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>> -		return true;
>> +	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
>> +	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
>> +		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
>> +		return false;
>>  	}
>> +
>> +	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
>> +			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
>> +	return true;
>>  }
>>
>>  static bool hotkey_notify_extended_hotkey(const u32 hkey)
>> @@ -11117,6 +11116,12 @@ static bool tpacpi_driver_event(const unsigned 
>> int hkey_event)
>>  		}
>>  		/* Key events are suppressed by default hotkey_user_mask */
>>  		return false;
>> +	case TP_HKEY_EV_DFR_CHANGE_ROW:
>> +		adaptive_keyboard_change_row();
>> +		return true;
>> +	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
>> +		adaptive_keyboard_s_quickview_row();
> 
> Was there a reason to get rid of error handling that was previously done with adaptive_keyboard_set_mode and is now not used here?

The error handling consisted of returning false instead of true
(for known_ev), causing an unknown event message to get logged on
top of the error already logged by adaptive_keyboard_get_mode() /
adaptive_keyboard_set_mode().

This second unknown event error is consfusin / not helpful so
I've dropped the "return false" on error behavior, note that
the new helpers do still return early if get_mode() fails just
as before.

I'll add a note about this to the commit message for v2 of
the series.

Regards,

Hans






> 
>> +		return true;
>>  	case TP_HKEY_EV_THM_CSM_COMPLETED:
>>  		lapsensor_refresh();
>>  		/* If we are already accessing DYTC then skip dytc update */
>> -- 
>> 2.44.0
> 
> Thanks
> Mark
> 

Re: [ibm-acpi-devel] [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Mark P. <mpe...@sq...>]

Hi Hans,

On Sun, Apr 21, 2024, at 11:45 AM, Hans de Goede wrote:
> Factor out the adaptive kbd non hotkey event handling into
> adaptive_keyboard_change_row() and adaptive_keyboard_s_quickview_row()
> helpers and move the handling of TP_HKEY_EV_DFR_CHANGE_ROW and
> TP_HKEY_EV_DFR_S_QUICKVIEW_ROW to tpacpi_driver_event().
>
> This groups all the handling of hotkey events which do not emit
> a key press event together in tpacpi_driver_event().
>
> This is a preparation patch for moving to sparse-keymaps.
>
> Signed-off-by: Hans de Goede <hde...@re...>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++++++-------------
>  1 file changed, 45 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index 0bbc462d604c..e8d30f4af126 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3677,51 +3677,50 @@ static int adaptive_keyboard_get_next_mode(int 
> mode)
>  	return adaptive_keyboard_modes[i];
>  }
> 
> +static void adaptive_keyboard_change_row(void)
> +{
> +	int mode;
> +
> +	if (adaptive_keyboard_mode_is_saved) {
> +		mode = adaptive_keyboard_prev_mode;
> +		adaptive_keyboard_mode_is_saved = false;
> +	} else {
> +		mode = adaptive_keyboard_get_mode();
> +		if (mode < 0)
> +			return;
> +		mode = adaptive_keyboard_get_next_mode(mode);
> +	}
> +
> +	adaptive_keyboard_set_mode(mode);
> +}
> +
> +static void adaptive_keyboard_s_quickview_row(void)
> +{
> +	int mode = adaptive_keyboard_get_mode();
> +
> +	if (mode < 0)
> +		return;
> +
> +	adaptive_keyboard_prev_mode = mode;
> +	adaptive_keyboard_mode_is_saved = true;
> +
> +	adaptive_keyboard_set_mode(FUNCTION_MODE);
> +}
> +
>  static bool adaptive_keyboard_hotkey_notify_hotkey(const u32 hkey)
>  {
> -	int current_mode = 0;
> -	int new_mode = 0;
> -
> -	switch (hkey) {
> -	case TP_HKEY_EV_DFR_CHANGE_ROW:
> -		if (adaptive_keyboard_mode_is_saved) {
> -			new_mode = adaptive_keyboard_prev_mode;
> -			adaptive_keyboard_mode_is_saved = false;
> -		} else {
> -			current_mode = adaptive_keyboard_get_mode();
> -			if (current_mode < 0)
> -				return false;
> -			new_mode = adaptive_keyboard_get_next_mode(
> -					current_mode);
> -		}
> -
> -		if (adaptive_keyboard_set_mode(new_mode) < 0)
> -			return false;
> -
> +	if (tpacpi_driver_event(hkey))
>  		return true;
> 
> -	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
> -		current_mode = adaptive_keyboard_get_mode();
> -		if (current_mode < 0)
> -			return false;
> -
> -		adaptive_keyboard_prev_mode = current_mode;
> -		adaptive_keyboard_mode_is_saved = true;
> -
> -		if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
> -			return false;
> -		return true;
> -
> -	default:
> -		if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
> -		    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
> -			pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
> -			return false;
> -		}
> -		tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
> -				      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
> -		return true;
> +	if (hkey < TP_HKEY_EV_ADAPTIVE_KEY_START ||
> +	    hkey > TP_HKEY_EV_ADAPTIVE_KEY_END) {
> +		pr_info("Unhandled adaptive keyboard key: 0x%x\n", hkey);
> +		return false;
>  	}
> +
> +	tpacpi_input_send_key(hkey - TP_HKEY_EV_ADAPTIVE_KEY_START +
> +			      TP_ACPI_HOTKEYSCAN_ADAPTIVE_START);
> +	return true;
>  }
> 
>  static bool hotkey_notify_extended_hotkey(const u32 hkey)
> @@ -11117,6 +11116,12 @@ static bool tpacpi_driver_event(const unsigned 
> int hkey_event)
>  		}
>  		/* Key events are suppressed by default hotkey_user_mask */
>  		return false;
> +	case TP_HKEY_EV_DFR_CHANGE_ROW:
> +		adaptive_keyboard_change_row();
> +		return true;
> +	case TP_HKEY_EV_DFR_S_QUICKVIEW_ROW:
> +		adaptive_keyboard_s_quickview_row();

Was there a reason to get rid of error handling that was previously done with adaptive_keyboard_set_mode and is now not used here?

> +		return true;
>  	case TP_HKEY_EV_THM_CSM_COMPLETED:
>  		lapsensor_refresh();
>  		/* If we are already accessing DYTC then skip dytc update */
> -- 
> 2.44.0

Thanks
Mark

Re: [ibm-acpi-devel] [PATCH 5/6] platform/x86: Use device_show_string() helper for sysfs attributes

[Hans de G. <hde...@re...>]

Hi,

On 4/20/24 10:00 PM, Lukas Wunner wrote:
> Deduplicate sysfs ->show() callbacks which expose a string at a static
> memory location.  Use the newly introduced device_show_string() helper
> in the driver core instead by declaring those sysfs attributes with
> DEVICE_STRING_ATTR_RO().
> 
> No functional change intended.
> 
> Signed-off-by: Lukas Wunner <lu...@wu...>

Thanks, patch looks good to me:

Acked-by: Hans de Goede <hde...@re...>

Feel free to upstream this though whatever git tree is convenient.

Regards,

Hans



> ---
>  drivers/platform/x86/asus-wmi.c      | 62 +++++++---------------------
>  drivers/platform/x86/thinkpad_acpi.c | 10 +----
>  drivers/platform/x86/toshiba_acpi.c  |  9 +---
>  3 files changed, 20 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 3f07bbf809ef..78d7579b2fdd 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -915,17 +915,12 @@ static ssize_t kbd_rgb_mode_store(struct device *dev,
>  }
>  static DEVICE_ATTR_WO(kbd_rgb_mode);
>  
> -static ssize_t kbd_rgb_mode_index_show(struct device *device,
> -						 struct device_attribute *attr,
> -						 char *buf)
> -{
> -	return sysfs_emit(buf, "%s\n", "cmd mode red green blue speed");
> -}
> -static DEVICE_ATTR_RO(kbd_rgb_mode_index);
> +static DEVICE_STRING_ATTR_RO(kbd_rgb_mode_index, 0444,
> +			     "cmd mode red green blue speed");
>  
>  static struct attribute *kbd_rgb_mode_attrs[] = {
>  	&dev_attr_kbd_rgb_mode.attr,
> -	&dev_attr_kbd_rgb_mode_index.attr,
> +	&dev_attr_kbd_rgb_mode_index.attr.attr,
>  	NULL,
>  };
>  
> @@ -967,17 +962,12 @@ static ssize_t kbd_rgb_state_store(struct device *dev,
>  }
>  static DEVICE_ATTR_WO(kbd_rgb_state);
>  
> -static ssize_t kbd_rgb_state_index_show(struct device *device,
> -						 struct device_attribute *attr,
> -						 char *buf)
> -{
> -	return sysfs_emit(buf, "%s\n", "cmd boot awake sleep keyboard");
> -}
> -static DEVICE_ATTR_RO(kbd_rgb_state_index);
> +static DEVICE_STRING_ATTR_RO(kbd_rgb_state_index, 0444,
> +			     "cmd boot awake sleep keyboard");
>  
>  static struct attribute *kbd_rgb_state_attrs[] = {
>  	&dev_attr_kbd_rgb_state.attr,
> -	&dev_attr_kbd_rgb_state_index.attr,
> +	&dev_attr_kbd_rgb_state_index.attr.attr,
>  	NULL,
>  };
>  
> @@ -2493,13 +2483,6 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	return count;
>  }
>  
> -static ssize_t fan1_label_show(struct device *dev,
> -					  struct device_attribute *attr,
> -					  char *buf)
> -{
> -	return sysfs_emit(buf, "%s\n", ASUS_FAN_DESC);
> -}
> -
>  static ssize_t asus_hwmon_temp1(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
> @@ -2534,13 +2517,6 @@ static ssize_t fan2_input_show(struct device *dev,
>  	return sysfs_emit(buf, "%d\n", value * 100);
>  }
>  
> -static ssize_t fan2_label_show(struct device *dev,
> -					  struct device_attribute *attr,
> -					  char *buf)
> -{
> -	return sysfs_emit(buf, "%s\n", ASUS_GPU_FAN_DESC);
> -}
> -
>  /* Middle/Center fan on modern ROG laptops */
>  static ssize_t fan3_input_show(struct device *dev,
>  					struct device_attribute *attr,
> @@ -2559,13 +2535,6 @@ static ssize_t fan3_input_show(struct device *dev,
>  	return sysfs_emit(buf, "%d\n", value * 100);
>  }
>  
> -static ssize_t fan3_label_show(struct device *dev,
> -					  struct device_attribute *attr,
> -					  char *buf)
> -{
> -	return sysfs_emit(buf, "%s\n", ASUS_MID_FAN_DESC);
> -}
> -
>  static ssize_t pwm2_enable_show(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
> @@ -2662,15 +2631,16 @@ static ssize_t pwm3_enable_store(struct device *dev,
>  static DEVICE_ATTR_RW(pwm1);
>  static DEVICE_ATTR_RW(pwm1_enable);
>  static DEVICE_ATTR_RO(fan1_input);
> -static DEVICE_ATTR_RO(fan1_label);
> +static DEVICE_STRING_ATTR_RO(fan1_label, 0444, ASUS_FAN_DESC);
> +
>  /* Fan2 - GPU fan */
>  static DEVICE_ATTR_RW(pwm2_enable);
>  static DEVICE_ATTR_RO(fan2_input);
> -static DEVICE_ATTR_RO(fan2_label);
> +static DEVICE_STRING_ATTR_RO(fan2_label, 0444, ASUS_GPU_FAN_DESC);
>  /* Fan3 - Middle/center fan */
>  static DEVICE_ATTR_RW(pwm3_enable);
>  static DEVICE_ATTR_RO(fan3_input);
> -static DEVICE_ATTR_RO(fan3_label);
> +static DEVICE_STRING_ATTR_RO(fan3_label, 0444, ASUS_MID_FAN_DESC);
>  
>  /* Temperature */
>  static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
> @@ -2681,11 +2651,11 @@ static struct attribute *hwmon_attributes[] = {
>  	&dev_attr_pwm2_enable.attr,
>  	&dev_attr_pwm3_enable.attr,
>  	&dev_attr_fan1_input.attr,
> -	&dev_attr_fan1_label.attr,
> +	&dev_attr_fan1_label.attr.attr,
>  	&dev_attr_fan2_input.attr,
> -	&dev_attr_fan2_label.attr,
> +	&dev_attr_fan2_label.attr.attr,
>  	&dev_attr_fan3_input.attr,
> -	&dev_attr_fan3_label.attr,
> +	&dev_attr_fan3_label.attr.attr,
>  
>  	&dev_attr_temp1_input.attr,
>  	NULL
> @@ -2702,17 +2672,17 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>  		if (asus->fan_type != FAN_TYPE_AGFN)
>  			return 0;
>  	} else if (attr == &dev_attr_fan1_input.attr
> -	    || attr == &dev_attr_fan1_label.attr
> +	    || attr == &dev_attr_fan1_label.attr.attr
>  	    || attr == &dev_attr_pwm1_enable.attr) {
>  		if (asus->fan_type == FAN_TYPE_NONE)
>  			return 0;
>  	} else if (attr == &dev_attr_fan2_input.attr
> -	    || attr == &dev_attr_fan2_label.attr
> +	    || attr == &dev_attr_fan2_label.attr.attr
>  	    || attr == &dev_attr_pwm2_enable.attr) {
>  		if (asus->gpu_fan_type == FAN_TYPE_NONE)
>  			return 0;
>  	} else if (attr == &dev_attr_fan3_input.attr
> -	    || attr == &dev_attr_fan3_label.attr
> +	    || attr == &dev_attr_fan3_label.attr.attr
>  	    || attr == &dev_attr_pwm3_enable.attr) {
>  		if (asus->mid_fan_type == FAN_TYPE_NONE)
>  			return 0;
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 82429e59999d..47a64a213d14 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10991,13 +10991,7 @@ static struct ibm_struct auxmac_data = {
>  	.name = "auxmac",
>  };
>  
> -static ssize_t auxmac_show(struct device *dev,
> -			   struct device_attribute *attr,
> -			   char *buf)
> -{
> -	return sysfs_emit(buf, "%s\n", auxmac);
> -}
> -static DEVICE_ATTR_RO(auxmac);
> +static DEVICE_STRING_ATTR_RO(auxmac, 0444, auxmac);
>  
>  static umode_t auxmac_attr_is_visible(struct kobject *kobj,
>  				      struct attribute *attr, int n)
> @@ -11006,7 +11000,7 @@ static umode_t auxmac_attr_is_visible(struct kobject *kobj,
>  }
>  
>  static struct attribute *auxmac_attributes[] = {
> -	&dev_attr_auxmac.attr,
> +	&dev_attr_auxmac.attr.attr,
>  	NULL
>  };
>  
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 291f14ef6702..01cf60a015bf 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -1814,12 +1814,7 @@ static DECLARE_WORK(kbd_bl_work, toshiba_acpi_kbd_bl_work);
>  /*
>   * Sysfs files
>   */
> -static ssize_t version_show(struct device *dev,
> -			    struct device_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", TOSHIBA_ACPI_VERSION);
> -}
> -static DEVICE_ATTR_RO(version);
> +static DEVICE_STRING_ATTR_RO(version, 0444, TOSHIBA_ACPI_VERSION);
>  
>  static ssize_t fan_store(struct device *dev,
>  			 struct device_attribute *attr,
> @@ -2428,7 +2423,7 @@ static ssize_t cooling_method_store(struct device *dev,
>  static DEVICE_ATTR_RW(cooling_method);
>  
>  static struct attribute *toshiba_attributes[] = {
> -	&dev_attr_version.attr,
> +	&dev_attr_version.attr.attr,
>  	&dev_attr_fan.attr,
>  	&dev_attr_kbd_backlight_mode.attr,
>  	&dev_attr_kbd_type.attr,

Re: [ibm-acpi-devel] [PATCH 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event()

[Andy S. <and...@gm...>]

On Mon, Apr 22, 2024 at 11:29 AM Ilpo Järvinen
<ilp...@li...> wrote:
> On Sun, 21 Apr 2024, Hans de Goede wrote:

...

> > +     int mode = adaptive_keyboard_get_mode();
> > +
> > +     if (mode < 0)
>
> Please try to keep call and it's error handling together, it costs one
> line but takes less effort to understand:
>
>         int mode;
>
>         mode = adaptive_keyboard_get_mode();
>         if (mode < 0)
>
> > +             return;

And not only that. In long-term maintenance the original code is prone
to subtle errors in case some other code is squeezed in between.

-- 
With Best Regards,
Andy Shevchenko

Re: [ibm-acpi-devel] [PATCH 03/24] platform/x86: thinkpad_acpi: Drop setting send_/ignore_acpi_ev defaults twice

[Andy S. <and...@gm...>]

On Mon, Apr 22, 2024 at 11:07 AM Ilpo Järvinen
<ilp...@li...> wrote:
> On Sun, 21 Apr 2024, Hans de Goede wrote:
>
> > send_acpi_ev, ignore_acpi_ev are already initialized to true resp. false by
>
> Wording here is odd (but I'm not native so could be I just don't
> understand what "true resp. false" is supposed to mean/fit into the
> general structure of this sentence). I could nonetheless guess the
> general meaning of the sentence despite that, but you might want to
> consider rewording it into something that is easier to understand.

I read that as "true and false respectively", which is probably better
to be spelled this way.

-- 
With Best Regards,
Andy Shevchenko

Re: [ibm-acpi-devel] [PATCH 00/24] platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys

[Mark P. <mpe...@sq...>]

On Sun, Apr 21, 2024, at 1:17 PM, Mark Pearson wrote:
> Thanks Hans!
>
> On Sun, Apr 21, 2024, at 11:44 AM, Hans de Goede wrote:
>> Hi All,
>>
>> My reply in the "[PATCH v2 1/4] platform/x86: thinkpad_acpi:
>> simplify known_ev handling" handling where I indicated that I would work
>> on converting the thinkpad_acpi hotkey handling to use sparse-keymaps
>> underestimated the work this required quite a bit.
>>
>> The hotkey code is quite old and crufty and full of special cases to
>> support many generations of ThinkPads, so I ended up doing some
>> significant refactoring and cleanup before doing the actual conversion
>> to sparse-keymaps.
>>
>> I have been vary careful to not introduce any changes wrt support for
>> the original ThinkPad models / hotkeys which use the hotkey_*_mask
>> related code.
>>
>> I've also done my best to avoid any *significant* functional change but
>> there are still some functional changes, which should all not impact
>> userspace compatibility:
>>
>> 1. Adaptive keyboard special keys will now also send EV_MSC events with
>>    the scancode, just like all the other hotkeys.
>>
>> 2. Rely on the input core for KEY_RESERVED suppression. This means that
>>    keys marked as KEY_RESERVED will still send EV_MSC evdev events when
>>    pressed (which helps users with mapping them to non reserved KEY_FOO
>>    values if desired).
>>
>> 3. Align the keycodes for volume up/down/mute and brightness up/down with
>>    those set by userspace through udev upstream's hwdb. Note these are all
>>    for keys which are suppressed by hotkey_reserved_mask by default.
>>    So this is only a functional change for users who override the default
>>    hotkey-mask *and* who do not have udev's default hwdb installed.
>>
>> 4. Suppress ACPI netlink event generation for unknown 0x1xxx hkey events to
>>    avoid userspace starting to rely on the netlink events for new hotkeys
>>    before these have been added to the keymap, only to have the netlink
>>    events get disabled by the adding of the new hotkeys to the keymap.
>>
>>    This should not cause a behavior change for existing models since all
>>    currently known 0x1xxx events have a mapping.
>>
>> Here is a quick breakdown of the patches in this series:
>>
>> Patch 1 - 2: Fix a small locking issue on rmmod the only problem here
>>    really is a lockdep warning, so I plan to route these fixes through
>>    for-next together with the rest to keep things simple.
>>
>> Patch 3 - 14: Do a bunch of cleanups and refactoring
>>
>> Patch 15: Implements functional change no 4. I really kinda want to just
>>    completely disable ACPI netlink event generation when also sending evdev
>>    events instead of reporting these twice. But for the 0x11xx / 0x13xx
>>    hkey events the kernel has send ACPI netlink events for years now. So
>>    this disables ACPI netlink events for any new hotkeys going forward.
>>
>> Patch 16 - 18: Refactor / cleanup reserved key handling
>>
>> Patch 19: Actually move to sparse-keymaps
>>
>> Patch 20: Update the keymap for 2 adaptive kbd Fn row keys
>>
>> Patch 21 - 24: Mark's original patches adding support for the new Fn + N
>>    key combo and for trackpoint doubletap slightly reworked to use
>>    the new sparse-keymap.
>>
>> Mark if you can make some time to review patches 1-20 that would be great.
>>
> Definitely will do.
>
> I'll do some testing on platforms here as much as I can. If there's 
> anything in particular that you think is worth taking extra care over 
> let me know (with a caveat that my team doesn't have all the platforms, 
> and for anything older than 5 years it can be hard to get hold of them)
>
> Thanks for your work on this.
>
> Mark
>
I've tested the series on a couple of platforms (Z13 G2 and X1 Carbon G12) and so far all looking good.
 - tried all hotkey combinations that are supported that I can think of and they work - including brightness control, volume control, platform profile toggle, airplane, snipping tool and privacy screen.
 - trackpoint double tap is working well on the Z13 G2 (set up custom key setting in gnome-settings and launched browser on a doubletap)
 - FN+N is working well on the X1 Carbon G12 (tested with evtest to confirm vendor key generated)

So for the series:
Tested-by: Mark Pearson <mpe...@sq...>

I have read through the patches, but not in enough depth for it to count as a review yet. Need a bit more time for that.

Mark

Re: [ibm-acpi-devel] [PATCH 17/24] platform/x86: thinkpad_acpi: Use correct keycodes for volume and brightness keys

[Andy S. <and...@gm...>]

On Sun, Apr 21, 2024 at 6:45 PM Hans de Goede <hde...@re...> wrote:
>
> Change the default keymap to report the correct keycodes for the volume and
> brightness keys. Reporting key events for these is already filtered out by
> the hotkey_reserved_mask which masks these keys out of hotkey_user_mask at
> initialization time, so there is no need to also map them to KEY_RESERVED.
>
> This avoids users, who want these to be reported, having to also remap
> the keycodes on top of overriding hotkey_user_mask to report these
> and Linux userspace has already been overridding the KEY_RESERVED mappings

overriding

> with the correct keycodes through udev/hwdb/60-keyboard.hwdb for years now.
>
> Also drop hotkey_unmap() it was only used to dynamically map the brightness
> keys to KEY_RESERVED and after removing that it has no remaining users.

...

> +               /* brightness: firmware always reacts to them.
> +                * Suppressed by default through hotkey_reserved_mask.
> +                */

> +               /* Thinklight: firmware always react to it.
> +                * Suppressed by default through hotkey_reserved_mask.
> +                */

>                 /* Volume: firmware always react to it and reprograms
>                  * the built-in *extra* mixer.  Never map it to control
> +                * another mixer by default.
> +                * Suppressed by default through hotkey_reserved_mask.
> +                */

Hmm... While at it, can we rectify the block comments to follow the
standard style?
(I meant those which you are touching here.)

-- 
With Best Regards,
Andy Shevchenko

Re: [ibm-acpi-devel] [PATCH 00/24] platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys

[Mark P. <mpe...@sq...>]

Thanks Hans!

On Sun, Apr 21, 2024, at 11:44 AM, Hans de Goede wrote:
> Hi All,
>
> My reply in the "[PATCH v2 1/4] platform/x86: thinkpad_acpi:
> simplify known_ev handling" handling where I indicated that I would work
> on converting the thinkpad_acpi hotkey handling to use sparse-keymaps
> underestimated the work this required quite a bit.
>
> The hotkey code is quite old and crufty and full of special cases to
> support many generations of ThinkPads, so I ended up doing some
> significant refactoring and cleanup before doing the actual conversion
> to sparse-keymaps.
>
> I have been vary careful to not introduce any changes wrt support for
> the original ThinkPad models / hotkeys which use the hotkey_*_mask
> related code.
>
> I've also done my best to avoid any *significant* functional change but
> there are still some functional changes, which should all not impact
> userspace compatibility:
>
> 1. Adaptive keyboard special keys will now also send EV_MSC events with
>    the scancode, just like all the other hotkeys.
>
> 2. Rely on the input core for KEY_RESERVED suppression. This means that
>    keys marked as KEY_RESERVED will still send EV_MSC evdev events when
>    pressed (which helps users with mapping them to non reserved KEY_FOO
>    values if desired).
>
> 3. Align the keycodes for volume up/down/mute and brightness up/down with
>    those set by userspace through udev upstream's hwdb. Note these are all
>    for keys which are suppressed by hotkey_reserved_mask by default.
>    So this is only a functional change for users who override the default
>    hotkey-mask *and* who do not have udev's default hwdb installed.
>
> 4. Suppress ACPI netlink event generation for unknown 0x1xxx hkey events to
>    avoid userspace starting to rely on the netlink events for new hotkeys
>    before these have been added to the keymap, only to have the netlink
>    events get disabled by the adding of the new hotkeys to the keymap.
>
>    This should not cause a behavior change for existing models since all
>    currently known 0x1xxx events have a mapping.
>
> Here is a quick breakdown of the patches in this series:
>
> Patch 1 - 2: Fix a small locking issue on rmmod the only problem here
>    really is a lockdep warning, so I plan to route these fixes through
>    for-next together with the rest to keep things simple.
>
> Patch 3 - 14: Do a bunch of cleanups and refactoring
>
> Patch 15: Implements functional change no 4. I really kinda want to just
>    completely disable ACPI netlink event generation when also sending evdev
>    events instead of reporting these twice. But for the 0x11xx / 0x13xx
>    hkey events the kernel has send ACPI netlink events for years now. So
>    this disables ACPI netlink events for any new hotkeys going forward.
>
> Patch 16 - 18: Refactor / cleanup reserved key handling
>
> Patch 19: Actually move to sparse-keymaps
>
> Patch 20: Update the keymap for 2 adaptive kbd Fn row keys
>
> Patch 21 - 24: Mark's original patches adding support for the new Fn + N
>    key combo and for trackpoint doubletap slightly reworked to use
>    the new sparse-keymap.
>
> Mark if you can make some time to review patches 1-20 that would be great.
>
Definitely will do.

I'll do some testing on platforms here as much as I can. If there's anything in particular that you think is worth taking extra care over let me know (with a caveat that my team doesn't have all the platforms, and for anything older than 5 years it can be hard to get hold of them)

Thanks for your work on this.

Mark

> Regards,
>
> Hans
>
>
> Hans de Goede (20):
>   platform/x86: thinkpad_acpi: Take hotkey_mutex during hotkey_exit()
>   platform/x86: thinkpad_acpi: Provide hotkey_poll_stop_sync() dummy
>   platform/x86: thinkpad_acpi: Drop setting send_/ignore_acpi_ev
>     defaults twice
>   platform/x86: thinkpad_acpi: Drop ignore_acpi_ev
>   platform/x86: thinkpad_acpi: Use tpacpi_input_send_key() in adaptive
>     kbd code
>   platform/x86: thinkpad_acpi: Do hkey to scancode translation later
>   platform/x86: thinkpad_acpi: Make tpacpi_driver_event() return if it
>     handled the event
>   platform/x86: thinkpad_acpi: Move adaptive kbd event handling to
>     tpacpi_driver_event()
>   platform/x86: thinkpad_acpi: Move special original hotkeys handling
>     out of switch-case
>   platform/x86: thinkpad_acpi: Move hotkey_user_mask check to
>     tpacpi_input_send_key()
>   platform/x86: thinkpad_acpi: Always call tpacpi_driver_event() for
>     hotkeys
>   platform/x86: thinkpad_acpi: Drop tpacpi_input_send_key_masked() and
>     hotkey_driver_event()
>   platform/x86: thinkpad_acpi: Move hkey > scancode mapping to
>     tpacpi_input_send_key()
>   platform/x86: thinkpad_acpi: Move tpacpi_driver_event() call to
>     tpacpi_input_send_key()
>   platform/x86: thinkpad_acpi: Do not send ACPI netlink events for
>     unknown hotkeys
>   platform/x86: thinkpad_acpi: Change hotkey_reserved_mask
>     initialization
>   platform/x86: thinkpad_acpi: Use correct keycodes for volume and
>     brightness keys
>   platform/x86: thinkpad_acpi: Drop KEY_RESERVED special handling
>   platform/x86: thinkpad_acpi: Switch to using sparse-keymap helpers
>   platform/x86: thinkpad_acpi: Add mappings for adaptive kbd
>     clipping-tool and cloud keys
>
> Mark Pearson (4):
>   platform/x86: thinkpad_acpi: Simplify known_ev handling
>   platform/x86: thinkpad_acpi: Support for trackpoint doubletap
>   platform/x86: thinkpad_acpi: Support for system debug info hotkey
>   platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint
>     doubletap
>
>  drivers/platform/x86/thinkpad_acpi.c | 849 +++++++++++----------------
>  1 file changed, 348 insertions(+), 501 deletions(-)
>
> -- 
> 2.44.0

[ibm-acpi-devel] [PATCH 24/24] platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint doubletap

[Hans de G. <hde...@re...>]

From: Mark Pearson <mpe...@sq...>

The hotkey combination Fn + G can be used to disable the trackpoint
doubletap feature on Windows. Add matching functionality for Linux.

Signed-off-by: Mark Pearson <mpe...@sq...>
Signed-off-by: Vishnu Sankar <vis...@gm...>
Link: https://lore.kernel.org/r/202...@sq...
[hde...@re...: Adjust for switch to sparse-keymap keymaps]
[hde...@re...: Do not log unknown event msg for doubletap when disabled]
Signed-off-by: Hans de Goede <hde...@re...>
---
 drivers/platform/x86/thinkpad_acpi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index dd8873d61126..7b9d48271d7d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -183,6 +183,7 @@ enum tpacpi_hkey_event_t {
 						   * directly in the sparse-keymap.
 						   */
 	TP_HKEY_EV_AMT_TOGGLE		= 0x131a, /* Toggle AMT on/off */
+	TP_HKEY_EV_DOUBLETAP_TOGGLE	= 0x131c, /* Toggle trackpoint doubletap on/off */
 	TP_HKEY_EV_PROFILE_TOGGLE	= 0x131f, /* Toggle platform profile */
 
 	/* Reasons for waking up from S3/S4 */
@@ -372,6 +373,7 @@ static struct {
 	u32 hotkey_poll_active:1;
 	u32 has_adaptive_kbd:1;
 	u32 kbd_lang:1;
+	u32 trackpoint_doubletap:1;
 	struct quirk_entry *quirks;
 } tp_features;
 
@@ -3523,6 +3525,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
 	hotkey_poll_setup_safe(true);
 
+	/* Enable doubletap by default */
+	tp_features.trackpoint_doubletap = 1;
+
 	return 0;
 }
 
@@ -3821,7 +3826,9 @@ static bool hotkey_notify_8xxx(const u32 hkey, bool *send_acpi_ev)
 {
 	switch (hkey) {
 	case TP_HKEY_EV_TRACK_DOUBLETAP:
-		tpacpi_input_send_key(hkey, send_acpi_ev);
+		if (tp_features.trackpoint_doubletap)
+			tpacpi_input_send_key(hkey, send_acpi_ev);
+
 		return true;
 	default:
 		return false;
@@ -11028,6 +11035,9 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 		else
 			dytc_control_amt(!dytc_amt_active);
 
+		return true;
+	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
+		tp_features.trackpoint_doubletap = !tp_features.trackpoint_doubletap;
 		return true;
 	case TP_HKEY_EV_PROFILE_TOGGLE:
 		platform_profile_cycle();
-- 
2.44.0