[PATCH 0/1] HID: apple: Properly handle function keys on Keychron keyboards

[PATCH 0/1] HID: apple: Properly handle function keys on Keychron keyboards

[Bryan Cain]

Hi,

As the new owner of a Keychron C1 keyboard, I recently found this thread from
this mailing list (linux-input):

https://lore.kernel.org/all/897e57a9-38d8-c05f-ceed-01d486f02726@redhat.com/T/

To summarize the findings in that thread:
* Keychron keyboards (C-series and K-series) use the vendor:product IDs from a
  2009 Apple keyboard. When set to "Mac" mode, they actually do behave like
  that device, but in "Windows" mode, the Fn key doesn't generate a scancode,
  making it impossible to use F1-F12 when the fnmode parameter is set to its
  default value of 1.
* The universally accepted "fix" among Keychron owners online is to set
  fnmode=2 in /etc/modprobe.d/hid_apple.conf.
  (See https://gist.github.com/andrebrait/961cefe730f4a2c41f57911e6195e444)
* Keychron devices can be distinguished from Apple ones by the USB manufacturer
  string, but it's impossible for the kernel to programatically detect whether
  a Keychron keyboard is in "Windows" or "Mac" mode.

The thread arrives at a conclusion I agree with: that fnmode=2 should be the
default behavior for Keychron keyboards. But no one has actually implemented
this change yet, so I decided to do it myself.

My patch sets the default fnmode to the new value of 3, which behaves like
fnmode=2 for Keychron keyboards, and like the previous default of fnmode=1
for real Apple keyboards and any non-Keychron clones. This should produce
sensible behavior in all cases, including the corner case where someone plugs
a Keychron keyboard into their MacBook.

This change does mean that Keychron function keys in "Mac" mode won't default
to Apple-like behavior, but even in that case, both the Fn and non-Fn versions
of the keys are still usable. And as Bastian Venthur said in the thread linked
above, there's no particular reason for a user to expect Apple-like behavior
when using a non-Apple device with a non-Apple operating system.

This is my first time contributing to the kernel, so please let me know if I
need to do anything differently. Also, I don't have an Apple keyboard on hand
to test with, so I'd appreciate if someone could test my patch with one to
verify that their function key behavior is unchanged.

Regards,
Bryan


Bryan Cain (1):
  HID: apple: Properly handle function keys on Keychron keyboards

 drivers/hid/hid-apple.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/1] HID: apple: Properly handle function keys on Keychron keyboards

[Bryan Cain]

Keychron's C-series and K-series of keyboards copy the vendor and
product IDs of an Apple keyboard, but only behave like that device when
set to "Mac" mode. In "Windows" mode, the Fn key doesn't generate a
scancode, so it's impossible to use the F1-F12 keys when fnmode is set
to its default value of 1.

To fix this, make fnmode default to the new value of 3, which behaves
like fnmode=2 for Keychron keyboards and like fnmode=1 for actual Apple
keyboards. This way, Keychron devices are fully usable in both "Windows"
and "Mac" modes, while behavior is unchanged for everything else.

Signed-off-by: Bryan Cain <bryancain3@gmail.com>
---
 drivers/hid/hid-apple.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 0cf35caee9fa..42a568902f49 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
+#include <linux/string.h>
 
 #include "hid-ids.h"
 
@@ -35,16 +36,17 @@
 #define APPLE_NUMLOCK_EMULATION	BIT(8)
 #define APPLE_RDESC_BATTERY	BIT(9)
 #define APPLE_BACKLIGHT_CTL	BIT(10)
+#define APPLE_IS_KEYCHRON	BIT(11)
 
 #define APPLE_FLAG_FKEY		0x01
 
 #define HID_COUNTRY_INTERNATIONAL_ISO	13
 #define APPLE_BATTERY_TIMEOUT_MS	60000
 
-static unsigned int fnmode = 1;
+static unsigned int fnmode = 3;
 module_param(fnmode, uint, 0644);
 MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled, "
