[PATCH v3 0/6] hid: Introduce device groups

[PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

Hi Jiri,

Here is the third version of the extension to the device-driver
matching mechanism. AFAICT, there are no outstanding issues. I think
we are getting close. :-)

Building on the report fixup patches, this patchset brings a bit of
transparency to the hid bus, by the addition of device groups.  This
allows a more versatile handling of hid drivers in userland, and
simplifies the logic in the kernel.

In particular, there can be one generic module per device group, and
those modules are handled automatically by udev. Dynamic rebinding of
drivers is fully supported. For instance, to load a special
out-of-tree driver instead of the a generic one, simply unbind the
device and load the new module. One can also keep _all_ generic
drivers as modules, significantly simplifying the process of adding
and testing new features.

And, of course, auto-loading of new multitouch drivers works, since
that was what triggered this patchset to begin with. :-)

The second patch contains a simple descriptor scanner that is
new. The last patch unifies the generic drivers into a single,
loadable module.

Enjoy,
Henrik

Henrik Rydberg (6):
  hid: Add device group to modalias
  hid: Scan the device for group info before adding it
  hid: Allow bus wildcard matching
  hid: Create a generic device group
  hid-multitouch: Switch to device groups
  hid: Create a common generic driver

 drivers/hid/Kconfig             |   12 +++
 drivers/hid/Makefile            |    2 +
 drivers/hid/hid-core.c          |  172 ++++++++++++++++++++-------------------
 drivers/hid/hid-generic.c       |   53 ++++++++++++
 drivers/hid/hid-input.c         |   11 ---
 drivers/hid/hid-multitouch.c    |    3 +-
 drivers/hid/usbhid/hid-core.c   |   16 ----
 include/linux/hid.h             |   20 +++--
 include/linux/mod_devicetable.h |    4 +-
 net/bluetooth/hidp/core.c       |   27 +-----
 scripts/mod/file2alias.c        |    5 +-
 11 files changed, 180 insertions(+), 145 deletions(-)
 create mode 100644 drivers/hid/hid-generic.c

-- 
1.7.10

[PATCH v2 1/6] hid: Add device group to modalias

[Henrik Rydberg]

HID devices are only partially presented to userland.  Hotplugged
devices emit events containing a modalias based on the basic bus,
vendor and product entities. However, in practise a hid device can
depend on details such as a single usb interface or a particular item
in a report descriptor.

This patch adds a device group to the hid device id, and broadcasts it
using uevent and the device modalias.  The module alias generation is
modified to match. As a consequence, a device with a non-zero group
will be processed by the corresponding group driver instead of by the
generic hid driver.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-core.c          |   24 ++++++++++++++++++++++--
 include/linux/hid.h             |    1 +
 include/linux/mod_devicetable.h |    3 ++-
 scripts/mod/file2alias.c        |    2 ++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9ed4ff3..cfcb69e 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1222,6 +1222,7 @@ static bool hid_match_one_id(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
 	return id->bus == hdev->bus &&
+		(id->group == HID_GROUP_ANY || id->group == hdev->group) &&
 		(id->vendor == HID_ANY_ID || id->vendor == hdev->vendor) &&
 		(id->product == HID_ANY_ID || id->product == hdev->product);
 }
@@ -1694,6 +1695,7 @@ static ssize_t store_new_id(struct device_driver *drv, const char *buf,
 		return -ENOMEM;
 
 	dynid->id.bus = bus;
+	dynid->id.group = HID_GROUP_ANY;
 	dynid->id.vendor = vendor;
 	dynid->id.product = product;
 	dynid->id.driver_data = driver_data;
@@ -1817,6 +1819,23 @@ static int hid_device_remove(struct device *dev)
 	return 0;
 }
 
+static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
+			     char *buf)
+{
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	int len;
+
+	len = snprintf(buf, PAGE_SIZE, "hid:b%04Xg%04Xv%08Xp%08X\n",
+		       hdev->bus, hdev->group, hdev->vendor, hdev->product);
+
+	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
+}
+
+static struct device_attribute hid_dev_attrs[] = {
+	__ATTR_RO(modalias),
+	__ATTR_NULL,
+};
+
 static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
@@ -1834,8 +1853,8 @@ static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (add_uevent_var(env, "HID_UNIQ=%s", hdev->uniq))
 		return -ENOMEM;
 
-	if (add_uevent_var(env, "MODALIAS=hid:b%04Xv%08Xp%08X",
-			hdev->bus, hdev->vendor, hdev->product))
+	if (add_uevent_var(env, "MODALIAS=hid:b%04Xg%04Xv%08Xp%08X",
+			   hdev->bus, hdev->group, hdev->vendor, hdev->product))
 		return -ENOMEM;
 
 	return 0;
