| Message ID | 20220529180230.17e9a0f9@ultrarare.space (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: apple: Reset quirks when Fn key is not found | expand |
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>
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
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
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
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
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
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
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;