-		"[1] = fkeyslast, 2 = fkeysfirst)");
+		"1 = fkeyslast, 2 = fkeysfirst, [3] = auto)");
 
 static int iso_layout = -1;
 module_param(iso_layout, int, 0644);
@@ -349,6 +351,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 	const struct apple_key_translation *trans, *table;
 	bool do_translate;
 	u16 code = 0;
+	unsigned int real_fnmode;
 
 	u16 fn_keycode = (swap_fn_leftctrl) ? (KEY_LEFTCTRL) : (KEY_FN);
 
@@ -359,7 +362,13 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 		return 1;
 	}
 
-	if (fnmode) {
+	if (fnmode == 3) {
+		real_fnmode = (asc->quirks & APPLE_IS_KEYCHRON) ? 2 : 1;
+	} else {
+		real_fnmode = fnmode;
+	}
+
+	if (real_fnmode) {
 		if (hid->product == USB_DEVICE_ID_APPLE_ALU_WIRELESS_ANSI ||
 		    hid->product == USB_DEVICE_ID_APPLE_ALU_WIRELESS_ISO ||
 		    hid->product == USB_DEVICE_ID_APPLE_ALU_WIRELESS_JIS ||
@@ -406,7 +415,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 
 			if (!code) {
 				if (trans->flags & APPLE_FLAG_FKEY) {
-					switch (fnmode) {
+					switch (real_fnmode) {
 					case 1:
 						do_translate = !asc->fn_on;
 						break;
@@ -660,6 +669,11 @@ static int apple_input_configured(struct hid_device *hdev,
 		asc->quirks &= ~APPLE_HAS_FN;
 	}
 
+	if (strncmp(hdev->name, "Keychron", 8) == 0) {
+		hid_info(hdev, "Keychron keyboard detected; function keys will default to fnmode=2 behavior\n");
+		asc->quirks |= APPLE_IS_KEYCHRON;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH 1/1] HID: apple: Properly handle function keys on Keychron keyboards

[Hans de Goede]

Hi Bryan,

On 5/5/22 21:12, Bryan Cain wrote:
> Keychron's C-series and K-series of keyboards copy the vendor and
> product IDs of an Apple keyboard, but only behave like that device when
> set to "Mac" mode. In "Windows" mode, the Fn key doesn't generate a
> scancode, so it's impossible to use the F1-F12 keys when fnmode is set
> to its default value of 1.
> 
> To fix this, make fnmode default to the new value of 3, which behaves
> like fnmode=2 for Keychron keyboards and like fnmode=1 for actual Apple
> keyboards. This way, Keychron devices are fully usable in both "Windows"
> and "Mac" modes, while behavior is unchanged for everything else.
> 
> Signed-off-by: Bryan Cain <bryancain3@gmail.com>

Thank you for doing this. This is pretty much what I had in mind
when discussing things in the previous thread, but I obviously
never got around to implementing this.

The patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/hid/hid-apple.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 0cf35caee9fa..42a568902f49 100644
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
> @@ -21,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/timer.h>
> +#include <linux/string.h>
>  
>  #include "hid-ids.h"
>  
> @@ -35,16 +36,17 @@
>  #define APPLE_NUMLOCK_EMULATION	BIT(8)
>  #define APPLE_RDESC_BATTERY	BIT(9)
>  #define APPLE_BACKLIGHT_CTL	BIT(10)
> +#define APPLE_IS_KEYCHRON	BIT(11)
>  
>  #define APPLE_FLAG_FKEY		0x01
>  
>  #define HID_COUNTRY_INTERNATIONAL_ISO	13
>  #define APPLE_BATTERY_TIMEOUT_MS	60000
>  
> -static unsigned int fnmode = 1;
> +static unsigned int fnmode = 3;
>  module_param(fnmode, uint, 0644);
>  MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled, "
> -		"[1] = fkeyslast, 2 = fkeysfirst)");
> +		"1 = fkeyslast, 2 = fkeysfirst, [3] = auto)");
>  
>  static int iso_layout = -1;
>  module_param(iso_layout, int, 0644);
> @@ -349,6 +351,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
>  	const struct apple_key_translation *trans, *table;
>  	bool do_translate;
>  	u16 code = 0;
> +	unsigned int real_fnmode;
>  
>  	u16 fn_keycode = (swap_fn_leftctrl) ? (KEY_LEFTCTRL) : (KEY_FN);
>  
> @@ -359,7 +362,13 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
>  		return 1;
>  	}
>  
> -	if (fnmode) {
> +	if (fnmode == 3) {
> +		real_fnmode = (asc->quirks & APPLE_IS_KEYCHRON) ? 2 : 1;
> +	} else {
> +		real_fnmode = fnmode;
> +	}
> +
> +	if (real_fnmode) {
>  		if (hid->product == USB_DEVICE_ID_APPLE_ALU_WIRELESS_ANSI ||
>  		    hid->product == USB_DEVICE_ID_APPLE_ALU_WIRELESS_ISO ||
>  		    hid->product == USB_DEVICE_ID_APPLE_ALU_WIRELESS_JIS ||
> @@ -406,7 +415,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
>  
>  			if (!code) {
>  				if (trans->flags & APPLE_FLAG_FKEY) {
> -					switch (fnmode) {
> +					switch (real_fnmode) {
>  					case 1:
>  						do_translate = !asc->fn_on;
>  						break;
> @@ -660,6 +669,11 @@ static int apple_input_configured(struct hid_device *hdev,
>  		asc->quirks &= ~APPLE_HAS_FN;
>  	}
>  
> +	if (strncmp(hdev->name, "Keychron", 8) == 0) {
> +		hid_info(hdev, "Keychron keyboard detected; function keys will default to fnmode=2 behavior\n");
> +		asc->quirks |= APPLE_IS_KEYCHRON;
> +	}
> +
>  	return 0;
>  }
>  

Re: [PATCH 1/1] HID: apple: Properly handle function keys on Keychron keyboards

[Bastien Nocera]

On Thu, 2022-05-05 at 13:12 -0600, Bryan Cain wrote:
> +       if (fnmode == 3) {
> +               real_fnmode = (asc->quirks & APPLE_IS_KEYCHRON) ? 2 :
> 1;
> +       } else {
> +               real_fnmode = fnmode;
> +       }

Wouldn't it be worth adding an enum for those values? (when the
configuration was added would have been a better time, but the second
best time to do it would be now ;)

Re: [PATCH 0/1] HID: apple: Properly handle function keys on Keychron keyboards

[José Expósito]

On Thu, 2022-05-05 at 13:12 -0600, Bryan Cain wrote:
> [...] Also, I don't have an Apple keyboard on hand
> to test with, so I'd appreciate if someone could test my patch with one to
> verify that their function key behavior is unchanged.

Tested with a Magic Keyboard 2015 and a Magic Keyboard 2021 with and
without fingerprint reader. It works as expected :)

Tested-by: José Expósito <jose.exposito89@gmail.com>

Re: [PATCH 1/1] HID: apple: Properly handle function keys on Keychron keyboards

[Jiri Kosina]

On Thu, 5 May 2022, Bryan Cain wrote:

> Keychron's C-series and K-series of keyboards copy the vendor and
> product IDs of an Apple keyboard, but only behave like that device when
> set to "Mac" mode. In "Windows" mode, the Fn key doesn't generate a
> scancode, so it's impossible to use the F1-F12 keys when fnmode is set
> to its default value of 1.
> 
> To fix this, make fnmode default to the new value of 3, which behaves
> like fnmode=2 for Keychron keyboards and like fnmode=1 for actual Apple
> keyboards. This way, Keychron devices are fully usable in both "Windows"
> and "Mac" modes, while behavior is unchanged for everything else.
> 
> Signed-off-by: Bryan Cain <bryancain3@gmail.com>

Applied, thanks Bryan.

-- 
Jiri Kosina
SUSE Labs