Re: [PATCH v2] [input-hid] Add hid-egalax driver to the unified hid-multitouch framework.
flat view
From: Benjamin Tissoires <hidden>
Date: 2011-03-08 08:55:26
Hi Richard, thanks for this new version. Just some more comments on top of Henrik's ones. On Tue, Mar 8, 2011 at 09:04, Henrik Rydberg [off-list ref] wrote:
Hi Richard,quoted
Hi Benjamin, Hendrick, Jiri and Stéphan,The name is Henrik.quoted
I would like to give this patch another shot, after Benjamin reviewed it (thanks for that!). As he pointed out, testing for the capacitive kind of egalax devices is needed. Maybe Stéphan, has the hardware and would be so kind to test it? I have tried it with my Samsung MB30 display and it worked: Bus 002 Device 002: ID 0eef:480e D-WAV Scientific Co., Ltd (thats USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH3) I am looking forward to your comments, Richard PS: The patch should apply to 2fa403e8b49 of Jiris for-next branch, although I had to cheat a little and rebase his tree to 2.6.38-rc7 because of some unrelated breakage (ecryptfs).Thanks for making this patch. Please find some comments inline. Also, since you are removing copyright from something that was largely copied into this driver, perhaps something should be done about that.quoted
[PATCH] [input-hid] Add hid-egalax driver to the unified hid-multitouch framework. This patch merges the hid-egalax driver into hid-multitouch and therefore adds two device classes (MT_CLS_EGALAX_CAPACITIVE/RESISTIVE). It also gains the capability to work around broken hid reports by overriding X/Y limits and emitting events for each finger (MT_QUIRK_SEND_AT_EACH_REPORT). As a side effect, this patch fixes the broken suspend/resume behavior with the old driver. Signed-off-by: Richard Nauber <redacted> --- drivers/hid/Kconfig | 9 +- drivers/hid/Makefile | 1 - drivers/hid/hid-egalax.c | 279 ------------------------------------------ drivers/hid/hid-multitouch.c | 68 ++++++++++- 4 files changed, 68 insertions(+), 289 deletions(-) delete mode 100644 drivers/hid/hid-egalax.c[...]quoted
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 65e7f20..a00f3d8 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c@@ -37,6 +37,7 @@ MODULE_LICENSE("GPL");#define MT_QUIRK_SLOT_IS_CONTACTNUMBER (1 << 3) #define MT_QUIRK_VALID_IS_INRANGE (1 << 4) #define MT_QUIRK_VALID_IS_CONFIDENCE (1 << 5) +#define MT_QUIRK_SEND_AT_EACH_REPORT (1 << 6) struct mt_slot { __s32 x, y, p, w, h;@@ -63,6 +64,9 @@ struct mt_class {__s32 sn_move; /* Signal/noise ratio for move events */ __s32 sn_pressure; /* Signal/noise ratio for pressure events */ __u8 maxcontacts; + __u8 override_logical_limits; /* correct the reported X/Y range */ + __u32 logical_min[2]; + __u32 logical_max[2];Please think about byte alignment here, keeping elements of the same size together. Also, the override needs a specific name, given that it applies to the whole class, not just the x and y positions. Perhaps the override should be triggered with a quirk instead?
I'm not in favor of a quirk in this particular case: the information is already here: max > 0. For the specific name, the too fields logical_min/max are not well chosen: either use an anonymous struct to have .x, .y so we can add .pressure, .width, etc... or rename logical_min to logical_min_xy. I just saw that Henrik suggested max_x, max_y, etc... chose this one, please. The alignment makes sense!
quoted
}; /* classes of device behavior */@@ -70,6 +74,8 @@ struct mt_class {#define MT_CLS_DUAL_INRANGE_CONTACTID 2 #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER 3 #define MT_CLS_CYPRESS 4 +#define MT_CLS_EGALAX_CAPACITIVE 5 +#define MT_CLS_EGALAX_RESISTIVE 6 /* * these device-dependent functions determine what slot corresponds@@ -119,7 +125,25 @@ struct mt_class mt_classes[] = {.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP | MT_QUIRK_CYPRESS, .maxcontacts = 10 }, - + { .name = MT_CLS_EGALAX_CAPACITIVE, + .quirks = MT_QUIRK_SLOT_IS_CONTACTID | + MT_QUIRK_VALID_IS_INRANGE, + .maxcontacts = 2, + .sn_move = 4096, + .sn_pressure = 32, + .override_logical_limits = 1, + .logical_min = { 0, 0 }, + .logical_max = { 32760, 32760 } }, + { .name = MT_CLS_EGALAX_RESISTIVE, + .quirks = MT_QUIRK_SLOT_IS_CONTACTID | + MT_QUIRK_VALID_IS_INRANGE | + MT_QUIRK_SEND_AT_EACH_REPORT, + .maxcontacts = 2, + .sn_move = 4096, + .sn_pressure = 32, + .override_logical_limits = 1, + .logical_min = { 0, 0 }, + .logical_max = { 32760, 32760 } }, { } };@@ -147,11 +171,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,{ struct mt_device *td = hid_get_drvdata(hdev); struct mt_class *cls = td->mtclass; +Please check for patch for unrelated changes like this.quoted
switch (usage->hid & HID_USAGE_PAGE) { case HID_UP_GENDESK: + switch (usage->hid) { case HID_GD_X: + /* fix up the reported X limits if necessary*/ + if (cls->override_logical_limits) { + field->logical_minimum = cls->logical_min[0]; + field->logical_maximum = cls->logical_max[0]; + } +Something like "if (quirks & MT_QUIRK_FIX_X)", and then on next line "field->logical_maximum = cls->max_x".
or just "if (cls->max_x)" and then the two "field->logical_maximum = cls->max_x" and "field->logical_minimum = cls->min_x".
quoted
hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_MT_POSITION_X); set_abs(hi->input, ABS_MT_POSITION_X, field,@@ -161,6 +193,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,td->last_slot_field = usage->hid; return 1; case HID_GD_Y: + /* fix up the reported Y limits if nessecary*/ + if (cls->override_logical_limits) { + field->logical_minimum = cls->logical_min[1]; + field->logical_maximum = cls->logical_max[1]; + }Ditto.quoted
hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_MT_POSITION_Y); set_abs(hi->input, ABS_MT_POSITION_Y, field,@@ -204,6 +241,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,td->last_slot_field = usage->hid; return 1; case HID_DG_TIPPRESSURE: + /* fix up the pressure range for some devices + with broken report */ + field->logical_minimum = 0; + hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_MT_PRESSURE); set_abs(hi->input, ABS_MT_PRESSURE, field,@@ -367,8 +408,12 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,if (usage->hid == td->last_slot_field) mt_complete_slot(td); - if (field->index == td->last_field_index + if ((field->index == td->last_field_index && td->num_received >= td->num_expected) + || ((quirks & MT_QUIRK_SEND_AT_EACH_REPORT) + && (usage->hid == td->last_slot_field))) + /* emit an event for every finger to work around + a corrupt last_field_index in the hid report*/ mt_emit_event(td, field->hidinput->input); }@@ -484,6 +529,25 @@ static const struct hid_device_id mt_devices[] = {HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) }, + /* Resistive eGalax devices */ + { .driver_data = MT_CLS_EGALAX_RESISTIVE, + HID_USB_DEVICE(USB_VENDOR_ID_DWAV, + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH) }, + { .driver_data = MT_CLS_EGALAX_RESISTIVE, + HID_USB_DEVICE(USB_VENDOR_ID_DWAV, + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH3) }, + + /* Capacitive eGalax devices */ + { .driver_data = MT_CLS_EGALAX_CAPACITIVE, + HID_USB_DEVICE(USB_VENDOR_ID_DWAV, + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH1) }, + { .driver_data = MT_CLS_EGALAX_CAPACITIVE, + HID_USB_DEVICE(USB_VENDOR_ID_DWAV, + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH2) }, + { .driver_data = MT_CLS_EGALAX_CAPACITIVE, + HID_USB_DEVICE(USB_VENDOR_ID_DWAV, + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH4) }, + { } }; MODULE_DEVICE_TABLE(hid, mt_devices); -- 1.7.1Thanks, Henrik
Thanks, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html