On Mon, Jan 14, 2019 at 4:13 PM William Whistler <wtbw@xxxxxxxxxx> wrote: > > I assumed an incorrect tab character count when spacing a line. > > The fix for that is the only change over v2. Thanks for the quick v3. BTW, to not add some extra work to the maintainers, all the discussion that you do not want to get applied on the patch should be put after the first '---' and before the list of changes. > > -- > Best regards, > William Whistler > > > From 9e46bfc1f5b01ef2b08fea3afaf85abde05864bd Mon Sep 17 00:00:00 2001 > From: William Whistler <wtbw@xxxxxxxxxx> > Date: Mon, 14 Jan 2019 15:10:53 +0000 > Subject: [PATCH] Support for Maltron L90 keyboard media keys > I hadn't noticed that there is no commit description to your patch. Please add something here that explains why you need to add this new driver. This will go in the git logs, and should be documented. You are also missing your Signed-off-by (as mentioned by checkpatch.pl). See https://www.kernel.org/doc/html/v4.20/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin > --- Here you can put whatever discussion you want, it will not be in the tree. So things like greetings, changes and other stuffs should be there. > drivers/hid/Kconfig | 7 ++ > drivers/hid/Makefile | 1 + > drivers/hid/hid-ids.h | 1 + > drivers/hid/hid-maltron.c | 166 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 175 insertions(+) > create mode 100644 drivers/hid/hid-maltron.c > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 41e9935fc584..37f82ef27b11 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -590,6 +590,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 should be "---help---" as in the rest of the file > + Adds support for the volume up, volume down, mute, and play/pause buttons > + of the Maltron L90 keyboard. > + > config HID_MAYFLASH > tristate "Mayflash game controller adapter force feedback" > depends on HID > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index 896a51ce7ce0..cf2752003253 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -66,6 +66,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 518fa76414f5..01d565357dbe 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -72,6 +72,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..a5508af7085d > --- /dev/null > +++ b/drivers/hid/hid-maltron.c > @@ -0,0 +1,166 @@ > +// SPDX-License-Identifier: GPL-2.0 regarding: "I've fixed the comments in the body of the file as requested. However, the SPDX tag style guide[1] says to use the // style for .c source files. I have followed that guideline despite the apparent conflict with your second statement here." That's a special case for the SPDX tag that needs to use '//' for the tools to be OK. I know, it feels weird, but that's the case in every file. > +/* > + * 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> > + */ > + > +#include <linux/device.h> > +#include <linux/hid.h> > +#include <linux/module.h> > + > +#include "hid-ids.h" > + > +/* The original buggy USB descriptor */ > +static u8 maltron_rdesc_o[] = { > +0x05, 0x01, /* Usage Page (Generic Desktop Ctrls) */ nitpicking: please add one tab before each element. > +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 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 */ this should probably stay like in the original: 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) */ Can you mark the 2 following lines like the ones you changed? > +0x14, /* Logical Minimum */ please 0x15, 0x00, /* Logical Minimum (0) */ > +0x26, 0xFF, 0x7F, /* Logical Maximum (32767) */ > +0x95, 0x01, /* Report Count (1) */ > +0x75, 0x10, /* Report Size (16) */ > +0x18, /* Usage Minimum */ please 0x19, 0x00, /* Usage Minimum (Unassigned) */ > +0x2A, 0xFF, 0x7F, /* Usage Maximum (0x7FFF) */ > +0x80, /* Input */ please 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) */ > +0x18, /* Usage Minimum */ please 0x19, 0x00, /* Usage Minimum (0x00) */ > +0x2A, 0xFF, 0x7F, /* Usage Maximum (0x7FFF) */ > +0x80, /* Input */ and please 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 */ > +}; > + > +static __u8 *maltron_report_fixup(struct hid_device *hdev, __u8 *rdesc, > + unsigned int *rsize) tabs are 8 characters in the kernel. So your second line is too far away :) > +{ > + if (*rsize == sizeof(maltron_rdesc_o)) { > + if (memcmp(maltron_rdesc_o, rdesc, sizeof(maltron_rdesc_o)) == 0) { you could save one tab by: if (*rsize == sizeof(maltron_rdesc_o) && !memcmp(maltron_rdesc_o, rdesc, sizeof(maltron_rdesc_o))) { ... This should also remove the 80 chars complains of checkpatch. Cheers, Benjamin > + 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"); > -- > 2.20.1 >
