Re: [PATCH v2] HID: apple: Workaround for non-Apple keyboards
From: <hidden>
Date: 2022-05-31 23:13:47
Also in:
lkml
On Tue, May 31, 2022 at 11:20 AM José Expósito wrote:
quoted hunk ↗ jump to hunk
+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!
quoted hunk ↗ jump to hunk
@@ -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