Hi Henrik,

thanks for the review.
Some comments inlined:

On Sun, May 6, 2012 at 9:01 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

The previous implementation introduced a randomness in the splitting
of the different touches reported by the device. This version is more
robust as we don't rely on hi->input->absbit, but on our own structure.

This also prepares hid-multitouch to better support Win8 devices.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c |   58 +++++++++++++++++++++++++++++++++--------
 1 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 84bb32e..c6ffb05 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -70,9 +70,16 @@ struct mt_class {

      bool is_indirect;       /* true for touchpads */
 };

+struct mt_fields {
+     unsigned usages[HID_MAX_FIELDS];
+     unsigned int length;
+};
+
 struct mt_device {
      struct mt_slot curdata; /* placeholder of incoming data */
      struct mt_class mtclass;        /* our mt device class */
+     struct mt_fields *fields;       /* temporary placeholder for storing the
+                                        multitouch fields */

Why not skip the pointer here?

well, the idea was to keep the memory footprint low. As these values
are only needed at init, then I freed them once I finished using them.
I can of course skip the pointer, but in that case, wouldn't the
struct declaration be worthless?

quoted

      unsigned last_field_index;      /* last field index of the report */
      unsigned last_slot_field;       /* the last field of a slot */
      __s8 inputmode;         /* InputMode HID feature, -1 if non-existent */

@@ -275,11 +282,15 @@ static void set_abs(struct input_dev *input, unsigned int code,

      input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
 }

-static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
+static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
              struct hid_input *hi)

How about adding the last_field_index here also, using a function like

I'm not a big fan of this idea.
last_field_index represent the last mt field, was it local or global
(i.e. per touch or global to the report).
mt_store_field stores only the local (per-touch) information to be
able to divide the array by the number of touch. Adding the global
items there too will force us to introduce a switch to exclude global
items.

Cheers,
Benjamin

static void mt_store_field(struct mt_device *td, struct hid_input *hi,
                       struct hid_field *field, struct hid_usage *usage)

quoted

 {
-     if (!test_bit(usage->hid, hi->input->absbit))
-             td->last_slot_field = usage->hid;
+     struct mt_fields *f = td->fields;

And inserting td->last_field_index = field->index here.

quoted

+
+     if (f->length >= HID_MAX_FIELDS)
+             return;
+
+     f->usages[f->length++] = usage->hid;
 }

 static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

@@ -330,7 +341,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

                              cls->sn_move);
                      /* touchscreen emulation */
                      set_abs(hi->input, ABS_X, field, cls->sn_move);
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_GD_Y:

@@ -340,7 +351,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

                              cls->sn_move);
                      /* touchscreen emulation */
                      set_abs(hi->input, ABS_Y, field, cls->sn_move);
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              }

@@ -349,24 +360,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

      case HID_UP_DIGITIZER:
              switch (usage->hid) {
              case HID_DG_INRANGE:
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_DG_CONFIDENCE:
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_DG_TIPSWITCH:
                      hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
                      input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_DG_CONTACTID:
                      if (!td->maxcontacts)
                              td->maxcontacts = MT_DEFAULT_MAXCONTACT;
                      input_mt_init_slots(hi->input, td->maxcontacts);
-                     td->last_slot_field = usage->hid;
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      td->touches_by_report++;
                      return 1;

@@ -375,7 +386,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

                                      EV_ABS, ABS_MT_TOUCH_MAJOR);
                      set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
                              cls->sn_width);
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_DG_HEIGHT:

@@ -385,7 +396,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

                              cls->sn_height);
                      input_set_abs_params(hi->input,
                                      ABS_MT_ORIENTATION, 0, 1, 0, 0);
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_DG_TIPPRESSURE:

@@ -396,7 +407,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,

                      /* touchscreen emulation */
                      set_abs(hi->input, ABS_PRESSURE, field,
                              cls->sn_pressure);
-                     set_last_slot_field(usage, td, hi);
+                     mt_store_field(usage, td, hi);
                      td->last_field_index = field->index;
                      return 1;
              case HID_DG_CONTACTCOUNT:

@@ -650,6 +661,16 @@ static void mt_post_parse_default_settings(struct mt_device *td)

      td->mtclass.quirks = quirks;
 }

+static void mt_post_parse(struct mt_device *td)
+{
+     struct mt_fields *f = td->fields;
+
+     if (td->touches_by_report > 0) {
+             int field_count_per_touch = f->length / td->touches_by_report;
+             td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
+     }
+}
+
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
      int ret, i;

@@ -680,6 +701,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)

      td->maxcontact_report_id = -1;
      hid_set_drvdata(hdev, td);

+     td->fields = kzalloc(sizeof(struct mt_fields), GFP_KERNEL);
+     if (!td->fields) {
+             dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
+             ret = -ENOMEM;
+             goto fail;
+     }
+

This can be skipped.

quoted

      ret = hid_parse(hdev);
      if (ret != 0)
              goto fail;

@@ -688,6 +716,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)

      if (ret)
              goto fail;

+     mt_post_parse(td);
+
      if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
              mt_post_parse_default_settings(td);

@@ -705,9 +735,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)

      mt_set_maxcontacts(hdev);
      mt_set_input_mode(hdev);

+     kfree(td->fields);
+     td->fields = NULL;
+

Ditto.

quoted

      return 0;

 fail:
+     kfree(td->fields);

Ditto.

quoted

      kfree(td);
      return ret;
 }
--
1.7.7.6

Thanks,
Henrik

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html