[PATCH v2 2/3] HID: wacom: Initialize MT slots for generic devices at post_parse_hid

[PATCH v2 2/3] HID: wacom: Initialize MT slots for generic devices at post_parse_hid

[Jason Gerecke]

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.

Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
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(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index eb55316..872aa0b 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -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)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index a8a19a5..c33f889 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -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

[PATCH v2 3/3] HID: wacom: Report input events for each finger on generic devices

[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.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.

Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
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(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index c33f889..75fc16e 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -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) || \
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 128cbb3..6e256206 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -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

[Benjamin Tissoires]

On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> 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.
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---

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

> 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(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index c33f889..75fc16e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -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.

> +       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 :)

>                         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) {

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.

> +               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)

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 )

> +                       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?

>  {
> -       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)

I think removing the touch_max variable hides a little bit the changes too.

> +               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.

> +
>         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) || \
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 128cbb3..6e256206 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -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

[Jason Gerecke]

On Wed, Dec 10, 2014 at 12:25 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
>> ---
>
> 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...

> Cheers,
> Benjamin
>
>> 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(-)
>>
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index c33f889..75fc16e 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -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.
>
>> +       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 :)
>
ACK.

>>                         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) {
>
> 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.
>
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.

>> +               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)
>
> 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.

>> +                       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).
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).

> We should rename this in something else
> (wacom_wac_finger_get_touches_count or something better - I am bad at
> naming things).
>
How about 'wacom_wac_finger_count_touches'?

> 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.

>>  {
>> -       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)
>
> I think removing the touch_max variable hides a little bit the changes too.
>
>> +               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.
>
I'll double-check to make sure. I hadn't considered that even the ST
case might match in 'wacom_finger_event'.

>> +
>>         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) || \
>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>> index 128cbb3..6e256206 100644
>> --- a/drivers/hid/wacom_wac.h
>> +++ b/drivers/hid/wacom_wac.h
>> @@ -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

[Benjamin Tissoires]

On Wed, Dec 10, 2014 at 6:01 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> On Wed, Dec 10, 2014 at 12:25 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>>> 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.
>>>
>>> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
>>> ---
>>
>> 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...
>
>> Cheers,
>> Benjamin
>>
>>> 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(-)
>>>
>>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>>> index c33f889..75fc16e 100644
>>> --- a/drivers/hid/wacom_wac.c
>>> +++ b/drivers/hid/wacom_wac.c
>>> @@ -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.
>>
>>> +       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 :)
>>
> ACK.
>
>>>                         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) {
>>
>> 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.
>>
> 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.

>
>>> +               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)
>>
>> 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.

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
)

>
>>> +                       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).
> 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 :)

>
>> We should rename this in something else
>> (wacom_wac_finger_get_touches_count or something better - I am bad at
>> naming things).
>>
> How about 'wacom_wac_finger_count_touches'?

works for me.

>
>> 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.

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.

>
>>>  {
>>> -       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)
>>
>> I think removing the touch_max variable hides a little bit the changes too.
>>
>>> +               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.
>>
> I'll double-check to make sure. I hadn't considered that even the ST
> case might match in 'wacom_finger_event'.

thanks

>
>>> +
>>>         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) || \
>>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>>> index 128cbb3..6e256206 100644
>>> --- a/drivers/hid/wacom_wac.h
>>> +++ b/drivers/hid/wacom_wac.h
>>> @@ -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
>>>

Cheers,
Benjamin

[PATCH v3 3/3] HID: wacom: Report input events for each finger on generic devices

[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.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.

Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
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(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 064fd6c..26f27bd 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -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) || \
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 5384043..bfad815 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -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

Re: [PATCH v3 3/3] HID: wacom: Report input events for each finger on generic devices

[Benjamin Tissoires]

On Wed, Dec 10, 2014 at 7:26 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> 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.
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---

Fine by me. Thanks for the quick respin Jason.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 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(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 064fd6c..26f27bd 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -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) || \
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 5384043..bfad815 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -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
>

Re: [PATCH v3 3/3] HID: wacom: Report input events for each finger on generic devices

[Jiri Kosina]

On Thu, 11 Dec 2014, Benjamin Tissoires wrote:

> On Wed, Dec 10, 2014 at 7:26 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> > 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.
> >
> > Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> > ---
> 
> Fine by me. Thanks for the quick respin Jason.
> 
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Applied, thanks to both of you.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/3] HID: wacom: Initialize MT slots for generic devices at post_parse_hid

[Benjamin Tissoires]

On Fri, Dec 5, 2014 at 4:37 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> 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.
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---

This one is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks for the v2 Jason!

Cheers,
Benjamin

> 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(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index eb55316..872aa0b 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -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)
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index a8a19a5..c33f889 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -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
>

Re: [PATCH v2 2/3] HID: wacom: Initialize MT slots for generic devices at post_parse_hid

[Jiri Kosina]

On Fri, 5 Dec 2014, Benjamin Tissoires wrote:

> > 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.
> >
> > Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> > ---
> 
> This one is
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Benjamin, does your Reviewed-by: apply to both 2/3 and 3/3?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/3] HID: wacom: Initialize MT slots for generic devices at post_parse_hid

[Benjamin Tissoires]

On Tue, Dec 9, 2014 at 3:00 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 5 Dec 2014, Benjamin Tissoires wrote:
>
>> > 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.
>> >
>> > Signed-off-by: Jason Gerecke <killertofu@gmail.com>
>> > ---
>>
>> This one is
>> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> 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

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

Re: [PATCH v2 2/3] HID: wacom: Initialize MT slots for generic devices at post_parse_hid

[Jiri Kosina]

On Tue, 9 Dec 2014, Benjamin Tissoires wrote:

> 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,

-- 
Jiri Kosina
SUSE Labs