@@ -1843,6 +1862,7 @@ static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static struct bus_type hid_bus_type = {
 	.name		= "hid",
+	.dev_attrs	= hid_dev_attrs,
 	.match		= hid_bus_match,
 	.probe		= hid_device_probe,
 	.remove		= hid_device_remove,
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d8e7cc7..a0e27dd 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -476,6 +476,7 @@ struct hid_device {							/* device report descriptor */
 	unsigned maxcollection;						/* Number of parsed collections */
 	unsigned maxapplication;					/* Number of applications */
 	__u16 bus;							/* BUS ID */
+	__u16 group;							/* Report group */
 	__u32 vendor;							/* Vendor ID */
 	__u32 product;							/* Product ID */
 	__u32 version;							/* HID version */
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 501da4c..55ed0b0 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -132,10 +132,11 @@ struct usb_device_id {
 #define USB_DEVICE_ID_MATCH_INT_PROTOCOL	0x0200
 
 #define HID_ANY_ID				(~0)
+#define HID_GROUP_ANY				0x0000
 
 struct hid_device_id {
 	__u16 bus;
-	__u16 pad1;
+	__u16 group;
 	__u32 vendor;
 	__u32 product;
 	kernel_ulong_t driver_data
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 8e730cc..fe967ce 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -336,10 +336,12 @@ static int do_hid_entry(const char *filename,
 			     struct hid_device_id *id, char *alias)
 {
 	id->bus = TO_NATIVE(id->bus);
+	id->group = TO_NATIVE(id->group);
 	id->vendor = TO_NATIVE(id->vendor);
 	id->product = TO_NATIVE(id->product);
 
 	sprintf(alias, "hid:b%04X", id->bus);
+	ADD(alias, "g", id->group != HID_GROUP_ANY, id->group);
 	ADD(alias, "v", id->vendor != HID_ANY_ID, id->vendor);
 	ADD(alias, "p", id->product != HID_ANY_ID, id->product);
 
-- 
1.7.10

[PATCH 2/6] hid: Scan the device for group info before adding it

[Henrik Rydberg]

In order to allow the report descriptor to influence the hid device
properties, one needs to parse the descriptor early, without reference
to any driver. Scan the descriptor for group information during device
add, before the device has been broadcast to userland. The device
modalias will contain group information which can be used to
differentiate between modules. For starters, just handle the generic
group.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-core.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h    |    5 ++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index cfcb69e..f932cd5 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -658,6 +658,58 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 	return NULL;
 }
 
+static void hid_scan_usage(struct hid_device *hid, u32 usage)
+{
+}
+
+/*
+ * Scan a report descriptor before the device is added to the bus.
+ * Sets device groups and other properties that determine what driver
+ * to load.
+ */
+static int hid_scan_report(struct hid_device *hid)
+{
+	unsigned int page = 0, delim = 0;
+	__u8 *start = hid->dev_rdesc;
+	__u8 *end = start + hid->dev_rsize;
+	unsigned int u, u_min = 0, u_max = 0;
+	struct hid_item item;
+
+	hid->group = HID_GROUP_GENERIC;
+	while ((start = fetch_item(start, end, &item)) != NULL) {
+		if (item.format != HID_ITEM_FORMAT_SHORT)
+			return -EINVAL;
+		if (item.type == HID_ITEM_TYPE_GLOBAL) {
+			if (item.tag == HID_GLOBAL_ITEM_TAG_USAGE_PAGE)
+				page = item_udata(&item) << 16;
+		} else if (item.type == HID_ITEM_TYPE_LOCAL) {
+			if (delim > 1)
+				break;
+			u = item_udata(&item);
+			if (item.size <= 2)
+				u += page;
+			switch (item.tag) {
+			case HID_LOCAL_ITEM_TAG_DELIMITER:
+				delim += !!u;
+				break;
+			case HID_LOCAL_ITEM_TAG_USAGE:
+				hid_scan_usage(hid, u);
+				break;
+			case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
+				u_min = u;
+				break;
+			case HID_LOCAL_ITEM_TAG_USAGE_MAXIMUM:
+				u_max = u;
+				for (u = u_min; u <= u_max; u++)
+					hid_scan_usage(hid, u);
+				break;
+			}
+		}
+	}
+
+	return 0;
+}
+
 /**
  * hid_parse_report - parse device report
  *
@@ -2171,6 +2223,16 @@ int hid_add_device(struct hid_device *hdev)
 	if (!hdev->dev_rdesc)
 		return -ENODEV;
 
+	/*
+	 * Scan generic devices for group information
+	 */
+	if (hid_ignore_special_drivers ||
+	    !hid_match_id(hdev, hid_have_special_driver)) {
+		ret = hid_scan_report(hdev);
+		if (ret)
+			hid_warn(hdev, "bad device descriptor (%d)\n", ret);
+	}
+
 	/* XXX hack, any other cleaner solution after the driver core
 	 * is converted to allow more than 20 bytes as the device name? */
 	dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
diff --git a/include/linux/hid.h b/include/linux/hid.h
index a0e27dd..f45d33d 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -325,6 +325,11 @@ struct hid_item {
 #define HID_QUIRK_NO_INPUT_SYNC			0x80000000
 
 /*
+ * HID device groups
+ */
+#define HID_GROUP_GENERIC			0x0001
+
+/*
  * This is the global environment of the parser. This information is
  * persistent for main-items. The global environment can be saved and
  * restored with PUSH/POP statements.
-- 
1.7.10

Re: [PATCH 2/6] hid: Scan the device for group info before adding it

[Jiri Kosina]

On Mon, 23 Apr 2012, Henrik Rydberg wrote:

> In order to allow the report descriptor to influence the hid device
> properties, one needs to parse the descriptor early, without reference
> to any driver. Scan the descriptor for group information during device
> add, before the device has been broadcast to userland. The device
> modalias will contain group information which can be used to
> differentiate between modules. For starters, just handle the generic
> group.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hid/hid-core.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/hid.h    |    5 ++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cfcb69e..f932cd5 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -658,6 +658,58 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
>  	return NULL;
>  }
>  
> +static void hid_scan_usage(struct hid_device *hid, u32 usage)
> +{
> +}
> +
> +/*
> + * Scan a report descriptor before the device is added to the bus.
> + * Sets device groups and other properties that determine what driver
> + * to load.
> + */
> +static int hid_scan_report(struct hid_device *hid)
> +{
> +	unsigned int page = 0, delim = 0;
> +	__u8 *start = hid->dev_rdesc;
> +	__u8 *end = start + hid->dev_rsize;

Where do these dev_rdesc and dev_rsize come from? Your patchset doesn't 
seem to be changing the corresponding struct device members (which are 
rdesc and rsize currently), right?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/6] hid: Scan the device for group info before adding it

[Henrik Rydberg]

> > +static int hid_scan_report(struct hid_device *hid)
> > +{
> > +	unsigned int page = 0, delim = 0;
> > +	__u8 *start = hid->dev_rdesc;
> > +	__u8 *end = start + hid->dev_rsize;
> 
> Where do these dev_rdesc and dev_rsize come from? Your patchset doesn't 
> seem to be changing the corresponding struct device members (which are 
> rdesc and rsize currently), right?

Those members are added in the enable-report-fixup-on-rebind patchset,
which this patchset builds on. I only sent that patchset once, to
avoid confusion. Maybe I created some instead. :-)

And yes, the scanning algorithms do not touch the rdesc/rsize
variables, nor the collection state. Those are handled upon attachment
of the driver.

Thanks,
Henrik

[PATCH v2 3/6] hid: Allow bus wildcard matching

[Henrik Rydberg]

Most HID drivers do not need to know what bus driver is in use.
A generic group driver can drive any hid device, and the device
list should not need to be duplicated for each new bus.

This patch adds wildcard matching to the HID bus, simplifying device
list handling for group drivers.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-core.c          |    2 +-
 include/linux/mod_devicetable.h |    1 +
 scripts/mod/file2alias.c        |    3 ++-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f932cd5..20daee5 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1273,7 +1273,7 @@ EXPORT_SYMBOL_GPL(hid_input_report);
 static bool hid_match_one_id(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
-	return id->bus == hdev->bus &&
+	return (id->bus == HID_BUS_ANY || id->bus == hdev->bus) &&
 		(id->group == HID_GROUP_ANY || id->group == hdev->group) &&
 		(id->vendor == HID_ANY_ID || id->vendor == hdev->vendor) &&
 		(id->product == HID_ANY_ID || id->product == hdev->product);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 55ed0b0..5db9382 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -132,6 +132,7 @@ struct usb_device_id {
 #define USB_DEVICE_ID_MATCH_INT_PROTOCOL	0x0200
 
 #define HID_ANY_ID				(~0)
+#define HID_BUS_ANY				0xffff
 #define HID_GROUP_ANY				0x0000
 
 struct hid_device_id {
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index fe967ce..056de31 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -340,7 +340,8 @@ static int do_hid_entry(const char *filename,
 	id->vendor = TO_NATIVE(id->vendor);
 	id->product = TO_NATIVE(id->product);
 
-	sprintf(alias, "hid:b%04X", id->bus);
+	sprintf(alias, "hid:");
+	ADD(alias, "b", id->bus != HID_BUS_ANY, id->bus);
 	ADD(alias, "g", id->group != HID_GROUP_ANY, id->group);
 	ADD(alias, "v", id->vendor != HID_ANY_ID, id->vendor);
 	ADD(alias, "p", id->product != HID_ANY_ID, id->product);
-- 
1.7.10

[PATCH v3 4/6] hid: Create a generic device group

[Henrik Rydberg]

Devices that do not have a special driver are handled by the generic
driver. This patch does the same thing using device groups; Instead of
forcing a particular driver, the appropriate driver is picked up by
udev. As a consequence, one can now move a device from generic to
specific handling by a simple rebind. By adding a new device id to the
generic driver, the same thing can be done in reverse.

Cc: Gustavo Padovan <gustavo@padovan.org>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-core.c        |    9 +--------
 drivers/hid/usbhid/hid-core.c |    2 +-
 include/linux/hid.h           |   12 ++++++------
 net/bluetooth/hidp/core.c     |    2 +-
 4 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 20daee5..cb5a103 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1800,14 +1800,7 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
 		!strncmp(hdrv->name, "hid-multitouch", 14))
 		return 1;
 
-	if (!hid_match_device(hdev, hdrv))
-		return 0;
-
-	/* generic wants all that don't have specialized driver */
-	if (!strncmp(hdrv->name, "generic-", 8) && !hid_ignore_special_drivers)
-		return !hid_match_id(hdev, hid_have_special_driver);
-
-	return 1;
+	return hid_match_device(hdev, hdrv) != NULL;
 }
 
 static int hid_device_probe(struct device *dev)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5bf91db..9abf718 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1505,7 +1505,7 @@ static struct usb_driver hid_driver = {
 };
 
 static const struct hid_device_id hid_usb_table[] = {
-	{ HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
+	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, HID_ANY_ID, HID_ANY_ID) },
 	{ }
 };
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f45d33d..dd694fb 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -586,12 +586,12 @@ struct hid_descriptor {
 	struct hid_class_descriptor desc[1];
 } __attribute__ ((packed));
 
-#define HID_DEVICE(b, ven, prod) \
-	.bus = (b), \
-	.vendor = (ven), .product = (prod)
-
-#define HID_USB_DEVICE(ven, prod)	HID_DEVICE(BUS_USB, ven, prod)
-#define HID_BLUETOOTH_DEVICE(ven, prod)	HID_DEVICE(BUS_BLUETOOTH, ven, prod)
+#define HID_DEVICE(b, g, ven, prod)					\
+	.bus = (b), .group = (g), .vendor = (ven), .product = (prod)
+#define HID_USB_DEVICE(ven, prod)				\
+	.bus = BUS_USB, .vendor = (ven), .product = (prod)
+#define HID_BLUETOOTH_DEVICE(ven, prod)					\
+	.bus = BUS_BLUETOOTH, .vendor = (ven), .product = (prod)
 
 #define HID_REPORT_ID(rep) \
 	.report_type = (rep)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index d478be1..b6fa982 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1196,7 +1196,7 @@ int hidp_get_conninfo(struct hidp_conninfo *ci)
 }
 
 static const struct hid_device_id hidp_table[] = {
-	{ HID_BLUETOOTH_DEVICE(HID_ANY_ID, HID_ANY_ID) },
+	{ HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_GENERIC, HID_ANY_ID, HID_ANY_ID) },
 	{ }
 };
 
-- 
1.7.10

Re: [PATCH v3 4/6] hid: Create a generic device group

[Gustavo Padovan]

Hi Henrik,

* Henrik Rydberg <rydberg@euromail.se> [2012-04-23 12:07:05 +0200]:

> Devices that do not have a special driver are handled by the generic
> driver. This patch does the same thing using device groups; Instead of
> forcing a particular driver, the appropriate driver is picked up by
> udev. As a consequence, one can now move a device from generic to
> specific handling by a simple rebind. By adding a new device id to the
> generic driver, the same thing can be done in reverse.
> 
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hid/hid-core.c        |    9 +--------
>  drivers/hid/usbhid/hid-core.c |    2 +-
>  include/linux/hid.h           |   12 ++++++------
>  net/bluetooth/hidp/core.c     |    2 +-
>  4 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 20daee5..cb5a103 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1800,14 +1800,7 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
>  		!strncmp(hdrv->name, "hid-multitouch", 14))
>  		return 1;
>  
> -	if (!hid_match_device(hdev, hdrv))
> -		return 0;
> -
> -	/* generic wants all that don't have specialized driver */
> -	if (!strncmp(hdrv->name, "generic-", 8) && !hid_ignore_special_drivers)
> -		return !hid_match_id(hdev, hid_have_special_driver);
> -
> -	return 1;
> +	return hid_match_device(hdev, hdrv) != NULL;
>  }
>  
>  static int hid_device_probe(struct device *dev)
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 5bf91db..9abf718 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1505,7 +1505,7 @@ static struct usb_driver hid_driver = {
>  };
>  
>  static const struct hid_device_id hid_usb_table[] = {
> -	{ HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
> +	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, HID_ANY_ID, HID_ANY_ID) },
>  	{ }
>  };
>  
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index f45d33d..dd694fb 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -586,12 +586,12 @@ struct hid_descriptor {
>  	struct hid_class_descriptor desc[1];
>  } __attribute__ ((packed));
>  
> -#define HID_DEVICE(b, ven, prod) \
> -	.bus = (b), \
> -	.vendor = (ven), .product = (prod)
> -
> -#define HID_USB_DEVICE(ven, prod)	HID_DEVICE(BUS_USB, ven, prod)
> -#define HID_BLUETOOTH_DEVICE(ven, prod)	HID_DEVICE(BUS_BLUETOOTH, ven, prod)
> +#define HID_DEVICE(b, g, ven, prod)					\
> +	.bus = (b), .group = (g), .vendor = (ven), .product = (prod)
> +#define HID_USB_DEVICE(ven, prod)				\
> +	.bus = BUS_USB, .vendor = (ven), .product = (prod)
> +#define HID_BLUETOOTH_DEVICE(ven, prod)					\
> +	.bus = BUS_BLUETOOTH, .vendor = (ven), .product = (prod)

It seems we don't use this macro anywhere else, why did you keep it?

	Gustavo

Re: [PATCH v3 4/6] hid: Create a generic device group

[Henrik Rydberg]

Hi Gustavo,

> > @@ -586,12 +586,12 @@ struct hid_descriptor {
> >  	struct hid_class_descriptor desc[1];
> >  } __attribute__ ((packed));
> >  
> > -#define HID_DEVICE(b, ven, prod) \
> > -	.bus = (b), \
> > -	.vendor = (ven), .product = (prod)
> > -
> > -#define HID_USB_DEVICE(ven, prod)	HID_DEVICE(BUS_USB, ven, prod)
> > -#define HID_BLUETOOTH_DEVICE(ven, prod)	HID_DEVICE(BUS_BLUETOOTH, ven, prod)
> > +#define HID_DEVICE(b, g, ven, prod)					\
> > +	.bus = (b), .group = (g), .vendor = (ven), .product = (prod)
> > +#define HID_USB_DEVICE(ven, prod)				\
> > +	.bus = BUS_USB, .vendor = (ven), .product = (prod)
> > +#define HID_BLUETOOTH_DEVICE(ven, prod)					\
> > +	.bus = BUS_BLUETOOTH, .vendor = (ven), .product = (prod)
> 
> It seems we don't use this macro anywhere else, why did you keep it?

It is actually used extensively in device lists, see for instance in
drivers/hid/hid-core.c.

Henrik

[PATCH v3 5/6] hid-multitouch: Switch to device groups

[Henrik Rydberg]

Switch the driver over to device group handling.  By adding the
HID_GROUP_MULTITOUCH group to hid-core, hid-generic will no longer
match multitouch devices.  By adding the HID_GROUP_MULTITOUCH entry to
the device list, hid-multitouch will match all unknown multitouch
devices, and udev will automatically load the module.

Since HID_QUIRK_MULTITOUCH never gets set, the special quirks handling
can be removed.  Since all HID MT devices have HID_DG_CONTACTID, they
can be removed from the hid_have_special_driver list.

Cc: Benjamin Tissoires <Benjamin_Tissoires@logitech.com>
Cc: Stephane Chatty <chatty@lii-enac.fr>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-core.c       |   75 +++---------------------------------------
 drivers/hid/hid-input.c      |   11 -------
 drivers/hid/hid-multitouch.c |    3 +-
 include/linux/hid.h          |    2 +-
 4 files changed, 7 insertions(+), 84 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index cb5a103..0a107fe 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -660,6 +660,8 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 
 static void hid_scan_usage(struct hid_device *hid, u32 usage)
 {
+	if (usage == HID_DG_CONTACTID)
+		hid->group = HID_GROUP_MULTITOUCH;
 }
 
 /*
@@ -1350,10 +1352,6 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 	if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
 				connect_mask & HID_CONNECT_HIDINPUT_FORCE))
 		hdev->claimed |= HID_CLAIMED_INPUT;
-	if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
-		/* this device should be handled by hid-multitouch, skip it */
-		return -ENODEV;
-	}
 
 	if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
 			!hdev->hiddev_connect(hdev,
@@ -1430,13 +1428,10 @@ EXPORT_SYMBOL_GPL(hid_disconnect);
 
 /* a list of devices for which there is a specialized driver on HID bus */
 static const struct hid_device_id hid_have_special_driver[] = {
-	{ HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M1968) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M2256) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_WCP32PU) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_X5_005D) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_RP_649) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ACRUX, 0x0802) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR, USB_DEVICE_ID_ACTIONSTAR_1011) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
@@ -1501,60 +1496,32 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUS_T91MT) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUSTEK_MULTITOUCH_YFO) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION_SOLAR) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_TACTICAL_PAD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT, USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CVTOUCH, USB_DEVICE_ID_CVTOUCH_SCREEN) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_1) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480D) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480E) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72A1) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7302) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ELO, USB_DEVICE_ID_ELO_TS2515) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_EMS, USB_DEVICE_ID_EMS_TRIO_LINKER_PLUS_II) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_EZKEY, USB_DEVICE_ID_BTC_8193) },
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_FRUCTEL, USB_DEVICE_ID_GAMETEL_MT_MODE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PSX_ADAPTOR) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PCS_ADAPTOR) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_GOODTOUCH, USB_DEVICE_ID_GOODTOUCH_000f) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0003) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0012) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_HANVON_ALT, USB_DEVICE_ID_HANVON_ALT_MULTITOUCH) },
- 	{ HID_USB_DEVICE(USB_VENDOR_ID_IDEACOM, USB_DEVICE_ID_IDEACOM_IDC6650) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK, USB_DEVICE_ID_HOLTEK_ON_LINE_GRIP) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_MULTITOUCH) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS, USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KEYTOUCH, USB_DEVICE_ID_KEYTOUCH_IEC) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
@@ -1563,7 +1530,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_LG, USB_DEVICE_ID_LG_MULTITOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
@@ -1596,8 +1562,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_LUMIO, USB_DEVICE_ID_CRYSTALTOUCH) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_LUMIO, USB_DEVICE_ID_CRYSTALTOUCH_DUAL) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
@@ -1629,15 +1593,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_18) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_PKB1700) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_WKB2000) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PANASONIC, USB_DEVICE_ID_PANABOARD_UBT780) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PANASONIC, USB_DEVICE_ID_PANABOARD_UBT880) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_PCI) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN1) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
@@ -1654,9 +1611,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_STANTUM, USB_DEVICE_ID_MTP) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM, USB_DEVICE_ID_MTP_STM) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX, USB_DEVICE_ID_MTP_SITRONIX) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SUNPLUS, USB_DEVICE_ID_SUNPLUS_WDESKTOP) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb300) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb304) },
@@ -1670,16 +1624,12 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_TIVO, USB_DEVICE_ID_TIVO_SLIDE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED, USB_DEVICE_ID_TOPSEED_CYBERLINK) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED2, USB_DEVICE_ID_TOPSEED2_RF_COMBO) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_TOUCH_INTL, USB_DEVICE_ID_TOUCH_INTL_MULTI_TOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_TWINHAN, USB_DEVICE_ID_TWINHAN_IR_REMOTE) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_PF1209) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP4030U) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP5540U) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP8060U) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP1062) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_UNITEC, USB_DEVICE_ID_UNITEC_USB_TOUCH_0709) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_UNITEC, USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SMARTJOY_PLUS) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SUPER_JOY_BOX_3) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_DUAL_USB_JOYPAD) },
@@ -1694,16 +1644,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_PID_0038) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_10_6_INCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XAT, USB_DEVICE_ID_XAT_CSR) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX1) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX1) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR1) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX2) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS, USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0005) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0030) },
@@ -1796,10 +1736,6 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
 	struct hid_driver *hdrv = container_of(drv, struct hid_driver, driver);
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 
-	if ((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
-		!strncmp(hdrv->name, "hid-multitouch", 14))
-		return 1;
-
 	return hid_match_device(hdev, hdrv) != NULL;
 }
 
@@ -1817,11 +1753,8 @@ static int hid_device_probe(struct device *dev)
 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
 		if (id == NULL) {
-			if (!((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
-				!strncmp(hdrv->name, "hid-multitouch", 14))) {
-				ret = -ENODEV;
-				goto unlock;
-			}
+			ret = -ENODEV;
+			goto unlock;
 		}
 
 		hdev->driver = hdrv;
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 002781c..7b43186 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -638,10 +638,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_key_clear(BTN_STYLUS2);
 			break;
 
-		case 0x51: /* ContactID */
-			device->quirks |= HID_QUIRK_MULTITOUCH;
-			goto unknown;
-
 		default:  goto unknown;
 		}
 		break;
@@ -1208,13 +1204,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 		}
 	}
 
-	if (hid->quirks & HID_QUIRK_MULTITOUCH) {
-		/* generic hid does not know how to handle multitouch devices */
-		if (hidinput)
-			goto out_cleanup;
-		goto out_unwind;
-	}
-
 	if (hidinput && input_register_device(hidinput->input))
 		goto out_cleanup;
 
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 1d5b941..125c1e7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -654,7 +654,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 * that emit events over several HID messages.
 	 */
 	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
-	hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
 
 	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
@@ -999,6 +998,8 @@ static const struct hid_device_id mt_devices[] = {
 		HID_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_CSR2) },
 
