Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list

[Rahul Rameshbabu]

On Wed, 08 Nov, 2023 00:08:31 +0800 "Yihong Cao" <caoyihong4@xxxxxxxxxxx> wrote:
> On Mon, Nov 06, 2023 at 03:11:09AM +0000, Rahul Rameshbabu wrote:
>> On Mon, 30 Oct, 2023 01:05:38 +0800 "Yihong Cao" <caoyihong4@xxxxxxxxxxx> wrote:
>> > Jamesdonkey A3R keyboard is identified as "Jamesdonkey A3R" in wired
>> > mode, "A3R-U" in wireless mode and "A3R" in bluetooth mode. Adding them
>> > to non-apple keyboards fixes function key.
>> >
>> > Signed-off-by: Yihong Cao <caoyihong4@xxxxxxxxxxx>
>> > ---
>> >  drivers/hid/hid-apple.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
>> > index 3ca45975c686..d9e9829b2200 100644
>> > --- a/drivers/hid/hid-apple.c
>> > +++ b/drivers/hid/hid-apple.c
>> > @@ -345,6 +345,8 @@ static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
>> >  	{ "AONE" },
>> >  	{ "GANSS" },
>> >  	{ "Hailuck" },
>> > +	{ "Jamesdonkey" },
>>
>> Sorry, maybe I misunderstood the commit message. In wired mode, if the
>> keyboard is identified as "Jamesdonkey A3R", shouldn't this value be
>> "Jamesdonkey A3R" instead of "Jamesdonkey"?
>>
>
> Hi!
>
> "Jamesdonkey" is the manufacturer and "A3R" is the model. I think adding
> manufacturer to non-apple list is suggested, just like commit
> c4444d8749f696384947192b602718fa310c1caf,
> 20afcc462579c0bd79a59ab2b87b82ffa833d118, and
> a0a05054583fed17f522172e101594f1ff265463 did.
>

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

Looking at the code where non_apple_keyboards is used, the values in the
array are compared against the hid device name attribute. This name will
be same as iManufacturer + iProduct output you shared with lsusb (thank
you for doing this).

> However, my keyboard's hardware is buggy, in wireless and wired mode, the
> manufacturer is empty, only model name exists. For your reference, the
> result of `lsusb -v` is pasted below.
>
> In wired mode, `lsusb -v` shows:
>
> Bus 003 Device 002: ID 05ac:024f Apple, Inc. Aluminium Keyboard (ANSI)
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass            0
>   bDeviceSubClass         0
>   bDeviceProtocol         0
>   bMaxPacketSize0        64
>   idVendor           0x05ac Apple, Inc.
>   idProduct          0x024f Aluminium Keyboard (ANSI)
>   bcdDevice            1.26
>   iManufacturer           1 Jamesdonkey
>   iProduct                2 A3R
>   iSerial                 0
>   bNumConfigurations      1

	if (dev->manufacturer)
		strscpy(hid->name, dev->manufacturer, sizeof(hid->name));

	if (dev->product) {
		if (dev->manufacturer)
			strlcat(hid->name, " ", sizeof(hid->name));
		strlcat(hid->name, dev->product, sizeof(hid->name));
	}

The above is from hid-core which indicates the name is Manufacturer +
Product separated by a single space character.

Ok, so in wired mode, one of the entries should be "Jamesdonkey A3R",
rather than just Jamesdonkey.

>
> In wireless mode:
>
> Bus 001 Device 003: ID 05ac:024f Apple, Inc. Aluminium Keyboard (ANSI)
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               1.10
>   bDeviceClass            0
>   bDeviceSubClass         0
>   bDeviceProtocol         0
>   bMaxPacketSize0         8
>   idVendor           0x05ac Apple, Inc.
>   idProduct          0x024f Aluminium Keyboard (ANSI)
>   bcdDevice            2.00
>   iManufacturer           0
>   iProduct                1 A3R-U
>
> And `dmesg` shows:
>
> [ 1779.692121] input: A3R-U as /devices/pci0000:00/0000:00:08.1/0000:06:00.3/usb1/1-2/1-2:1.0/0003:05AC:024F.0008/input/input35
> [ 1779.749037] apple 0003:05AC:024F.0008: input,hidraw2: USB HID v1.10 Keyboard [A3R-U] on usb-0000:06:00.3-2/input0
>
> In bluetooth mode, the iProduct is "A3R".
>
> Adding "A3R" to non-apple list makes keyboard to work in both wireless
> and bluetooth mode.

Are you sure that works? I am pretty sure this value needs to be "A3R-U"
because of the conditional, at least from reading the code. Would you
mind adding a print for hdev->name in the driver right before
apple_is_non_apple_keyboard is called in apple_input_configured to
confirm that the values printed do match up (feel free to share the
dmesg in your reply).

>
> Best wishes,
>
> Yihong Cao
>
>> > +	{ "A3R" },
>> >  };
>> >
>> >  static bool apple_is_non_apple_keyboard(struct hid_device *hdev)

To me, it seems this patch needs to be changed to use "Jamesdonkey A3R"
for wired and "A3R-U" for wireless. Keep in mind, all this function is
used for is adding a special quirk where you see the following
information. Aside from that, Apple vendor keyboards from different
manufacturers will work as expected without any change.

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

--
Thanks for the patch,

Rahul Rameshbabu