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
