| Message ID | 20190328033435.17931-2-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/2] HID: macally: Add support for Macally ikey keyboard | expand |
Hi Alex, On Thu, Mar 28, 2019 at 4:34 AM Alex Henrie <alexhenrie24@gmail.com> wrote: > > This enables the brightness-down and brightness-up keys on the Macally > QKEY keyboard. Similar workarounds are probably needed for quite a few > Macally keyboard models. > > Based on the key translation code in the Apple keyboard driver. Well, this driver is much more complex and we already have all the facility in HID to do plain remapping (and we could even do that in userspace with a hwdb entry). So: > > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > drivers/hid/Kconfig | 1 + > drivers/hid/hid-ids.h | 3 +++ > drivers/hid/hid-macally.c | 57 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 61 insertions(+) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index aef4a2a690e1..082900477df5 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -239,6 +239,7 @@ config HID_MACALLY > > supported devices: > - Macally ikey keyboard > + - Macally QKEY keyboard > > config HID_PRODIKEYS > tristate "Prodikeys PC-MIDI Keyboard support" > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index aacc7534b076..5afc3b7fe8ca 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -776,6 +776,9 @@ > #define USB_DEVICE_ID_CRYSTALTOUCH 0x0006 > #define USB_DEVICE_ID_CRYSTALTOUCH_DUAL 0x0007 > > +#define USB_VENDOR_ID_MACALLY 0x2222 > +#define USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD 0x0039 > + > #define USB_VENDOR_ID_MADCATZ 0x0738 > #define USB_DEVICE_ID_MADCATZ_BEATPAD 0x4540 > #define USB_DEVICE_ID_MADCATZ_RAT5 0x1705 > diff --git a/drivers/hid/hid-macally.c b/drivers/hid/hid-macally.c > index 6f62f059b795..2567babe8200 100644 > --- a/drivers/hid/hid-macally.c > +++ b/drivers/hid/hid-macally.c > @@ -31,9 +31,64 @@ static __u8 *macally_report_fixup(struct hid_device *hdev, __u8 *rdesc, > return rdesc; > } > > +struct macally_key_translation > +{ > + u16 from; > + u16 to; > +}; > + > +static const struct macally_key_translation qkey_brightness_keys[] = > +{ > + { KEY_SCROLLLOCK, KEY_BRIGHTNESSDOWN }, > + { KEY_PAUSE, KEY_BRIGHTNESSUP }, > + { } > +}; This declaration and its associated struct should be dropped and inlined in input_mapping() (see below) > + > +static int macally_event(struct hid_device *hdev, struct hid_field *field, > + struct hid_usage *usage, __s32 value) > +{ > + const struct macally_key_translation *trans; > + > + switch (hdev->product) { > + case USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD: > + trans = qkey_brightness_keys; > + break; > + default: > + trans = NULL; > + } > + > + if (trans) { > + while (trans->from) { > + if (trans->from == usage->code) { > + input_event(field->hidinput->input, usage->type, > + trans->to, value); > + return 1; > + } > + trans++; > + } > + } > + > + return 0; > +} Please drop the entire callback above > + > +static int macally_input_mapping(struct hid_device *hdev, struct hid_input *hi, > + struct hid_field *field, struct hid_usage *usage, > + unsigned long **bit, int *max) > +{ > + const struct macally_key_translation *trans; > + > + /* Enable all needed keys */ > + for (trans = qkey_brightness_keys; trans->from; trans++) > + set_bit(trans->to, hi->input->keybit); See hid-accutouch: if ((usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON) { hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH); return 1; } So just add a switch instead of a plain 'if', check on the KEY_SCROLLLOCK or KEY_PAUSE keycode and remap them to the correct brightness up/down. But again, you can get tot he same result by adding a hwdb entry. However, given that you need to fix up the report descriptor for the (other?) keyboard, I can be convinced to take this patch too. Dmitry, Hans, do you think this belong to the kernel or should this be taken into a udev rule? Cheers, Benjamin > + > + return 0; > +} > + > static struct hid_device_id macally_id_table[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, > USB_DEVICE_ID_MACALLY_IKEY_KEYBOARD) }, > + { HID_USB_DEVICE(USB_VENDOR_ID_MACALLY, > + USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD) }, > { } > }; > MODULE_DEVICE_TABLE(hid, macally_id_table); > @@ -42,6 +97,8 @@ static struct hid_driver macally_driver = { > .name = "macally", > .id_table = macally_id_table, > .report_fixup = macally_report_fixup, > + .event = macally_event, > + .input_mapping = macally_input_mapping, > }; > > module_hid_driver(macally_driver); > -- > 2.21.0 >
Hi, On 28-03-19 09:10, Benjamin Tissoires wrote: > Hi Alex, > > On Thu, Mar 28, 2019 at 4:34 AM Alex Henrie <alexhenrie24@gmail.com> wrote: >> >> This enables the brightness-down and brightness-up keys on the Macally >> QKEY keyboard. Similar workarounds are probably needed for quite a few >> Macally keyboard models. >> >> Based on the key translation code in the Apple keyboard driver. > > Well, this driver is much more complex and we already have all the > facility in HID to do plain remapping (and we could even do that in > userspace with a hwdb entry). > So: > >> >> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> >> --- >> drivers/hid/Kconfig | 1 + >> drivers/hid/hid-ids.h | 3 +++ >> drivers/hid/hid-macally.c | 57 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 61 insertions(+) >> >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >> index aef4a2a690e1..082900477df5 100644 >> --- a/drivers/hid/Kconfig >> +++ b/drivers/hid/Kconfig >> @@ -239,6 +239,7 @@ config HID_MACALLY >> >> supported devices: >> - Macally ikey keyboard >> + - Macally QKEY keyboard >> >> config HID_PRODIKEYS >> tristate "Prodikeys PC-MIDI Keyboard support" >> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h >> index aacc7534b076..5afc3b7fe8ca 100644 >> --- a/drivers/hid/hid-ids.h >> +++ b/drivers/hid/hid-ids.h >> @@ -776,6 +776,9 @@ >> #define USB_DEVICE_ID_CRYSTALTOUCH 0x0006 >> #define USB_DEVICE_ID_CRYSTALTOUCH_DUAL 0x0007 >> >> +#define USB_VENDOR_ID_MACALLY 0x2222 >> +#define USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD 0x0039 >> + >> #define USB_VENDOR_ID_MADCATZ 0x0738 >> #define USB_DEVICE_ID_MADCATZ_BEATPAD 0x4540 >> #define USB_DEVICE_ID_MADCATZ_RAT5 0x1705 >> diff --git a/drivers/hid/hid-macally.c b/drivers/hid/hid-macally.c >> index 6f62f059b795..2567babe8200 100644 >> --- a/drivers/hid/hid-macally.c >> +++ b/drivers/hid/hid-macally.c >> @@ -31,9 +31,64 @@ static __u8 *macally_report_fixup(struct hid_device *hdev, __u8 *rdesc, >> return rdesc; >> } >> >> +struct macally_key_translation >> +{ >> + u16 from; >> + u16 to; >> +}; >> + >> +static const struct macally_key_translation qkey_brightness_keys[] = >> +{ >> + { KEY_SCROLLLOCK, KEY_BRIGHTNESSDOWN }, >> + { KEY_PAUSE, KEY_BRIGHTNESSUP }, >> + { } >> +}; > > This declaration and its associated struct should be dropped and > inlined in input_mapping() (see below) > >> + >> +static int macally_event(struct hid_device *hdev, struct hid_field *field, >> + struct hid_usage *usage, __s32 value) >> +{ >> + const struct macally_key_translation *trans; >> + >> + switch (hdev->product) { >> + case USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD: >> + trans = qkey_brightness_keys; >> + break; >> + default: >> + trans = NULL; >> + } >> + >> + if (trans) { >> + while (trans->from) { >> + if (trans->from == usage->code) { >> + input_event(field->hidinput->input, usage->type, >> + trans->to, value); >> + return 1; >> + } >> + trans++; >> + } >> + } >> + >> + return 0; >> +} > > Please drop the entire callback above > >> + >> +static int macally_input_mapping(struct hid_device *hdev, struct hid_input *hi, >> + struct hid_field *field, struct hid_usage *usage, >> + unsigned long **bit, int *max) >> +{ >> + const struct macally_key_translation *trans; >> + >> + /* Enable all needed keys */ >> + for (trans = qkey_brightness_keys; trans->from; trans++) >> + set_bit(trans->to, hi->input->keybit); > > See hid-accutouch: > > if ((usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON) { > hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH); > return 1; > } > > So just add a switch instead of a plain 'if', check on the > KEY_SCROLLLOCK or KEY_PAUSE keycode and remap them to the correct > brightness up/down. > > But again, you can get tot he same result by adding a hwdb entry. > However, given that you need to fix up the report descriptor for the > (other?) keyboard, I can be convinced to take this patch too. > > Dmitry, Hans, do you think this belong to the kernel or should this be > taken into a udev rule? Descriptor fixups obviously need to be done in the kernel, but I believe that key-remapping which can be done through hwdb is best done there as it is a lot easier to update 60-keyboard.hwdb then to change to running a (lot) newer kernel and more importantly we should strive to keep the kernel code as small as possible. Now about this specific keyboards, a quick duckduckgo found me this picture: https://www.bhphotovideo.com/images/images1500x1500/macally_mkeyx_104_key_full_sized_1274821.jpg And to me it seems scroll-lock and pause-break should be mapped to zoom-put resp. zoom-in and Fn + scroll-lock / pause-break should send the brightness down/up events. Alex have you checked if using Fn + these keys sends a different hid usage-code event? Hmm, I see all the top-tow keys have a special meaning and a Fn + key meaning, where Fn seems to be required to get them to behave as the normal key in that position, so I guess the scroll-lock / pause-break is actually send when using the keys with Fn ? Regards, Hans
On Thu, Mar 28, 2019 at 3:20 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Descriptor fixups obviously need to be done in the kernel, but I > believe that key-remapping which can be done through hwdb is best done > there as it is a lot easier to update 60-keyboard.hwdb then to change > to running a (lot) newer kernel and more importantly we should strive > to keep the kernel code as small as possible. > > Now about this specific keyboards, a quick duckduckgo found me this > picture: > > https://www.bhphotovideo.com/images/images1500x1500/macally_mkeyx_104_key_full_sized_1274821.jpg The picture you found is of the Macally MKEYX, which I do not have. This is the Macally ikey: https://cultek2.com/products/macally-ikey-wired-usb-keyboard-for-macintosh-or-pc-preowned?variant=12129379156040 And this is the Macally QKEY: https://images-na.ssl-images-amazon.com/images/I/61c7zcQJMzL._SL1500_.jpg The brightness-down and brightness-up keys are Fn+F1 and Fn+F2. The keys work as expected on a Mac, so I'm guessing that a long time ago Apple decided to repurpose the scancodes for Scroll Lock and Pause, leading to the headache we have now. But as you and Benjamin pointed out, systemd is a better place to fix this. So, how about committing the first patch and forgetting about the second one? Does the ikey driver need any changes or can it be accepted into the kernel as-is? -Alex
Hi, On 28-03-19 23:18, Alex Henrie wrote: > On Thu, Mar 28, 2019 at 3:20 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Descriptor fixups obviously need to be done in the kernel, but I >> believe that key-remapping which can be done through hwdb is best done >> there as it is a lot easier to update 60-keyboard.hwdb then to change >> to running a (lot) newer kernel and more importantly we should strive >> to keep the kernel code as small as possible. >> >> Now about this specific keyboards, a quick duckduckgo found me this >> picture: >> >> https://www.bhphotovideo.com/images/images1500x1500/macally_mkeyx_104_key_full_sized_1274821.jpg > > The picture you found is of the Macally MKEYX, which I do not have. > This is the Macally ikey: > > https://cultek2.com/products/macally-ikey-wired-usb-keyboard-for-macintosh-or-pc-preowned?variant=12129379156040 > > And this is the Macally QKEY: > > https://images-na.ssl-images-amazon.com/images/I/61c7zcQJMzL._SL1500_.jpg > > The brightness-down and brightness-up keys are Fn+F1 and Fn+F2. The > keys work as expected on a Mac, so I'm guessing that a long time ago > Apple decided to repurpose the scancodes for Scroll Lock and Pause, > leading to the headache we have now. Ok. > But as you and Benjamin pointed > out, systemd is a better place to fix this. Agreed. > So, how about committing the first patch and forgetting about the > second one? Does the ikey driver need any changes or can it be > accepted into the kernel as-is? That is up to Benjamin. Regards, Hans
On Thu, Mar 28, 2019 at 11:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 28-03-19 23:18, Alex Henrie wrote: > > On Thu, Mar 28, 2019 at 3:20 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Descriptor fixups obviously need to be done in the kernel, but I > >> believe that key-remapping which can be done through hwdb is best done > >> there as it is a lot easier to update 60-keyboard.hwdb then to change > >> to running a (lot) newer kernel and more importantly we should strive > >> to keep the kernel code as small as possible. > >> > >> Now about this specific keyboards, a quick duckduckgo found me this > >> picture: > >> > >> https://www.bhphotovideo.com/images/images1500x1500/macally_mkeyx_104_key_full_sized_1274821.jpg > > > > The picture you found is of the Macally MKEYX, which I do not have. > > This is the Macally ikey: > > > > https://cultek2.com/products/macally-ikey-wired-usb-keyboard-for-macintosh-or-pc-preowned?variant=12129379156040 > > > > And this is the Macally QKEY: > > > > https://images-na.ssl-images-amazon.com/images/I/61c7zcQJMzL._SL1500_.jpg > > > > The brightness-down and brightness-up keys are Fn+F1 and Fn+F2. The > > keys work as expected on a Mac, so I'm guessing that a long time ago > > Apple decided to repurpose the scancodes for Scroll Lock and Pause, > > leading to the headache we have now. > > Ok. > > > But as you and Benjamin pointed > > out, systemd is a better place to fix this. > > Agreed. > > > So, how about committing the first patch and forgetting about the > > second one? Does the ikey driver need any changes or can it be > > accepted into the kernel as-is? > > That is up to Benjamin. OK for merging the first patch only. I would need a small nitpick then, given that patch 2/2 is dropped. I'll answer in its patch. Cheers, Benjamin
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index aef4a2a690e1..082900477df5 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -239,6 +239,7 @@ config HID_MACALLY supported devices: - Macally ikey keyboard + - Macally QKEY keyboard config HID_PRODIKEYS tristate "Prodikeys PC-MIDI Keyboard support" diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index aacc7534b076..5afc3b7fe8ca 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -776,6 +776,9 @@ #define USB_DEVICE_ID_CRYSTALTOUCH 0x0006 #define USB_DEVICE_ID_CRYSTALTOUCH_DUAL 0x0007 +#define USB_VENDOR_ID_MACALLY 0x2222 +#define USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD 0x0039 + #define USB_VENDOR_ID_MADCATZ 0x0738 #define USB_DEVICE_ID_MADCATZ_BEATPAD 0x4540 #define USB_DEVICE_ID_MADCATZ_RAT5 0x1705 diff --git a/drivers/hid/hid-macally.c b/drivers/hid/hid-macally.c index 6f62f059b795..2567babe8200 100644 --- a/drivers/hid/hid-macally.c +++ b/drivers/hid/hid-macally.c @@ -31,9 +31,64 @@ static __u8 *macally_report_fixup(struct hid_device *hdev, __u8 *rdesc, return rdesc; } +struct macally_key_translation +{ + u16 from; + u16 to; +}; + +static const struct macally_key_translation qkey_brightness_keys[] = +{ + { KEY_SCROLLLOCK, KEY_BRIGHTNESSDOWN }, + { KEY_PAUSE, KEY_BRIGHTNESSUP }, + { } +}; + +static int macally_event(struct hid_device *hdev, struct hid_field *field, + struct hid_usage *usage, __s32 value) +{ + const struct macally_key_translation *trans; + + switch (hdev->product) { + case USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD: + trans = qkey_brightness_keys; + break; + default: + trans = NULL; + } + + if (trans) { + while (trans->from) { + if (trans->from == usage->code) { + input_event(field->hidinput->input, usage->type, + trans->to, value); + return 1; + } + trans++; + } + } + + return 0; +} + +static int macally_input_mapping(struct hid_device *hdev, struct hid_input *hi, + struct hid_field *field, struct hid_usage *usage, + unsigned long **bit, int *max) +{ + const struct macally_key_translation *trans; + + /* Enable all needed keys */ + for (trans = qkey_brightness_keys; trans->from; trans++) + set_bit(trans->to, hi->input->keybit); + + return 0; +} + static struct hid_device_id macally_id_table[] = { { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, USB_DEVICE_ID_MACALLY_IKEY_KEYBOARD) }, + { HID_USB_DEVICE(USB_VENDOR_ID_MACALLY, + USB_DEVICE_ID_MACALLY_QKEY_KEYBOARD) }, { } }; MODULE_DEVICE_TABLE(hid, macally_id_table); @@ -42,6 +97,8 @@ static struct hid_driver macally_driver = { .name = "macally", .id_table = macally_id_table, .report_fixup = macally_report_fixup, + .event = macally_event, + .input_mapping = macally_input_mapping, }; module_hid_driver(macally_driver);
This enables the brightness-down and brightness-up keys on the Macally QKEY keyboard. Similar workarounds are probably needed for quite a few Macally keyboard models. Based on the key translation code in the Apple keyboard driver. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- drivers/hid/Kconfig | 1 + drivers/hid/hid-ids.h | 3 +++ drivers/hid/hid-macally.c | 57 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+)