linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
@ 2023-10-29 17:05 Yihong Cao
  2023-11-06  3:11 ` Rahul Rameshbabu
  2023-11-21  8:32 ` Jiri Kosina
  0 siblings, 2 replies; 6+ messages in thread
From: Yihong Cao @ 2023-10-29 17:05 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: Yihong Cao, open list:HID CORE LAYER, open list

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" },
+	{ "A3R" },
 };
 
 static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
  2023-10-29 17:05 Yihong Cao
@ 2023-11-06  3:11 ` Rahul Rameshbabu
  2023-11-07 16:08   ` Yihong Cao
  2023-11-21  8:32 ` Jiri Kosina
  1 sibling, 1 reply; 6+ messages in thread
From: Rahul Rameshbabu @ 2023-11-06  3:11 UTC (permalink / raw)
  To: Yihong Cao; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
  2023-11-06  3:11 ` Rahul Rameshbabu
@ 2023-11-07 16:08   ` Yihong Cao
  2023-11-13  1:46     ` Rahul Rameshbabu
  0 siblings, 1 reply; 6+ messages in thread
From: Yihong Cao @ 2023-11-07 16:08 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
  2023-11-07 16:08   ` Yihong Cao
@ 2023-11-13  1:46     ` Rahul Rameshbabu
  0 siblings, 0 replies; 6+ messages in thread
From: Rahul Rameshbabu @ 2023-11-13  1:46 UTC (permalink / raw)
  To: Yihong Cao; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
@ 2023-11-13  2:08 Rahul Rameshbabu
  0 siblings, 0 replies; 6+ messages in thread
From: Rahul Rameshbabu @ 2023-11-13  2:08 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: Yihong Cao, Jiri Kosina, Benjamin Tissoires, linux-input,
	linux-kernel


On Mon, 13 Nov, 2023 01:46:25 +0000 "Rahul Rameshbabu" <sergeantsagara@protonmail.com> wrote:
> 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)

Sorry, my brain slipped here. This is essentially a prefix check. Your
commit works fine :). I consider this patch reviewed. Sorry about that
confusion.

Reviewed-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
  2023-10-29 17:05 Yihong Cao
  2023-11-06  3:11 ` Rahul Rameshbabu
@ 2023-11-21  8:32 ` Jiri Kosina
  1 sibling, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2023-11-21  8:32 UTC (permalink / raw)
  To: Yihong Cao; +Cc: Benjamin Tissoires, open list:HID CORE LAYER, open list

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.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-11-21  8:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-13  2:08 [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list Rahul Rameshbabu
  -- strict thread matches above, loose matches on Subject: below --
2023-10-29 17:05 Yihong Cao
2023-11-06  3:11 ` Rahul Rameshbabu
2023-11-07 16:08   ` Yihong Cao
2023-11-13  1:46     ` Rahul Rameshbabu
2023-11-21  8:32 ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).