Hi Guys,
Last week, I received two new interesting devices report:
- N-trig win 8 certified pen/touch panel
- Samsung Nexio 42"
N-trig device
-------------
The first one is the origin of patches 1 to 6.
The multiouch part worked flawlessly with the win 8 patches I sent before,
but the pen part was completely ignored.
I could have used the quirk MULTI_INPUT, but by testing this quirk against
several devices report I have (https://github.com/bentiss/hid-devices), it
was a pain because some of them create 4 or 5 useless inputs.
I choose to allow the hid driver to control the creation of input devices, thus
patches 1 to 3.
Nexio device
------------
The second one was more problematic. Indeed, it was not working at all with the
current release of hid-multitouch. I had several ghost points, and any of the
available quirks worked.
I finaly found the trick, and this trick applies to all the win7 and win8
devices I saw so far (same url as before).
So I think I finally understood why the windows driver was better than us: it
first looks at the announced contact count, and treat only the right number. It
was so simple... and it works so well...
However, for us, I need to get this information from the raw_event because most
of the devices put the contact count field at the end of the report.
I also decided to change the default class as it is much more tolerant than the
previous one. I could have changed all the devices, but in the end, I changed
only those that get a benefit and that I could test.
Debug tool
----------
I was able to discover this trick only recently because I made a small C program
that allows me to replay the hid events through hid-multitouch. The code is
here: https://github.com/bentiss/hid-replay and you will need a kernel 3.6
to make it work (it requires uhid).
However, be careful, this program can be the root of many kernel oopses if the
targeted hid module tries to directly handle the usb or with any of the usbhid
function.
So, Henrik, I really need you to push your abstraction of usbhid in all hid
modules :)
Anyway, this tool can be very helpful to debug hid devices, that's why I share
it there... and also because I work for an open-source company :)
Happy reviewing.
Cheers,
Benjamin
Benjamin Tissoires (11):
HID: hid-input factorize hid_input allocation
HID: hid-input: simplify hid_input allocation and registration
HID: hid-input: export hidinput_allocation function
HID: hid-multitouch: creates and handle stylus report with dual-sensors
HID: hid-multitouch: manually send sync event for pen input report
HID: hid-multitouch: append " Pen" to the name of the stylus input
HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU
HID: hid-multitouch: add support for Nexio 42" panel
HID: hid-multitouch: check if ContactCount is given for default quirk
HID: hid-multitouch: fix protocol for 3 devices
HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices
drivers/hid/hid-ids.h | 3 +
drivers/hid/hid-input.c | 100 +++++++++++++---------
drivers/hid/hid-multitouch.c | 198 +++++++++++++++++++++++++++++++++++--------
include/linux/hid.h | 1 +
4 files changed, 229 insertions(+), 73 deletions(-)
--
1.8.0
During the probe, third party drivers can now safely create a new
input devices depending on the parsing of the reports descriptor.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 14 +++++++++++---
include/linux/hid.h | 1 +
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index eea02b0..b0572d0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1163,7 +1163,7 @@ static void report_features(struct hid_device *hid)
}
}
-static struct hid_input *hidinput_allocate(struct hid_device *hid)
+struct hid_input *hidinput_allocate(struct hid_device *hid)
{
struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
struct input_dev *input_dev = input_allocate_device();
@@ -1190,10 +1190,16 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
input_dev->id.version = hid->version;
input_dev->dev.parent = hid->dev.parent;
hidinput->input = input_dev;
- list_add_tail(&hidinput->list, &hid->inputs);
+ list_add(&hidinput->list, &hid->inputs);
return hidinput;
}
+EXPORT_SYMBOL_GPL(hidinput_allocate);
+
+static struct hid_input *hid_get_latest_hidinput(struct hid_device *hid)
+{
+ return list_first_entry(&hid->inputs, struct hid_input, list);
+}
/*
* Register the input device; print a message.
@@ -1243,9 +1249,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
}
for (i = 0; i < report->maxfield; i++)
- for (j = 0; j < report->field[i]->maxusage; j++)
+ for (j = 0; j < report->field[i]->maxusage; j++) {
+ hidinput = hid_get_latest_hidinput(hid);
hidinput_configure_usage(hidinput, report->field[i],
report->field[i]->usage + j);
+ }
if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
/* This will leave hidinput NULL, so that it
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d2c42dd..42b02d6 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -701,6 +701,7 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
extern int hidinput_connect(struct hid_device *hid, unsigned int force);
extern void hidinput_disconnect(struct hid_device *);
+extern struct hid_input *hidinput_allocate(struct hid_device *hid);
int hid_set_field(struct hid_field *, unsigned, __s32);
int hid_input_report(struct hid_device *, int type, u8 *, int, int);
--
1.8.0
Since hid-multitouch sets the quirk HID_QUIRK_NO_INPUT_SYNC, we need
to manually send the events and then make the SYN_EV.
This patch needs to export hidinput_hid_event
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 1 +
drivers/hid/hid-multitouch.c | 17 ++++++++++++++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index b0572d0..66ddbb6 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1057,6 +1057,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
if ((field->flags & HID_MAIN_ITEM_RELATIVE) && (usage->type == EV_KEY))
input_event(input, usage->type, usage->code, 0);
}
+EXPORT_SYMBOL_GPL(hidinput_hid_event);
void hidinput_report_event(struct hid_device *hid, struct hid_report *report)
{
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c894306..0a4c062 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -84,6 +84,7 @@ struct mt_device {
struct mt_fields *fields; /* temporary placeholder for storing the
multitouch fields */
unsigned last_field_index; /* last field index of the report */
+ unsigned last_pen_field_index; /* last field index of the pen report */
unsigned last_slot_field; /* the last field of a slot */
__s8 inputmode; /* InputMode HID feature, -1 if non-existent */
__s8 inputmode_index; /* InputMode HID feature index in the report */
@@ -371,8 +372,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
td->physical = field->physical;
}
- if (field->physical == HID_DG_STYLUS)
+ if (field->physical == HID_DG_STYLUS) {
+ td->last_pen_field_index = field->index;
return 0;
+ }
if (usage->usage_index)
prev_usage = &field->usage[usage->usage_index - 1];
@@ -598,8 +601,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
struct mt_device *td = hid_get_drvdata(hid);
__s32 quirks = td->mtclass.quirks;
- if (field->physical == HID_DG_STYLUS)
- return 0;
+ if (field->physical == HID_DG_STYLUS) {
+ if (hid->claimed & HID_CLAIMED_INPUT)
+ hidinput_hid_event(hid, field, usage, value);
+ if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
+ hid->hiddev_hid_event(hid, field, usage, value);
+ if (usage->usage_index + 1 == field->report_count &&
+ field->index == td->last_pen_field_index)
+ input_sync(field->hidinput->input);
+ return 1;
+ }
if (hid->claimed & HID_CLAIMED_INPUT) {
switch (usage->hid) {
--
1.8.0
By testing the new MT_CLS_DEFAULT against the collection of device
I have[1], I noticed that eGalax ones do not work because they don't
provide the field ContactCount in their report descriptor.
Removing this quirk for this kind of device allows them to work.
[1] https://github.com/bentiss/hid-devices -> all these devices can be
reinjected in the hid subsystem through uhid (kernel > 3.6) and
hid-replay here: https://github.com/bentiss/hid-replay
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-multitouch.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 36cf346..5e0a4d7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -263,6 +263,9 @@ static ssize_t mt_set_quirks(struct device *dev,
td->mtclass.quirks = val;
+ if (!td->contactcount)
+ td->mtclass.quirks &= ~MT_QUIRK_CONTACT_COUNT_ACCURATE;
+
return count;
}
@@ -856,6 +859,9 @@ static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
+ if (!td->contactcount)
+ cls->quirks &= ~MT_QUIRK_CONTACT_COUNT_ACCURATE;
+
td->mt_flags = 0;
}
--
1.8.0
Win 8 devices can use MT_QUIRK_CONTACT_COUNT_ACCURATE instead of
MT_QUIRK_IGNORE_DUPLICATES. The process is a little bit faster
for MT_QUIRK_CONTACT_COUNT_ACCURATE, so let's use it.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-multitouch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index b374a5a..15d2587 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -315,7 +315,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
/* Win 8 devices need special quirks */
__s32 *quirks = &td->mtclass.quirks;
*quirks |= MT_QUIRK_ALWAYS_VALID;
- *quirks |= MT_QUIRK_IGNORE_DUPLICATES;
+ *quirks |= MT_QUIRK_CONTACT_COUNT_ACCURATE;
*quirks |= MT_QUIRK_HOVERING;
*quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
*quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
--
1.8.0
Cando 2087:0a02 was broken, this fixes it.
ActionStar and LG panels can be optimized by using the new default
class.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-multitouch.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5e0a4d7..b374a5a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -997,7 +997,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_3M3266) },
/* ActionStar panels */
- { .driver_data = MT_CLS_NSMU,
+ { .driver_data = MT_CLS_DEFAULT,
MT_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR,
USB_DEVICE_ID_ACTIONSTAR_1011) },
@@ -1017,7 +1017,7 @@ static const struct hid_device_id mt_devices[] = {
{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
- { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
+ { .driver_data = MT_CLS_DEFAULT,
MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
@@ -1155,7 +1155,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
/* LG Display panels */
- { .driver_data = MT_CLS_NSMU,
+ { .driver_data = MT_CLS_DEFAULT,
MT_USB_DEVICE(USB_VENDOR_ID_LG,
USB_DEVICE_ID_LG_MULTITOUCH) },
--
1.8.0
This is not just cosmetics, it can help to write udev and X.org
rules.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0a4c062..f10105f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -32,6 +32,7 @@
#include <linux/slab.h>
#include <linux/usb.h>
#include <linux/input/mt.h>
+#include <linux/string.h>
#include "usbhid/usbhid.h"
@@ -341,6 +342,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
struct mt_class *cls = &td->mtclass;
int code;
struct hid_usage *prev_usage = NULL;
+ char *name;
/* Only map fields from TouchScreen or TouchPad collections.
* We need to ignore fields that belong to other collections
@@ -370,6 +372,13 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
hi = hidinput_allocate(hdev);
td->application = field->application;
td->physical = field->physical;
+ if (field->physical == HID_DG_STYLUS) {
+ name = kzalloc(sizeof(hdev->name) + 4, GFP_KERNEL);
+ if (name) {
+ sprintf(name, "%s Pen", hdev->name);
+ hi->input->name = name;
+ }
+ }
}
if (field->physical == HID_DG_STYLUS) {
@@ -791,6 +800,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
int ret, i;
struct mt_device *td;
struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
+ struct hid_input *hi;
for (i = 0; mt_classes[i].name ; i++) {
if (id->driver_data == mt_classes[i].name) {
@@ -830,7 +840,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret)
- goto fail;
+ goto hid_fail;
ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
@@ -842,6 +852,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
return 0;
+hid_fail:
+ list_for_each_entry(hi, &hdev->inputs, list)
+ if (hi->input->name != hdev->name)
+ kfree(hi->input->name);
fail:
kfree(td->fields);
kfree(td);
@@ -886,8 +900,15 @@ static int mt_resume(struct hid_device *hdev)
static void mt_remove(struct hid_device *hdev)
{
struct mt_device *td = hid_get_drvdata(hdev);
+ struct hid_input *hi;
+
sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
hid_hw_stop(hdev);
+
+ list_for_each_entry(hi, &hdev->inputs, list)
+ if (hi->input->name != hdev->name)
+ kfree(hi->input->name);
+
kfree(td);
hid_set_drvdata(hdev, NULL);
}
--
1.8.0
In order to provide fine control for the creation of different
input devices in probe function of third party drivers, this patch
split the allocations, the registrations and the free of input
devices.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 47f98a3..eea02b0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1206,6 +1206,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
struct hid_driver *drv = hid->driver;
struct hid_report *report;
struct hid_input *hidinput = NULL;
+ struct hid_input *next;
int i, j, k;
INIT_LIST_HEAD(&hid->inputs);
@@ -1238,7 +1239,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
if (!hidinput) {
hidinput = hidinput_allocate(hid);
if (!hidinput)
- goto out_unwind;
+ goto out_cleanup;
}
for (i = 0; i < report->maxfield; i++)
@@ -1253,29 +1254,36 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
* UGCI) cram a lot of unrelated inputs into the
* same interface. */
hidinput->report = report;
- if (drv->input_configured)
- drv->input_configured(hid, hidinput);
- if (input_register_device(hidinput->input))
- goto out_cleanup;
hidinput = NULL;
}
}
}
- if (hidinput) {
+ list_for_each_entry(hidinput, &hid->inputs, list) {
if (drv->input_configured)
drv->input_configured(hid, hidinput);
if (input_register_device(hidinput->input))
- goto out_cleanup;
+ goto out_unwind;
}
return 0;
out_cleanup:
- list_del(&hidinput->list);
- input_free_device(hidinput->input);
- kfree(hidinput);
+ list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
+ list_del(&hidinput->list);
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+ }
+ return -1;
+
out_unwind:
+ /* free the non-registered hidinput, starting from the faulty one */
+ list_for_each_entry_safe_from(hidinput, next, &hid->inputs, list) {
+ list_del(&hidinput->list);
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+ }
+
/* unwind the ones we already registered */
hidinput_disconnect(hid);
--
1.8.0
This just refactors the allocation of hid_input.
No semantic changes.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 61 +++++++++++++++++++++++++++----------------------
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7015080..47f98a3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1163,6 +1163,38 @@ static void report_features(struct hid_device *hid)
}
}
+static struct hid_input *hidinput_allocate(struct hid_device *hid)
+{
+ struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
+ struct input_dev *input_dev = input_allocate_device();
+ if (!hidinput || !input_dev) {
+ kfree(hidinput);
+ input_free_device(input_dev);
+ hid_err(hid, "Out of memory during hid input probe\n");
+ return NULL;
+ }
+
+ input_set_drvdata(input_dev, hid);
+ input_dev->event = hid->ll_driver->hidinput_input_event;
+ input_dev->open = hidinput_open;
+ input_dev->close = hidinput_close;
+ input_dev->setkeycode = hidinput_setkeycode;
+ input_dev->getkeycode = hidinput_getkeycode;
+
+ input_dev->name = hid->name;
+ input_dev->phys = hid->phys;
+ input_dev->uniq = hid->uniq;
+ input_dev->id.bustype = hid->bus;
+ input_dev->id.vendor = hid->vendor;
+ input_dev->id.product = hid->product;
+ input_dev->id.version = hid->version;
+ input_dev->dev.parent = hid->dev.parent;
+ hidinput->input = input_dev;
+ list_add_tail(&hidinput->list, &hid->inputs);
+
+ return hidinput;
+}
+
/*
* Register the input device; print a message.
* Configure the input layer interface
@@ -1174,7 +1206,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
struct hid_driver *drv = hid->driver;
struct hid_report *report;
struct hid_input *hidinput = NULL;
- struct input_dev *input_dev;
int i, j, k;
INIT_LIST_HEAD(&hid->inputs);
@@ -1205,33 +1236,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
continue;
if (!hidinput) {
- hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
- input_dev = input_allocate_device();
- if (!hidinput || !input_dev) {
- kfree(hidinput);
- input_free_device(input_dev);
- hid_err(hid, "Out of memory during hid input probe\n");
+ hidinput = hidinput_allocate(hid);
+ if (!hidinput)
goto out_unwind;
- }
-
- input_set_drvdata(input_dev, hid);
- input_dev->event =
- hid->ll_driver->hidinput_input_event;
- input_dev->open = hidinput_open;
- input_dev->close = hidinput_close;
- input_dev->setkeycode = hidinput_setkeycode;
- input_dev->getkeycode = hidinput_getkeycode;
-
- input_dev->name = hid->name;
- input_dev->phys = hid->phys;
- input_dev->uniq = hid->uniq;
- input_dev->id.bustype = hid->bus;
- input_dev->id.vendor = hid->vendor;
- input_dev->id.product = hid->product;
- input_dev->id.version = hid->version;
- input_dev->dev.parent = hid->dev.parent;
- hidinput->input = input_dev;
- list_add_tail(&hidinput->list, &hid->inputs);
}
for (i = 0; i < report->maxfield; i++)
--
1.8.0
While trying to add support for Nexio 1870:0100, I found that the
trick required to make it work is much more reliable than the previous
default class.
Rename the current default class for backward compatibility.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-multitouch.c | 48 +++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f10105f..50fb79f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -115,6 +115,7 @@ struct mt_device {
#define MT_CLS_DUAL_INRANGE_CONTACTNUMBER 0x0007
#define MT_CLS_DUAL_NSMU_CONTACTID 0x0008
#define MT_CLS_INRANGE_CONTACTNUMBER 0x0009
+#define MT_CLS_NSMU 0x000a
/* vendor specific classes */
#define MT_CLS_3M 0x0101
@@ -147,7 +148,8 @@ static int cypress_compute_slot(struct mt_device *td)
}
static struct mt_class mt_classes[] = {
- { .name = MT_CLS_DEFAULT,
+ { .name = MT_CLS_DEFAULT},
+ { .name = MT_CLS_NSMU,
.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
{ .name = MT_CLS_SERIAL,
.quirks = MT_QUIRK_ALWAYS_VALID},
@@ -927,7 +929,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_3M3266) },
/* ActionStar panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR,
USB_DEVICE_ID_ACTIONSTAR_1011) },
@@ -940,7 +942,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_ATMEL_MXT_DIGITIZER) },
/* Baanto multitouch devices */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_BAANTO,
USB_DEVICE_ID_BAANTO_MT_190W2) },
/* Cando panels */
@@ -958,12 +960,12 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
/* Chunghwa Telecom touch panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT,
USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) },
/* CVTouch panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_CVTOUCH,
USB_DEVICE_ID_CVTOUCH_SCREEN) },
@@ -1052,12 +1054,12 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PWT_TENFINGERS) },
/* Gametel game controller */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_BT_DEVICE(USB_VENDOR_ID_FRUCTEL,
USB_DEVICE_ID_GAMETEL_MT_MODE) },
/* GoodTouch panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_GOODTOUCH,
USB_DEVICE_ID_GOODTOUCH_000f) },
@@ -1075,7 +1077,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_IDEACOM_IDC6651) },
/* Ilitek dual touch panel */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_ILITEK,
USB_DEVICE_ID_ILITEK_MULTITOUCH) },
@@ -1085,7 +1087,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
/* LG Display panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_LG,
USB_DEVICE_ID_LG_MULTITOUCH) },
@@ -1117,7 +1119,7 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_PANABOARD_UBT880) },
/* Novatek Panel */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
USB_DEVICE_ID_NOVATEK_PCT) },
@@ -1173,48 +1175,48 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_TOPSEED2_PERIPAD_701) },
/* Touch International panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_TOUCH_INTL,
USB_DEVICE_ID_TOUCH_INTL_MULTI_TOUCH) },
/* Unitec panels */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_UNITEC,
USB_DEVICE_ID_UNITEC_USB_TOUCH_0709) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_UNITEC,
USB_DEVICE_ID_UNITEC_USB_TOUCH_0A19) },
/* XAT */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XAT,
USB_DEVICE_ID_XAT_CSR) },
/* Xiroku */
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_SPX) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_MPX) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_CSR) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_SPX1) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_MPX1) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_CSR1) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_SPX2) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_MPX2) },
- { .driver_data = MT_CLS_DEFAULT,
+ { .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
USB_DEVICE_ID_XIROKU_CSR2) },
--
1.8.0
This device is the worst device I saw. It keeps TipSwitch and InRange
at 1 for fingers that are not touching the panel.
The solution is to rely on the field ContactCount, which is accurate
as the correct information are packed at the begining of the frame.
Unfortunately, CountactCount is most of the time at the end of the report.
The solution is to pick it when we have the whole report in raw_event.
Fortunately, it occurs that this behavior is pretty well compliant
with all the devices I saw so far. We can make this class the default then.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-ids.h | 3 ++
drivers/hid/hid-multitouch.c | 82 ++++++++++++++++++++++++++++++++++++++++----
2 files changed, 78 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b84790a..9dfc465 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -599,6 +599,9 @@
#define USB_VENDOR_ID_NEC 0x073e
#define USB_DEVICE_ID_NEC_USB_GAME_PAD 0x0301
+#define USB_VENDOR_ID_NEXIO 0x1870
+#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420 0x0100
+
#define USB_VENDOR_ID_NEXTWINDOW 0x1926
#define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 50fb79f..36cf346 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -33,6 +33,8 @@
#include <linux/usb.h>
#include <linux/input/mt.h>
#include <linux/string.h>
+#include <asm/unaligned.h>
+#include <asm/byteorder.h>
#include "usbhid/usbhid.h"
@@ -55,6 +57,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_NO_AREA (1 << 9)
#define MT_QUIRK_IGNORE_DUPLICATES (1 << 10)
#define MT_QUIRK_HOVERING (1 << 11)
+#define MT_QUIRK_CONTACT_COUNT_ACCURATE (1 << 12)
struct mt_slot {
__s32 x, y, cx, cy, p, w, h;
@@ -84,6 +87,8 @@ struct mt_device {
struct mt_class mtclass; /* our mt device class */
struct mt_fields *fields; /* temporary placeholder for storing the
multitouch fields */
+ struct hid_field *contactcount; /* the hid_field contact count that
+ will be picked in mt_raw_event */
unsigned last_field_index; /* last field index of the report */
unsigned last_pen_field_index; /* last field index of the pen report */
unsigned last_slot_field; /* the last field of a slot */
@@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td)
}
static struct mt_class mt_classes[] = {
- { .name = MT_CLS_DEFAULT},
+ { .name = MT_CLS_DEFAULT,
+ .quirks = MT_QUIRK_ALWAYS_VALID |
+ MT_QUIRK_CONTACT_COUNT_ACCURATE },
{ .name = MT_CLS_NSMU,
.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
{ .name = MT_CLS_SERIAL,
@@ -487,6 +494,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
td->last_field_index = field->index;
return 1;
case HID_DG_CONTACTCOUNT:
+ td->contactcount = field;
td->last_field_index = field->index;
return 1;
case HID_DG_CONTACTMAX:
@@ -554,6 +562,10 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
*/
static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
{
+ if ((td->mtclass.quirks & MT_QUIRK_CONTACT_COUNT_ACCURATE) &&
+ td->num_received >= td->num_expected)
+ return;
+
if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
int slotnum = mt_compute_slot(td, input);
struct mt_slot *s = &td->curdata;
@@ -665,12 +677,6 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
td->curdata.h = value;
break;
case HID_DG_CONTACTCOUNT:
- /*
- * Includes multi-packet support where subsequent
- * packets are sent with zero contactcount.
- */
- if (value)
- td->num_expected = value;
break;
case HID_DG_TOUCH:
/* do nothing */
@@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
return 1;
}
+/*
+ * Extract/implement a data field from/to a little endian report (bit array).
+ * Copied from hid-core.c.
+ *
+ * Code sort-of follows HID spec:
+ * http://www.usb.org/developers/devclass_docs/HID1_11.pdf
+ *
+ * While the USB HID spec allows unlimited length bit fields in "report
+ * descriptors", most devices never use more than 16 bits.
+ * One model of UPS is claimed to report "LINEV" as a 32-bit field.
+ * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
+ */
+
+static __u32 extract(const struct hid_device *hid, __u8 *report,
+ unsigned offset, unsigned n)
+{
+ u64 x;
+
+ if (n > 32)
+ hid_warn(hid, "extract() called with n (%d) > 32! (%s)\n",
+ n, current->comm);
+
+ report += offset >> 3; /* adjust byte index */
+ offset &= 7; /* now only need bit offset into one byte */
+ x = get_unaligned_le64(report);
+ x = (x >> offset) & ((1ULL << n) - 1); /* extract bit field */
+ return (u32) x;
+}
+
+
+static int mt_raw_event(struct hid_device *hid, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct mt_device *td = hid_get_drvdata(hid);
+ struct hid_field *field = td->contactcount;
+ unsigned value;
+
+ if (field && report->id == field->report->id) {
+ /*
+ * Pick in advance the field HID_DG_CONTACTCOUNT as it is
+ * often placed at the end of the report.
+ */
+ if (report->id)
+ data++;
+ value = extract(hid, data, field->report_offset,
+ field->report_size);
+ /*
+ * Includes multi-packet support where subsequent
+ * packets are sent with zero contactcount.
+ */
+ if (value)
+ td->num_expected = value;
+ }
+ return 0;
+}
+
static void mt_set_input_mode(struct hid_device *hdev)
{
struct mt_device *td = hid_get_drvdata(hdev);
@@ -1110,6 +1172,11 @@ static const struct hid_device_id mt_devices[] = {
MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
+ /* Nexio panels */
+ { .driver_data = MT_CLS_DEFAULT,
+ MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
+ USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
+
/* Panasonic panels */
{ .driver_data = MT_CLS_PANASONIC,
MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
@@ -1247,6 +1314,7 @@ static struct hid_driver mt_driver = {
.feature_mapping = mt_feature_mapping,
.usage_table = mt_grabbed_usages,
.event = mt_event,
+ .raw_event = mt_raw_event,
#ifdef CONFIG_PM
.reset_resume = mt_reset_resume,
.resume = mt_resume,
--
1.8.0
Now that drivers can create new inputs, let's use this to split
the reports coming from the pen and from the touch.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-multitouch.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 61543c0..c894306 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -98,6 +98,8 @@ struct mt_device {
bool serial_maybe; /* need to check for serial protocol */
bool curvalid; /* is the current contact valid? */
unsigned mt_flags; /* flags to pass to input-mt */
+ unsigned application; /* the current HID application (pen or touch) */
+ unsigned physical; /* the current HID physical (pen or touch) */
};
/* classes of device behavior */
@@ -342,7 +344,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
/* Only map fields from TouchScreen or TouchPad collections.
* We need to ignore fields that belong to other collections
* such as Mouse that might have the same GenericDesktop usages. */
- if (field->application == HID_DG_TOUCHSCREEN)
+ if (field->application == HID_DG_TOUCHSCREEN ||
+ field->application == HID_DG_PEN)
td->mt_flags |= INPUT_MT_DIRECT;
else if (field->application != HID_DG_TOUCHPAD)
return 0;
@@ -354,11 +357,22 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
(usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
td->mt_flags |= INPUT_MT_POINTER;
- /* eGalax devices provide a Digitizer.Stylus input which overrides
- * the correct Digitizers.Finger X/Y ranges.
- * Let's just ignore this input. */
+ /*
+ * eGalax and newer N-Trig devices provide a Digitizer.Stylus input.
+ * Create a new input device for this collection and let hid-input
+ * handling it.
+ */
+ if (td->application != field->application ||
+ td->physical != field->physical) {
+ if (td->application)
+ /* a hidinput is already here, allocate a new one */
+ hi = hidinput_allocate(hdev);
+ td->application = field->application;
+ td->physical = field->physical;
+ }
+
if (field->physical == HID_DG_STYLUS)
- return -1;
+ return 0;
if (usage->usage_index)
prev_usage = &field->usage[usage->usage_index - 1];
@@ -492,6 +506,9 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
struct hid_field *field, struct hid_usage *usage,
unsigned long **bit, int *max)
{
+ if (field->physical == HID_DG_STYLUS)
+ return 0;
+
if (usage->type == EV_KEY || usage->type == EV_ABS)
set_bit(usage->type, hi->input->evbit);
@@ -581,6 +598,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
struct mt_device *td = hid_get_drvdata(hid);
__s32 quirks = td->mtclass.quirks;
+ if (field->physical == HID_DG_STYLUS)
+ return 0;
+
if (hid->claimed & HID_CLAIMED_INPUT) {
switch (usage->hid) {
case HID_DG_INRANGE:
--
1.8.0
On Fri, Nov 23, 2012 at 4:31 PM, Benjamin Tissoires
<[email protected]> wrote:
> This device is the worst device I saw. It keeps TipSwitch and InRange
> at 1 for fingers that are not touching the panel.
> The solution is to rely on the field ContactCount, which is accurate
> as the correct information are packed at the begining of the frame.
>
> Unfortunately, CountactCount is most of the time at the end of the report.
> The solution is to pick it when we have the whole report in raw_event.
>
> Fortunately, it occurs that this behavior is pretty well compliant
> with all the devices I saw so far. We can make this class the default then.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-ids.h | 3 ++
> drivers/hid/hid-multitouch.c | 82 ++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 78 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b84790a..9dfc465 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -599,6 +599,9 @@
> #define USB_VENDOR_ID_NEC 0x073e
> #define USB_DEVICE_ID_NEC_USB_GAME_PAD 0x0301
>
> +#define USB_VENDOR_ID_NEXIO 0x1870
> +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420 0x0100
Oops, I made a typo here. The PID should be 0x010d.
I can send a v2 if needed.
Cheers,
Benjamin
> +
> #define USB_VENDOR_ID_NEXTWINDOW 0x1926
> #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 50fb79f..36cf346 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -33,6 +33,8 @@
> #include <linux/usb.h>
> #include <linux/input/mt.h>
> #include <linux/string.h>
> +#include <asm/unaligned.h>
> +#include <asm/byteorder.h>
> #include "usbhid/usbhid.h"
>
>
> @@ -55,6 +57,7 @@ MODULE_LICENSE("GPL");
> #define MT_QUIRK_NO_AREA (1 << 9)
> #define MT_QUIRK_IGNORE_DUPLICATES (1 << 10)
> #define MT_QUIRK_HOVERING (1 << 11)
> +#define MT_QUIRK_CONTACT_COUNT_ACCURATE (1 << 12)
>
> struct mt_slot {
> __s32 x, y, cx, cy, p, w, h;
> @@ -84,6 +87,8 @@ struct mt_device {
> struct mt_class mtclass; /* our mt device class */
> struct mt_fields *fields; /* temporary placeholder for storing the
> multitouch fields */
> + struct hid_field *contactcount; /* the hid_field contact count that
> + will be picked in mt_raw_event */
> unsigned last_field_index; /* last field index of the report */
> unsigned last_pen_field_index; /* last field index of the pen report */
> unsigned last_slot_field; /* the last field of a slot */
> @@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td)
> }
>
> static struct mt_class mt_classes[] = {
> - { .name = MT_CLS_DEFAULT},
> + { .name = MT_CLS_DEFAULT,
> + .quirks = MT_QUIRK_ALWAYS_VALID |
> + MT_QUIRK_CONTACT_COUNT_ACCURATE },
> { .name = MT_CLS_NSMU,
> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
> { .name = MT_CLS_SERIAL,
> @@ -487,6 +494,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> td->last_field_index = field->index;
> return 1;
> case HID_DG_CONTACTCOUNT:
> + td->contactcount = field;
> td->last_field_index = field->index;
> return 1;
> case HID_DG_CONTACTMAX:
> @@ -554,6 +562,10 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
> */
> static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
> {
> + if ((td->mtclass.quirks & MT_QUIRK_CONTACT_COUNT_ACCURATE) &&
> + td->num_received >= td->num_expected)
> + return;
> +
> if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
> int slotnum = mt_compute_slot(td, input);
> struct mt_slot *s = &td->curdata;
> @@ -665,12 +677,6 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
> td->curdata.h = value;
> break;
> case HID_DG_CONTACTCOUNT:
> - /*
> - * Includes multi-packet support where subsequent
> - * packets are sent with zero contactcount.
> - */
> - if (value)
> - td->num_expected = value;
> break;
> case HID_DG_TOUCH:
> /* do nothing */
> @@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
> return 1;
> }
>
> +/*
> + * Extract/implement a data field from/to a little endian report (bit array).
> + * Copied from hid-core.c.
> + *
> + * Code sort-of follows HID spec:
> + * http://www.usb.org/developers/devclass_docs/HID1_11.pdf
> + *
> + * While the USB HID spec allows unlimited length bit fields in "report
> + * descriptors", most devices never use more than 16 bits.
> + * One model of UPS is claimed to report "LINEV" as a 32-bit field.
> + * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
> + */
> +
> +static __u32 extract(const struct hid_device *hid, __u8 *report,
> + unsigned offset, unsigned n)
> +{
> + u64 x;
> +
> + if (n > 32)
> + hid_warn(hid, "extract() called with n (%d) > 32! (%s)\n",
> + n, current->comm);
> +
> + report += offset >> 3; /* adjust byte index */
> + offset &= 7; /* now only need bit offset into one byte */
> + x = get_unaligned_le64(report);
> + x = (x >> offset) & ((1ULL << n) - 1); /* extract bit field */
> + return (u32) x;
> +}
> +
> +
> +static int mt_raw_event(struct hid_device *hid, struct hid_report *report,
> + u8 *data, int size)
> +{
> + struct mt_device *td = hid_get_drvdata(hid);
> + struct hid_field *field = td->contactcount;
> + unsigned value;
> +
> + if (field && report->id == field->report->id) {
> + /*
> + * Pick in advance the field HID_DG_CONTACTCOUNT as it is
> + * often placed at the end of the report.
> + */
> + if (report->id)
> + data++;
> + value = extract(hid, data, field->report_offset,
> + field->report_size);
> + /*
> + * Includes multi-packet support where subsequent
> + * packets are sent with zero contactcount.
> + */
> + if (value)
> + td->num_expected = value;
> + }
> + return 0;
> +}
> +
> static void mt_set_input_mode(struct hid_device *hdev)
> {
> struct mt_device *td = hid_get_drvdata(hdev);
> @@ -1110,6 +1172,11 @@ static const struct hid_device_id mt_devices[] = {
> MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
> USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
>
> + /* Nexio panels */
> + { .driver_data = MT_CLS_DEFAULT,
> + MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
> + USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
> +
> /* Panasonic panels */
> { .driver_data = MT_CLS_PANASONIC,
> MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
> @@ -1247,6 +1314,7 @@ static struct hid_driver mt_driver = {
> .feature_mapping = mt_feature_mapping,
> .usage_table = mt_grabbed_usages,
> .event = mt_event,
> + .raw_event = mt_raw_event,
> #ifdef CONFIG_PM
> .reset_resume = mt_reset_resume,
> .resume = mt_resume,
> --
> 1.8.0
>
Hi Benjamin,
On Fri, Nov 23, 2012 at 04:31:24PM +0100, Benjamin Tissoires wrote:
> This just refactors the allocation of hid_input.
I think "breaks out the allocation" would be a more appropriate description.
> No semantic changes.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-input.c | 61 +++++++++++++++++++++++++++----------------------
> 1 file changed, 34 insertions(+), 27 deletions(-)
Reviewed-by: Henrik Rydberg <[email protected]>
Thanks,
Henrik
On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
> This just refactors the allocation of hid_input.
> No semantic changes.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
Fine by me, thanks.
> ---
> drivers/hid/hid-input.c | 61 +++++++++++++++++++++++++++----------------------
> 1 file changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 7015080..47f98a3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1163,6 +1163,38 @@ static void report_features(struct hid_device *hid)
> }
> }
>
> +static struct hid_input *hidinput_allocate(struct hid_device *hid)
> +{
> + struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> + struct input_dev *input_dev = input_allocate_device();
> + if (!hidinput || !input_dev) {
> + kfree(hidinput);
> + input_free_device(input_dev);
> + hid_err(hid, "Out of memory during hid input probe\n");
> + return NULL;
> + }
> +
> + input_set_drvdata(input_dev, hid);
> + input_dev->event = hid->ll_driver->hidinput_input_event;
> + input_dev->open = hidinput_open;
> + input_dev->close = hidinput_close;
> + input_dev->setkeycode = hidinput_setkeycode;
> + input_dev->getkeycode = hidinput_getkeycode;
> +
> + input_dev->name = hid->name;
> + input_dev->phys = hid->phys;
> + input_dev->uniq = hid->uniq;
> + input_dev->id.bustype = hid->bus;
> + input_dev->id.vendor = hid->vendor;
> + input_dev->id.product = hid->product;
> + input_dev->id.version = hid->version;
> + input_dev->dev.parent = hid->dev.parent;
> + hidinput->input = input_dev;
> + list_add_tail(&hidinput->list, &hid->inputs);
> +
> + return hidinput;
> +}
> +
> /*
> * Register the input device; print a message.
> * Configure the input layer interface
> @@ -1174,7 +1206,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> struct hid_driver *drv = hid->driver;
> struct hid_report *report;
> struct hid_input *hidinput = NULL;
> - struct input_dev *input_dev;
> int i, j, k;
>
> INIT_LIST_HEAD(&hid->inputs);
> @@ -1205,33 +1236,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> continue;
>
> if (!hidinput) {
> - hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> - input_dev = input_allocate_device();
> - if (!hidinput || !input_dev) {
> - kfree(hidinput);
> - input_free_device(input_dev);
> - hid_err(hid, "Out of memory during hid input probe\n");
> + hidinput = hidinput_allocate(hid);
> + if (!hidinput)
> goto out_unwind;
> - }
> -
> - input_set_drvdata(input_dev, hid);
> - input_dev->event =
> - hid->ll_driver->hidinput_input_event;
> - input_dev->open = hidinput_open;
> - input_dev->close = hidinput_close;
> - input_dev->setkeycode = hidinput_setkeycode;
> - input_dev->getkeycode = hidinput_getkeycode;
> -
> - input_dev->name = hid->name;
> - input_dev->phys = hid->phys;
> - input_dev->uniq = hid->uniq;
> - input_dev->id.bustype = hid->bus;
> - input_dev->id.vendor = hid->vendor;
> - input_dev->id.product = hid->product;
> - input_dev->id.version = hid->version;
> - input_dev->dev.parent = hid->dev.parent;
> - hidinput->input = input_dev;
> - list_add_tail(&hidinput->list, &hid->inputs);
> }
>
> for (i = 0; i < report->maxfield; i++)
> --
> 1.8.0
>
--
Jiri Kosina
SUSE Labs
Hi Benjamin,
> In order to provide fine control for the creation of different
> input devices in probe function of third party drivers, this patch
> split the allocations, the registrations and the free of input
> devices.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
I don't like this patch, nor its purpose. Drivers should not depend on
the hid core working in a particular way internally, that spells
disaster. There must be some other way in which the same effect can be
achieved?
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 47f98a3..eea02b0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1206,6 +1206,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> struct hid_driver *drv = hid->driver;
> struct hid_report *report;
> struct hid_input *hidinput = NULL;
> + struct hid_input *next;
> int i, j, k;
>
> INIT_LIST_HEAD(&hid->inputs);
> @@ -1238,7 +1239,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> if (!hidinput) {
> hidinput = hidinput_allocate(hid);
> if (!hidinput)
> - goto out_unwind;
> + goto out_cleanup;
> }
>
> for (i = 0; i < report->maxfield; i++)
> @@ -1253,29 +1254,36 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> * UGCI) cram a lot of unrelated inputs into the
> * same interface. */
> hidinput->report = report;
> - if (drv->input_configured)
> - drv->input_configured(hid, hidinput);
> - if (input_register_device(hidinput->input))
> - goto out_cleanup;
> hidinput = NULL;
> }
> }
> }
>
> - if (hidinput) {
> + list_for_each_entry(hidinput, &hid->inputs, list) {
> if (drv->input_configured)
> drv->input_configured(hid, hidinput);
> if (input_register_device(hidinput->input))
> - goto out_cleanup;
> + goto out_unwind;
> }
>
> return 0;
>
> out_cleanup:
> - list_del(&hidinput->list);
> - input_free_device(hidinput->input);
> - kfree(hidinput);
> + list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
> + list_del(&hidinput->list);
> + input_free_device(hidinput->input);
> + kfree(hidinput);
> + }
> + return -1;
Irregular return in the error path?
> +
> out_unwind:
> + /* free the non-registered hidinput, starting from the faulty one */
> + list_for_each_entry_safe_from(hidinput, next, &hid->inputs, list) {
> + list_del(&hidinput->list);
> + input_free_device(hidinput->input);
> + kfree(hidinput);
> + }
> +
> /* unwind the ones we already registered */
> hidinput_disconnect(hid);
>
> --
> 1.8.0
>
Thanks,
Henrik
On Tue, 27 Nov 2012, Henrik Rydberg wrote:
> > In order to provide fine control for the creation of different
> > input devices in probe function of third party drivers, this patch
> > split the allocations, the registrations and the free of input
> > devices.
> >
> > Signed-off-by: Benjamin Tissoires <[email protected]>
> > ---
> > drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
> > 1 file changed, 18 insertions(+), 10 deletions(-)
>
> I don't like this patch, nor its purpose. Drivers should not depend on
> the hid core working in a particular way internally, that spells
> disaster. There must be some other way in which the same effect can be
> achieved?
The changelog doesn't seem to be really verbose enough to me.
What exactly is the scenario you are looking at here, Benjamin, please?
Thanks,
--
Jiri Kosina
SUSE Labs
On Fri, Nov 23, 2012 at 04:31:26PM +0100, Benjamin Tissoires wrote:
> During the probe, third party drivers can now safely create a new
> input devices depending on the parsing of the reports descriptor.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-input.c | 14 +++++++++++---
> include/linux/hid.h | 1 +
> 2 files changed, 12 insertions(+), 3 deletions(-)
I can think of two mechanisms that might be useful in finding a
way to achieve this cleanly: a) Let a driver return a value telling
whether to change input device, and b) Let a second driver have a go
at the same device report. Some return value or state could determine
logic in the hid core saying "we are not done with this device, try
another driver". Or something. Just not this way, please.
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index eea02b0..b0572d0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1163,7 +1163,7 @@ static void report_features(struct hid_device *hid)
> }
> }
>
> -static struct hid_input *hidinput_allocate(struct hid_device *hid)
> +struct hid_input *hidinput_allocate(struct hid_device *hid)
> {
> struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> struct input_dev *input_dev = input_allocate_device();
> @@ -1190,10 +1190,16 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
> input_dev->id.version = hid->version;
> input_dev->dev.parent = hid->dev.parent;
> hidinput->input = input_dev;
> - list_add_tail(&hidinput->list, &hid->inputs);
> + list_add(&hidinput->list, &hid->inputs);
>
> return hidinput;
> }
> +EXPORT_SYMBOL_GPL(hidinput_allocate);
> +
> +static struct hid_input *hid_get_latest_hidinput(struct hid_device *hid)
> +{
> + return list_first_entry(&hid->inputs, struct hid_input, list);
> +}
>
> /*
> * Register the input device; print a message.
> @@ -1243,9 +1249,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> }
>
> for (i = 0; i < report->maxfield; i++)
> - for (j = 0; j < report->field[i]->maxusage; j++)
> + for (j = 0; j < report->field[i]->maxusage; j++) {
> + hidinput = hid_get_latest_hidinput(hid);
> hidinput_configure_usage(hidinput, report->field[i],
> report->field[i]->usage + j);
> + }
>
> if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
> /* This will leave hidinput NULL, so that it
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index d2c42dd..42b02d6 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -701,6 +701,7 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
> extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
> extern int hidinput_connect(struct hid_device *hid, unsigned int force);
> extern void hidinput_disconnect(struct hid_device *);
> +extern struct hid_input *hidinput_allocate(struct hid_device *hid);
>
> int hid_set_field(struct hid_field *, unsigned, __s32);
> int hid_input_report(struct hid_device *, int type, u8 *, int, int);
> --
> 1.8.0
>
Thanks,
Henrik
On Fri, Nov 23, 2012 at 04:31:27PM +0100, Benjamin Tissoires wrote:
> Now that drivers can create new inputs, let's use this to split
> the reports coming from the pen and from the touch.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-multitouch.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
NAK on this one in its present form. I appreciate the idea, just not
the implementation, sorry.
Thanks,
Henrik
On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
> This just refactors the allocation of hid_input.
> No semantic changes.
As this is a generic cleanup, I am taking this one through
for-3.8/upstream branch.
Thanks,
--
Jiri Kosina
SUSE Labs
On Thu, Nov 29, 2012 at 3:00 PM, Jiri Kosina <[email protected]> wrote:
> On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
>
>> This just refactors the allocation of hid_input.
>> No semantic changes.
>
> As this is a generic cleanup, I am taking this one through
> for-3.8/upstream branch.
Thanks Jiri.
Sorry for not answering earlier, I was working on an other solution
for pen devices before speculating on the review of the other patches
:)
Cheers,
Benjamin
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
On Tue, Nov 27, 2012 at 9:21 PM, Henrik Rydberg <[email protected]> wrote:
> On Fri, Nov 23, 2012 at 04:31:26PM +0100, Benjamin Tissoires wrote:
>> During the probe, third party drivers can now safely create a new
>> input devices depending on the parsing of the reports descriptor.
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> drivers/hid/hid-input.c | 14 +++++++++++---
>> include/linux/hid.h | 1 +
>> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> I can think of two mechanisms that might be useful in finding a
> way to achieve this cleanly: a) Let a driver return a value telling
> whether to change input device, and b) Let a second driver have a go
> at the same device report. Some return value or state could determine
> logic in the hid core saying "we are not done with this device, try
> another driver". Or something. Just not this way, please.
Hi Henrik,
ok for the implementation of this patch series, it has to be reworked.
As for your proposals:
a) We can not rely on input_mapping because there is a temporal issue:
we already want to have the new input when we are in input_mapping.
So, this implies to create a new callback.
b) This would implies just too much work in hid-core for taking into
account a special case of one type of devices.
Here, we have in the same usb interface 2 different type of reports
coming from different sensors. It's far too different from the usual
configuration we have with legacy devices: when we have several hid
drivers handling the same usb device, it was when hardware makers
split the different sensors in different interfaces. This situation is
correctly handled in the usb subsystem and the hid layer has only to
deal with one driver at a time for a specific interface.
So, in the next series, I propose a new callback ("new_report" -- the
name is awful, but I can not manage to find another one ATM) which
will be called before we call input_mapping for a whole report.
The driver will then have the possibility to:
- continue normally (default behavior)
- ask for a new input device
- skip the entire report
Anyway, Henrik , could you also have a look at patches 7 to 11, they
have nothing to do with pen support, and I'm sure that you want to say
something on them too.
Cheers,
Benjamin
>
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index eea02b0..b0572d0 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1163,7 +1163,7 @@ static void report_features(struct hid_device *hid)
>> }
>> }
>>
>> -static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> +struct hid_input *hidinput_allocate(struct hid_device *hid)
>> {
>> struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
>> struct input_dev *input_dev = input_allocate_device();
>> @@ -1190,10 +1190,16 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> input_dev->id.version = hid->version;
>> input_dev->dev.parent = hid->dev.parent;
>> hidinput->input = input_dev;
>> - list_add_tail(&hidinput->list, &hid->inputs);
>> + list_add(&hidinput->list, &hid->inputs);
>>
>> return hidinput;
>> }
>> +EXPORT_SYMBOL_GPL(hidinput_allocate);
>> +
>> +static struct hid_input *hid_get_latest_hidinput(struct hid_device *hid)
>> +{
>> + return list_first_entry(&hid->inputs, struct hid_input, list);
>> +}
>>
>> /*
>> * Register the input device; print a message.
>> @@ -1243,9 +1249,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>> }
>>
>> for (i = 0; i < report->maxfield; i++)
>> - for (j = 0; j < report->field[i]->maxusage; j++)
>> + for (j = 0; j < report->field[i]->maxusage; j++) {
>> + hidinput = hid_get_latest_hidinput(hid);
>> hidinput_configure_usage(hidinput, report->field[i],
>> report->field[i]->usage + j);
>> + }
>>
>> if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
>> /* This will leave hidinput NULL, so that it
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index d2c42dd..42b02d6 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -701,6 +701,7 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
>> extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
>> extern int hidinput_connect(struct hid_device *hid, unsigned int force);
>> extern void hidinput_disconnect(struct hid_device *);
>> +extern struct hid_input *hidinput_allocate(struct hid_device *hid);
>>
>> int hid_set_field(struct hid_field *, unsigned, __s32);
>> int hid_input_report(struct hid_device *, int type, u8 *, int, int);
>> --
>> 1.8.0
>>
>
> Thanks,
> Henrik