diff mbox series

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

Message ID 20220505191221.36172-2-bryancain3@gmail.com (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series HID: apple: Properly handle function keys on Keychron keyboards | expand

Commit Message

Bryan Cain May 5, 2022, 7:12 p.m. UTC
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(-)

Comments

Hans de Goede May 6, 2022, 9:43 a.m. UTC | #1
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;
>  }
>
Bastien Nocera May 6, 2022, 10:04 a.m. UTC | #2
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 ;)
Jiri Kosina May 11, 2022, 12:22 p.m. UTC | #3
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.
diff mbox series

Patch

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;
 }