Hi William,
On Sun, Jan 13, 2019 at 7:41 PM William Whistler <wtbw@xxxxxxxxxx> wrote:
>
> Hello,
>
> I sent this patch back in September and haven't heard anything back.
Sorry for not coming back earlier to you. Actually, you haven't Cc-ed
linux-input@ and the email doesn't show up in my patch queue. Needless
to day that it got buried by hundreds of other email in my INBOX.
>
> This is my first attempt at submitting a kernel patch, so if I've done
> something incorrectly (procedurally or otherwise) please let me know!
Except for forgetting to CC the list, this seems OK from a rough point
of view. Even pinging the maintainer is a good thing to do, as we can
easily overlook patches.
>
> --
> Best regards,
> William Whistler
>
>
> From 9f6cf93efecac336e872c9bb52dfd73c501a6786 Mon Sep 17 00:00:00 2001
> From: William Whistler <wtbw@xxxxxxxxxx>
> Date: Tue, 18 Sep 2018 11:09:33 +0100
> Subject: [PATCH] Support for Maltron L90 keyboard
>
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 61e1953ff921..dcdce1273d6e 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -577,6 +577,13 @@ config HID_MAGICMOUSE
> Say Y here if you want support for the multi-touch features of the
> Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
>
> +config HID_MALTRON
> + tristate "Maltron L90 keyboard"
> + depends on HID
> + ---help---
> + Support for the volume up/down, mute, and play/pause buttons.
I guess the driver also supports the other keys on the keyboard. Maybe
you should tell that this driver adds those various keys (well, there
are a lot of example in this file, I am sure you can make something
correct).
> +
> +
> config HID_MAYFLASH
> tristate "Mayflash game controller adapter force feedback"
> depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index bd7ac53b75c5..ec82a913d22d 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -65,6 +65,7 @@ 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_MAGICMOUSE) += hid-magicmouse.o
> +obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
> obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
> obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 5146ee029db4..07d701f6d388 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -69,6 +69,7 @@
>
> #define USB_VENDOR_ID_ALCOR 0x058f
> #define USB_DEVICE_ID_ALCOR_USBRS232 0x9720
> +#define USB_DEVICE_ID_ALCOR_MALTRON_KB 0x9410
>
> #define USB_VENDOR_ID_ALPS 0x0433
> #define USB_DEVICE_ID_IBM_GAMEPAD 0x1101
> diff --git a/drivers/hid/hid-maltron.c b/drivers/hid/hid-maltron.c
> new file mode 100644
> index 000000000000..e10e68fdb412
> --- /dev/null
> +++ b/drivers/hid/hid-maltron.c
> @@ -0,0 +1,171 @@
> +/*
> + * HID driver for Maltron L90
> + *
> + * Copyright (c) 1999 Andreas Gal
> + * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@xxxxxxx>
> + * Copyright (c) 2005 Michael Haboustak <mike-@xxxxxxxxxxxx> for Concept2, Inc
> + * Copyright (c) 2008 Jiri Slaby
> + * Copyright (c) 2012 David Dillow <dave@xxxxxxxxxxxxxx>
> + * Copyright (c) 2006-2013 Jiri Kosina
> + * Copyright (c) 2013 Colin Leitner <colin.leitner@xxxxxxxxx>
> + * Copyright (c) 2014-2016 Frank Praznik <frank.praznik@xxxxxxxxx>
> + * Copyright (c) 2010 Richard Nauber <Richard.Nauber@xxxxxxxxx>
> + * Copyright (c) 2016 Yuxuan Shui <yshuiv7@xxxxxxxxx>
> + * Copyright (c) 2018 William Whistler <wtbw@xxxxxxxxxx>
That's a lot of copyright for a small driver that just does a report fixup.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
You should use the SPDX tag now, and drop the license text.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +
> +#include "hid-ids.h"
> +
> +//The original buggy USB descriptor
No C++ comments. Please use /* */ even for one liners (so in the
report descriptors too).
And there is a script that checks for these kind of things, you should
really run it before hitting send:
./scripts/checkpatch.pl
Hint, you can run the script on your git tip by running:
`./scripts/checkpatch.pl -g HEAD`.
Currently, you have with this patch:
total: 200 errors, 5 warnings, 222 lines checked
That's a tad too much :)
There are rules we can break (the 80 chars one is one of those, but
trailing whitespaces or DOS line ending are not accepted).
> +static u8 maltron_rdesc_o[] = {
> +0x05, 0x01, // Usage Page (Generic Desktop Ctrls)
> +0x09, 0x80, // Usage (Sys Control)
> +0xA1, 0x01, // Collection (Application)
> +0x85, 0x02, // Report ID (2)
> +0x75, 0x01, // Report Size (1)
> +0x95, 0x01, // Report Count (1)
> +0x15, 0x00, // Logical Minimum (0)
> +0x25, 0x01, // Logical Maximum (1)
> +0x09, 0x82, // Usage (Sys Sleep)
> +0x81, 0x06, // Input (Data,Var,Rel)
> +0x09, 0x82, // Usage (Sys Sleep)
> +0x81, 0x06, // Input (Data,Var,Rel)
> +0x09, 0x83, // Usage (Sys Wake Up)
> +0x81, 0x06, // Input (Data,Var,Rel)
> +0x75, 0x05, // Report Size (5)
> +0x81, 0x01, // Input (Const,Array,Abs)
> +0xC0, // End Collection
> +0x05, 0x0C, // Usage Page (Consumer)
> +0x09, 0x01, // Usage (Consumer Control)
> +0xA1, 0x01, // Collection (Application)
> +0x85, 0x03, // Report ID (3)
> +0x95, 0x01, // Report Count (1)
> +0x75, 0x10, // Report Size (16)
> +0x19, 0x00, // Usage Minimum (Unassigned)
> +0x2A, 0xFF, 0x7F, // Usage Maximum (0x7FFF)
> +0x81, 0x00, // Input (Data,Array,Abs)
> +0xC0, // End Collection
> +0x06, 0x7F, 0xFF, // Usage Page (Vendor Defined 0xFF7F)
> +0x09, 0x01, // Usage (0x01)
> +0xA1, 0x01, // Collection (Application)
> +0x85, 0x04, // Report ID (4)
> +0x95, 0x01, // Report Count (1)
> +0x75, 0x10, // Report Size (16)
> +0x19, 0x00, // Usage Minimum (0x00)
> +0x2A, 0xFF, 0x7F, // Usage Maximum (0x7FFF)
> +0x81, 0x00, // Input (Data,Array,Abs)
> +0x75, 0x02, // Report Size (2)
> +0x25, 0x02, // Logical Maximum (2)
> +0x09, 0x90, // Usage (0x90)
> +0xB1, 0x02, // Feature (Data,Var,Abs)
> +0x75, 0x06, // Report Size (6)
> +0xB1, 0x01, // Feature (Const,Array,Abs)
> +0x75, 0x01, // Report Size (1)
> +0x25, 0x01, // Logical Maximum (1)
> +0x05, 0x08, // Usage Page (LEDs)
> +0x09, 0x2A, // Usage (On-Line)
> +0x91, 0x02, // Output (Data,Var,Abs)
> +0x09, 0x4B, // Usage (Generic Indicator)
> +0x91, 0x02, // Output (Data,Var,Abs)
> +0x75, 0x06, // Report Size (6)
> +0x95, 0x01, // Report Count (1)
> +0x91, 0x01, // Output (Const,Array,Abs)
> +0xC0 // End Collection
> +};
> +
> +
> +//The patched USB descriptor, allowing media key events to be accepted as valid
> +static u8 maltron_rdesc[] = {
> +0x05, 0x01, // Usage Page (Generic Desktop Ctrls)
> +0x09, 0x80, // Usage (Sys Control)
> +0xA1, 0x01, // Collection (Application)
> +0x85, 0x02, // Report ID (2)
> +0x75, 0x01, // Report Size (1)
> +0x95, 0x01, // Report Count (1)
> +0x14, // Logical Minimum
> +0x25, 0x01, // Logical Maximum (1)
> +0x09, 0x82, // Usage (Sys Sleep)
> +0x81, 0x06, // Input (Data,Var,Rel)
> +0x09, 0x82, // Usage (Sys Sleep)
> +0x81, 0x06, // Input (Data,Var,Rel)
> +0x09, 0x83, // Usage (Sys Wake Up)
> +0x81, 0x06, // Input (Data,Var,Rel)
> +0x75, 0x05, // Report Size (5)
> +0x81, 0x01, // Input (Const,Array,Abs)
> +0xC0, // End Collection
> +0x05, 0x0C, // Usage Page (Consumer)
> +0x09, 0x01, // Usage (Consumer Control)
> +0xA1, 0x01, // Collection (Application)
> +0x85, 0x03, // Report ID (3)
> +0x14, // Logical Minimum
> +0x26, 0xFF, 0x7F, // Logical Maximum (32767)
> +0x95, 0x01, // Report Count (1)
> +0x75, 0x10, // Report Size (16)
> +0x18, // Usage Minimum
> +0x2A, 0xFF, 0x7F, // Usage Maximum (0x7FFF)
> +0x80, // Input
> +0xC0, // End Collection
> +0x06, 0x7F, 0xFF, // Usage Page (Vendor Defined 0xFF7F)
> +0x09, 0x01, // Usage (0x01)
> +0xA1, 0x01, // Collection (Application)
> +0x85, 0x04, // Report ID (4)
> +0x95, 0x01, // Report Count (1)
> +0x75, 0x10, // Report Size (16)
> +0x18, // Usage Minimum
> +0x2A, 0xFF, 0x7F, // Usage Maximum (0x7FFF)
> +0x80, // Input
> +0x75, 0x02, // Report Size (2)
> +0x25, 0x02, // Logical Maximum (2)
> +0x09, 0x90, // Usage (0x90)
> +0xB1, 0x02, // Feature (Data,Var,Abs)
> +0x75, 0x06, // Report Size (6)
> +0xB1, 0x01, // Feature (Const,Array,Abs)
> +0x75, 0x01, // Report Size (1)
> +0x25, 0x01, // Logical Maximum (1)
> +0x05, 0x08, // Usage Page (LEDs)
> +0x09, 0x2A, // Usage (On-Line)
> +0x91, 0x02, // Output (Data,Var,Abs)
> +0x09, 0x4B, // Usage (Generic Indicator)
> +0x91, 0x02, // Output (Data,Var,Abs)
> +0x75, 0x06, // Report Size (6)
> +0x95, 0x01, // Report Count (1)
> +0x91, 0x01, // Output (Const,Array,Abs)
> +0xC0 // End Collection
> +};
> +
> +static __u8 *maltron_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int *rsize)
Personnal taste: please align the second line under 'struct hid_device'.
IIRC the kernel guidelines says to use 2 tabs, but I find it better to
align the parameters.
> +{
> + if (*rsize == sizeof(maltron_rdesc_o)) {
> + if (memcmp(maltron_rdesc_o, rdesc, sizeof(maltron_rdesc_o)) == 0) {
> + hid_info(hdev, "Replacing Maltron L90 Keyboard report descriptor\n");
> + *rsize = sizeof(maltron_rdesc);
> + return maltron_rdesc;
> + }
> + }
> + return rdesc;
> +}
> +
> +static const struct hid_device_id maltron_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_ALCOR, USB_DEVICE_ID_ALCOR_MALTRON_KB)},
> + { }
> +};
> +MODULE_DEVICE_TABLE(hid, maltron_devices);
> +
> +static struct hid_driver maltron_driver = {
> + .name = "maltron",
> + .id_table = maltron_devices,
> + .report_fixup = maltron_report_fixup
> +};
> +module_hid_driver(maltron_driver);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 249d49b6b16c..c721e58dfbda 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -479,6 +479,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_GAMECUBE1) },
> { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_GAMECUBE2) },
> #endif
> +#if IS_ENABLED(CONFIG_HID_MALTRON)
> + { HID_USB_DEVICE(USB_VENDOR_ID_ALCOR, USB_DEVICE_ID_ALCOR_MALTRON_KB)},
> +#endif
Please drop this hunk. It shouldn't be required since v4.17 (or v4.16).
This is even more required that your driver is a keyboard one, and if
a distribution compiles it but doesn't include it in the initrd, then
your users can't type their LUKS password :/
Thanks for the submission. This is a small driver that shouldn't take
too many revisions to apply.
Cheers,
Benjamin
> #if IS_ENABLED(CONFIG_HID_MICROSOFT)
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_KEYBOARD) },
>
>
> --
> Best regards,
> William Whistler
