[PATCH] HID: apple: Reset quirks when Fn key is not found

[PATCH] HID: apple: Reset quirks when Fn key is not found

[Hilton Chain]

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(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f49..3b666dcb63f0 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -21,7 +21,6 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
-#include <linux/string.h>
 
 #include "hid-ids.h"
 
@@ -36,17 +35,16 @@
 #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 = 3;
+static unsigned int fnmode = 1;
 module_param(fnmode, uint, 0644);
 MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled, "
-		"1 = fkeyslast, 2 = fkeysfirst, [3] = auto)");
+		"[1] = fkeyslast, 2 = fkeysfirst)");
 
 static int iso_layout = -1;
 module_param(iso_layout, int, 0644);
@@ -351,7 +349,6 @@ 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);
 
@@ -362,13 +359,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 		return 1;
 	}
 
-	if (fnmode == 3) {
-		real_fnmode = (asc->quirks & APPLE_IS_KEYCHRON) ? 2 : 1;
-	} else {
-		real_fnmode = fnmode;
-	}
-
-	if (real_fnmode) {
+	if (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 ||
@@ -415,7 +406,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 
 			if (!code) {
 				if (trans->flags & APPLE_FLAG_FKEY) {
-					switch (real_fnmode) {
+					switch (fnmode) {
 					case 1:
 						do_translate = !asc->fn_on;
 						break;
@@ -664,14 +655,10 @@ static int apple_input_configured(struct hid_device *hdev,
 {
 	struct apple_sc *asc = hid_get_drvdata(hdev);
 
+	/* Handling some non-Apple keyboards which use Apple's vendor ID */
 	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;
+		asc->quirks = 0;
 	}
 
 	return 0;

base-commit: fdaf9a5840acaab18694a19e0eb0aa51162eeeed
-- 
2.36.1

Re: [PATCH] HID: apple: Reset quirks when Fn key is not found

[José Expósito]

Hi Hilton,

> 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.

Reverting that commit will break battery reporting on the Magic
Keyboards 2015 and 2021.

When a keyboard has the APPLE_HAS_FN and another valid quirk, in this
case APPLE_RDESC_BATTERY, setting asc->quirks = 0 (i.e., removing all
quirks) also removes the valid ones.

> 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")

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.

Thanks,Jose

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

Re: [PATCH] HID: apple: Reset quirks when Fn key is not found

[Hilton Chain]

Hi Jose,

> Reverting that commit will break battery reporting on the Magic
> Keyboards 2015 and 2021.
>
> When a keyboard has the APPLE_HAS_FN and another valid quirk, in this
> case APPLE_RDESC_BATTERY, setting asc->quirks = 0 (i.e., removing all
> quirks) also removes the valid ones.

Thanks for the explanation!

> 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.

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

[PATCH] HID: apple: Properly handle function keys on misset non-apple keyboards

[Hilton Chain]

This commit extends fa33382c7f74 ("HID: apple: Properly handle function
keys on Keychron keyboards") to support all misset non-apple keyboards.

Signed-off-by: Hilton Chain <hako@ultrarare.space>
---

 drivers/hid/hid-apple.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f49..3b15753be467 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_BAD_APPLE	BIT(11)

 #define APPLE_FLAG_FKEY		0x01

@@ -363,7 +363,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_BAD_APPLE) ? 2 : 1;
 	} else {
 		real_fnmode = fnmode;
 	}
@@ -669,9 +669,9 @@ 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;
+	if (strncmp(hdev->name, "Apple", 5)) {
+		hid_info(hdev, "Non-apple keyboard detected; function keys will default to fnmode=2 behavior\n");
+		asc->quirks |= APPLE_IS_BAD_APPLE;
 	}

 	return 0;

base-commit: b00ed48bb0a7c295facf9036135a573a5cdbe7de
--
2.36.1

Re: [PATCH] HID: apple: Reset quirks when Fn key is not found

[José Expósito]

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

Re: [PATCH] HID: apple: Reset quirks when Fn key is not found

[Hilton Chain]

On Mon, 30 May 2022 08:18:12 +0200 José Expósito wrote:
> 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".

Well, editable name. I have a bad feeling about this...

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

Done. However I couldn't figure out corresponding names from other known
bug reports, so that the initial array only contains two devices.

> 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

T^T

Stay boiled,
Chain

Re: [PATCH v2] HID: apple: Workaround for non-Apple keyboards

[Hilton Chain]

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
-- 
2.36.1

Re: [PATCH v2] HID: apple: Workaround for non-Apple keyboards

[José Expósito]

Hi Hilton,

Thanks for sending v2 of this patch.
Please find a couple of minor comments inline:

On Tue, May 31, 2022 at 10:33:30PM +0800, Hilton Chain wrote:
> 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" },
> +	{ }

Could the "non_apple && strlen(non_apple)" check be avoided by removing
this empty item?

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

Here you coud take advantage of the "ARRAY_SIZE" macro:

https://kernelnewbies.org/MagicMacros

It'll also allow you to use an int. Something similar to:

	int i;

	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
		[...]

> +
> +	for (i = 0; i < non_apple_total; i++) {
> +		char *non_apple = non_apple_keyboards[i].name;
> +
> +		if (non_apple && strlen(non_apple) &&

This is the check I meant in my first comment ^

> +				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");

Checkpatch nitpick:

	CHECK: Alignment should match open parenthesis
	FILE: drivers/hid/hid-apple.c:700:
	hid_info(hdev,
		"Non-apple keyboard detected; function keys will default to fnmode=2 behavior\n");

It suggest to add  an extra space before "Non-apple ...".

In case you don't know the tool, it helps to find style errors, I
usually run it like:

$ ./scripts/checkpatch.pl --strict --codespell --git HEAD-1


> +			asc->quirks |= APPLE_IS_NON_APPLE;
> +		}

This slightly changes the behaviour from the previous patch.
Previously, the APPLE_IS_NON_APPLE quirk was set even if APPLE_HAS_FN
was not present. Now the condition is nested.

I'm not saying it is wrong (I don't have the required hardware to test
it), I'm just pointing it out in case it was an accidental change.
Bryan, should be able to confirm if it works with his keyboard.

>  	}
>  
>  	return 0;
> 
> base-commit: 8ab2afa23bd197df47819a87f0265c0ac95c5b6a
> -- 
> 2.36.1
> 


Best wishes,
José Expósito

Re: [PATCH v2] HID: apple: Workaround for non-Apple keyboards

[Bryan Cain]

On Tue, May 31, 2022 at 11:20 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Hi Hilton,
>
> Thanks for sending v2 of this patch.
> Please find a couple of minor comments inline:
>
> On Tue, May 31, 2022 at 10:33:30PM +0800, Hilton Chain wrote:
> > 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" },
> > +     { }
>
> Could the "non_apple && strlen(non_apple)" check be avoided by removing
> this empty item?
>
> > +};
> > +
> > +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);
>
> Here you coud take advantage of the "ARRAY_SIZE" macro:
>
> https://kernelnewbies.org/MagicMacros
>
> It'll also allow you to use an int. Something similar to:
>
>         int i;
>
>         for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
>                 [...]
>
> > +
> > +     for (i = 0; i < non_apple_total; i++) {
> > +             char *non_apple = non_apple_keyboards[i].name;
> > +
> > +             if (non_apple && strlen(non_apple) &&
>
> This is the check I meant in my first comment ^
>
> > +                             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");
>
> Checkpatch nitpick:
>
>         CHECK: Alignment should match open parenthesis
>         FILE: drivers/hid/hid-apple.c:700:
>         hid_info(hdev,
>                 "Non-apple keyboard detected; function keys will default to fnmode=2 behavior\n");
>
> It suggest to add  an extra space before "Non-apple ...".
>
> In case you don't know the tool, it helps to find style errors, I
> usually run it like:
>
> $ ./scripts/checkpatch.pl --strict --codespell --git HEAD-1
>
>
> > +                     asc->quirks |= APPLE_IS_NON_APPLE;
> > +             }
>
> This slightly changes the behaviour from the previous patch.
> Previously, the APPLE_IS_NON_APPLE quirk was set even if APPLE_HAS_FN
> was not present. Now the condition is nested.
>
> I'm not saying it is wrong (I don't have the required hardware to test
> it), I'm just pointing it out in case it was an accidental change.
> Bryan, should be able to confirm if it works with his keyboard.

I haven't tested it, but I can tell from reading the patch that it will break
compatibility with Keychron keyboards like mine, precisely because of the
nesting.

The biggest reason that my Keychron patch was needed at all was that Keychron
devices advertise the Fn key, and thus don't hit the first clone check since
asc->fn_found is actually true for them. So nesting the check for the Keychron
manufacturer/product name inside of that check won't work.

To tell the truth, I'm still a bit confused about the precise behavior of the
Sonix firmware that this patch is made to work around. If it's not advertising
an Apple-style Fn key, why isn't the existing behavior of disabling Fn-key
handling enough to make it work? The fnmode parameter is ignored entirely
when APPLE_HAS_FN isn't set, so it's hard to imagine that the change to fnmode
behavior would even do anything in that case.

Regards,
Bryan

>
> >       }
> >
> >       return 0;
> >
> > base-commit: 8ab2afa23bd197df47819a87f0265c0ac95c5b6a
> > --
> > 2.36.1
> >
>
>
> Best wishes,
> José Expósito

Re: [PATCH v2] HID: apple: Workaround for non-Apple keyboards

[hako]

On Tue, May 31, 2022 at 11:20 AM José Expósito wrote:

> +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" },
> + { }
> 
> Could the "non_apple && strlen(non_apple)" check be avoided by removing
> this empty item?

Hi Jose,
If there's a chance that the device name is an empty string? In such case the
strlen should be preserved.

> +};
> +
> +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);
> 
> Here you coud take advantage of the "ARRAY_SIZE" macro:
> 
> https://kernelnewbies.org/MagicMacros
> 
> It'll also allow you to use an int. Something similar to:
> 
> int i;
> 
> for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
> [...]

