Message-ID: <CAPnXWxG8gbe1arQK9kBtwM1Xcta+wreTN742kgtBBr1v0ewKug@mail.gmail.com>
Date:   Tue, 31 May 2022 13:50:30 -0600
From:   Bryan Cain <bryancain3@...il.com>
To:     José Expósito <jose.exposito89@...il.com>
Cc:     Hilton Chain <hako@...rarare.space>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH v2] HID: apple: Workaround for non-Apple keyboards

On Tue, May 31, 2022 at 11:20 AM José Expósito
<jose.exposito89@...il.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@...rarare.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

Powered by blists - more mailing lists