+	/* Generic MT device */
+	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, mt_devices);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index dd694fb..2c7a195 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -317,7 +317,6 @@ struct hid_item {
 #define HID_QUIRK_BADPAD			0x00000020
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
-#define HID_QUIRK_MULTITOUCH			0x00000100
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
@@ -328,6 +327,7 @@ struct hid_item {
  * HID device groups
  */
 #define HID_GROUP_GENERIC			0x0001
+#define HID_GROUP_MULTITOUCH			0x0002
 
 /*
  * This is the global environment of the parser. This information is
-- 
1.7.10

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Benjamin Tissoires]

Hi Henrik,

though I was not able to test it with the devices we have, I remarked
a small regression in this patch: the detection of the serial protocol
is not handled anymore. I was indeed relying on the fact that the
parameter "id" in mt_probe was null to know that the device was not
already in the list of known devices. I think that this small pitfall
can be assessed in a separate commit later (after more testing) to
keep the patch clear.

By looking at the whole code of the patch set, Acked-by: Benjamin
Tissoires <benjamin.tissoires@gmail.com>

I'll tell you by the end of the week if anything goes wrong with our
multitouch devices, but I don't see any reasons why it could fail.

Thanks for the work Henrik,
Benjamin

On Mon, Apr 23, 2012 at 12:07, Henrik Rydberg <rydberg@euromail.se> wrote:
> Switch the driver over to device group handling.  By adding the
> HID_GROUP_MULTITOUCH group to hid-core, hid-generic will no longer
> match multitouch devices.  By adding the HID_GROUP_MULTITOUCH entry to
> the device list, hid-multitouch will match all unknown multitouch
> devices, and udev will automatically load the module.
>
> Since HID_QUIRK_MULTITOUCH never gets set, the special quirks handling
> can be removed.  Since all HID MT devices have HID_DG_CONTACTID, they
> can be removed from the hid_have_special_driver list.
>
> Cc: Benjamin Tissoires <Benjamin_Tissoires@logitech.com>
> Cc: Stephane Chatty <chatty@lii-enac.fr>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hid/hid-core.c       |   75 +++---------------------------------------
>  drivers/hid/hid-input.c      |   11 -------
>  drivers/hid/hid-multitouch.c |    3 +-
>  include/linux/hid.h          |    2 +-
>  4 files changed, 7 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cb5a103..0a107fe 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -660,6 +660,8 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
>
>  static void hid_scan_usage(struct hid_device *hid, u32 usage)
>  {
> +       if (usage == HID_DG_CONTACTID)
> +               hid->group = HID_GROUP_MULTITOUCH;
>  }
>
>  /*
> @@ -1350,10 +1352,6 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
>        if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
>                                connect_mask & HID_CONNECT_HIDINPUT_FORCE))
>                hdev->claimed |= HID_CLAIMED_INPUT;
> -       if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
> -               /* this device should be handled by hid-multitouch, skip it */
> -               return -ENODEV;
> -       }
>
>        if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
>                        !hdev->hiddev_connect(hdev,
> @@ -1430,13 +1428,10 @@ EXPORT_SYMBOL_GPL(hid_disconnect);
>
>  /* a list of devices for which there is a specialized driver on HID bus */
>  static const struct hid_device_id hid_have_special_driver[] = {
> -       { HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M1968) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M2256) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_WCP32PU) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_X5_005D) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_RP_649) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ACRUX, 0x0802) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR, USB_DEVICE_ID_ACTIONSTAR_1011) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
> @@ -1501,60 +1496,32 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUS_T91MT) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUSTEK_MULTITOUCH_YFO) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION_SOLAR) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_TACTICAL_PAD) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT, USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CVTOUCH, USB_DEVICE_ID_CVTOUCH_SCREEN) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_1) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480D) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480E) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72A1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7302) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001) },
>        { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ELO, USB_DEVICE_ID_ELO_TS2515) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_EMS, USB_DEVICE_ID_EMS_TRIO_LINKER_PLUS_II) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_EZKEY, USB_DEVICE_ID_BTC_8193) },
> -       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_FRUCTEL, USB_DEVICE_ID_GAMETEL_MT_MODE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PSX_ADAPTOR) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PCS_ADAPTOR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_GOODTOUCH, USB_DEVICE_ID_GOODTOUCH_000f) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0003) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0012) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_HANVON_ALT, USB_DEVICE_ID_HANVON_ALT_MULTITOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_IDEACOM, USB_DEVICE_ID_IDEACOM_IDC6650) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK, USB_DEVICE_ID_HOLTEK_ON_LINE_GRIP) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_MULTITOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS, USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_KEYTOUCH, USB_DEVICE_ID_KEYTOUCH_IEC) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
> @@ -1563,7 +1530,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_LG, USB_DEVICE_ID_LG_MULTITOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
> @@ -1596,8 +1562,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_LUMIO, USB_DEVICE_ID_CRYSTALTOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_LUMIO, USB_DEVICE_ID_CRYSTALTOUCH_DUAL) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
> @@ -1629,15 +1593,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_18) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_PKB1700) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_WKB2000) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PANASONIC, USB_DEVICE_ID_PANABOARD_UBT780) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PANASONIC, USB_DEVICE_ID_PANABOARD_UBT880) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_PCI) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
> @@ -1654,9 +1611,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
>        { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_STANTUM, USB_DEVICE_ID_MTP) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM, USB_DEVICE_ID_MTP_STM) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX, USB_DEVICE_ID_MTP_SITRONIX) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_SUNPLUS, USB_DEVICE_ID_SUNPLUS_WDESKTOP) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb300) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb304) },
> @@ -1670,16 +1624,12 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_TIVO, USB_DEVICE_ID_TIVO_SLIDE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED, USB_DEVICE_ID_TOPSEED_CYBERLINK) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED2, USB_DEVICE_ID_TOPSEED2_RF_COMBO) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_TOUCH_INTL, USB_DEVICE_ID_TOUCH_INTL_MULTI_TOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_TWINHAN, USB_DEVICE_ID_TWINHAN_IR_REMOTE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_PF1209) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP4030U) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP5540U) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP8060U) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP1062) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_UNITEC, USB_DEVICE_ID_UNITEC_USB_TOUCH_0709) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_UNITEC, USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SMARTJOY_PLUS) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SUPER_JOY_BOX_3) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_DUAL_USB_JOYPAD) },
> @@ -1694,16 +1644,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_PID_0038) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_10_6_INCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XAT, USB_DEVICE_ID_XAT_CSR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS, USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0005) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0030) },
> @@ -1796,10 +1736,6 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
>        struct hid_driver *hdrv = container_of(drv, struct hid_driver, driver);
>        struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>
> -       if ((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
> -               !strncmp(hdrv->name, "hid-multitouch", 14))
> -               return 1;
> -
>        return hid_match_device(hdev, hdrv) != NULL;
>  }
>
> @@ -1817,11 +1753,8 @@ static int hid_device_probe(struct device *dev)
>        if (!hdev->driver) {
>                id = hid_match_device(hdev, hdrv);
>                if (id == NULL) {
> -                       if (!((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
> -                               !strncmp(hdrv->name, "hid-multitouch", 14))) {
> -                               ret = -ENODEV;
> -                               goto unlock;
> -                       }
> +                       ret = -ENODEV;
> +                       goto unlock;
>                }
>
>                hdev->driver = hdrv;
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 002781c..7b43186 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -638,10 +638,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                        map_key_clear(BTN_STYLUS2);
>                        break;
>
> -               case 0x51: /* ContactID */
> -                       device->quirks |= HID_QUIRK_MULTITOUCH;
> -                       goto unknown;
> -
>                default:  goto unknown;
>                }
>                break;
> @@ -1208,13 +1204,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>                }
>        }
>
> -       if (hid->quirks & HID_QUIRK_MULTITOUCH) {
> -               /* generic hid does not know how to handle multitouch devices */
> -               if (hidinput)
> -                       goto out_cleanup;
> -               goto out_unwind;
> -       }
> -
>        if (hidinput && input_register_device(hidinput->input))
>                goto out_cleanup;
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 1d5b941..125c1e7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -654,7 +654,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         * that emit events over several HID messages.
>         */
>        hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
> -       hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
>
>        td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
>        if (!td) {
> @@ -999,6 +998,8 @@ static const struct hid_device_id mt_devices[] = {
>                HID_USB_DEVICE(USB_VENDOR_ID_XIROKU,
>                        USB_DEVICE_ID_XIROKU_CSR2) },
>
> +       /* Generic MT device */
> +       { HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
>        { }
>  };
>  MODULE_DEVICE_TABLE(hid, mt_devices);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index dd694fb..2c7a195 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -317,7 +317,6 @@ struct hid_item {
>  #define HID_QUIRK_BADPAD                       0x00000020
>  #define HID_QUIRK_MULTI_INPUT                  0x00000040
>  #define HID_QUIRK_HIDINPUT_FORCE               0x00000080
> -#define HID_QUIRK_MULTITOUCH                   0x00000100
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>  #define HID_QUIRK_FULLSPEED_INTERVAL           0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS              0x20000000
> @@ -328,6 +327,7 @@ struct hid_item {
>  * HID device groups
>  */
>  #define HID_GROUP_GENERIC                      0x0001
> +#define HID_GROUP_MULTITOUCH                   0x0002
>
>  /*
>  * This is the global environment of the parser. This information is
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Benjamin Tissoires]

Hi Henrik,

though I was not able to test it with the devices we have, I remarked
a small regression in this patch: the detection of the serial protocol
is not handled anymore. I was indeed relying on the fact that the
parameter "id" in mt_probe was null to know that the device was not
already in the list of known devices. I think that this small pitfall
can be assessed in a separate commit later (after more testing) to
keep the patch clear.

By looking at the whole code of the patch set, Acked-by: Benjamin
Tissoires <benjamin.tissoires@gmail.com>

I'll tell you by the end of the week if anything goes wrong with our
multitouch devices, but I don't see any reasons why it could fail.

Thanks for the work Henrik,
Benjamin

On Mon, Apr 23, 2012 at 12:07, Henrik Rydberg <rydberg@euromail.se> wrote:
> Switch the driver over to device group handling.  By adding the
> HID_GROUP_MULTITOUCH group to hid-core, hid-generic will no longer
> match multitouch devices.  By adding the HID_GROUP_MULTITOUCH entry to
> the device list, hid-multitouch will match all unknown multitouch
> devices, and udev will automatically load the module.
>
> Since HID_QUIRK_MULTITOUCH never gets set, the special quirks handling
> can be removed.  Since all HID MT devices have HID_DG_CONTACTID, they
> can be removed from the hid_have_special_driver list.
>
> Cc: Benjamin Tissoires <Benjamin_Tissoires@logitech.com>
> Cc: Stephane Chatty <chatty@lii-enac.fr>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hid/hid-core.c       |   75 +++---------------------------------------
>  drivers/hid/hid-input.c      |   11 -------
>  drivers/hid/hid-multitouch.c |    3 +-
>  include/linux/hid.h          |    2 +-
>  4 files changed, 7 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index cb5a103..0a107fe 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -660,6 +660,8 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
>
>  static void hid_scan_usage(struct hid_device *hid, u32 usage)
>  {
> +       if (usage == HID_DG_CONTACTID)
> +               hid->group = HID_GROUP_MULTITOUCH;
>  }
>
>  /*
> @@ -1350,10 +1352,6 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
>        if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
>                                connect_mask & HID_CONNECT_HIDINPUT_FORCE))
>                hdev->claimed |= HID_CLAIMED_INPUT;
> -       if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
> -               /* this device should be handled by hid-multitouch, skip it */
> -               return -ENODEV;
> -       }
>
>        if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
>                        !hdev->hiddev_connect(hdev,
> @@ -1430,13 +1428,10 @@ EXPORT_SYMBOL_GPL(hid_disconnect);
>
>  /* a list of devices for which there is a specialized driver on HID bus */
>  static const struct hid_device_id hid_have_special_driver[] = {
> -       { HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M1968) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_3M, USB_DEVICE_ID_3M2256) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_WCP32PU) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_X5_005D) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_RP_649) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ACRUX, 0x0802) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR, USB_DEVICE_ID_ACTIONSTAR_1011) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
> @@ -1501,60 +1496,32 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUS_T91MT) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUSTEK_MULTITOUCH_YFO) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION_SOLAR) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_TACTICAL_PAD) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT, USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CVTOUCH, USB_DEVICE_ID_CVTOUCH_SCREEN) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_1) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480D) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_480E) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72A1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7302) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001) },
>        { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ELO, USB_DEVICE_ID_ELO_TS2515) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_EMS, USB_DEVICE_ID_EMS_TRIO_LINKER_PLUS_II) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_EZKEY, USB_DEVICE_ID_BTC_8193) },
> -       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_FRUCTEL, USB_DEVICE_ID_GAMETEL_MT_MODE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PSX_ADAPTOR) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PCS_ADAPTOR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_GOODTOUCH, USB_DEVICE_ID_GOODTOUCH_000f) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0003) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0012) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_HANVON_ALT, USB_DEVICE_ID_HANVON_ALT_MULTITOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_IDEACOM, USB_DEVICE_ID_IDEACOM_IDC6650) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK, USB_DEVICE_ID_HOLTEK_ON_LINE_GRIP) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_MULTITOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS, USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_KEYTOUCH, USB_DEVICE_ID_KEYTOUCH_IEC) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
> @@ -1563,7 +1530,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_LG, USB_DEVICE_ID_LG_MULTITOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
> @@ -1596,8 +1562,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_LUMIO, USB_DEVICE_ID_CRYSTALTOUCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_LUMIO, USB_DEVICE_ID_CRYSTALTOUCH_DUAL) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
> @@ -1629,15 +1593,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_18) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_PKB1700) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_WKB2000) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PANASONIC, USB_DEVICE_ID_PANABOARD_UBT780) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PANASONIC, USB_DEVICE_ID_PANABOARD_UBT880) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_PCI) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ARVO) },
> @@ -1654,9 +1611,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
>        { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_STANTUM, USB_DEVICE_ID_MTP) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM, USB_DEVICE_ID_MTP_STM) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX, USB_DEVICE_ID_MTP_SITRONIX) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_SUNPLUS, USB_DEVICE_ID_SUNPLUS_WDESKTOP) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb300) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_THRUSTMASTER, 0xb304) },
> @@ -1670,16 +1624,12 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_TIVO, USB_DEVICE_ID_TIVO_SLIDE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED, USB_DEVICE_ID_TOPSEED_CYBERLINK) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_TOPSEED2, USB_DEVICE_ID_TOPSEED2_RF_COMBO) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_TOUCH_INTL, USB_DEVICE_ID_TOUCH_INTL_MULTI_TOUCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_TWINHAN, USB_DEVICE_ID_TWINHAN_IR_REMOTE) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_PF1209) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP4030U) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP5540U) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP8060U) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP1062) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_UNITEC, USB_DEVICE_ID_UNITEC_USB_TOUCH_0709) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_UNITEC, USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SMARTJOY_PLUS) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_SUPER_JOY_BOX_3) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP, USB_DEVICE_ID_DUAL_USB_JOYPAD) },
> @@ -1694,16 +1644,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>        { HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_PID_0038) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_10_6_INCH) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XAT, USB_DEVICE_ID_XAT_CSR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR1) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_SPX2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_MPX2) },
> -       { HID_USB_DEVICE(USB_VENDOR_ID_XIROKU, USB_DEVICE_ID_XIROKU_CSR2) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS, USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0005) },
>        { HID_USB_DEVICE(USB_VENDOR_ID_ZEROPLUS, 0x0030) },
> @@ -1796,10 +1736,6 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
>        struct hid_driver *hdrv = container_of(drv, struct hid_driver, driver);
>        struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>
> -       if ((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
> -               !strncmp(hdrv->name, "hid-multitouch", 14))
> -               return 1;
> -
>        return hid_match_device(hdev, hdrv) != NULL;
>  }
>
> @@ -1817,11 +1753,8 @@ static int hid_device_probe(struct device *dev)
>        if (!hdev->driver) {
>                id = hid_match_device(hdev, hdrv);
>                if (id == NULL) {
> -                       if (!((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
> -                               !strncmp(hdrv->name, "hid-multitouch", 14))) {
> -                               ret = -ENODEV;
> -                               goto unlock;
> -                       }
> +                       ret = -ENODEV;
> +                       goto unlock;
>                }
>
>                hdev->driver = hdrv;
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 002781c..7b43186 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -638,10 +638,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                        map_key_clear(BTN_STYLUS2);
>                        break;
>
> -               case 0x51: /* ContactID */
> -                       device->quirks |= HID_QUIRK_MULTITOUCH;
> -                       goto unknown;
> -
>                default:  goto unknown;
>                }
>                break;
> @@ -1208,13 +1204,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>                }
>        }
>
> -       if (hid->quirks & HID_QUIRK_MULTITOUCH) {
> -               /* generic hid does not know how to handle multitouch devices */
> -               if (hidinput)
> -                       goto out_cleanup;
> -               goto out_unwind;
> -       }
> -
>        if (hidinput && input_register_device(hidinput->input))
>                goto out_cleanup;
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 1d5b941..125c1e7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -654,7 +654,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         * that emit events over several HID messages.
>         */
>        hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
> -       hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
>
>        td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
>        if (!td) {
> @@ -999,6 +998,8 @@ static const struct hid_device_id mt_devices[] = {
>                HID_USB_DEVICE(USB_VENDOR_ID_XIROKU,
>                        USB_DEVICE_ID_XIROKU_CSR2) },
>
> +       /* Generic MT device */
> +       { HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
>        { }
>  };
>  MODULE_DEVICE_TABLE(hid, mt_devices);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index dd694fb..2c7a195 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -317,7 +317,6 @@ struct hid_item {
>  #define HID_QUIRK_BADPAD                       0x00000020
>  #define HID_QUIRK_MULTI_INPUT                  0x00000040
>  #define HID_QUIRK_HIDINPUT_FORCE               0x00000080
> -#define HID_QUIRK_MULTITOUCH                   0x00000100
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>  #define HID_QUIRK_FULLSPEED_INTERVAL           0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS              0x20000000
> @@ -328,6 +327,7 @@ struct hid_item {
>  * HID device groups
>  */
>  #define HID_GROUP_GENERIC                      0x0001
> +#define HID_GROUP_MULTITOUCH                   0x0002
>
>  /*
>  * This is the global environment of the parser. This information is
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Jiri Kosina]

On Mon, 30 Apr 2012, Benjamin Tissoires wrote:

> though I was not able to test it with the devices we have, I remarked
> a small regression in this patch: the detection of the serial protocol
> is not handled anymore. I was indeed relying on the fact that the
> parameter "id" in mt_probe was null to know that the device was not
> already in the list of known devices. I think that this small pitfall
> can be assessed in a separate commit later (after more testing) to
> keep the patch clear.
> 
> By looking at the whole code of the patch set, Acked-by: Benjamin
> Tissoires <benjamin.tissoires@gmail.com>
> 
> I'll tell you by the end of the week if anything goes wrong with our
> multitouch devices, but I don't see any reasons why it could fail.

Thanks for the review.

I will still wait for the mt_probe() serial protocol fix, as I'd like to 
be certain that what I push into for-next doesn't introduce any known 
regressions for which there is not a fix floating around yet.

One super-minor thing I have noticed when reviewing this patchset is that 
we have never put a upper bound on HID_DG_CONTACTMAX as presented by the 
device. In an extreme potential case of (likely broken) device this might 
result in memory corruption in mt_probe(), as the argument of kzalloc() 
when allocating the slots will overflow.

But this is not a regressions and has always been there, so something to 
fix separately.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Henrik Rydberg]

> I remarked a small regression in this patch: the detection of the
> serial protocol is not handled anymore. I was indeed relying on the
> fact that the parameter "id" in mt_probe was null to know that the
> device was not already in the list of known devices.

I see. Will this suffice?

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 125c1e7..507981e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -673,8 +673,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
        if (ret)
                goto fail;
 
-       if (!id && td->touches_by_report == 1) {
-               /* the device has been sent by hid-generic */
+       /* unknown serial devices need special quirks */
+       if ((!id || id->product == HID_ANY_ID) && td->touches_by_report == 1) {
                mtclass = &td->mtclass;
                mtclass->quirks |= MT_QUIRK_ALWAYS_VALID;
                mtclass->quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;

Cheers,
Henrik

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Benjamin Tissoires]

On Mon, Apr 30, 2012 at 20:30, Henrik Rydberg <rydberg@euromail.se> wrote:
>> I remarked a small regression in this patch: the detection of the
>> serial protocol is not handled anymore. I was indeed relying on the
>> fact that the parameter "id" in mt_probe was null to know that the
>> device was not already in the list of known devices.
>
> I see. Will this suffice?

Yes, but I'd rather do the test against either id->product or
id->driver_data as we may use HID_ANY_ID for vendors specific
handling. For instance, I had the confirmation from EETI (eGalax) that
they will always use the same protocol for their usb products, thus
the same quirks could be handled by HID_DEVICE(BUS_USB,
HID_GROUP_MULTITOUCH, USB_VENDOR_ID_DWAV, HID_ANY_ID).

Thanks,
Benjamin

>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 125c1e7..507981e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -673,8 +673,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>        if (ret)
>                goto fail;
>
> -       if (!id && td->touches_by_report == 1) {
> -               /* the device has been sent by hid-generic */
> +       /* unknown serial devices need special quirks */
> +       if ((!id || id->product == HID_ANY_ID) && td->touches_by_report == 1) {
>                mtclass = &td->mtclass;
>                mtclass->quirks |= MT_QUIRK_ALWAYS_VALID;
>                mtclass->quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
>
> Cheers,
> Henrik

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Benjamin Tissoires]

On Mon, Apr 30, 2012 at 20:30, Henrik Rydberg <rydberg@euromail.se> wrote:
>> I remarked a small regression in this patch: the detection of the
>> serial protocol is not handled anymore. I was indeed relying on the
>> fact that the parameter "id" in mt_probe was null to know that the
>> device was not already in the list of known devices.
>
> I see. Will this suffice?

Yes, but I'd rather do the test against either id->product or
id->driver_data as we may use HID_ANY_ID for vendors specific
handling. For instance, I had the confirmation from EETI (eGalax) that
they will always use the same protocol for their usb products, thus
the same quirks could be handled by HID_DEVICE(BUS_USB,
HID_GROUP_MULTITOUCH, USB_VENDOR_ID_DWAV, HID_ANY_ID).

Thanks,
Benjamin

>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 125c1e7..507981e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -673,8 +673,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>        if (ret)
>                goto fail;
>
> -       if (!id && td->touches_by_report == 1) {
> -               /* the device has been sent by hid-generic */
> +       /* unknown serial devices need special quirks */
> +       if ((!id || id->product == HID_ANY_ID) && td->touches_by_report == 1) {
>                mtclass = &td->mtclass;
>                mtclass->quirks |= MT_QUIRK_ALWAYS_VALID;
>                mtclass->quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
>
> Cheers,
> Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/6] hid-multitouch: Switch to device groups

[Henrik Rydberg]

On Mon, Apr 30, 2012 at 08:51:28PM +0200, Benjamin Tissoires wrote:
> On Mon, Apr 30, 2012 at 20:30, Henrik Rydberg <rydberg@euromail.se> wrote:
> >> I remarked a small regression in this patch: the detection of the
> >> serial protocol is not handled anymore. I was indeed relying on the
> >> fact that the parameter "id" in mt_probe was null to know that the
> >> device was not already in the list of known devices.
> >
> > I see. Will this suffice?
> 
> Yes, but I'd rather do the test against either id->product or
> id->driver_data as we may use HID_ANY_ID for vendors specific
> handling. For instance, I had the confirmation from EETI (eGalax) that
> they will always use the same protocol for their usb products, thus
> the same quirks could be handled by HID_DEVICE(BUS_USB,
> HID_GROUP_MULTITOUCH, USB_VENDOR_ID_DWAV, HID_ANY_ID).

Sounds good. I am sending out a fourth version of the patch which will
enable this possibility as well.

On a different note, I wonder why the various slot allocations, sysfs
entries and post-parse settings are done after the device has been
started?

Thanks for testing,
Henrik

[PATCH 6/6] hid: Create a common generic driver

[Henrik Rydberg]

Move the hid drivers of the bus drivers to a common generic hid
driver, and make it a proper module. This ought to simplify device
handling moving forward.

Cc: Gustavo Padovan <gustavo@padovan.org>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/Kconfig           |   12 ++++++++++
 drivers/hid/Makefile          |    2 ++
 drivers/hid/hid-generic.c     |   53 +++++++++++++++++++++++++++++++++++++++++
 drivers/hid/usbhid/hid-core.c |   16 -------------
 net/bluetooth/hidp/core.c     |   27 +--------------------
 5 files changed, 68 insertions(+), 42 deletions(-)
 create mode 100644 drivers/hid/hid-generic.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index ffddcba..9a581e1 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -60,6 +60,18 @@ source "drivers/hid/usbhid/Kconfig"
 menu "Special HID drivers"
 	depends on HID
 
+config HID_GENERIC
+	tristate "Generic HID driver"
+	depends on HID
+	default y
+	---help---
+	Support for generic HID devices.
+
+	To compile this driver as a module, choose M here: the module
+	will be called hid-generic.
+
+	If unsure, say Y.
+
 config HID_A4TECH
 	tristate "A4 tech mice" if EXPERT
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 22f1d16..a04cffa 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -9,6 +9,8 @@ endif
 
 obj-$(CONFIG_HID)		+= hid.o
 
+obj-$(CONFIG_HID_GENERIC)	+= hid-generic.o
+
 hid-$(CONFIG_HIDRAW)		+= hidraw.o
 
 hid-logitech-y		:= hid-lg.o
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
new file mode 100644
index 0000000..a8b3148
--- /dev/null
+++ b/drivers/hid/hid-generic.c
@@ -0,0 +1,53 @@
+/*
+ *  HID support for Linux
+ *
+ *  Copyright (c) 1999 Andreas Gal
+ *  Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ *  Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ *  Copyright (c) 2007-2008 Oliver Neukum
+ *  Copyright (c) 2006-2012 Jiri Kosina
+ *  Copyright (c) 2012 Henrik Rydberg
+ */
+
+/*
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <asm/unaligned.h>
+#include <asm/byteorder.h>
+
+#include <linux/hid.h>
+
+static const struct hid_device_id hid_table[] = {
+	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_GENERIC, HID_ANY_ID, HID_ANY_ID) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, hid_table);
+
+static struct hid_driver hid_generic = {
+	.name = "hid-generic",
+	.id_table = hid_table,
+};
+
+static int __init hid_init(void)
+{
+	return hid_register_driver(&hid_generic);
+}
+
+static void __exit hid_exit(void)
+{
+	hid_unregister_driver(&hid_generic);
+}
+
+module_init(hid_init);
+module_exit(hid_exit);
+
+MODULE_AUTHOR("Henrik Rydberg");
+MODULE_DESCRIPTION("HID generic driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 9abf718..9cba500 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1504,28 +1504,15 @@ static struct usb_driver hid_driver = {
 	.supports_autosuspend = 1,
 };
 
-static const struct hid_device_id hid_usb_table[] = {
-	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, HID_ANY_ID, HID_ANY_ID) },
-	{ }
-};
-
 struct usb_interface *usbhid_find_interface(int minor)
 {
 	return usb_find_interface(&hid_driver, minor);
 }
 
-static struct hid_driver hid_usb_driver = {
-	.name = "generic-usb",
-	.id_table = hid_usb_table,
-};
-
 static int __init hid_init(void)
 {
 	int retval = -ENOMEM;
 
-	retval = hid_register_driver(&hid_usb_driver);
-	if (retval)
-		goto hid_register_fail;
 	retval = usbhid_quirks_init(quirks_param);
 	if (retval)
 		goto usbhid_quirks_init_fail;
@@ -1538,8 +1525,6 @@ static int __init hid_init(void)
 usb_register_fail:
 	usbhid_quirks_exit();
 usbhid_quirks_init_fail:
-	hid_unregister_driver(&hid_usb_driver);
-hid_register_fail:
 	return retval;
 }
 
@@ -1547,7 +1532,6 @@ static void __exit hid_exit(void)
 {
 	usb_deregister(&hid_driver);
 	usbhid_quirks_exit();
-	hid_unregister_driver(&hid_usb_driver);
 }
 
 module_init(hid_init);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index b6fa982..2c20d76 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1195,41 +1195,16 @@ int hidp_get_conninfo(struct hidp_conninfo *ci)
 	return err;
 }
 
-static const struct hid_device_id hidp_table[] = {
-	{ HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_GENERIC, HID_ANY_ID, HID_ANY_ID) },
-	{ }
-};
-
-static struct hid_driver hidp_driver = {
-	.name = "generic-bluetooth",
-	.id_table = hidp_table,
-};
-
 static int __init hidp_init(void)
 {
-	int ret;
-
 	BT_INFO("HIDP (Human Interface Emulation) ver %s", VERSION);
 
-	ret = hid_register_driver(&hidp_driver);
-	if (ret)
-		goto err;
-
-	ret = hidp_init_sockets();
-	if (ret)
-		goto err_drv;
-
-	return 0;
-err_drv:
-	hid_unregister_driver(&hidp_driver);
-err:
-	return ret;
+	return hidp_init_sockets();
 }
 
 static void __exit hidp_exit(void)
 {
 	hidp_cleanup_sockets();
-	hid_unregister_driver(&hidp_driver);
 }
 
 module_init(hidp_init);
-- 
1.7.10

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

Hi Henrik,

Thanks for handling this. I still won't have the time to review this
patch set before early may as I'm out of my office till then. Also
sorry for not answering the previous mails, I did not had a reliable
internet connection during my holidays.

Thanks,
Benjamin

Le 23 avr. 2012 12:03, "Henrik Rydberg" <rydberg@euromail.se> a écrit :
>
> Hi Jiri.
>
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)
>
> Building on the report fixup patches, this patchset brings a bit of
> transparency to the hid bus, by the addition of device groups.  This
> allows a more versatile handling of hid drivers in userland, and
> simplifies the logic in the kernel.
>
> In particular, there can be one generic module per device group, and
> those modules are handled automatically by udev. Dynamic rebinding of
> drivers is fully supported. For instance, to load a special
> out-of-tree driver instead of the a generic one, simply unbind the
> device and load the new module. One can also keep _all_ generic
> drivers as modules, significantly simplifying the process of adding
> and testing new features.
>
> And, of course, auto-loading of new multitouch drivers works, since
> that was what triggered this patchset to begin with. :-)
>
> The second patch contains a simple descriptor scanner that is
> new. The last patch unifies the generic drivers into a single,
> loadable module.
>
> Enjoy,
> Henrik
>
> Henrik Rydberg (6):
>  hid: Add device group to modalias
>  hid: Scan the device for group info before adding it
>  hid: Allow bus wildcard matching
>  hid: Create a generic device group
>  hid-multitouch: Switch to device groups
>  hid: Create a common generic driver
>
>  drivers/hid/Kconfig             |   12 +++
>  drivers/hid/Makefile            |    2 +
>  drivers/hid/hid-core.c          |  172 ++++++++++++++++++++-------------------
>  drivers/hid/hid-generic.c       |   53 ++++++++++++
>  drivers/hid/hid-input.c         |   11 ---
>  drivers/hid/hid-multitouch.c    |    3 +-
>  drivers/hid/usbhid/hid-core.c   |   16 ----
>  include/linux/hid.h             |   20 +++--
>  include/linux/mod_devicetable.h |    4 +-
>  net/bluetooth/hidp/core.c       |   27 +-----
>  scripts/mod/file2alias.c        |    5 +-
>  11 files changed, 180 insertions(+), 145 deletions(-)
>  create mode 100644 drivers/hid/hid-generic.c
>
> --
> 1.7.10
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

Hi Henrik,

Thanks for handling this. I still won't have the time to review this
patch set before early may as I'm out of my office till then. Also
sorry for not answering the previous mails, I did not had a reliable
internet connection during my holidays.

Thanks,
Benjamin

Le 23 avr. 2012 12:03, "Henrik Rydberg" <rydberg@euromail.se> a écrit :
>
> Hi Jiri.
>
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)
>
> Building on the report fixup patches, this patchset brings a bit of
> transparency to the hid bus, by the addition of device groups.  This
> allows a more versatile handling of hid drivers in userland, and
> simplifies the logic in the kernel.
>
> In particular, there can be one generic module per device group, and
> those modules are handled automatically by udev. Dynamic rebinding of
> drivers is fully supported. For instance, to load a special
> out-of-tree driver instead of the a generic one, simply unbind the
> device and load the new module. One can also keep _all_ generic
> drivers as modules, significantly simplifying the process of adding
> and testing new features.
>
> And, of course, auto-loading of new multitouch drivers works, since
> that was what triggered this patchset to begin with. :-)
>
> The second patch contains a simple descriptor scanner that is
> new. The last patch unifies the generic drivers into a single,
> loadable module.
>
> Enjoy,
> Henrik
>
> Henrik Rydberg (6):
>  hid: Add device group to modalias
>  hid: Scan the device for group info before adding it
>  hid: Allow bus wildcard matching
>  hid: Create a generic device group
>  hid-multitouch: Switch to device groups
>  hid: Create a common generic driver
>
>  drivers/hid/Kconfig             |   12 +++
>  drivers/hid/Makefile            |    2 +
>  drivers/hid/hid-core.c          |  172 ++++++++++++++++++++-------------------
>  drivers/hid/hid-generic.c       |   53 ++++++++++++
>  drivers/hid/hid-input.c         |   11 ---
>  drivers/hid/hid-multitouch.c    |    3 +-
>  drivers/hid/usbhid/hid-core.c   |   16 ----
>  include/linux/hid.h             |   20 +++--
>  include/linux/mod_devicetable.h |    4 +-
>  net/bluetooth/hidp/core.c       |   27 +-----
>  scripts/mod/file2alias.c        |    5 +-
>  11 files changed, 180 insertions(+), 145 deletions(-)
>  create mode 100644 drivers/hid/hid-generic.c
>
> --
> 1.7.10
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

Hi Henrik,

Thanks for handling this. I still won't have the time to review this
patch set before early may as I'm out of my office. Also sorry for not
answering the previous mails, I did not had a reliable internet
connection during my holidays.

Thanks,
Benjamin

Le 23 avr. 2012 12:03, "Henrik Rydberg" <rydberg@euromail.se> a écrit :
>
> Hi Jiri.
>
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)
>
> Building on the report fixup patches, this patchset brings a bit of
> transparency to the hid bus, by the addition of device groups.  This
> allows a more versatile handling of hid drivers in userland, and
> simplifies the logic in the kernel.
>
> In particular, there can be one generic module per device group, and
> those modules are handled automatically by udev. Dynamic rebinding of
> drivers is fully supported. For instance, to load a special
> out-of-tree driver instead of the a generic one, simply unbind the
> device and load the new module. One can also keep _all_ generic
> drivers as modules, significantly simplifying the process of adding
> and testing new features.
>
> And, of course, auto-loading of new multitouch drivers works, since
> that was what triggered this patchset to begin with. :-)
>
> The second patch contains a simple descriptor scanner that is
> new. The last patch unifies the generic drivers into a single,
> loadable module.
>
> Enjoy,
> Henrik
>
> Henrik Rydberg (6):
>  hid: Add device group to modalias
>  hid: Scan the device for group info before adding it
>  hid: Allow bus wildcard matching
>  hid: Create a generic device group
>  hid-multitouch: Switch to device groups
>  hid: Create a common generic driver
>
>  drivers/hid/Kconfig             |   12 +++
>  drivers/hid/Makefile            |    2 +
>  drivers/hid/hid-core.c          |  172 ++++++++++++++++++++-------------------
>  drivers/hid/hid-generic.c       |   53 ++++++++++++
>  drivers/hid/hid-input.c         |   11 ---
>  drivers/hid/hid-multitouch.c    |    3 +-
>  drivers/hid/usbhid/hid-core.c   |   16 ----
>  include/linux/hid.h             |   20 +++--
>  include/linux/mod_devicetable.h |    4 +-
>  net/bluetooth/hidp/core.c       |   27 +-----
>  scripts/mod/file2alias.c        |    5 +-
>  11 files changed, 180 insertions(+), 145 deletions(-)
>  create mode 100644 drivers/hid/hid-generic.c
>
> --
> 1.7.10
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

Hi Henrik,

Thanks for handling this. I still won't have the time to review this
patch set before early may as I'm out of my office. Also sorry for not
answering the previous mails, I did not had a reliable internet
connection during my holidays.

Thanks,
Benjamin

Le 23 avr. 2012 12:03, "Henrik Rydberg" <rydberg@euromail.se> a écrit :
>
> Hi Jiri.
>
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)
>
> Building on the report fixup patches, this patchset brings a bit of
> transparency to the hid bus, by the addition of device groups.  This
> allows a more versatile handling of hid drivers in userland, and
> simplifies the logic in the kernel.
>
> In particular, there can be one generic module per device group, and
> those modules are handled automatically by udev. Dynamic rebinding of
> drivers is fully supported. For instance, to load a special
> out-of-tree driver instead of the a generic one, simply unbind the
> device and load the new module. One can also keep _all_ generic
> drivers as modules, significantly simplifying the process of adding
> and testing new features.
>
> And, of course, auto-loading of new multitouch drivers works, since
> that was what triggered this patchset to begin with. :-)
>
> The second patch contains a simple descriptor scanner that is
> new. The last patch unifies the generic drivers into a single,
> loadable module.
>
> Enjoy,
> Henrik
>
> Henrik Rydberg (6):
>  hid: Add device group to modalias
>  hid: Scan the device for group info before adding it
>  hid: Allow bus wildcard matching
>  hid: Create a generic device group
>  hid-multitouch: Switch to device groups
>  hid: Create a common generic driver
>
>  drivers/hid/Kconfig             |   12 +++
>  drivers/hid/Makefile            |    2 +
>  drivers/hid/hid-core.c          |  172 ++++++++++++++++++++-------------------
>  drivers/hid/hid-generic.c       |   53 ++++++++++++
>  drivers/hid/hid-input.c         |   11 ---
>  drivers/hid/hid-multitouch.c    |    3 +-
>  drivers/hid/usbhid/hid-core.c   |   16 ----
>  include/linux/hid.h             |   20 +++--
>  include/linux/mod_devicetable.h |    4 +-
>  net/bluetooth/hidp/core.c       |   27 +-----
>  scripts/mod/file2alias.c        |    5 +-
>  11 files changed, 180 insertions(+), 145 deletions(-)
>  create mode 100644 drivers/hid/hid-generic.c
>
> --
> 1.7.10
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/6] hid: Introduce device groups

[Jiri Kosina]

On Mon, 23 Apr 2012, Henrik Rydberg wrote:

> Hi Jiri,
> 
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)

I think so, and overall I like the aproach and the implementation. Thanks!

Just got the stacktrace on [1] though when running kernel with this 
patchset. The trace popped during shutdown, and the machine froze 
completely; I didn't have any kind of external console connected, so 
unfortunately I don't have the beginning of the whole thing.

I haven't been able to reproduce it so far. This was after several 
"parallel" plug/remove cycles of multiple HID devices driven by multiple 
different drivers.

I haven't performed any analysis what this might be yet.

[1] http://www.jikos.cz/jikos/junk/autoloading-trace.jpg


-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Jiri Kosina]

On Mon, 30 Apr 2012, Jiri Kosina wrote:

> > Here is the third version of the extension to the device-driver
> > matching mechanism. AFAICT, there are no outstanding issues. I think
> > we are getting close. :-)
> 
> I think so, and overall I like the aproach and the implementation. Thanks!
> 
> Just got the stacktrace on [1] though when running kernel with this 
> patchset. The trace popped during shutdown, and the machine froze 
> completely; I didn't have any kind of external console connected, so 
> unfortunately I don't have the beginning of the whole thing.
> 
> I haven't been able to reproduce it so far. This was after several 
> "parallel" plug/remove cycles of multiple HID devices driven by multiple 
> different drivers.
> 
> I haven't performed any analysis what this might be yet.
> 
> [1] http://www.jikos.cz/jikos/junk/autoloading-trace.jpg

It actually seems to be spinlock lockup (due to the NMI trigger being 
apparent at the very first line) on kbd_event_lock ...

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

Hi Jiri,

> > Just got the stacktrace on [1] though when running kernel with this 
> > patchset. The trace popped during shutdown, and the machine froze 
> > completely; I didn't have any kind of external console connected, so 
> > unfortunately I don't have the beginning of the whole thing.
> > 
> > I haven't been able to reproduce it so far. This was after several 
> > "parallel" plug/remove cycles of multiple HID devices driven by multiple 
> > different drivers.
> > 
> > I haven't performed any analysis what this might be yet.
> > 
> > [1] http://www.jikos.cz/jikos/junk/autoloading-trace.jpg
> 
> It actually seems to be spinlock lockup (due to the NMI trigger being 
> apparent at the very first line) on kbd_event_lock ...

Ah, yes. I take it you are talking about tty/vt/keyboard.c. So some
random keypress during shutdown triggers the event, which eventually
reaches input_pass_event(). From there on, the trace stays in the
mentioned driver. First kbd_event() gets called, which takes the lock
and goes on to, in turn, call kbd_keycode(), k_handler[2]() ==
k_spec(), fn_handler[9]() == fn_hold(), which goes on to call
stop_tty(). This function comes back to the driver, via con_stop(), as
vt_kbd_con_stop(), which in turn takes the same lock. So unless the
teardown of something in hid affects the choices made in the tty
driver, it appears this is a different problem. Or?

Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Jiri Kosina]

On Mon, 30 Apr 2012, Henrik Rydberg wrote:

> > > Just got the stacktrace on [1] though when running kernel with this 
> > > patchset. The trace popped during shutdown, and the machine froze 
> > > completely; I didn't have any kind of external console connected, so 
> > > unfortunately I don't have the beginning of the whole thing.
> > > 
> > > I haven't been able to reproduce it so far. This was after several 
> > > "parallel" plug/remove cycles of multiple HID devices driven by multiple 
> > > different drivers.
> > > 
> > > I haven't performed any analysis what this might be yet.
> > > 
> > > [1] http://www.jikos.cz/jikos/junk/autoloading-trace.jpg
> > 
> > It actually seems to be spinlock lockup (due to the NMI trigger being 
> > apparent at the very first line) on kbd_event_lock ...
> 
> Ah, yes. I take it you are talking about tty/vt/keyboard.c. So some
> random keypress during shutdown triggers the event, which eventually
> reaches input_pass_event(). From there on, the trace stays in the
> mentioned driver. First kbd_event() gets called, which takes the lock
> and goes on to, in turn, call kbd_keycode(), k_handler[2]() ==
> k_spec(), fn_handler[9]() == fn_hold(), which goes on to call
> stop_tty(). This function comes back to the driver, via con_stop(), as
> vt_kbd_con_stop(), which in turn takes the same lock. So unless the
> teardown of something in hid affects the choices made in the tty
> driver, it appears this is a different problem. Or?

I just came to the same conclusion a few minutes ago ... i.e. this is 
likely unrelated to the patchset and I just triggered it by pure 
coincidence on the patched kernel.

I will keep looking into it a little bit more. Dmitry, any immediate ideas 
by any chance?

Otherwise the series seems indeed fine and if I don't come across anything 
substantial once I am done with the review, I am considering pushing it 
for -next (I still have to look at quirks propagation Nikolai pointed 
out).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Jiri Kosina]

On Mon, 30 Apr 2012, Jiri Kosina wrote:

> > Ah, yes. I take it you are talking about tty/vt/keyboard.c. So some
> > random keypress during shutdown triggers the event, which eventually
> > reaches input_pass_event(). From there on, the trace stays in the
> > mentioned driver. First kbd_event() gets called, which takes the lock
> > and goes on to, in turn, call kbd_keycode(), k_handler[2]() ==
> > k_spec(), fn_handler[9]() == fn_hold(), which goes on to call
> > stop_tty(). This function comes back to the driver, via con_stop(), as
> > vt_kbd_con_stop(), which in turn takes the same lock. So unless the
> > teardown of something in hid affects the choices made in the tty
> > driver, it appears this is a different problem. Or?
> 
> I just came to the same conclusion a few minutes ago ... i.e. this is 
> likely unrelated to the patchset and I just triggered it by pure 
> coincidence on the patched kernel.
> 
> I will keep looking into it a little bit more. Dmitry, any immediate ideas 
> by any chance?

Okay, someone was able to get lockdep complain about this and Alan is 
apparently on it already:

	https://lkml.org/lkml/2012/5/1/69

So nothing to be worried about.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Jiri Kosina]

On Mon, 23 Apr 2012, Henrik Rydberg wrote:

> Hi Jiri,
> 
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)

I have now updated the 'might-rebase/autoloading' branch of git.hid tree 
(this is a rebasing branch, so simple pull will not work for you as a 
fast-forward, you'll have to delete and repull the branch).

It's currently in a shape I would be okay to pushing out for -next and 
scheduling for next merge window, and I will do so tomorrow if nothing 
else pops up during testing or reviews.

Thanks everybody, especially Henrik of course, for excellent work on this.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

Hi Jiri,

> I have now updated the 'might-rebase/autoloading' branch of git.hid tree 
> (this is a rebasing branch, so simple pull will not work for you as a 
> fast-forward, you'll have to delete and repull the branch).
> 
> It's currently in a shape I would be okay to pushing out for -next and 
> scheduling for next merge window, and I will do so tomorrow if nothing 
> else pops up during testing or reviews.

The branch looks good in this end.

Thanks,
Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

On Tue, May 1, 2012 at 1:03 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 23 Apr 2012, Henrik Rydberg wrote:
>
>> Hi Jiri,
>>
>> Here is the third version of the extension to the device-driver
>> matching mechanism. AFAICT, there are no outstanding issues. I think
>> we are getting close. :-)
>
> I have now updated the 'might-rebase/autoloading' branch of git.hid tree
> (this is a rebasing branch, so simple pull will not work for you as a
> fast-forward, you'll have to delete and repull the branch).
>
> It's currently in a shape I would be okay to pushing out for -next and
> scheduling for next merge window, and I will do so tomorrow if nothing
> else pops up during testing or reviews.

Hi Jiri,

Well, I'm back in my lab this morning, and I'm testing your branch
against the different devices we have here. I'm observing a strange
problem: when connecting some devices, the first touch is always at
0,0. If I rmmod and insmod hid_multitouch it seems to work after....

I'm investigating this bug now. I'll tell you when I'm done. I still
don't now if it's related to Henrik's changes or 3.4-rc4.

Cheers,
Benjamin


>
> Thanks everybody, especially Henrik of course, for excellent work on this.
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Jiri Kosina]

On Wed, 2 May 2012, Benjamin Tissoires wrote:

> > It's currently in a shape I would be okay to pushing out for -next and
> > scheduling for next merge window, and I will do so tomorrow if nothing
> > else pops up during testing or reviews.
> 
> Hi Jiri,
> 
> Well, I'm back in my lab this morning, and I'm testing your branch
> against the different devices we have here. I'm observing a strange
> problem: when connecting some devices, the first touch is always at
> 0,0. If I rmmod and insmod hid_multitouch it seems to work after....
> 
> I'm investigating this bug now. I'll tell you when I'm done. I still
> don't now if it's related to Henrik's changes or 3.4-rc4.

I see, thanks a lot for the heads-up, I will be waiting for your analysis 
of this.

I have already renamed the branch to 'device-groups' and merged it into 
branch that goes into next, so please send me any followup patches on top 
of that queue now.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

On Wed, May 2, 2012 at 10:45 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 2 May 2012, Benjamin Tissoires wrote:
>
>> > It's currently in a shape I would be okay to pushing out for -next and
>> > scheduling for next merge window, and I will do so tomorrow if nothing
>> > else pops up during testing or reviews.
>>
>> Hi Jiri,
>>
>> Well, I'm back in my lab this morning, and I'm testing your branch
>> against the different devices we have here. I'm observing a strange
>> problem: when connecting some devices, the first touch is always at
>> 0,0. If I rmmod and insmod hid_multitouch it seems to work after....
>>
>> I'm investigating this bug now. I'll tell you when I'm done. I still
>> don't now if it's related to Henrik's changes or 3.4-rc4.
>
> I see, thanks a lot for the heads-up, I will be waiting for your analysis
> of this.
>
> I have already renamed the branch to 'device-groups' and merged it into
> branch that goes into next, so please send me any followup patches on top
> of that queue now.

Ok, I think I understood: nothing related to Henrik's changes. The bug
is also present in the upstream kernel.
In the function set_last_slot_field, I was doing a:
test_bit(usage->hid, hi->input->absbit);

for instance, 0x10030 (X hid field) in the absbit bit field which
contains only ABS_CNT (0x40) bits.... That's why it was a random bug.
Now I understood why Ubuntu complained about this test ;-)

I won't have the time to submit the patch right now, I have to give a
lecture this afternoon. Maybe this evening or tomorrow.

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

On Wed, May 2, 2012 at 12:55 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, May 2, 2012 at 10:45 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> On Wed, 2 May 2012, Benjamin Tissoires wrote:
>>
>>> > It's currently in a shape I would be okay to pushing out for -next and
>>> > scheduling for next merge window, and I will do so tomorrow if nothing
>>> > else pops up during testing or reviews.
>>>
>>> Hi Jiri,
>>>
>>> Well, I'm back in my lab this morning, and I'm testing your branch
>>> against the different devices we have here. I'm observing a strange
>>> problem: when connecting some devices, the first touch is always at
>>> 0,0. If I rmmod and insmod hid_multitouch it seems to work after....
>>>
>>> I'm investigating this bug now. I'll tell you when I'm done. I still
>>> don't now if it's related to Henrik's changes or 3.4-rc4.
>>
>> I see, thanks a lot for the heads-up, I will be waiting for your analysis
>> of this.
>>
>> I have already renamed the branch to 'device-groups' and merged it into
>> branch that goes into next, so please send me any followup patches on top
>> of that queue now.
>
> Ok, I think I understood: nothing related to Henrik's changes. The bug
> is also present in the upstream kernel.
> In the function set_last_slot_field, I was doing a:
> test_bit(usage->hid, hi->input->absbit);
>
> for instance, 0x10030 (X hid field) in the absbit bit field which
> contains only ABS_CNT (0x40) bits.... That's why it was a random bug.
> Now I understood why Ubuntu complained about this test ;-)
>
> I won't have the time to submit the patch right now, I have to give a
> lecture this afternoon. Maybe this evening or tomorrow.
>
> Cheers,
> Benjamin

Hi guys,

I'm currently on the bug fix I told you earlier. However, I found a
more problematic bug in the hid_groups functionality.

Some device, like the Perixx peripad, present several interfaces
(mouse, keyboard and multitouch).
The hid groups functionality detects the HID field Contact ID, and
then forwards all interfaces to hid-multitouch. The point is that
hid-multitouch does not know how to handle mice and keyboards, and
then fails handling the interfaces of the device.

This particular device is then fully broken (as anyone pumped the events).
I also noticed the same problem (but less problematic) with cypress
panels: it presents different vendor interfaces and they are handled
by hid-multitouch.

Sorry for spotting this that late,
Benjamin


>
>>
>> Thanks,
>>
>> --
>> Jiri Kosina
>> SUSE Labs

Re: [PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

> I'm currently on the bug fix I told you earlier. However, I found a
> more problematic bug in the hid_groups functionality.
> 
> Some device, like the Perixx peripad, present several interfaces
> (mouse, keyboard and multitouch).
> The hid groups functionality detects the HID field Contact ID, and
> then forwards all interfaces to hid-multitouch. The point is that
> hid-multitouch does not know how to handle mice and keyboards, and
> then fails handling the interfaces of the device.

I am a bit unclear as to which devices this applies to, but I see two
possible solutions:

1) Add the devices in question back to the have_special_drivers list.

2) Add the interface type to the group descision, which should
probably be done anyway. I have a patch in the pipe that, will send it
later today.

> This particular device is then fully broken (as anyone pumped the events).
> I also noticed the same problem (but less problematic) with cypress
> panels: it presents different vendor interfaces and they are handled
> by hid-multitouch.

It would be great if you could test soution 1) before on a device -
something seems wrong if those interfaces were handled by hid-generic
before, but before getting the logic straight, it does not hurt to
try. :-)

Thanks,
Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

On Thu, May 3, 2012 at 2:23 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> I'm currently on the bug fix I told you earlier. However, I found a
>> more problematic bug in the hid_groups functionality.
>>
>> Some device, like the Perixx peripad, present several interfaces
>> (mouse, keyboard and multitouch).
>> The hid groups functionality detects the HID field Contact ID, and
>> then forwards all interfaces to hid-multitouch. The point is that
>> hid-multitouch does not know how to handle mice and keyboards, and
>> then fails handling the interfaces of the device.
>
> I am a bit unclear as to which devices this applies to, but I see two
> possible solutions:
>
> 1) Add the devices in question back to the have_special_drivers list.

Well... The device presents valid mouse and keyboard interface that
should be handled by hid-generic.
The behavior of this particular device is the following:
- when 1 finger is in use, then it sends events over the mouse interface
- when 2 fingers are present, it sends events over the multitouch interface
- when you physically trigger the switch mode button, a keyboard
appears and it sends key events over the keyboard interface, and
eventually mouse events if you press the "mouse" key.... ;-)

This crap is all inherited by the fact that Microsoft do not want to
handle indirect touch, and the device maker found this solution to
counter this.

To sum up, adding it to the have_special_drivers driver list won't
work as we need part of the device to be handled by hid-generic.

>
> 2) Add the interface type to the group descision, which should
> probably be done anyway. I have a patch in the pipe that, will send it
> later today.

A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
earlier patch (I don't know why it disappeared).

The problem came out because:
- hid-multitouch registered the triplet BUS_USB / VID / PID.
- For each interface, it asks udev (or the kernel) which driver to
use, and whatever .group was, it was always hid-multitouch that came
out.

So it's just safer to specify the group for all multitouch devices.

Cheers,
Benjamin

>
>> This particular device is then fully broken (as anyone pumped the events).
>> I also noticed the same problem (but less problematic) with cypress
>> panels: it presents different vendor interfaces and they are handled
>> by hid-multitouch.
>
> It would be great if you could test soution 1) before on a device -
> something seems wrong if those interfaces were handled by hid-generic
> before, but before getting the logic straight, it does not hurt to
> try. :-)
>
> Thanks,
> Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

> > 1) Add the devices in question back to the have_special_drivers list.
> 
> Well... The device presents valid mouse and keyboard interface that
> should be handled by hid-generic.
> The behavior of this particular device is the following:
> - when 1 finger is in use, then it sends events over the mouse interface
> - when 2 fingers are present, it sends events over the multitouch interface
> - when you physically trigger the switch mode button, a keyboard
> appears and it sends key events over the keyboard interface, and
> eventually mouse events if you press the "mouse" key.... ;-)
> 
> This crap is all inherited by the fact that Microsoft do not want to
> handle indirect touch, and the device maker found this solution to
> counter this.
> 
> To sum up, adding it to the have_special_drivers driver list won't
> work as we need part of the device to be handled by hid-generic.

So was this particular device never listed in have_special_drivers?

> > 2) Add the interface type to the group descision, which should
> > probably be done anyway. I have a patch in the pipe that, will send it
> > later today.
> 
> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
> earlier patch (I don't know why it disappeared).

No, the specific entries in the hid-multitouch device list matches any
group, so those defines were simplified away in the second version.

> The problem came out because:
> - hid-multitouch registered the triplet BUS_USB / VID / PID.
> - For each interface, it asks udev (or the kernel) which driver to
> use, and whatever .group was, it was always hid-multitouch that came
> out.
> 
> So it's just safer to specify the group for all multitouch devices.

This is still confusing. I thought the real problem was that the
non-mt interfaces do not match hid-generic. Solution 2) should take
care of that. What I don't understand is how those other interfaces
came to be handled by hid-generic before this patch, unless this
device was never listed in have_special_driver.

Are we talking about USB_DEVICE_ID_TOPSEED2_PERIPAD_701 here?

Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

On Thu, May 3, 2012 at 3:19 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > 1) Add the devices in question back to the have_special_drivers list.
>>
>> Well... The device presents valid mouse and keyboard interface that
>> should be handled by hid-generic.
>> The behavior of this particular device is the following:
>> - when 1 finger is in use, then it sends events over the mouse interface
>> - when 2 fingers are present, it sends events over the multitouch interface
>> - when you physically trigger the switch mode button, a keyboard
>> appears and it sends key events over the keyboard interface, and
>> eventually mouse events if you press the "mouse" key.... ;-)
>>
>> This crap is all inherited by the fact that Microsoft do not want to
>> handle indirect touch, and the device maker found this solution to
>> counter this.
>>
>> To sum up, adding it to the have_special_drivers driver list won't
>> work as we need part of the device to be handled by hid-generic.
>
> So was this particular device never listed in have_special_drivers?

No, and that's the way it should (not being part of have_special_driver).

>
>> > 2) Add the interface type to the group descision, which should
>> > probably be done anyway. I have a patch in the pipe that, will send it
>> > later today.
>>
>> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
>> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
>> earlier patch (I don't know why it disappeared).
>
> No, the specific entries in the hid-multitouch device list matches any
> group, so those defines were simplified away in the second version.

disagree: a device can present several interface (because it has
several "devices") and only those presenting Contact ID can and should
be handled by hid-multitouch.

Cypress for instance presents one interface for the multitouch layer,
and one other for specific controls that are seen as a keyboard.
However, in this particular case, I'm not sure we want to show this
interface to the end user.... ;-)

>
>> The problem came out because:
>> - hid-multitouch registered the triplet BUS_USB / VID / PID.
>> - For each interface, it asks udev (or the kernel) which driver to
>> use, and whatever .group was, it was always hid-multitouch that came
>> out.
>>
>> So it's just safer to specify the group for all multitouch devices.
>
> This is still confusing. I thought the real problem was that the
> non-mt interfaces do not match hid-generic. Solution 2) should take
> care of that. What I don't understand is how those other interfaces
> came to be handled by hid-generic before this patch, unless this
> device was never listed in have_special_driver.

The think is that they do match hid-generic (they get the group
HID_GROUP_GENERIC).
However they also match hid-multitouch (as hid-multitouch does not ask
for a particular group). So, if hid-multitouch is loaded __before__
hid-generic, it will be given the device whatever the match with
hid-generic.

And again, yes it was never listed in have_special_driver.

>
> Are we talking about USB_DEVICE_ID_TOPSEED2_PERIPAD_701 here?

yep


- For consistency, I'd rather specifying the group for any devices.
This because hid-multitouch can not handle other interfaces than
multitouch one. Though the catchall is interesting in the sense that
it may help us to hide unwanted interfaces.

- For backward compatibility, we should adapt each device (currently,
I only spotted this particular one) to decide if we need to catch the
group or not.

Jiri, any thought?

Benjamin

>
> Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

> >> > 2) Add the interface type to the group descision, which should
> >> > probably be done anyway. I have a patch in the pipe that, will send it
> >> > later today.
> >>
> >> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
> >> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
> >> earlier patch (I don't know why it disappeared).
> >
> > No, the specific entries in the hid-multitouch device list matches any
> > group, so those defines were simplified away in the second version.
> 
> disagree: a device can present several interface (because it has
> several "devices") and only those presenting Contact ID can and should
> be handled by hid-multitouch.

Obviously this is only a problem for the devices with mixed
interfaces, but for those, solution 2) together with specifying the
group as you suggest should work. We can definitely change all devices
in the list, it just was not necessary before (or so I thought).

> The think is that they do match hid-generic (they get the group
> HID_GROUP_GENERIC).
> However they also match hid-multitouch (as hid-multitouch does not ask
> for a particular group). So, if hid-multitouch is loaded __before__
> hid-generic, it will be given the device whatever the match with
> hid-generic.

I suppose what you describe here is how it was working before the
device groups. Ok, I see what to do now. I will be back shortly with a
patch which should make USB_DEVICE_ID_TOPSEED2_PERIPAD_701 work, and
let's take it from there.

Thanks for testing,
Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Benjamin Tissoires]

On Thu, May 3, 2012 at 3:54 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> >> > 2) Add the interface type to the group descision, which should
>> >> > probably be done anyway. I have a patch in the pipe that, will send it
>> >> > later today.
>> >>
>> >> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
>> >> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
>> >> earlier patch (I don't know why it disappeared).
>> >
>> > No, the specific entries in the hid-multitouch device list matches any
>> > group, so those defines were simplified away in the second version.
>>
>> disagree: a device can present several interface (because it has
>> several "devices") and only those presenting Contact ID can and should
>> be handled by hid-multitouch.
>
> Obviously this is only a problem for the devices with mixed
> interfaces, but for those, solution 2) together with specifying the
> group as you suggest should work. We can definitely change all devices
> in the list, it just was not necessary before (or so I thought).
>
>> The think is that they do match hid-generic (they get the group
>> HID_GROUP_GENERIC).
>> However they also match hid-multitouch (as hid-multitouch does not ask
>> for a particular group). So, if hid-multitouch is loaded __before__
>> hid-generic, it will be given the device whatever the match with
>> hid-generic.
>
> I suppose what you describe here is how it was working before the
> device groups.

Just to be clear: no, this last paragraph occurs after device groups.
Before, it was working as hid-generic catch the device first, then
released it while looking at the report descriptors. Now, it's purely
dependent on which driver is loaded first for this particaular case.

Benjamin

> Ok, I see what to do now. I will be back shortly with a
> patch which should make USB_DEVICE_ID_TOPSEED2_PERIPAD_701 work, and
> let's take it from there.
>
> Thanks for testing,
> Henrik

Re: [PATCH v3 0/6] hid: Introduce device groups

[Henrik Rydberg]

> > I suppose what you describe here is how it was working before the
> > device groups.
> 
> Just to be clear: no, this last paragraph occurs after device groups.
> Before, it was working as hid-generic catch the device first, then
> released it while looking at the report descriptors. Now, it's purely
> dependent on which driver is loaded first for this particaular case.

Thanks for the clarification, and I agree. It also means you are
completely right, it will suffice to change those devices from the
wildcard group to the multitouch group. I don't mind switching them
all.

Thanks,
Henrik