Hi,
On Wed, Apr 3, 2019 at 9:06 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
> On 03-04-19 05:18, Alex Henrie wrote:
> > This enables the power and equals keys on the Macally ikey keyboard.
> >
> > Based on the Cougar gaming keyboard HID driver, which uses the same
> > vendor ID.
> >
> > Signed-off-by: Alex Henrie <alexhenrie24@xxxxxxxxx>
>
> The driver looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Thanks, I should be all set to take this for 5.2 then. I'll let you
know when it is pushed upstream.
>
> I do have something to discuss about this with the HID subsys maintainers,
> see comment inline.
>
/me ducks away....
> > ---
> > drivers/hid/Kconfig | 10 +++++++++
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 1 +
> > drivers/hid/hid-macally.c | 45 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 57 insertions(+)
> > create mode 100644 drivers/hid/hid-macally.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 6ca8d322b487..aef4a2a690e1 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -230,6 +230,16 @@ config HID_COUGAR
> > Supported devices:
> > - Cougar 500k Gaming Keyboard
> >
> > +config HID_MACALLY
> > + tristate "Macally devices"
> > + depends on HID
> > + help
> > + Support for Macally devices that are not fully compliant with the
> > + HID standard.
> > +
> > + supported devices:
> > + - Macally ikey keyboard
> > +
> > config HID_PRODIKEYS
> > tristate "Prodikeys PC-MIDI Keyboard support"
> > depends on HID && SND
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 170163b41303..44b9caea46a7 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -65,6 +65,7 @@ obj-$(CONFIG_HID_LENOVO) += hid-lenovo.o
> > obj-$(CONFIG_HID_LOGITECH) += hid-logitech.o
> > obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-dj.o
> > obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
> > +obj-$(CONFIG_HID_MACALLY) += hid-macally.o
> > obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> > obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
> > obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index b6d93f4ad037..aacc7534b076 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -1034,6 +1034,7 @@
> > #define USB_DEVICE_ID_SINO_LITE_CONTROLLER 0x3008
> >
> > #define USB_VENDOR_ID_SOLID_YEAR 0x060b
> > +#define USB_DEVICE_ID_MACALLY_IKEY_KEYBOARD 0x0001
> > #define USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD 0x500a
> > #define USB_DEVICE_ID_COUGAR_700K_GAMING_KEYBOARD 0x700a
> >
> > diff --git a/drivers/hid/hid-macally.c b/drivers/hid/hid-macally.c
> > new file mode 100644
> > index 000000000000..9a4fc7dffb14
> > --- /dev/null
> > +++ b/drivers/hid/hid-macally.c
> > @@ -0,0 +1,45 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * HID driver for quirky Macally devices
> > + *
> > + * Copyright (c) 2019 Alex Henrie <alexhenrie24@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +
> > +#include "hid-ids.h"
> > +
> > +MODULE_AUTHOR("Alex Henrie <alexhenrie24@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Macally devices");
> > +MODULE_LICENSE("GPL");
> > +
> > +/*
> > + * The Macally ikey keyboard says that its logical and usage maximums are both
> > + * 101, but the power key is 102 and the equals key is 103
> > + */
>
> I was wondering if this is something which more keyboards suffer from, a quick
> grep finds a couple which do exactly the same thing (but with a different new
> maximum value:
>
> hid-apple.c:
> static __u8 *apple_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> struct apple_sc *asc = hid_get_drvdata(hdev);
>
> if ((asc->quirks & APPLE_RDESC_JIS) && *rsize >= 60 &&
> rdesc[53] == 0x65 && rdesc[59] == 0x65) {
> hid_info(hdev,
> "fixing up MacBook JIS keyboard report descriptor\n");
> rdesc[53] = rdesc[59] = 0xe7;
> }
> return rdesc;
> }
>
> hid-nti.c:
> static __u8 *nti_usbsun_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> if (*rsize >= 60 && rdesc[53] == 0x65 && rdesc[59] == 0x65) {
> hid_info(hdev, "fixing up NTI USB-SUN keyboard adapter report de
> rdesc[53] = rdesc[59] = 0xe7;
> }
> return rdesc;
> }
>
> And a few wich are close, but slightly different:
>
> hid-asus.c:
> static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>
> if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT &&
> *rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65)
> hid_info(hdev, "Fixing up Asus notebook report descriptor\n");
> rdesc[55] = 0xdd;
> }
> ... (other quirks for other devices)
> }
>
> hid-aureal.c:
> static __u8 *aureal_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> if (*rsize >= 54 && rdesc[52] == 0x25 && rdesc[53] == 0x01) {
> dev_info(&hdev->dev, "fixing Aureal Cy se W-01RN USB_V3.1 report
> rdesc[53] = 0x65;
> }
> return rdesc;
> }
>
> hid-ortek.c:
> static __u8 *ortek_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> if (*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x01) {
> hid_info(hdev, "Fixing up logical maximum in report descriptor (
> rdesc[55] = 0x92;
> } else if (*rsize >= 54 && rdesc[52] == 0x25 && rdesc[53] == 0x01) {
> hid_info(hdev, "Fixing up logical maximum in report descriptor (
> rdesc[53] = 0x65;
> }
> return rdesc;
> }
>
> So I'm wondering if we cannot come up with some generic helper for this,
> which drivers could then call from their report-fixup; or, even better
> maybe have a HID_QUIRK_FIX_KEYBOARD_MAXIMUM which causes the generic code
> to bump the maximum to 0xe7 (*), then the hid-nti, hid-macally and hid-ortek
> drivers could be dropped and replaced with a single line entry in hid-quirks.c
I am not a big fan of the idea. Having a specific quirk is not going
to help much IMO because it will be quite hard to grasp what it does.
Right now adding a new module doesn't cost much, especially since
hid-generic is nice enough to unbind itself.
With Jiri, we talked (a lot) about having those report descriptors
overrides provided by userspace. I know I already mentioned this to
you a while ago (and more recently IIRC), but one idea could be to
have the override as a firmware file that would get loaded by udev
through request_firmware() (if I remember correctly the API).
But I am also wondering if we could not use eBPF to actually fix those
HID devices programmatically, which mean we could externalize those
fixes without actually needing the full report descriptor.
Anyway, until we have a generic and good enough solution, I'd rather
we keep accepting those report fixups in the kernel.
Cheers,
Benjamin
>
> Regards,
>
> Hans
>
> *) Or probably 0xdf, 0xe0 - 0xe7 are the ctrl/shift/alt/etc. modifiers.
>
>
>
>
>
> > +static __u8 *macally_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > + unsigned int *rsize)
> > +{
> > + if (*rsize >= 60 && rdesc[53] == 0x65 && rdesc[59] == 0x65) {
> > + hid_info(hdev,
> > + "fixing up Macally ikey keyboard report descriptor\n");
> > + rdesc[53] = rdesc[59] = 0x67;
> > + }
> > + return rdesc;
> > +}
> > +
> > +static struct hid_device_id macally_id_table[] = {
> > + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR,
> > + USB_DEVICE_ID_MACALLY_IKEY_KEYBOARD) },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(hid, macally_id_table);
> > +
> > +static struct hid_driver macally_driver = {
> > + .name = "macally",
> > + .id_table = macally_id_table,
> > + .report_fixup = macally_report_fixup,
> > +};
> > +
> > +module_hid_driver(macally_driver);
> >