Thanks for the information!

> @@ -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");
> 
> Checkpatch nitpick:
> 
> CHECK: Alignment should match open parenthesis
> FILE: drivers/hid/hid-apple.c:700:
> hid_info(hdev,
> "Non-apple keyboard detected; function keys will default to fnmode=2 behavior\n");
> 
> It suggest to add an extra space before "Non-apple ...".
> 
> In case you don't know the tool, it helps to find style errors, I
> usually run it like:
> 
> $ ./scripts/checkpatch.pl --strict --codespell --git HEAD-1
> 
> + asc->quirks |= APPLE_IS_NON_APPLE;
> + }
> 
> This slightly changes the behaviour from the previous patch.
> Previously, the APPLE_IS_NON_APPLE quirk was set even if APPLE_HAS_FN
> was not present. Now the condition is nested.
> 
> I'm not saying it is wrong (I don't have the required hardware to test
> it), I'm just pointing it out in case it was an accidental change.
> Bryan, should be able to confirm if it works with his keyboard.

Thanks again!

On Tue, 31 May 2022 13:50:30 -0600 Bryan Cain wrote:

> I haven't tested it, but I can tell from reading the patch that it will break
> compatibility with Keychron keyboards like mine, precisely because of the
> nesting.
> 
> The biggest reason that my Keychron patch was needed at all was that Keychron
> devices advertise the Fn key, and thus don't hit the first clone check since
> asc->fn_found is actually true for them. So nesting the check for the Keychron
> manufacturer/product name inside of that check won't work.
> 
> To tell the truth, I'm still a bit confused about the precise behavior of the
> Sonix firmware that this patch is made to work around. If it's not advertising
> an Apple-style Fn key, why isn't the existing behavior of disabling Fn-key
> handling enough to make it work? The fnmode parameter is ignored entirely
> when APPLE_HAS_FN isn't set, so it's hard to imagine that the change to fnmode
> behavior would even do anything in that case.

