From 6813a0a2c0f1a965f650abba3e1e4a8e79b40c26 Mon Sep 17 00:00:00 2001
From: Hilton Chain <[email protected]>
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/[email protected]/
Fixes: a5fe7864d8ad ("HID: apple: Do not reset quirks when the Fn key is not found")
Signed-off-by: Hilton Chain <[email protected]>
---
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
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
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