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

[Hans de Goede <hdegoede@redhat.com>]

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