| Message ID | SYYP282MB2110B4E87983EAFEDC8741E49BA2A@SYYP282MB2110.AUSP282.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: apple: add Jamesdonkey and A3R to non-apple keyboards list | expand |
On Mon, 30 Oct, 2023 01:05:38 +0800 "Yihong Cao" <caoyihong4@outlook.com> 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@outlook.com> > --- > 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"? > + { "A3R" }, > }; > > static bool apple_is_non_apple_keyboard(struct hid_device *hdev) -- Thanks, Rahul Rameshbabu
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@outlook.com> 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@outlook.com> > > --- > > 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. 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 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. Best wishes, Yihong Cao > > + { "A3R" }, > > }; > > > > static bool apple_is_non_apple_keyboard(struct hid_device *hdev) > > -- > Thanks, > > Rahul Rameshbabu >
On Wed, 08 Nov, 2023 00:08:31 +0800 "Yihong Cao" <caoyihong4@outlook.com> 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@outlook.com> 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@outlook.com> >> > --- >> > 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
On Mon, 30 Oct 2023, Yihong Cao 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. Applied, thanks.
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" }, + { "A3R" }, }; static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
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@outlook.com> --- drivers/hid/hid-apple.c | 2 ++ 1 file changed, 2 insertions(+)