From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:31:47
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
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:31:51
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 <redacted>
---
drivers/hid/hid-input.c | 14 +++++++++++---
include/linux/hid.h | 1 +
2 files changed, 12 insertions(+), 3 deletions(-)
@@ -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
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:31:55
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 <redacted>
---
drivers/hid/hid-input.c | 1 +
drivers/hid/hid-multitouch.c | 17 ++++++++++++++---
2 files changed, 15 insertions(+), 3 deletions(-)
@@ -84,6 +84,7 @@ struct mt_device {structmt_fields*fields;/* temporary placeholder for storing themultitouchfields*/unsignedlast_field_index;/* last field index of the report */+unsignedlast_pen_field_index;/* last field index of the pen report */unsignedlast_slot_field;/* the last field of a slot */__s8inputmode;/* InputMode HID feature, -1 if non-existent */__s8inputmode_index;/* InputMode HID feature index in the report */
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:32:02
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 <redacted>
---
drivers/hid/hid-multitouch.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:32:11
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 <redacted>
---
drivers/hid/hid-multitouch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:32:37
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 <redacted>
---
drivers/hid/hid-multitouch.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:32:55
This is not just cosmetics, it can help to write udev and X.org
rules.
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
@@ -341,6 +342,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,structmt_class*cls=&td->mtclass;intcode;structhid_usage*prev_usage=NULL;+char*name;/* Only map fields from TouchScreen or TouchPad collections.*Weneedtoignorefieldsthatbelongtoothercollections
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:33:27
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 <redacted>
---
drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
@@ -1206,6 +1206,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)structhid_driver*drv=hid->driver;structhid_report*report;structhid_input*hidinput=NULL;+structhid_input*next;inti,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)-gotoout_unwind;+gotoout_cleanup;}for(i=0;i<report->maxfield;i++)
@@ -1253,29 +1254,36 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)*UGCI)cramalotofunrelatedinputsintothe*sameinterface.*/hidinput->report=report;-if(drv->input_configured)-drv->input_configured(hid,hidinput);-if(input_register_device(hidinput->input))-gotoout_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))-gotoout_cleanup;+gotoout_unwind;}return0;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);
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:33:45
This just refactors the allocation of hid_input.
No semantic changes.
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/hid-input.c | 61 +++++++++++++++++++++++++++----------------------
1 file changed, 34 insertions(+), 27 deletions(-)
@@ -1163,6 +1163,38 @@ static void report_features(struct hid_device *hid)}}+staticstructhid_input*hidinput_allocate(structhid_device*hid)+{+structhid_input*hidinput=kzalloc(sizeof(*hidinput),GFP_KERNEL);+structinput_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");+returnNULL;+}++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);++returnhidinput;+}+/**Registertheinputdevice;printamessage.*Configuretheinputlayerinterface
@@ -1174,7 +1206,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)structhid_driver*drv=hid->driver;structhid_report*report;structhid_input*hidinput=NULL;-structinput_dev*input_dev;inti,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)gotoout_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++)
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:37:55
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 <redacted>
---
drivers/hid/hid-multitouch.c | 48 +++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 23 deletions(-)
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:38:45
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 <redacted>
---
drivers/hid/hid-ids.h | 3 ++
drivers/hid/hid-multitouch.c | 82 ++++++++++++++++++++++++++++++++++++++++----
2 files changed, 78 insertions(+), 7 deletions(-)
@@ -84,6 +87,8 @@ struct mt_device {structmt_classmtclass;/* our mt device class */structmt_fields*fields;/* temporary placeholder for storing themultitouchfields*/+structhid_field*contactcount;/* the hid_field contact count that+willbepickedinmt_raw_event*/unsignedlast_field_index;/* last field index of the report */unsignedlast_pen_field_index;/* last field index of the pen report */unsignedlast_slot_field;/* the last field of a slot */
@@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td)}staticstructmt_classmt_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,
@@ -665,12 +677,6 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,td->curdata.h=value;break;caseHID_DG_CONTACTCOUNT:-/*-*Includesmulti-packetsupportwheresubsequent-*packetsaresentwithzerocontactcount.-*/-if(value)-td->num_expected=value;break;caseHID_DG_TOUCH:/* do nothing */
@@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,return1;}+/*+*Extract/implementadatafieldfrom/toalittleendianreport(bitarray).+*Copiedfromhid-core.c.+*+*Codesort-offollowsHIDspec:+*http://www.usb.org/developers/devclass_docs/HID1_11.pdf+*+*WhiletheUSBHIDspecallowsunlimitedlengthbitfieldsin"report+*descriptors", most devices never use more than 16 bits.+*OnemodelofUPSisclaimedtoreport"LINEV"asa32-bitfield.+*Searchlinux-kernelandlinux-usb-develarchivesfor"hid-core extract".+*/++static__u32extract(conststructhid_device*hid,__u8*report,+unsignedoffset,unsignedn)+{+u64x;++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;+}+++staticintmt_raw_event(structhid_device*hid,structhid_report*report,+u8*data,intsize)+{+structmt_device*td=hid_get_drvdata(hid);+structhid_field*field=td->contactcount;+unsignedvalue;++if(field&&report->id==field->report->id){+/*+*PickinadvancethefieldHID_DG_CONTACTCOUNTasitis+*oftenplacedattheendofthereport.+*/+if(report->id)+data++;+value=extract(hid,data,field->report_offset,+field->report_size);+/*+*Includesmulti-packetsupportwheresubsequent+*packetsaresentwithzerocontactcount.+*/+if(value)+td->num_expected=value;+}+return0;+}+staticvoidmt_set_input_mode(structhid_device*hdev){structmt_device*td=hid_get_drvdata(hdev);
From: Benjamin Tissoires <hidden> Date: 2012-11-23 15:39:22
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 <redacted>
---
drivers/hid/hid-multitouch.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
@@ -98,6 +98,8 @@ struct mt_device {boolserial_maybe;/* need to check for serial protocol */boolcurvalid;/* is the current contact valid? */unsignedmt_flags;/* flags to pass to input-mt */+unsignedapplication;/* the current HID application (pen or touch) */+unsignedphysical;/* 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.*Weneedtoignorefieldsthatbelongtoothercollections*suchasMousethatmighthavethesameGenericDesktopusages.*/-if(field->application==HID_DG_TOUCHSCREEN)+if(field->application==HID_DG_TOUCHSCREEN||+field->application==HID_DG_PEN)td->mt_flags|=INPUT_MT_DIRECT;elseif(field->application!=HID_DG_TOUCHPAD)return0;
@@ -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-*thecorrectDigitizers.FingerX/Yranges.-*Let'sjustignorethisinput.*/+/*+*eGalaxandnewerN-TrigdevicesprovideaDigitizer.Stylusinput.+*Createanewinputdeviceforthiscollectionandlethid-input+*handlingit.+*/+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;+return0;if(usage->usage_index)prev_usage=&field->usage[usage->usage_index-1];
From: Benjamin Tissoires <hidden> Date: 2012-11-26 08:37:47
On Fri, Nov 23, 2012 at 4:31 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/hid/hid-ids.h | 3 ++
drivers/hid/hid-multitouch.c | 82 ++++++++++++++++++++++++++++++++++++++++----
2 files changed, 78 insertions(+), 7 deletions(-)
@@ -84,6 +87,8 @@ struct mt_device {structmt_classmtclass;/* our mt device class */structmt_fields*fields;/* temporary placeholder for storing themultitouchfields*/+structhid_field*contactcount;/* the hid_field contact count that+willbepickedinmt_raw_event*/unsignedlast_field_index;/* last field index of the report */unsignedlast_pen_field_index;/* last field index of the pen report */unsignedlast_slot_field;/* the last field of a slot */
@@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td)}staticstructmt_classmt_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,
@@ -665,12 +677,6 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,td->curdata.h=value;break;caseHID_DG_CONTACTCOUNT:-/*-*Includesmulti-packetsupportwheresubsequent-*packetsaresentwithzerocontactcount.-*/-if(value)-td->num_expected=value;break;caseHID_DG_TOUCH:/* do nothing */
@@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,return1;}+/*+*Extract/implementadatafieldfrom/toalittleendianreport(bitarray).+*Copiedfromhid-core.c.+*+*Codesort-offollowsHIDspec:+*http://www.usb.org/developers/devclass_docs/HID1_11.pdf+*+*WhiletheUSBHIDspecallowsunlimitedlengthbitfieldsin"report+*descriptors", most devices never use more than 16 bits.+*OnemodelofUPSisclaimedtoreport"LINEV"asa32-bitfield.+*Searchlinux-kernelandlinux-usb-develarchivesfor"hid-core extract".+*/++static__u32extract(conststructhid_device*hid,__u8*report,+unsignedoffset,unsignedn)+{+u64x;++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;+}+++staticintmt_raw_event(structhid_device*hid,structhid_report*report,+u8*data,intsize)+{+structmt_device*td=hid_get_drvdata(hid);+structhid_field*field=td->contactcount;+unsignedvalue;++if(field&&report->id==field->report->id){+/*+*PickinadvancethefieldHID_DG_CONTACTCOUNTasitis+*oftenplacedattheendofthereport.+*/+if(report->id)+data++;+value=extract(hid,data,field->report_offset,+field->report_size);+/*+*Includesmulti-packetsupportwheresubsequent+*packetsaresentwithzerocontactcount.+*/+if(value)+td->num_expected=value;+}+return0;+}+staticvoidmt_set_input_mode(structhid_device*hdev){structmt_device*td=hid_get_drvdata(hdev);
@@ -1163,6 +1163,38 @@ static void report_features(struct hid_device *hid)}}+staticstructhid_input*hidinput_allocate(structhid_device*hid)+{+structhid_input*hidinput=kzalloc(sizeof(*hidinput),GFP_KERNEL);+structinput_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");+returnNULL;+}++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);++returnhidinput;+}+/**Registertheinputdevice;printamessage.*Configuretheinputlayerinterface
@@ -1174,7 +1206,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)structhid_driver*drv=hid->driver;structhid_report*report;structhid_input*hidinput=NULL;-structinput_dev*input_dev;inti,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)gotoout_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++)
From: Henrik Rydberg <hidden> Date: 2012-11-27 20:13:08
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 <redacted>
---
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?
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 <redacted>
---
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
From: Henrik Rydberg <hidden> Date: 2012-11-27 20:20:50
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 <redacted>
---
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.
@@ -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
From: Henrik Rydberg <hidden> Date: 2012-11-27 20:24:53
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 <redacted>
---
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
From: Benjamin Tissoires <hidden> Date: 2012-11-29 14:58:39
On Thu, Nov 29, 2012 at 3:00 PM, Jiri Kosina [off-list ref] wrote:
On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
quoted
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
From: Benjamin Tissoires <hidden> Date: 2012-11-29 15:31:47
On Tue, Nov 27, 2012 at 9:21 PM, Henrik Rydberg [off-list ref] wrote:
On Fri, Nov 23, 2012 at 04:31:26PM +0100, Benjamin Tissoires wrote:
quoted
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 <redacted>
---
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
@@ -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
From: Henrik Rydberg <hidden> Date: 2012-12-02 08:07:05
Hi Benjamin,
quoted
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.
I may look like a special case, but perhaps it is not. Routing
different sensors to different drivers is what we do all the time, but
we call that the device-driver bus model. To fit into that concept, we
would need to split the sensors into separate devices first, then
apply the driver logic onto those devices. Thus, the problem is not
really a driver issue at all, but a bus driver one.
Imagine a partition function that is called before device add, and
which distributes the sensors of a usb device onto a set of hid
devices. We already started small in this direction by introducing
device groups, and this particular problem seems to be one of the
issues that would be resolved by such a construct.
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
This sounds like a better solution, yes, but the root problem still
remains: do we really want to handle both pen and touch in the same
driver? And do we _have_ to?
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.
Indeed, I will just comment here, saying I do not see why you change
the name of the default. I would prefer it if you resend that set
cleaned up, with the default name unchanged.
Thanks,
Henrik
Last week, I received two new interesting devices report:
- N-trig win 8 certified pen/touch panel
- Samsung Nexio 42"
Bejmanin, Henrik, what are the plans with this patchset please? Planning
respin for 3.9?
Thanks.
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
From: Benjamin Tissoires <hidden> Date: 2013-01-03 11:34:47
On Thu, Jan 3, 2013 at 10:50 AM, Jiri Kosina [off-list ref] wrote:
On Fri, 23 Nov 2012, Benjamin Tissoires wrote:
quoted
Last week, I received two new interesting devices report:
- N-trig win 8 certified pen/touch panel
- Samsung Nexio 42"
Bejmanin, Henrik, what are the plans with this patchset please? Planning
respin for 3.9?
Hi Jiri,
sure. I intend to make a respin of this patch series. I spent a lot of
time lately to automate regressions tests for hid-multitouch. Lucky
me, because some patches of the original series would have broken some
devices. It also helped me to find out 2 other broken devices (they
were broken since their inclusion in hid-multitouch)....
The tests automation is pretty useful, however it can not be used as
this on a vanilla kernel due to the direct dependencies against
usbhid.
Henrik, you told us that you have a patch set fixing these
dependencies on all the hid drivers, will you prepare it for 3.9? Do
you want me to continue your work?
Cheers,
Benjamin
Thanks.
quoted
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
From: Henrik Rydberg <hidden> Date: 2013-01-06 20:01:19
Hi Benjamin,
Henrik, you told us that you have a patch set fixing these
dependencies on all the hid drivers, will you prepare it for 3.9? Do
you want me to continue your work?
Yes, no, yes. :-)
That is, it would be great if you want to pick up those patches. I
will send you a pm shortly.
Thanks,
Henrik
Henrik, you told us that you have a patch set fixing these
dependencies on all the hid drivers, will you prepare it for 3.9? Do
you want me to continue your work?
Yes, no, yes. :-)
That is, it would be great if you want to pick up those patches. I
will send you a pm shortly.
So, what is the status of this code now, please?
Thanks,
--
Jiri Kosina
SUSE Labs
From: Benjamin Tissoires <hidden> Date: 2013-01-16 02:55:14
On Tue, Jan 15, 2013 at 5:04 PM, Jiri Kosina [off-list ref] wrote:
On Sun, 6 Jan 2013, Henrik Rydberg wrote:
quoted
quoted
Henrik, you told us that you have a patch set fixing these
dependencies on all the hid drivers, will you prepare it for 3.9? Do
you want me to continue your work?
Yes, no, yes. :-)
That is, it would be great if you want to pick up those patches. I
will send you a pm shortly.
So, what is the status of this code now, please?
Hi Jiri,
well here are my plans:
1. polishing the support of Nexio 42" + new default class (95% done)
2. add support for pen (it may requires more reviews) (60% done)
3. cleanup Henrik's patches and send them. (1% done - I just went
through them). Honestly, I don't know if I will be able to make it for
3.9 for this series. For the time being, it only bothers me in my
regressions tests, so depending on the work done, we may postpone it
for later.
Last thing, this week, I'm in Boston. The good is that I should be
able to test tomorrow Mika's patch about i2c-hid at Intel's lab
directly, but I won't have much time to work on hid-multitouch before
next Wednesday (the 23rd).
Cheers,
Benjamin