Hi Bryan,
Sorry for my inconsiderateness...

It advertises such function:
- FN Function Keys Make Control easily.
FN+F1:My Computer
FN+F2:Browser
FN+F3:Email
...

I made a vital mistake by not properly checking the patch before sending,
that's totally my fault.

Sorry again for the mess I made.

Best wishes,
Chain

[PATCH v3] HID: apple: Properly handle function keys on non-Apple keyboard

[Hilton Chain]

This commit extends fa33382c7f74 ("HID: apple: Properly handle function
keys on Keychron keyboards") by adding an array of known non-Apple
keyboards' device names, and the 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 | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f49..7d56530d9e3a 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,25 @@ 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)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
+		char *non_apple = non_apple_keyboards[i].name;
+
+		if (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 +386,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;
 	}
@@ -669,9 +692,9 @@ 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;
+	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: e1cbc3b96a9974746b2a80c3a6c8a0f7eff7b1b5
-- 
2.36.1

[PATCH v4] HID: apple: Properly handle function keys on non-Apple keyboard

[Hilton Chain]

This commit extends fa33382c7f74 ("HID: apple: Properly handle function
keys on Keychron keyboards") by adding an array of known non-Apple
keyboards' device names, and the function apple_is_non_apple_keyboard()
to identify and create exception for them.

Signed-off-by: Hilton Chain <hako@ultrarare.space>
---

V3 -> V4: Removed unnecessary strlen()

 drivers/hid/hid-apple.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f49..4ec39c5e762a 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,25 @@ 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)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
+		char *non_apple = non_apple_keyboards[i].name;
+
+		if (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 +386,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;
 	}
@@ -669,9 +692,9 @@ 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;
+	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: 700170bf6b4d773e328fa54ebb70ba444007c702
-- 
2.36.1

Re: [PATCH v4] HID: apple: Properly handle function keys on non-Apple keyboard

[José Expósito]

On Wed, Jun 01, 2022 at 12:17:37PM +0800, Hilton Chain wrote:
> This commit extends fa33382c7f74 ("HID: apple: Properly handle function
> keys on Keychron keyboards") by adding an array of known non-Apple
> keyboards' device names, and the function apple_is_non_apple_keyboard()
> to identify and create exception for them.
> 
> Signed-off-by: Hilton Chain <hako@ultrarare.space>


v4 looks good to me, thanks for adding the suggested changes!

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


> ---
> 
> V3 -> V4: Removed unnecessary strlen()
> 
>  drivers/hid/hid-apple.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 42a568902f49..4ec39c5e762a 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,25 @@ 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)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
> +		char *non_apple = non_apple_keyboards[i].name;
> +
> +		if (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 +386,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;
>  	}
> @@ -669,9 +692,9 @@ 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;
> +	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: 700170bf6b4d773e328fa54ebb70ba444007c702
> -- 
> 2.36.1
> 

Re: [PATCH v3] HID: apple: Properly handle function keys on non-Apple keyboard

[Bryan Cain]

I can't reply to v4 since I wasn't cc'd on that one and I'm not subscribed to
the mailing list, but while I haven't tested it yet, it looks good.

The only feedback I have is that according to my searching[1][2][3], Varmilo
keyboards pretending to be Apple devices report themselves as "AONE Varmilo
Keyboard", so it might be a good idea to add "AONE" to the list of clone
vendors.

Regards,
Bryan

[1] https://geekhack.org/index.php?topic=110250.msg3013629#msg3013629
[2] https://www.mail-archive.com/debian-bugs-dist@lists.debian.org/msg1750654.html
[3] https://forums.servethehome.com/index.php?threads/varmilo-keyboard-fn-keys-under-linux.29041/

On Tue, May 31, 2022 at 5:27 PM Hilton Chain <hako@ultrarare.space> wrote:
>
> This commit extends fa33382c7f74 ("HID: apple: Properly handle function
> keys on Keychron keyboards") by adding an array of known non-Apple
> keyboards' device names, and the 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 | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 42a568902f49..7d56530d9e3a 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,25 @@ 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)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
> +               char *non_apple = non_apple_keyboards[i].name;
> +
> +               if (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 +386,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;
>         }
> @@ -669,9 +692,9 @@ 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;
> +       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: e1cbc3b96a9974746b2a80c3a6c8a0f7eff7b1b5
> --
> 2.36.1
>

[PATCH v5] HID: apple: Properly handle function keys on non-Apple keyboard

[Hilton Chain]

This commit extends fa33382c7f74 ("HID: apple: Properly handle function
keys on Keychron keyboards") by adding an array of known non-Apple
keyboards' device names, and the function apple_is_non_apple_keyboard()
to identify and create exception for them.

Signed-off-by: Hilton Chain <hako@ultrarare.space>
---

V4 -> V5: Add Varmilo keyboards' name "AONE" to the exception list
V3 -> V4: Remove unnecessary strlen()

 drivers/hid/hid-apple.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f49..7fbde58e1219 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,26 @@ static const struct apple_key_translation swapped_fn_leftctrl_keys[] = {
 	{ }
 };
 
+static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
+	{ "SONiX USB DEVICE" },
+	{ "Keychron" },
+	{ "AONE" }
+};
+
+static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
+		char *non_apple = non_apple_keyboards[i].name;
+
+		if (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 +387,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;
 	}
@@ -669,9 +693,9 @@ 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;
+	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: d1dc87763f406d4e67caf16dbe438a5647692395
-- 
2.36.1

Re: [PATCH v5] HID: apple: Properly handle function keys on non-Apple keyboard

[Bryan Cain]

This version looks good, and works without issue on my Keychron C1.
Thanks for implementing it!

Reviewed-by: Bryan Cain <bryancain3@gmail.com>
Tested-by: Bryan Cain <bryancain3@gmail.com>

Regards,
Bryan

On Thu, Jun 2, 2022 at 2:12 AM Hilton Chain <hako@ultrarare.space> wrote:
>
> This commit extends fa33382c7f74 ("HID: apple: Properly handle function
> keys on Keychron keyboards") by adding an array of known non-Apple
> keyboards' device names, and the function apple_is_non_apple_keyboard()
> to identify and create exception for them.
>
> Signed-off-by: Hilton Chain <hako@ultrarare.space>
> ---
>
> V4 -> V5: Add Varmilo keyboards' name "AONE" to the exception list
> V3 -> V4: Remove unnecessary strlen()
>
>  drivers/hid/hid-apple.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 42a568902f49..7fbde58e1219 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,26 @@ static const struct apple_key_translation swapped_fn_leftctrl_keys[] = {
>         { }
>  };
>
> +static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
> +       { "SONiX USB DEVICE" },
> +       { "Keychron" },
> +       { "AONE" }
> +};
> +
> +static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
> +               char *non_apple = non_apple_keyboards[i].name;
> +
> +               if (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 +387,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;
>         }
> @@ -669,9 +693,9 @@ 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;
> +       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: d1dc87763f406d4e67caf16dbe438a5647692395
> --
> 2.36.1
>

Re: [PATCH v5] HID: apple: Properly handle function keys on non-Apple keyboard

[José Expósito]

On Thu, Jun 02, 2022 at 04:12:19PM +0800, Hilton Chain wrote:
> This commit extends fa33382c7f74 ("HID: apple: Properly handle function
> keys on Keychron keyboards") by adding an array of known non-Apple
> keyboards' device names, and the function apple_is_non_apple_keyboard()
> to identify and create exception for them.
> 
> Signed-off-by: Hilton Chain <hako@ultrarare.space>

Minor change from V4, feel free to add my:

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

Jose

> ---
> 
> V4 -> V5: Add Varmilo keyboards' name "AONE" to the exception list
> V3 -> V4: Remove unnecessary strlen()
> 
>  drivers/hid/hid-apple.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 42a568902f49..7fbde58e1219 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,26 @@ static const struct apple_key_translation swapped_fn_leftctrl_keys[] = {
>  	{ }
>  };
>  
> +static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
> +	{ "SONiX USB DEVICE" },
> +	{ "Keychron" },
> +	{ "AONE" }
> +};
> +
> +static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
> +		char *non_apple = non_apple_keyboards[i].name;
> +
> +		if (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 +387,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;
>  	}
> @@ -669,9 +693,9 @@ 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;
> +	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: d1dc87763f406d4e67caf16dbe438a5647692395
> -- 
> 2.36.1
> 

Re: [PATCH v5] HID: apple: Properly handle function keys on non-Apple keyboard

[Jiri Kosina]

On Thu, 2 Jun 2022, Hilton Chain wrote:

> This commit extends fa33382c7f74 ("HID: apple: Properly handle function
> keys on Keychron keyboards") by adding an array of known non-Apple
> keyboards' device names, and the function apple_is_non_apple_keyboard()
> to identify and create exception for them.
> 
> Signed-off-by: Hilton Chain <hako@ultrarare.space>
> ---
> 
> V4 -> V5: Add Varmilo keyboards' name "AONE" to the exception list
> V3 -> V4: Remove unnecessary strlen()

Applied to hid.git, thank you.

-- 
Jiri Kosina
SUSE Labs

[PATCH v6] HID: apple: Properly handle function keys on non-Apple keyboard

[Hilton Chain]

This commit extends fa33382c7f74 ("HID: apple: Properly handle function
keys on Keychron keyboards") by adding an array of known non-Apple
keyboards' device names, and the function apple_is_non_apple_keyboard()
to identify and create exception for them.

Signed-off-by: Hilton Chain <hako@ultrarare.space>
---

V5 -> V6: Add "GANSS" to the exception list.

I just found out my keyboard would use another name in bluetooth mode, as I
forgot to test this scenario... Sorry for the inconvenience!


drivers/hid/hid-apple.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 42a568902f492..3cf8eafe00b7b 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,27 @@ static const struct apple_key_translation swapped_fn_leftctrl_keys[] = {
 	{ }
 };

+static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
+	{ "SONiX USB DEVICE" },
+	{ "GANSS" },
+	{ "Keychron" },
+	{ "AONE" }
+};
+
+static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(non_apple_keyboards); i++) {
+		char *non_apple = non_apple_keyboards[i].name;
+
+		if (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 +388,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;
 	}
@@ -669,9 +694,9 @@ 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;
+	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: 4b35035bcf80ddb47c0112c4fbd84a63a2836a18
--
2.36.1