HID: wacom: Initialize MT slots for generic devices at post_parse_hid
Subsystems (2)
PATCH v2 2/3 HID: wacom: Initialize MT slots for generic devices at post_parse_hid
If a HID descriptor places HID_DG_CONTACTID before HID_DG_X and HID_DG_Y then the ABS_X and ABS_Y will not be automatically initialized by the call to input_mt_init_slots. To ensure that this is not a problem, we relocate that call to occur after HID parsing has been completed and we've initalized all the multitouch axes.
---
Changes from v1:
* Patch renamed from "HID: wacom: Manually declare ABS_{X,Y} for pointer emulation"
* Patch moves call to input_mt_init_slots rather than manually declare ABS_{X,Y}
drivers/hid/wacom_sys.c | 18 ++++++++++++++++++ drivers/hid/wacom_wac.c | 3 --- 2 files changed, 18 insertions(+), 3 deletions(-)
drivers/hid/wacom_sys.c +18 -0
@@ -13,6 +13,7 @@#include "wacom_wac.h" #include "wacom.h" +#include <linux/input/mt.h> #define WAC_MSG_RETRIES 5
@@ -236,6 +237,21 @@ static void wacom_usage_mapping(struct hid_device *hdev,wacom_wac_usage_mapping (hdev, field, usage); } +static void wacom_post_parse_hid (struct hid_device *hdev, +struct wacom_features *features) +{ +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; + +if (features->type ==HID_GENERIC ) { +/* Any last-minute generic device setup */ +if (features->touch_max >1 ) { +input_mt_init_slots (wacom_wac->input, wacom_wac->features.touch_max, +INPUT_MT_DIRECT ); + } + } +} +static void wacom_parse_hid (struct hid_device *hdev,struct wacom_features *features) {
@@ -270,6 +286,8 @@ static void wacom_parse_hid(struct hid_device *hdev,wacom_usage_mapping (hdev, hreport->field[i], hreport->field[i]->usage + j); } + +wacom_post_parse_hid (hdev, features); }static int wacom_hid_set_device_mode (struct hid_device *hdev)
drivers/hid/wacom_wac.c +0 -3
@@ -1381,7 +1381,6 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; -struct input_dev *input = wacom_wac->input;unsigned touch_max = wacom_wac->features.touch_max;switch (usage->hid) {
@@ -1400,8 +1399,6 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, ABS_MT_POSITION_Y, 4);break ;case HID_DG_CONTACTID : -input_mt_init_slots (input, wacom_wac->features.touch_max, -INPUT_MT_DIRECT );break ;case HID_DG_INRANGE :break ;
Signature · 1 line
2.1.3
PATCH v2 3/3 HID: wacom: Report input events for each finger on generic devices
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from hid-multitouch.c to discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.
---
Changes from v1:
* Patch renamed from "HID: wacom: Report input events immediately upon receipt"
* Instead of reporting on-reciept, events are bundled and sent at the end of each contact
* Adds code to discover 'last_slot_field' based on hid-multitouch.c implementation
drivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++----------------- drivers/hid/wacom_wac.h | 1 + 2 files changed, 63 insertions(+), 32 deletions(-)
drivers/hid/wacom_wac.c +62 -32
@@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; -unsigned touch_max = wacom_wac->features.touch_max; +struct wacom_features *features = &wacom_wac->features;switch (usage->hid) {case HID_GD_X : -if (touch_max ==1 ) + features->last_slot_field = usage->hid; +if (features->touch_max ==1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 );else wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_MT_POSITION_X ,4 );break ;case HID_GD_Y : -if (touch_max ==1 ) + features->last_slot_field = usage->hid; +if (features->touch_max ==1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 );else wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_MT_POSITION_Y ,4 );break ;case HID_DG_CONTACTID : + features->last_slot_field = usage->hid;break ;case HID_DG_INRANGE : + features->last_slot_field = usage->hid;break ;case HID_DG_INVERT : + features->last_slot_field = usage->hid;break ;case HID_DG_TIPSWITCH : + features->last_slot_field = usage->hid;wacom_map_usage (wacom, usage, field,EV_KEY ,BTN_TOUCH ,0 );break ; } } +static void wacom_wac_finger_slot (struct wacom_wac *wacom_wac, +struct input_dev *input) +{ +struct hid_data *hid_data = &wacom_wac->hid_data; +bool mt = wacom_wac->features.touch_max >1 ; +bool prox = hid_data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; + +if (mt) { +int slot; + + slot =input_mt_get_slot_by_key (input, hid_data->id); +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox); + } +else { +input_report_key (input,BTN_TOUCH , prox); + } + +if (prox) { +input_report_abs (input, mt ?ABS_MT_POSITION_X :ABS_X , + hid_data->x); +input_report_abs (input, mt ?ABS_MT_POSITION_Y :ABS_Y , + hid_data->y); + } +} +static int wacom_wac_finger_event (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage,__s32 value) {
@@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev, } +if (usage->usage_index +1 == field->report_count) { +if (usage->hid == wacom_wac->features.last_slot_field) +wacom_wac_finger_slot (wacom_wac, wacom_wac->input); + } +return 0 ; } -static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_touches (struct hid_device *hdev) { -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; - slot =input_mt_get_slot_by_key (input, hid_data->id); +if (touch_max ==1 ) +return wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity; -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_mt_sync_frame (input); -} - -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); - } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,
@@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity; -unsigned touch_max = wacom_wac->features.touch_max; -if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); +if (wacom_wac->features.touch_max >1 ) +input_mt_sync_frame (input);else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +wacom_wac_finger_slot (wacom_wac, input); +input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \
drivers/hid/wacom_wac.h +1 -0
@@ -144,6 +144,7 @@ struct wacom_features {int pktlen;bool check_for_hid_type;int hid_type; +int last_slot_field; };struct wacom_shared {
Signature · 1 line
2.1.3
Re: [PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices
Quoted 9 lines · Jason Gerecke
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from
hid-multitouch.cto discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.Signed-off-by Jason Gerecke <[email protected]>---
Thanks for the v2 Jason.
I think we are closes to it. I have some comments on the style, but now that I have read it more carefully, I have a better idea of it, and the patch is not so scary.
Jiri. If you reviewed it and took it, that's fine. If not, then I think there is still room for improvements.
Cheers,
Benjamin
Quoted 14 lines
Changes from v1:
* Patch renamed from "HID: wacom: Report input events immediately upon receipt"
* Instead of reporting on-reciept, events are bundled and sent at the end of each contact
* Adds code to discover 'last_slot_field' based onhid-multitouch.cimplementationdrivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++----------------- drivers/hid/wacom_wac.h | 1 + 2 files changed, 63 insertions(+), 32 deletions(-)drivers/hid/wacom_wac.c +0
-0
drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1384
Quoted 4 lines
drivers/hid/wacom_wac.c +0
-1@@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; -unsigned touch_max = wacom_wac->features.touch_max;
not sure we really need to remove the temporary variable touch_max here.
drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1389
Quoted 7 lines
drivers/hid/wacom_wac.c +3
-1+struct wacom_features *features = &wacom_wac->features;switch (usage->hid) {case HID_GD_X : -if (touch_max ==1 ) + features->last_slot_field = usage->hid; +if (features->touch_max ==1 )
these changes (touchmax => features->touch_max) hinder a little bit the actual change, that is worrisome :)
Quoted 29 lines
drivers/hid/wacom_wac.c +6
-1wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); else wacom_map_usage(wacom, usage, field, EV_ABS, ABS_MT_POSITION_X, 4); break; case HID_GD_Y: - if (touch_max == 1) + features->last_slot_field = usage->hid; + if (features->touch_max == 1) wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4); else wacom_map_usage(wacom, usage, field, EV_ABS, ABS_MT_POSITION_Y, 4); break; case HID_DG_CONTACTID: + features->last_slot_field = usage->hid; break; case HID_DG_INRANGE: + features->last_slot_field = usage->hid; break; case HID_DG_INVERT: + features->last_slot_field = usage->hid; break; case HID_DG_TIPSWITCH: + features->last_slot_field = usage->hid; wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0); break; } }
drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1427
Quoted 10 lines
drivers/hid/wacom_wac.c +9
-0+static void wacom_wac_finger_slot (struct wacom_wac *wacom_wac, +struct input_dev *input) +{ +struct hid_data *hid_data = &wacom_wac->hid_data; +bool mt = wacom_wac->features.touch_max >1 ; +bool prox = hid_data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; + +if (mt) {
Personal taste. I don't like the "if (mt)" approach in this patch. I preferred the old approach:
if (mt)
wacom_wac_finger_mt_slot()
else
wacom_wac_finger_touch()
i.e. one function per case.
Quoted 22 lines
drivers/hid/wacom_wac.c +18
-0+int slot; + + slot =input_mt_get_slot_by_key (input, hid_data->id); +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox); + } +else { +input_report_key (input,BTN_TOUCH , prox); + } + +if (prox) { +input_report_abs (input, mt ?ABS_MT_POSITION_X :ABS_X , + hid_data->x); +input_report_abs (input, mt ?ABS_MT_POSITION_Y :ABS_Y , + hid_data->y); + } +} +static int wacom_wac_finger_event (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage,__s32 value) {
drivers/hid/wacom_wac.c static int wacom_wac_finger_event(struct hid_device *hdev, · line 1469
Quoted 5 lines
drivers/hid/wacom_wac.c +2
-0@@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev, } +if (usage->usage_index +1 == field->report_count) { +if (usage->hid == wacom_wac->features.last_slot_field)
OK, I think it would be fair to assume that the FW guys will send N full touches by report. IIRC, I had the case where sometimes they add some random axis in the end (like X or Y), just to make me cry :) should be good here though (and we can have access to the FW guys if they do something too much funny :-P )
drivers/hid/wacom_wac.c static int wacom_wac_finger_event(struct hid_device *hdev, · line 1476
Quoted 9 lines
drivers/hid/wacom_wac.c +4
-2+wacom_wac_finger_slot (wacom_wac, wacom_wac->input); + } +return 0 ; } -static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_touches (struct hid_device *hdev)
It took me a while to understand what this function was. It simply returns the number of touches that has been recorded in the report (I was mislead by the git diff I think).
We should rename this in something else (wacom_wac_finger_get_touches_count or something better - I am bad at naming things).
Second question, why the need to return the touch count?
Quoted 44 lines
drivers/hid/wacom_wac.c +15
-20{ -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; - slot =input_mt_get_slot_by_key (input, hid_data->id); +if (touch_max ==1 ) +return wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity; -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_mt_sync_frame (input); -} - -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); - } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,
drivers/hid/wacom_wac.c static void wacom_wac_finger_report(struct hid_device *hdev, · line 1506
Quoted 10 lines
drivers/hid/wacom_wac.c +1
-5@@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity; -unsigned touch_max = wacom_wac->features.touch_max; -if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); +if (wacom_wac->features.touch_max >1 )
I think removing the touch_max variable hides a little bit the changes too.
drivers/hid/wacom_wac.c static void wacom_wac_finger_report(struct hid_device *hdev, · line 1509
Quoted 4 lines
drivers/hid/wacom_wac.c +2
-1+input_mt_sync_frame (input); else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +wacom_wac_finger_slot (wacom_wac ,input );
Not sure there is a need to call wacom_wac_finger_slot() for single touch devices. Your current implementation in wacom_finger_event should match also single finger tablets.
Quoted 24 lines
drivers/hid/wacom_wac.c +2
-1+input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \drivers/hid/wacom_wac.h +1
-1@@ -144,6 +144,7 @@ struct wacom_features {int pktlen;bool check_for_hid_type;int hid_type; +int last_slot_field; };struct wacom_shared { --2.1.3
Re: [PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices
Quoted 11 lines · Benjamin Tissoires
Quoted 9 lines · Jason Gerecke
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from
hid-multitouch.cto discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.Signed-off-by Jason Gerecke <[email protected]>---
Quoted 9 lines
Thanks for the v2 Jason.
I think we are closes to it. I have some comments on the style, but now that I have read it more carefully, I have a better idea of it, and the patch is not so scary.
Jiri. If you reviewed it and took it, that's fine. If not, then I think there is still room for improvements.
I'm really hoping to get this in for 3.19 if its possible, so I'm going to try to get this turned-around today...
Quoted 26 lines
Cheers,
BenjaminQuoted 14 lines
Changes from v1:
* Patch renamed from "HID: wacom: Report input events immediately upon receipt"
* Instead of reporting on-reciept, events are bundled and sent at the end of each contact
* Adds code to discover 'last_slot_field' based onhid-multitouch.cimplementationdrivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++----------------- drivers/hid/wacom_wac.h | 1 + 2 files changed, 63 insertions(+), 32 deletions(-)drivers/hid/wacom_wac.c +0
-0drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1384
Quoted 4 lines
drivers/hid/wacom_wac.c +0
-1@@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; -unsigned touch_max = wacom_wac->features.touch_max;not sure we really need to remove the temporary variable
touch_maxhere.Quoted 2 lines
drivers/hid/wacom_wac.c +1
-0+struct wacom_features *features = &wacom_wac->features;
Quoted 9 lines
drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1389
Quoted 5 lines
drivers/hid/wacom_wac.c +2
-1switch (usage->hid) {case HID_GD_X : -if (touch_max ==1 ) + features->last_slot_field = usage->hid; +if (features->touch_max ==1 )these changes (touchmax =>
features->touch_max) hinder a little bit the actual change, that is worrisome :)
ACK.
Quoted 47 lines
Quoted 29 lines
drivers/hid/wacom_wac.c +6
-1wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); else wacom_map_usage(wacom, usage, field, EV_ABS, ABS_MT_POSITION_X, 4); break; case HID_GD_Y: - if (touch_max == 1) + features->last_slot_field = usage->hid; + if (features->touch_max == 1) wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4); else wacom_map_usage(wacom, usage, field, EV_ABS, ABS_MT_POSITION_Y, 4); break; case HID_DG_CONTACTID: + features->last_slot_field = usage->hid; break; case HID_DG_INRANGE: + features->last_slot_field = usage->hid; break; case HID_DG_INVERT: + features->last_slot_field = usage->hid; break; case HID_DG_TIPSWITCH: + features->last_slot_field = usage->hid; wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0); break; } }drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1427
Quoted 10 lines
drivers/hid/wacom_wac.c +9
-0+static void wacom_wac_finger_slot (struct wacom_wac *wacom_wac, +struct input_dev *input) +{ +struct hid_data *hid_data = &wacom_wac->hid_data; +bool mt = wacom_wac->features.touch_max >1 ; +bool prox = hid_data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; + +if (mt) {Personal taste. I don't like the "if (mt)" approach in this patch. I preferred the old approach:
if (mt)
wacom_wac_finger_mt_slot()
else
wacom_wac_finger_touch()
Quoted 2 lines
i.e. one function per case.
I don't really like the idea of breaking this into two functions. The differences between how a each MT contact is reported and how the sole ST contact is reported are trivial, and I feel that breaking them apart leads to unnecessary duplication. I'll split it out if you feel that is warranted though.
Quoted 25 lines
Quoted 25 lines
drivers/hid/wacom_wac.c +18
-0+int slot; + + slot =input_mt_get_slot_by_key (input, hid_data->id); +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox); + } +else { +input_report_key (input,BTN_TOUCH , prox); + } + +if (prox) { +input_report_abs (input, mt ?ABS_MT_POSITION_X :ABS_X , + hid_data->x); +input_report_abs (input, mt ?ABS_MT_POSITION_Y :ABS_Y , + hid_data->y); + } +} +static int wacom_wac_finger_event (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage,__s32 value) {@@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev, }
Quoted 9 lines
drivers/hid/wacom_wac.c static int wacom_wac_finger_event(struct hid_device *hdev, · line 1469
Quoted 2 lines
drivers/hid/wacom_wac.c +2
-0+if (usage->usage_index +1 == field->report_count) { +if (usage->hid == wacom_wac->features.last_slot_field)OK, I think it would be fair to assume that the FW guys will send N full touches by report. IIRC, I had the case where sometimes they add some random axis in the end (like X or Y), just to make me cry :) should be good here though (and we can have access to the FW guys if they do something too much funny :-P )
That explains why the hid-multitouch code seemed so much more convoluted than necessary :) Our FW team hasn't made any descriptors which are that pathological, and I certainly hope they don't start.
Quoted 6 lines
Quoted 6 lines
drivers/hid/wacom_wac.c +3
-0+wacom_wac_finger_slot (wacom_wac, wacom_wac->input); + } +return 0 ; }
Quoted 7 lines
drivers/hid/wacom_wac.c static int wacom_wac_finger_event(struct hid_device *hdev, · line 1476
Quoted 3 lines
drivers/hid/wacom_wac.c +1
-2-static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_touches (struct hid_device *hdev)It took me a while to understand what this function was. It simply returns the number of touches that has been recorded in the report (I was mislead by the git diff I think).
Almost correct -- it returns the number of touches that are currently active (which may be larger than the number of touches recorded in the report).
Quoted 4 lines
We should rename this in something else (
wacom_wac_finger_get_touches_countor something better - I am bad at naming things).
How about 'wacom_wac_finger_count_touches'?
Quoted 2 lines
Second question, why the need to return the touch count?
This function is used only to update the (boolean) value of 'wacom_wac->shared->touch_down'. Strictly speaking it could save a small amount of processing time and return a boolean directly, but it seemed trivial enough to just return the count in case it was useful in the future.
If the question was more of "why do we need to bother going through the slot state?" then the answer is that we can't rely on the information in any single report. Many of our MT devices use what Microsoft calls "hybrid mode", so the complete sensor state may span multiple reports. All the fingers in one report may go up even while other fingers remain on the tablet.
Quoted 57 lines
Quoted 44 lines
drivers/hid/wacom_wac.c +15
-20{ -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; - slot =input_mt_get_slot_by_key (input, hid_data->id); +if (touch_max ==1 ) +return wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity; -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_mt_sync_frame (input); -} - -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); - } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,drivers/hid/wacom_wac.c static void wacom_wac_finger_report(struct hid_device *hdev, · line 1506
Quoted 10 lines
drivers/hid/wacom_wac.c +1
-5@@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity; -unsigned touch_max = wacom_wac->features.touch_max; -if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); +if (wacom_wac->features.touch_max >1 )I think removing the
touch_maxvariable hides a little bit the changes too.
Quoted 9 lines
drivers/hid/wacom_wac.c static void wacom_wac_finger_report(struct hid_device *hdev, · line 1509
Quoted 4 lines
drivers/hid/wacom_wac.c +2
-1+input_mt_sync_frame (input); else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +wacom_wac_finger_slot (wacom_wac ,input );Not sure there is a need to call
wacom_wac_finger_slot()for single touch devices. Your current implementation inwacom_finger_eventshould match also single finger tablets.
I'll double-check to make sure. I hadn't considered that even the ST case might match in 'wacom_finger_event'.
Quoted 24 lines
Quoted 24 lines
drivers/hid/wacom_wac.c +2
-1+input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \drivers/hid/wacom_wac.h +1
-1@@ -144,6 +144,7 @@ struct wacom_features {int pktlen;bool check_for_hid_type;int hid_type; +int last_slot_field; };struct wacom_shared { --2.1.3
Re: [PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices
Quoted 110 lines · Jason Gerecke
Quoted 11 lines · Benjamin Tissoires
Quoted 9 lines · Jason Gerecke
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from
hid-multitouch.cto discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.Signed-off-by Jason Gerecke <[email protected]>---
Quoted 9 lines
Thanks for the v2 Jason.
I think we are closes to it. I have some comments on the style, but now that I have read it more carefully, I have a better idea of it, and the patch is not so scary.
Jiri. If you reviewed it and took it, that's fine. If not, then I think there is still room for improvements.
I'm really hoping to get this in for 3.19 if its possible, so I'm going to try to get this turned-around today...
Quoted 26 lines
Cheers,
BenjaminQuoted 14 lines
Changes from v1:
* Patch renamed from "HID: wacom: Report input events immediately upon receipt"
* Instead of reporting on-reciept, events are bundled and sent at the end of each contact
* Adds code to discover 'last_slot_field' based onhid-multitouch.cimplementationdrivers/hid/wacom_wac.c | 94 ++++++++++++++++++++++++++++++++----------------- drivers/hid/wacom_wac.h | 1 + 2 files changed, 63 insertions(+), 32 deletions(-)drivers/hid/wacom_wac.c +0
-0drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1384
Quoted 4 lines
drivers/hid/wacom_wac.c +0
-1@@ -1381,35 +1381,68 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; -unsigned touch_max = wacom_wac->features.touch_max;not sure we really need to remove the temporary variable
touch_maxhere.Quoted 2 lines
drivers/hid/wacom_wac.c +1
-0+struct wacom_features *features = &wacom_wac->features;Quoted 9 lines
drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1389
Quoted 5 lines
drivers/hid/wacom_wac.c +2
-1switch (usage->hid) {case HID_GD_X : -if (touch_max ==1 ) + features->last_slot_field = usage->hid; +if (features->touch_max ==1 )these changes (touchmax =>
features->touch_max) hinder a little bit the actual change, that is worrisome :)ACK.
Quoted 47 lines
Quoted 29 lines
drivers/hid/wacom_wac.c +6
-1wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 ); else wacom_map_usage(wacom, usage, field, EV_ABS, ABS_MT_POSITION_X, 4); break; case HID_GD_Y: - if (touch_max == 1) + features->last_slot_field = usage->hid; + if (features->touch_max == 1) wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4); else wacom_map_usage(wacom, usage, field, EV_ABS, ABS_MT_POSITION_Y, 4); break; case HID_DG_CONTACTID: + features->last_slot_field = usage->hid; break; case HID_DG_INRANGE: + features->last_slot_field = usage->hid; break; case HID_DG_INVERT: + features->last_slot_field = usage->hid; break; case HID_DG_TIPSWITCH: + features->last_slot_field = usage->hid; wacom_map_usage(wacom, usage, field, EV_KEY, BTN_TOUCH, 0); break; } }drivers/hid/wacom_wac.c static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, · line 1427
Quoted 10 lines
drivers/hid/wacom_wac.c +9
-0+static void wacom_wac_finger_slot (struct wacom_wac *wacom_wac, +struct input_dev *input) +{ +struct hid_data *hid_data = &wacom_wac->hid_data; +bool mt = wacom_wac->features.touch_max >1 ; +bool prox = hid_data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; + +if (mt) {Personal taste. I don't like the "if (mt)" approach in this patch. I preferred the old approach:
if (mt)
wacom_wac_finger_mt_slot()
else
wacom_wac_finger_touch()
Quoted 7 lines
Quoted 2 lines
i.e. one function per case.
I don't really like the idea of breaking this into two functions. The differences between how a each MT contact is reported and how the sole ST contact is reported are trivial, and I feel that breaking them apart leads to unnecessary duplication. I'll split it out if you feel that is warranted though.
No, do whatever you like. It was more a comment I added before reading the entire patch, so after having the global view, it's fine this way too.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
drivers/hid/wacom_wac.c +18
-0+int slot; + + slot =input_mt_get_slot_by_key (input, hid_data->id); +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox); + } +else { +input_report_key (input,BTN_TOUCH , prox); + } + +if (prox) { +input_report_abs (input, mt ?ABS_MT_POSITION_X :ABS_X , + hid_data->x); +input_report_abs (input, mt ?ABS_MT_POSITION_Y :ABS_Y , + hid_data->y); + } +} +static int wacom_wac_finger_event (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage,__s32 value) {@@ -1432,36 +1465,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev, }Quoted 9 lines
drivers/hid/wacom_wac.c static int wacom_wac_finger_event(struct hid_device *hdev, · line 1469
Quoted 2 lines
drivers/hid/wacom_wac.c +2
-0+if (usage->usage_index +1 == field->report_count) { +if (usage->hid == wacom_wac->features.last_slot_field)OK, I think it would be fair to assume that the FW guys will send N full touches by report. IIRC, I had the case where sometimes they add some random axis in the end (like X or Y), just to make me cry :) should be good here though (and we can have access to the FW guys if they do something too much funny :-P )
Quoted 3 lines
That explains why the hid-multitouch code seemed so much more convoluted than necessary :) Our FW team hasn't made any descriptors which are that pathological, and I certainly hope they don't start.
Actually, I may not be fair with the FW guys. I think the problem we had was that while we were parsing the multitouch report and then the mouse emulation report, we ended up in having ABS_Y as the last slot field. Either we (I) were (was) not smart enough to check on the type of the report, or either it was difficult to detect in input_mapping which collection was the mouse, and which was the touch (and this is when the FW guys go nuts and do anything in the report descriptor :-P )
Quoted 14 lines
Quoted 7 lines
Quoted 7 lines
drivers/hid/wacom_wac.c +3
-0+wacom_wac_finger_slot (wacom_wac, wacom_wac->input); + } +return 0 ; }Quoted 7 lines
drivers/hid/wacom_wac.c static int wacom_wac_finger_event(struct hid_device *hdev, · line 1476
Quoted 3 lines
drivers/hid/wacom_wac.c +1
-2-static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_touches (struct hid_device *hdev)It took me a while to understand what this function was. It simply returns the number of touches that has been recorded in the report (I was mislead by the git diff I think).
Quoted 3 lines
Almost correct -- it returns the number of touches that are currently active (which may be larger than the number of touches recorded in the report).
Yeah, that's what I meant, you understood me :)
Quoted 6 lines
Quoted 5 lines
We should rename this in something else (
wacom_wac_finger_get_touches_countor something better - I am bad at naming things).How about '
wacom_wac_finger_count_touches'?
works for me.
Quoted 9 lines
Quoted 3 lines
Second question, why the need to return the touch count?
This function is used only to update the (boolean) value of '
wacom_wac->shared->touch_down'. Strictly speaking it could save a small amount of processing time and return a boolean directly, but it seemed trivial enough to just return the count in case it was useful in the future.
Quoted 6 lines
If the question was more of "why do we need to bother going through the slot state?" then the answer is that we can't rely on the information in any single report. Many of our MT devices use what Microsoft calls "hybrid mode", so the complete sensor state may span multiple reports. All the fingers in one report may go up even while other fingers remain on the tablet.
I like when you are able to decrypt my mind :)
ok, fine.
We could still abort earlier in the function to report a boolean, but the finger count is fine too.
Quoted 63 lines
Quoted 63 lines
Quoted 45 lines
drivers/hid/wacom_wac.c +15
-20{ -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; - slot =input_mt_get_slot_by_key (input, hid_data->id); +if (touch_max ==1 ) +return wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity; -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_mt_sync_frame (input); -} - -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); - } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,drivers/hid/wacom_wac.c static void wacom_wac_finger_report(struct hid_device *hdev, · line 1506
Quoted 10 lines
drivers/hid/wacom_wac.c +1
-5@@ -1470,18 +1502,16 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity; -unsigned touch_max = wacom_wac->features.touch_max; -if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); +if (wacom_wac->features.touch_max >1 )I think removing the
touch_maxvariable hides a little bit the changes too.Quoted 4 lines
drivers/hid/wacom_wac.c +2
-1+input_mt_sync_frame (input); else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +wacom_wac_finger_slot (wacom_wac ,input );
Quoted 6 lines
Quoted 4 lines
Not sure there is a need to call
wacom_wac_finger_slot()for single touch devices. Your current implementation inwacom_finger_eventshould match also single finger tablets.I'll double-check to make sure. I hadn't considered that even the ST case might match in '
wacom_finger_event'.
thanks
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
drivers/hid/wacom_wac.c +2
-1+input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \drivers/hid/wacom_wac.h +1
-0@@ -144,6 +144,7 @@ struct wacom_features {int pktlen;bool check_for_hid_type;int hid_type; +int last_slot_field; };
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/hid/wacom_wac.h +0
-1struct wacom_shared { --2.1.3
Cheers,
Benjamin
PATCH v3 3/3 HID: wacom: Report input events for each finger on generic devices
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from hid-multitouch.c to discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.
---
Changes from v2:
* No longer removing 'touch_max' variables to make changes clearer
* Renamed 'wacom_wac_finger_touches' to 'wacom_wac_finger_count_touches'
* Removed call to 'wacom_wac_finger_slot' in 'wacom_wac_finger_report' since it is already called for the single-touch case within 'wacom_wac_finger_event'.
drivers/hid/wacom_wac.c | 86 +++++++++++++++++++++++++++++++++---------------- drivers/hid/wacom_wac.h | 1 + 2 files changed, 59 insertions(+), 28 deletions(-)
drivers/hid/wacom_wac.c +58 -28
@@ -1381,10 +1381,12 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct wacom_features *features = &wacom_wac->features;unsigned touch_max = wacom_wac->features.touch_max;switch (usage->hid) {case HID_GD_X : + features->last_slot_field = usage->hid;if (touch_max ==1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 );else
@@ -1392,6 +1394,7 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, ABS_MT_POSITION_X, 4);break ;case HID_GD_Y : + features->last_slot_field = usage->hid;if (touch_max ==1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 );else
@@ -1399,17 +1402,48 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, ABS_MT_POSITION_Y, 4);break ;case HID_DG_CONTACTID : + features->last_slot_field = usage->hid;break ;case HID_DG_INRANGE : + features->last_slot_field = usage->hid;break ;case HID_DG_INVERT : + features->last_slot_field = usage->hid;break ;case HID_DG_TIPSWITCH : + features->last_slot_field = usage->hid;wacom_map_usage (wacom, usage, field,EV_KEY ,BTN_TOUCH ,0 );break ; } } +static void wacom_wac_finger_slot (struct wacom_wac *wacom_wac, +struct input_dev *input) +{ +struct hid_data *hid_data = &wacom_wac->hid_data; +bool mt = wacom_wac->features.touch_max >1 ; +bool prox = hid_data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; + +if (mt) { +int slot; + + slot =input_mt_get_slot_by_key (input, hid_data->id); +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox); + } +else { +input_report_key (input,BTN_TOUCH , prox); + } + +if (prox) { +input_report_abs (input, mt ?ABS_MT_POSITION_X :ABS_X , + hid_data->x); +input_report_abs (input, mt ?ABS_MT_POSITION_Y :ABS_Y , + hid_data->y); + } +} +static int wacom_wac_finger_event (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage,__s32 value) {
@@ -1432,36 +1466,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev, } +if (usage->usage_index +1 == field->report_count) { +if (usage->hid == wacom_wac->features.last_slot_field) +wacom_wac_finger_slot (wacom_wac, wacom_wac->input); + } +return 0 ; } -static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_count_touches (struct hid_device *hdev) { -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; - slot =input_mt_get_slot_by_key (input, hid_data->id); +if (touch_max ==1 ) +return wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity; -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_mt_sync_frame (input); -} - -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); - } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,
@@ -1470,18 +1503,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity;unsigned touch_max = wacom_wac->features.touch_max;if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); -else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +input_mt_sync_frame (input); +input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_count_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \
drivers/hid/wacom_wac.h +1 -0
@@ -145,6 +145,7 @@ struct wacom_features {int pktlen;bool check_for_hid_type;int hid_type; +int last_slot_field; };struct wacom_shared {
Signature · 1 line
2.1.3
Re: [PATCH v3 3/3] HID: wacom: Report input events for each finger on generic devices
Quoted 9 lines · Jason Gerecke
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from
hid-multitouch.cto discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.Signed-off-by Jason Gerecke <[email protected]>---
Fine by me. Thanks for the quick respin Jason.
Cheers,
Benjamin
Quoted 180 lines
Changes from v2:
* No longer removing 'touch_max' variables to make changes clearer
* Renamed 'wacom_wac_finger_touches' to 'wacom_wac_finger_count_touches'
* Removed call to 'wacom_wac_finger_slot' in 'wacom_wac_finger_report' since it is already called for the single-touch case within 'wacom_wac_finger_event'.drivers/hid/wacom_wac.c | 86 +++++++++++++++++++++++++++++++++---------------- drivers/hid/wacom_wac.h | 1 + 2 files changed, 59 insertions(+), 28 deletions(-)drivers/hid/wacom_wac.c +58
-28@@ -1381,10 +1381,12 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct wacom_features *features = &wacom_wac->features;unsigned touch_max = wacom_wac->features.touch_max;switch (usage->hid) {case HID_GD_X : + features->last_slot_field = usage->hid;if (touch_max ==1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_X ,4 );else @@ -1392,6 +1394,7 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, ABS_MT_POSITION_X, 4);break ;case HID_GD_Y : + features->last_slot_field = usage->hid;if (touch_max ==1 )wacom_map_usage (wacom, usage, field,EV_ABS ,ABS_Y ,4 );else @@ -1399,17 +1402,48 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, ABS_MT_POSITION_Y, 4);break ;case HID_DG_CONTACTID : + features->last_slot_field = usage->hid;break ;case HID_DG_INRANGE : + features->last_slot_field = usage->hid;break ;case HID_DG_INVERT : + features->last_slot_field = usage->hid;break ;case HID_DG_TIPSWITCH : + features->last_slot_field = usage->hid;wacom_map_usage (wacom, usage, field,EV_KEY ,BTN_TOUCH ,0 );break ; } } +static void wacom_wac_finger_slot (struct wacom_wac *wacom_wac, +struct input_dev *input) +{ +struct hid_data *hid_data = &wacom_wac->hid_data; +bool mt = wacom_wac->features.touch_max >1 ; +bool prox = hid_data->tipswitch && + !wacom_wac->shared->stylus_in_proximity; + +if (mt) { +int slot; + + slot =input_mt_get_slot_by_key (input, hid_data->id); +input_mt_slot (input, slot); +input_mt_report_slot_state (input,MT_TOOL_FINGER , prox); + } +else { +input_report_key (input,BTN_TOUCH , prox); + } + +if (prox) { +input_report_abs (input, mt ?ABS_MT_POSITION_X :ABS_X , + hid_data->x); +input_report_abs (input, mt ?ABS_MT_POSITION_Y :ABS_Y , + hid_data->y); + } +} +static int wacom_wac_finger_event (struct hid_device *hdev,struct hid_field *field,struct hid_usage *usage,__s32 value) {@@ -1432,36 +1466,35 @@ static int wacom_wac_finger_event(struct hid_device *hdev, } +if (usage->usage_index +1 == field->report_count) { +if (usage->hid == wacom_wac->features.last_slot_field) +wacom_wac_finger_slot (wacom_wac, wacom_wac->input); + } +return 0 ; } -static void wacom_wac_finger_mt_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) +static int wacom_wac_finger_count_touches (struct hid_device *hdev) { -int slot; -struct hid_data *hid_data = &wacom_wac->hid_data; +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; +struct input_dev *input = wacom_wac->input; +unsigned touch_max = wacom_wac->features.touch_max; +int count =0 ; +int i; - slot =input_mt_get_slot_by_key (input, hid_data->id); +if (touch_max ==1 ) +return wacom_wac->hid_data.tipswitch && + !wacom_wac->shared->stylus_in_proximity; -input_mt_slot (input, slot); -input_mt_report_slot_state (input,MT_TOOL_FINGER , touch); -if (touch) { -input_report_abs (input,ABS_MT_POSITION_X , hid_data->x); -input_report_abs (input,ABS_MT_POSITION_Y , hid_data->y); +for (i =0 ; i < input->mt->num_slots; i++) { +struct input_mt_slot *ps = &input->mt->slots[i]; +int id =input_mt_get_value (ps,ABS_MT_TRACKING_ID ); +if (id >=0 ) + count++; } -input_mt_sync_frame (input); -} - -static void wacom_wac_finger_single_touch_report (struct wacom_wac *wacom_wac, -struct input_dev *input,bool touch) -{ -struct hid_data *hid_data = &wacom_wac->hid_data; -if (touch) { -input_report_abs (input,ABS_X , hid_data->x); -input_report_abs (input,ABS_Y , hid_data->y); - } -input_report_key (input,BTN_TOUCH , touch); +return count; } static void wacom_wac_finger_report(struct hid_device *hdev,@@ -1470,18 +1503,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac;struct input_dev *input = wacom_wac->input; -bool touch = wacom_wac->hid_data.tipswitch && - !wacom_wac->shared->stylus_in_proximity;unsigned touch_max = wacom_wac->features.touch_max;if (touch_max >1 ) -wacom_wac_finger_mt_report (wacom_wac, input, touch); -else -wacom_wac_finger_single_touch_report (wacom_wac, input, touch); +input_mt_sync_frame (input); +input_sync (input);/* keep touch state for pen event */ - wacom_wac->shared->touch_down = touch; + wacom_wac->shared->touch_down =wacom_wac_finger_count_touches (hdev); }#define WACOM_PEN_FIELD (f) (((f)->logical == HID_DG_STYLUS) || \drivers/hid/wacom_wac.h +1
-1@@ -145,6 +145,7 @@ struct wacom_features {int pktlen;bool check_for_hid_type;int hid_type; +int last_slot_field; };struct wacom_shared { --2.1.3
Jiri Kosina
AppliedRe: [PATCH v3 3/3] HID: wacom: Report input events for each finger on generic devices
Quoted 8 lines · Benjamin Tissoires
Quoted 7 lines · Jason Gerecke
The existing generic touch code only reports events after reading an entire HID report, which practically means that only data about the last contact in a report will ever be provided to userspace. This patch uses a trick from
hid-multitouch.cto discover what type of field is at the end of each contact; when such a field is encountered all the stored contact data will be reported.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>---
Fine by me. Thanks for the quick respin Jason.
Reviewed-by Benjamin Tissoires <[email protected]>
Applied, thanks to both of you.
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 8 lines · Jason Gerecke
If a HID descriptor places
HID_DG_CONTACTIDbeforeHID_DG_XandHID_DG_Ythen theABS_XandABS_Ywill not be automatically initialized by the call toinput_mt_init_slots. To ensure that this is not a problem, we relocate that call to occur after HID parsing has been completed and we've initalized all the multitouch axes.Signed-off-by Jason Gerecke <[email protected]>---
This one is
Thanks for the v2 Jason!
Cheers,
Benjamin
Quoted 75 lines
Changes from v1:
* Patch renamed from "HID: wacom: Manually declare ABS_{X,Y} for pointer emulation"
* Patch moves call toinput_mt_init_slotsrather than manually declare ABS_{X,Y}drivers/hid/wacom_sys.c | 18 ++++++++++++++++++ drivers/hid/wacom_wac.c | 3 --- 2 files changed, 18 insertions(+), 3 deletions(-)drivers/hid/wacom_sys.c +18
-0@@ -13,6 +13,7 @@#include "wacom_wac.h" #include "wacom.h" +#include <linux/input/mt.h> #define WAC_MSG_RETRIES 5@@ -236,6 +237,21 @@ static void wacom_usage_mapping(struct hid_device *hdev,wacom_wac_usage_mapping (hdev, field, usage); } +static void wacom_post_parse_hid (struct hid_device *hdev, +struct wacom_features *features) +{ +struct wacom *wacom =hid_get_drvdata (hdev); +struct wacom_wac *wacom_wac = &wacom->wacom_wac; + +if (features->type ==HID_GENERIC ) { +/* Any last-minute generic device setup */ +if (features->touch_max >1 ) { +input_mt_init_slots (wacom_wac->input, wacom_wac->features.touch_max, +INPUT_MT_DIRECT ); + } + } +} +static void wacom_parse_hid (struct hid_device *hdev,struct wacom_features *features) {@@ -270,6 +286,8 @@ static void wacom_parse_hid(struct hid_device *hdev,wacom_usage_mapping (hdev, hreport->field[i], hreport->field[i]->usage + j); } + +wacom_post_parse_hid (hdev, features); }static int wacom_hid_set_device_mode (struct hid_device *hdev)drivers/hid/wacom_wac.c +0
-4@@ -1381,7 +1381,6 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, {struct wacom *wacom =hid_get_drvdata (hdev);struct wacom_wac *wacom_wac = &wacom->wacom_wac; -struct input_dev *input = wacom_wac->input;unsigned touch_max = wacom_wac->features.touch_max;switch (usage->hid) {@@ -1400,8 +1399,6 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev, ABS_MT_POSITION_Y, 4);break ;case HID_DG_CONTACTID : -input_mt_init_slots (input, wacom_wac->features.touch_max, -INPUT_MT_DIRECT );break ;case HID_DG_INRANGE :break ; --2.1.3
Quoted 11 lines · Benjamin Tissoires
Quoted 8 lines
If a HID descriptor places
HID_DG_CONTACTIDbeforeHID_DG_XandHID_DG_Ythen theABS_XandABS_Ywill not be automatically initialized by the call toinput_mt_init_slots. To ensure that this is not a problem, we relocate that call to occur after HID parsing has been completed and we've initalized all the multitouch axes.Signed-off-by Jason Gerecke <[email protected]>---
This one is
Reviewed-by Benjamin Tissoires <[email protected]>
Benjamin, does your Reviewed-by: apply to both 2/3 and 3/3?
Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 8 lines · Jiri Kosina
Quoted 6 lines · Benjamin Tissoires
Quoted 6 lines
If a HID descriptor places
HID_DG_CONTACTIDbeforeHID_DG_XandHID_DG_Ythen theABS_XandABS_Ywill not be automatically initialized by the call toinput_mt_init_slots. To ensure that this is not a problem, we relocate that call to occur after HID parsing has been completed and we've initalized all the multitouch axes.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>---
This one is
Reviewed-by Benjamin Tissoires <[email protected]>Benjamin, does your Reviewed-by: apply to both 2/3 and 3/3?
Hi Jiri,
Well, I wanted to make some cosmetic changes in 3/3 and double check that it was generic enough, but I've been sidetracked, and today I have to keep my sick daughter.
Please apply my rev-by on this one only. Feel free to apply 3/3 however - if you can dedicate a little bit of time to check it - (without my rev-by then), it's not that it will prevent us to change it later if we have to.
Cheers,
Benjamin
Quoted 5 lines
Thanks,
--
Jiri Kosina
SUSE Labs
Quoted 6 lines · Benjamin Tissoires
Well, I wanted to make some cosmetic changes in 3/3 and double check that it was generic enough, but I've been sidetracked, and today I have to keep my sick daughter. Please apply my rev-by on this one only. Feel free to apply 3/3 however - if you can dedicate a little bit of time to check it - (without my rev-by then), it's not that it will prevent us to change it later if we have to.
Alright, thanks. I am now applying 2/3. If I have time to review 3/3 today or tomorrow (not guaranteed), I might be applying it for this merge window as well.
Otherwise it'll be processed in the next cycle.
Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs