HID: apple: Reset quirks when Fn key is not found

From 6813a0a2c0f1a965f650abba3e1e4a8e79b40c26 Mon Sep 17 00:00:00 2001
From: Hilton Chain <hako@ultrarare.space>
Date: Sun, 29 May 2022 16:25:57 +0800
Subject: [PATCH] HID: apple: Reset quirks when Fn key is not found

Commit a5fe7864d8ad ("HID: apple: Do not reset quirks when the Fn key is
not found") re-involves the fnmode issue fixed in commit a5d81646fa29
("HID: apple: Disable Fn-key key-re-mapping on clone keyboards"), as linked
below.

To make things work again, this commit reverts a5fe7864d8ad ("HID: apple:
Do not reset quirks when the Fn key is not found")  and the recent
workaround fa33382c7f74 ("HID: apple: Properly handle function keys on
Keychron keyboards")

Link: https://lore.kernel.org/linux-input/f82dd7a1-a5c6-b651-846c-29f6df9436af@redhat.com/
Fixes: a5fe7864d8ad ("HID: apple: Do not reset quirks when the Fn key is not found")
Signed-off-by: Hilton Chain <hako@ultrarare.space>
---
 drivers/hid/hid-apple.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)


base-commit: fdaf9a5840acaab18694a19e0eb0aa51162eeeed

On Mon, May 30, 2022 at 08:42:32AM +0800, Hilton Chain wrote:
> > My understanding of Bryan's patch (in cc) was that the new config option
> > worked out of the box for Keychron and Apple keyboards and allowed for
> > manual configuration where required.
> >
> > Could you explain a bit which bug is fixed by reverting these 2
> > commits, please? I don't own a Keychron keyboard for testing, so it is
> > not obvious to me why this change is required.
> 
> I own a GANSS keyboard which encounters this issue as well, related device
> information given by `lsusb -v` below:
> 
>     idVendor           0x05ac Apple, Inc.
>     idProduct          0x024f Aluminium Keyboard (ANSI)
>     iManufacturer           1 SONiX
>     iProduct                2 USB DEVICE
> 
> As I searching through, I found similar reports regarding another GANSS
> model[1], and other brands like Varmilo[2] (a lot!) and Keychron. As a
> common pattern, they mostly use 05ac:024f.
> 
> Currently I have two idea:
> 
> 1. Modify Bryan's patch, so that fnmode default to 2 if device name not
> starting with "Apple" (But I can't validate my assumption since I don't
> own any Apple keyboards), I'll attach this patch in the next email.

That could be problematic because Apple keyboards can be renamed after
connecting them to a Mac.

For example, the name of my keyboard is: "José Expósito’s Keyboard".
 
> 2. Find out which quirk pattern solves this issue brute-forcely, I may
> attach this patch later when I finally find a solution.
> 
> What's your opinion?
> 
> Stay boiled,
> Hilton Chain
> 
> ---
> [1]: https://www.amazon.com/gp/customer-reviews/R1EV0B1FG21GGD
> [2]: https://unix.stackexchange.com/questions/604791/keyboard-function-keys-always-trigger-media-shortcuts-regardless-of-whether-fn

I think it'd be safer to assume that the device is an Apple keyboard
and create exceptions for know non-Apple keyboards  because the
majority of the devices handled by this driver are Apple keyboards and
because there is already a config option available (real_fnmode) to fix
the problematic devices in the meanwhile.

In my opinion, creating a function like "apple_is_non_apple_keyboard"
(or similar) containing all the rules to identify devices from
Keychron, GANSS, etc could do the trick. Something similar to:

  if (apple_is_non_apple_keyboard(hdev)) {
          hid_info(hdev, "Non-apple keyboard detected; function keys will default to fnmode=2 behavior\n");
          asc->quirks |= APPLE_IS_NON_APPLE;
  }

Unfortunately, I can't think of a generic way to detect those devices
as they have the same vendor and product as the Apple ones :S

Best wishes,
Jose

There's a bunch of non-Apple keyboard misuses Apple's vendor and product
id, causing hid_apple to be served for them. However they can't handle the
default fnmode.

This commit adds an array of non-Apple keyboards' device names, together
with a function apple_is_non_apple_keyboard() to identify and create
exception for them.

Signed-off-by: Hilton Chain <hako@ultrarare.space>
---
 drivers/hid/hid-apple.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f49..4429b25ae3d8 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -36,7 +36,7 @@
 #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_IS_NON_APPLE	BIT(11)
 
 #define APPLE_FLAG_FKEY		0x01
 
@@ -65,6 +65,10 @@ MODULE_PARM_DESC(swap_fn_leftctrl, "Swap the Fn and left Control keys. "
 		"(For people who want to keep PC keyboard muscle memory. "
 		"[0] = as-is, Mac layout, 1 = swapped, PC layout)");
 
+struct apple_non_apple_keyboard {
+	char *name;
+};
+
 struct apple_sc_backlight {
 	struct led_classdev cdev;
 	struct hid_device *hdev;
@@ -313,6 +317,29 @@ static const struct apple_key_translation swapped_fn_leftctrl_keys[] = {
 	{ }
 };
 
+static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
+	{ "SONiX USB DEVICE" },
+	{ "Keychron" },
+	{ }
+};
+
+static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
+{
+	unsigned long i;
+	unsigned long non_apple_total = sizeof(non_apple_keyboards) /
+					sizeof(struct apple_non_apple_keyboard);
+
+	for (i = 0; i < non_apple_total; i++) {
+		char *non_apple = non_apple_keyboards[i].name;
+
+		if (non_apple && strlen(non_apple) &&
+				strncmp(hdev->name, non_apple, strlen(non_apple)) == 0)
+			return true;
+	}
+
+	return false;
+}
+
 static inline void apple_setup_key_translation(struct input_dev *input,
 		const struct apple_key_translation *table)
 {
@@ -363,7 +390,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 	}
 
 	if (fnmode == 3) {
-		real_fnmode = (asc->quirks & APPLE_IS_KEYCHRON) ? 2 : 1;
+		real_fnmode = (asc->quirks & APPLE_IS_NON_APPLE) ? 2 : 1;
 	} else {
 		real_fnmode = fnmode;
 	}
@@ -667,11 +694,12 @@ static int apple_input_configured(struct hid_device *hdev,
 	if ((asc->quirks & APPLE_HAS_FN) && !asc->fn_found) {
 		hid_info(hdev, "Fn key not found (Apple Wireless Keyboard clone?), disabling Fn key handling\n");
 		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;
+		if (apple_is_non_apple_keyboard(hdev)) {
+			hid_info(hdev,
+				"Non-apple keyboard detected; function keys will default to fnmode=2 behavior\n");
+			asc->quirks |= APPLE_IS_NON_APPLE;
+		}
 	}
 
 	return 0;

base-commit: 8ab2afa23bd197df47819a87f0265c0ac95c5b6a