[PATCH 1/6 v2] HID: magicmouse: don't allow hidinput to initialize the device

[PATCH 1/6 v2] HID: magicmouse: don't allow hidinput to initialize the device

[Chase Douglas]

From: Chase Douglas <chase.douglas@ubuntu.com>

The driver listens only for raw events from the device. If we allow
the hidinput layer to initialize, we can hit NULL pointer dereferences
in the hidinput layer because disconnecting only removes the hidinput
devices from the hid device while leaving the hid fields configured.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
Note that this mimics what the hid-picolcd module does.

 drivers/hid/hid-magicmouse.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 319b0e5..d38b529 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -404,15 +404,20 @@ static int magicmouse_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	/* When registering a hid device, one of hidinput, hidraw, or hiddev
+	 * subsystems must claim the device. We are bypassing hidinput due to
+	 * our raw event processing, and hidraw and hiddev may not claim the
+	 * device. We get around this by telling hid_hw_start that input has
+	 * claimed the device already, and then flipping the bit back.
+	 */
+	hdev->claimed = HID_CLAIMED_INPUT;
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDINPUT);
+	hdev->claimed &= ~HID_CLAIMED_INPUT;
 	if (ret) {
 		dev_err(&hdev->dev, "magicmouse hw start failed\n");
 		goto err_free;
 	}
 
-	/* we are handling the input ourselves */
-	hidinput_disconnect(hdev);
-
 	report = hid_register_report(hdev, HID_INPUT_REPORT, TOUCH_REPORT_ID);
 	if (!report) {
 		dev_err(&hdev->dev, "unable to register touch report\n");
-- 
1.7.1

[PATCH 2/6 v2] HID: magicmouse: move features reports to static array

[Chase Douglas]

From: Chase Douglas <chase.douglas@ubuntu.com>

By moving the feature reports to an array, we can easily support
more products with different initialization reports. This will be
useful for enabling the Apple Magic Trackpad.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-magicmouse.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index d38b529..3a2147d 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -377,14 +377,23 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 	}
 }
 
+struct feature {
+	__u8 data[3];
+	__u8 length;
+};
+
+static struct feature mouse_features[] = {
+	{ { 0xd7, 0x01 }, 2 },
+	{ { 0xf8, 0x01, 0x32 }, 3 }
+};
+
 static int magicmouse_probe(struct hid_device *hdev,
 	const struct hid_device_id *id)
 {
-	__u8 feature_1[] = { 0xd7, 0x01 };
-	__u8 feature_2[] = { 0xf8, 0x01, 0x32 };
 	struct input_dev *input;
 	struct magicmouse_sc *msc;
 	struct hid_report *report;
+	int i;
 	int ret;
 
 	msc = kzalloc(sizeof(*msc), GFP_KERNEL);
@@ -426,19 +435,15 @@ static int magicmouse_probe(struct hid_device *hdev,
 	}
 	report->size = 6;
 
-	ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
-			HID_FEATURE_REPORT);
-	if (ret != sizeof(feature_1)) {
-		dev_err(&hdev->dev, "unable to request touch data (1:%d)\n",
-				ret);
-		goto err_stop_hw;
-	}
-	ret = hdev->hid_output_raw_report(hdev, feature_2,
-			sizeof(feature_2), HID_FEATURE_REPORT);
-	if (ret != sizeof(feature_2)) {
-		dev_err(&hdev->dev, "unable to request touch data (2:%d)\n",
-				ret);
-		goto err_stop_hw;
+	for (i = 0; i < ARRAY_SIZE(mouse_features); i++) {
+		ret = hdev->hid_output_raw_report(hdev, mouse_features[i].data,
+				mouse_features[i].length, HID_FEATURE_REPORT);
+		if (ret != mouse_features[i].length) {
+			dev_err(&hdev->dev,
+				"unable to request touch data (%d:%d)\n",
+				i, ret);
+			goto err_stop_hw;
+		}
 	}
 
 	input = input_allocate_device();
-- 
1.7.1

[PATCH 3/6 v2] HID: magicmouse: simplify touch data bit manipulation

[Chase Douglas]

The new format should be easier to read to determine which bits
correspond to which data. It also brings all the manipulation logic to
the top of the function. This makes size and orientation reading more
clear.

Note that the impetus for this change is the forthcoming support for the
Magic Trackpad, which has a different touch data protocol.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-magicmouse.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 3a2147d..98bbc53 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -166,18 +166,21 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
 static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
 {
 	struct input_dev *input = msc->input;
-	__s32 x_y = tdata[0] << 8 | tdata[1] << 16 | tdata[2] << 24;
-	int misc = tdata[5] | tdata[6] << 8;
-	int id = (misc >> 6) & 15;
-	int x = x_y << 12 >> 20;
-	int y = -(x_y >> 20);
-	int down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;
+	int id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
+	int x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
+	int y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
+	int size = tdata[5] & 0x3f;
+	int orientation = (tdata[6] >> 2) - 32;
+	int touch_major = tdata[3];
+	int touch_minor = tdata[4];
+	int state = tdata[7] & TOUCH_STATE_MASK;
+	int down = state != TOUCH_STATE_NONE;
 
 	/* Store tracking ID and other fields. */
 	msc->tracking_ids[raw_id] = id;
 	msc->touches[id].x = x;
 	msc->touches[id].y = y;
-	msc->touches[id].size = misc & 63;
+	msc->touches[id].size = size;
 
 	/* If requested, emulate a scroll wheel by detecting small
 	 * vertical touch motions.
@@ -188,7 +191,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		int step_y = msc->touches[id].scroll_y - y;
 
 		/* Calculate and apply the scroll motion. */
-		switch (tdata[7] & TOUCH_STATE_MASK) {
+		switch (state) {
 		case TOUCH_STATE_START:
 			msc->touches[id].scroll_x = x;
 			msc->touches[id].scroll_y = y;
@@ -224,13 +227,11 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 
 	/* Generate the input events for this touch. */
 	if (report_touches && down) {
-		int orientation = (misc >> 10) - 32;
-
 		msc->touches[id].down = 1;
 
 		input_report_abs(input, ABS_MT_TRACKING_ID, id);
-		input_report_abs(input, ABS_MT_TOUCH_MAJOR, tdata[3]);
-		input_report_abs(input, ABS_MT_TOUCH_MINOR, tdata[4]);
+		input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
+		input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
 		input_report_abs(input, ABS_MT_ORIENTATION, orientation);
 		input_report_abs(input, ABS_MT_POSITION_X, x);
 		input_report_abs(input, ABS_MT_POSITION_Y, y);
-- 
1.7.1

[PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

The Magic Mouse device is very precise. No driver filtering of input
data needs to be performed.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-magicmouse.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 98bbc53..0fe2464 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -357,11 +357,11 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 		__set_bit(EV_ABS, input->evbit);
 
 		input_set_abs_params(input, ABS_MT_TRACKING_ID, 0, 15, 0, 0);
-		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 4, 0);
-		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 4, 0);
-		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 1, 0);
+		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
+		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
 		input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
-				4, 0);
+				0, 0);
 		/* Note: Touch Y position from the device is inverted relative
 		 * to how pointer motion is reported (and relative to how USB
 		 * HID recommends the coordinates work).  This driver keeps
@@ -369,7 +369,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 		 * inverse of the reported Y.
 		 */
 		input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
-				4, 0);
+				0, 0);
 	}
 
 	if (report_undeciphered) {
-- 
1.7.1

[PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

[Chase Douglas]

The trackpad speaks a similar, but different, protocol from the magic
mouse. However, only small code tweaks here and there are needed to make
basic multitouch work.

Extra logic is required for single-touch emulation of the touchpad. The
changes made here take the approach that only one finger may emulate the
single pointer when multiple fingers have touched the screen. Once that
finger is raised, all touches must be raised before any further single
touch events can be sent.

Sometimes the magic trackpad sends two distinct touch reports as one big
report. Simply splitting the packet in two and resending them through
magicmouse_raw_event ensures they are handled properly.

I also added myself to the copyright statement.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-core.c       |    1 +
 drivers/hid/hid-ids.h        |    1 +
 drivers/hid/hid-magicmouse.c |  229 ++++++++++++++++++++++++++++++++----------
 3 files changed, 176 insertions(+), 55 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 616bddc..5226fd1 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1248,6 +1248,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_ISO) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER_ANSI) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 7b3ca1d..9204cac 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -63,6 +63,7 @@
 #define USB_VENDOR_ID_APPLE		0x05ac
 #define USB_DEVICE_ID_APPLE_MIGHTYMOUSE	0x0304
 #define USB_DEVICE_ID_APPLE_MAGICMOUSE	0x030d
+#define USB_DEVICE_ID_APPLE_MAGICTRACKPAD	0x030e
 #define USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI	0x020e
 #define USB_DEVICE_ID_APPLE_FOUNTAIN_ISO	0x020f
 #define USB_DEVICE_ID_APPLE_GEYSER_ANSI	0x0214
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 0fe2464..364556a 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -2,6 +2,7 @@
  *   Apple "Magic" Wireless Mouse driver
  *
  *   Copyright (c) 2010 Michael Poole <mdpoole@troilus.org>
+ *   Copyright (c) 2010 Chase Douglas <chase.douglas@canonical.com>
  */
 
 /*
@@ -53,7 +54,9 @@ static bool report_undeciphered;
 module_param(report_undeciphered, bool, 0644);
 MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state field using a MSC_RAW event");
 
-#define TOUCH_REPORT_ID   0x29
+#define TRACKPAD_REPORT_ID 0x28
+#define MOUSE_REPORT_ID    0x29
+#define DOUBLE_REPORT_ID   0xf7
 /* These definitions are not precise, but they're close enough.  (Bits
  * 0x03 seem to indicate the aspect ratio of the touch, bits 0x70 seem
  * to be some kind of bit mask -- 0x20 may be a near-field reading,
@@ -101,6 +104,7 @@ struct magicmouse_sc {
 		u8 down;
 	} touches[16];
 	int tracking_ids[16];
+	int single_touch_id;
 };
 
 static int magicmouse_firm_touch(struct magicmouse_sc *msc)
@@ -166,15 +170,29 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
 static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
 {
 	struct input_dev *input = msc->input;
-	int id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
-	int x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
-	int y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
-	int size = tdata[5] & 0x3f;
-	int orientation = (tdata[6] >> 2) - 32;
-	int touch_major = tdata[3];
-	int touch_minor = tdata[4];
-	int state = tdata[7] & TOUCH_STATE_MASK;
-	int down = state != TOUCH_STATE_NONE;
+	int id, x, y, size, orientation, touch_major, touch_minor, state, down;
+
+	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+		id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
+		x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
+		y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
+		size = tdata[5] & 0x3f;
+		orientation = (tdata[6] >> 2) - 32;
+		touch_major = tdata[3];
+		touch_minor = tdata[4];
+		state = tdata[7] & TOUCH_STATE_MASK;
+		down = state != TOUCH_STATE_NONE;
+	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+		id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
+		x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
+		y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
+		size = tdata[6] & 0x3f;
+		orientation = (tdata[7] >> 2) - 32;
+		touch_major = tdata[4];
+		touch_minor = tdata[5];
+		state = tdata[8] & TOUCH_STATE_MASK;
+		down = state != TOUCH_STATE_NONE;
+	}
 
 	/* Store tracking ID and other fields. */
 	msc->tracking_ids[raw_id] = id;
@@ -225,10 +243,15 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		}
 	}
 
-	/* Generate the input events for this touch. */
-	if (report_touches && down) {
+	if (down) {
 		msc->touches[id].down = 1;
+		if (msc->single_touch_id == -1)
+			msc->single_touch_id = id;
+	} else if (msc->single_touch_id == id)
+		msc->single_touch_id = -2;
 
+	/* Generate the input events for this touch. */
+	if (report_touches && down) {
 		input_report_abs(input, ABS_MT_TRACKING_ID, id);
 		input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
 		input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
@@ -236,8 +259,12 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		input_report_abs(input, ABS_MT_POSITION_X, x);
 		input_report_abs(input, ABS_MT_POSITION_Y, y);
 
-		if (report_undeciphered)
-			input_event(input, EV_MSC, MSC_RAW, tdata[7]);
+		if (report_undeciphered) {
+			if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
+				input_event(input, EV_MSC, MSC_RAW, tdata[7]);
+			else /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+				input_event(input, EV_MSC, MSC_RAW, tdata[8]);
+		}
 
 		input_mt_sync(input);
 	}
@@ -248,7 +275,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 {
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
 	struct input_dev *input = msc->input;
-	int x, y, ts, ii, clicks, last_up;
+	int x = 0, y = 0, ts, ii, clicks = 0, last_up;
 
 	switch (data[0]) {
 	case 0x10:
@@ -258,7 +285,19 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		y = (__s16)(data[4] | data[5] << 8);
 		clicks = data[1];
 		break;
-	case TOUCH_REPORT_ID:
+	case TRACKPAD_REPORT_ID:
+		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
+		if (size < 4 || ((size - 4) % 9) != 0)
+			return 0;
+		ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
+		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
+		msc->last_timestamp = ts;
+		msc->ntouches = (size - 4) / 9;
+		for (ii = 0; ii < msc->ntouches; ii++)
+			magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
+		clicks = data[1];
+		break;
+	case MOUSE_REPORT_ID:
 		/* Expect six bytes of prefix, and N*8 bytes of touch data. */
 		if (size < 6 || ((size - 6) % 8) != 0)
 			return 0;
@@ -269,19 +308,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		for (ii = 0; ii < msc->ntouches; ii++)
 			magicmouse_emit_touch(msc, ii, data + ii * 8 + 6);
 
-		if (report_touches) {
-			last_up = 1;
-			for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
-				if (msc->touches[ii].down) {
-					last_up = 0;
-					msc->touches[ii].down = 0;
-				}
-			}
-			if (last_up) {
-				input_mt_sync(input);
-			}
-		}
-
 		/* When emulating three-button mode, it is important
 		 * to have the current touch information before
 		 * generating a click event.
@@ -290,6 +316,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
 		clicks = data[3];
 		break;
+	case DOUBLE_REPORT_ID:
+		/* Sometimes the trackpad sends two touch reports in one
+		 * packet.
+		 */
+		magicmouse_raw_event(hdev, report, data + 2, data[1]);
+		magicmouse_raw_event(hdev, report, data + 2 + data[1],
+			size - 2 - data[1]);
+		break;
 	case 0x20: /* Theoretically battery status (0-100), but I have
 		    * never seen it -- maybe it is only upon request.
 		    */
@@ -301,9 +335,41 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		return 0;
 	}
 
-	magicmouse_emit_buttons(msc, clicks & 3);
-	input_report_rel(input, REL_X, x);
-	input_report_rel(input, REL_Y, y);
+	if ((data[0] == MOUSE_REPORT_ID || data[0] == TRACKPAD_REPORT_ID)) {
+		last_up = 1;
+		for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
+			if (msc->touches[ii].down) {
+				last_up = 0;
+				msc->touches[ii].down = 0;
+			}
+		}
+		if (last_up) {
+			msc->single_touch_id = -1;
+			msc->ntouches = 0;
+			if (report_touches)
+				input_mt_sync(input);
+		}
+	}
+
+	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+		magicmouse_emit_buttons(msc, clicks & 3);
+		input_report_rel(input, REL_X, x);
+		input_report_rel(input, REL_Y, y);
+	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+		input_report_key(input, BTN_MOUSE, clicks & 1);
+		input_report_key(input, BTN_TOUCH, msc->ntouches > 0);
+		input_report_key(input, BTN_TOOL_FINGER, msc->ntouches == 1);
+		input_report_key(input, BTN_TOOL_DOUBLETAP, msc->ntouches == 2);
+		input_report_key(input, BTN_TOOL_TRIPLETAP, msc->ntouches == 3);
+		input_report_key(input, BTN_TOOL_QUADTAP, msc->ntouches == 4);
+		if (msc->single_touch_id >= 0) {
+			input_report_abs(input, ABS_X,
+				msc->touches[msc->single_touch_id].x);
+			input_report_abs(input, ABS_Y,
+				msc->touches[msc->single_touch_id].y);
+		}
+	}
+
 	input_sync(input);
 	return 1;
 }
@@ -339,18 +405,27 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 	input->dev.parent = hdev->dev.parent;
 
 	__set_bit(EV_KEY, input->evbit);
-	__set_bit(BTN_LEFT, input->keybit);
-	__set_bit(BTN_RIGHT, input->keybit);
-	if (emulate_3button)
-		__set_bit(BTN_MIDDLE, input->keybit);
-	__set_bit(BTN_TOOL_FINGER, input->keybit);
-
-	__set_bit(EV_REL, input->evbit);
-	__set_bit(REL_X, input->relbit);
-	__set_bit(REL_Y, input->relbit);
-	if (emulate_scroll_wheel) {
-		__set_bit(REL_WHEEL, input->relbit);
-		__set_bit(REL_HWHEEL, input->relbit);
+
+	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+		__set_bit(BTN_LEFT, input->keybit);
+		__set_bit(BTN_RIGHT, input->keybit);
+		if (emulate_3button)
+			__set_bit(BTN_MIDDLE, input->keybit);
+
+		__set_bit(EV_REL, input->evbit);
+		__set_bit(REL_X, input->relbit);
+		__set_bit(REL_Y, input->relbit);
+		if (emulate_scroll_wheel) {
+			__set_bit(REL_WHEEL, input->relbit);
+			__set_bit(REL_HWHEEL, input->relbit);
+		}
+	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+		__set_bit(BTN_MOUSE, input->keybit);
+		__set_bit(BTN_TOOL_FINGER, input->keybit);
+		__set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
+		__set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
+		__set_bit(BTN_TOOL_QUADTAP, input->keybit);
+		__set_bit(BTN_TOUCH, input->keybit);
 	}
 
 	if (report_touches) {
@@ -360,16 +435,26 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
 		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
 		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
-		input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
-				0, 0);
+
 		/* Note: Touch Y position from the device is inverted relative
 		 * to how pointer motion is reported (and relative to how USB
 		 * HID recommends the coordinates work).  This driver keeps
 		 * the origin at the same position, and just uses the additive
 		 * inverse of the reported Y.
 		 */
-		input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
-				0, 0);
+		if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+			input_set_abs_params(input, ABS_MT_POSITION_X, -1100,
+				1358, 0, 0);
+			input_set_abs_params(input, ABS_MT_POSITION_Y, -1589,
+				2047, 0, 0);
+		} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+			input_set_abs_params(input, ABS_X, -2909, 3167, 0, 0);
+			input_set_abs_params(input, ABS_Y, -2456, 2565, 0, 0);
+			input_set_abs_params(input, ABS_MT_POSITION_X, -2909,
+				3167, 0, 0);
+			input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
+				2565, 0, 0);
+		}
 	}
 
 	if (report_undeciphered) {
@@ -388,12 +473,29 @@ static struct feature mouse_features[] = {
 	{ { 0xf8, 0x01, 0x32 }, 3 }
 };
 
+static struct feature trackpad_features[] = {
+	{ { 0xf1, 0xdb }, 2 },
+	{ { 0xf1, 0xdc }, 2 },
+	{ { 0xf0, 0x00 }, 2 },
+	{ { 0xf1, 0xdd }, 2 },
+	{ { 0xf0, 0x02 }, 2 },
+	{ { 0xf1, 0xc8 }, 2 },
+	{ { 0xf0, 0x09 }, 2 },
+	{ { 0xf1, 0xdc }, 2 },
+	{ { 0xf0, 0x00 }, 2 },
+	{ { 0xf1, 0xdd }, 2 },
+	{ { 0xf0, 0x02 }, 2 },
+	{ { 0xd7, 0x01 }, 2 },
+};
+
 static int magicmouse_probe(struct hid_device *hdev,
 	const struct hid_device_id *id)
 {
 	struct input_dev *input;
 	struct magicmouse_sc *msc;
 	struct hid_report *report;
+	struct feature *features;
+	int features_size;
 	int i;
 	int ret;
 
@@ -408,6 +510,8 @@ static int magicmouse_probe(struct hid_device *hdev,
 	msc->quirks = id->driver_data;
 	hid_set_drvdata(hdev, msc);
 
+	msc->single_touch_id = -1;
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		dev_err(&hdev->dev, "magicmouse hid parse failed\n");
@@ -428,7 +532,20 @@ static int magicmouse_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
-	report = hid_register_report(hdev, HID_INPUT_REPORT, TOUCH_REPORT_ID);
+	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+		report = hid_register_report(hdev, HID_INPUT_REPORT,
+			MOUSE_REPORT_ID);
+		features = mouse_features;
+		features_size = ARRAY_SIZE(mouse_features);
+	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+		report = hid_register_report(hdev, HID_INPUT_REPORT,
+			TRACKPAD_REPORT_ID);
+		report = hid_register_report(hdev, HID_INPUT_REPORT,
+			DOUBLE_REPORT_ID);
+		features = trackpad_features;
+		features_size = ARRAY_SIZE(trackpad_features);
+	}
+
 	if (!report) {
 		dev_err(&hdev->dev, "unable to register touch report\n");
 		ret = -ENOMEM;
@@ -436,10 +553,10 @@ static int magicmouse_probe(struct hid_device *hdev,
 	}
 	report->size = 6;
 
-	for (i = 0; i < ARRAY_SIZE(mouse_features); i++) {
-		ret = hdev->hid_output_raw_report(hdev, mouse_features[i].data,
-				mouse_features[i].length, HID_FEATURE_REPORT);
-		if (ret != mouse_features[i].length) {
+	for (i = 0; i < features_size; i++) {
+		ret = hdev->hid_output_raw_report(hdev, features[i].data,
+				features[i].length, HID_FEATURE_REPORT);
+		if (ret != features[i].length) {
 			dev_err(&hdev->dev,
 				"unable to request touch data (%d:%d)\n",
 				i, ret);
@@ -482,8 +599,10 @@ static void magicmouse_remove(struct hid_device *hdev)
 }
 
 static const struct hid_device_id magic_mice[] = {
-	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE),
-		.driver_data = 0 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+		USB_DEVICE_ID_APPLE_MAGICMOUSE), .driver_data = 0 },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+		USB_DEVICE_ID_APPLE_MAGICTRACKPAD), .driver_data = 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, magic_mice);
-- 
1.7.1

[PATCH 6/6 v2] HID: magicmouse: Adjust major / minor axes to scale

[Chase Douglas]

From: Henrik Rydberg <rydberg@euromail.se>

By visual inspection, the reported touch_major and touch_minor axes
are roughly a factor of four too small. The factor is approximate,
since the protocol is not known and the HID report encodes touch size
with fewer bits than positions. This patch scales the reported values
by a factor of four.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-magicmouse.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 364556a..898d0b8 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -253,8 +253,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 	/* Generate the input events for this touch. */
 	if (report_touches && down) {
 		input_report_abs(input, ABS_MT_TRACKING_ID, id);
-		input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
-		input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
+		input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major << 2);
+		input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor << 2);
 		input_report_abs(input, ABS_MT_ORIENTATION, orientation);
 		input_report_abs(input, ABS_MT_POSITION_X, x);
 		input_report_abs(input, ABS_MT_POSITION_Y, y);
-- 
1.7.1

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Henrik Rydberg]

On 08/31/2010 08:41 PM, Chase Douglas wrote:

> The Magic Mouse device is very precise. No driver filtering of input
> data needs to be performed.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> Acked-by: Michael Poole <mdpoole@troilus.org>
> ---


I am still not sure this is a good idea. Bandwidth from MT devices is a big
deal. A statement roughly how much data comes out of mtdev (which does the
filtering for type A devices) before and after this change would be reassuring.

Cheers,
Henrik

>  drivers/hid/hid-magicmouse.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 98bbc53..0fe2464 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -357,11 +357,11 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  		__set_bit(EV_ABS, input->evbit);
>  
>  		input_set_abs_params(input, ABS_MT_TRACKING_ID, 0, 15, 0, 0);
> -		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 4, 0);
> -		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 4, 0);
> -		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 1, 0);
> +		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> +		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
>  		input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
> -				4, 0);
> +				0, 0);
>  		/* Note: Touch Y position from the device is inverted relative
>  		 * to how pointer motion is reported (and relative to how USB
>  		 * HID recommends the coordinates work).  This driver keeps
> @@ -369,7 +369,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  		 * inverse of the reported Y.
>  		 */
>  		input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
> -				4, 0);
> +				0, 0);
>  	}
>  
>  	if (report_undeciphered) {

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> 
> > The Magic Mouse device is very precise. No driver filtering of input
> > data needs to be performed.
> > 
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > Acked-by: Michael Poole <mdpoole@troilus.org>
> > ---
> 
> 
> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> deal. A statement roughly how much data comes out of mtdev (which does the
> filtering for type A devices) before and after this change would be reassuring.

As it is right now, hid-magicmouse doesn't support MT slots. I think all
the fuzz code ends up comparing in the MT case is between one touch and
another touch, not between one touch's current location and its previous
location. If I'm correct, then it means a fuzz > 0 is incorrect for
non-slotted MT devices.

In fact, the code in drivers/input/input.c around line 194 looks like it
discards defuzzing in this case, so one could say this patch is making
things more correct :).

Now a fuzz > 0 for the non-MT ABS axes may be useful, but this device
exhibits no jitter, and we're not really worried about bandwidth in the
single touch case.

-- Chase

> >  drivers/hid/hid-magicmouse.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> > index 98bbc53..0fe2464 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -357,11 +357,11 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> >  		__set_bit(EV_ABS, input->evbit);
> >  
> >  		input_set_abs_params(input, ABS_MT_TRACKING_ID, 0, 15, 0, 0);
> > -		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 4, 0);
> > -		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 4, 0);
> > -		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 1, 0);
> > +		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > +		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> > +		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
> >  		input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
> > -				4, 0);
> > +				0, 0);
> >  		/* Note: Touch Y position from the device is inverted relative
> >  		 * to how pointer motion is reported (and relative to how USB
> >  		 * HID recommends the coordinates work).  This driver keeps
> > @@ -369,7 +369,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> >  		 * inverse of the reported Y.
> >  		 */
> >  		input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
> > -				4, 0);
> > +				0, 0);
> >  	}
> >  
> >  	if (report_undeciphered) {

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Henrik Rydberg]

On 08/31/2010 10:58 PM, Chase Douglas wrote:

> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
>>
>>> The Magic Mouse device is very precise. No driver filtering of input
>>> data needs to be performed.
>>>
>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>>> Acked-by: Michael Poole <mdpoole@troilus.org>
>>> ---
>>
>>
>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
>> deal. A statement roughly how much data comes out of mtdev (which does the
>> filtering for type A devices) before and after this change would be reassuring.
> 
> As it is right now, hid-magicmouse doesn't support MT slots. I think all
> the fuzz code ends up comparing in the MT case is between one touch and
> another touch, not between one touch's current location and its previous
> location. If I'm correct, then it means a fuzz > 0 is incorrect for
> non-slotted MT devices.
> 
> In fact, the code in drivers/input/input.c around line 194 looks like it
> discards defuzzing in this case, so one could say this patch is making
> things more correct :).


For type A devices, the filtering is performed in userspace, in mtdev, in the
same manner as it would have been performed in the kernel in the MT slot case.
Therefore, knowing the amount of messages coming out of mtdev is a direct
measurement of the effect of filtering.

> Now a fuzz > 0 for the non-MT ABS axes may be useful, but this device
> exhibits no jitter, and we're not really worried about bandwidth in the
> single touch case.


The jitter is better measured by the actual amount of events.

Henrik

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
> On 08/31/2010 10:58 PM, Chase Douglas wrote:
> 
> > On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> >> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> >>
> >>> The Magic Mouse device is very precise. No driver filtering of input
> >>> data needs to be performed.
> >>>
> >>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >>> Acked-by: Michael Poole <mdpoole@troilus.org>
> >>> ---
> >>
> >>
> >> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> >> deal. A statement roughly how much data comes out of mtdev (which does the
> >> filtering for type A devices) before and after this change would be reassuring.
> > 
> > As it is right now, hid-magicmouse doesn't support MT slots. I think all
> > the fuzz code ends up comparing in the MT case is between one touch and
> > another touch, not between one touch's current location and its previous
> > location. If I'm correct, then it means a fuzz > 0 is incorrect for
> > non-slotted MT devices.
> > 
> > In fact, the code in drivers/input/input.c around line 194 looks like it
> > discards defuzzing in this case, so one could say this patch is making
> > things more correct :).
> 
> 
> For type A devices, the filtering is performed in userspace, in mtdev, in the
> same manner as it would have been performed in the kernel in the MT slot case.
> Therefore, knowing the amount of messages coming out of mtdev is a direct
> measurement of the effect of filtering.

Yes, but we're only interested in the kernel driver when reviewing this
patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
axes. On the other axes, such as the touch orientation, it's probably
more harmful than good.

> > Now a fuzz > 0 for the non-MT ABS axes may be useful, but this device
> > exhibits no jitter, and we're not really worried about bandwidth in the
> > single touch case.
> 
> 
> The jitter is better measured by the actual amount of events.

I would agree, if there was any jitter to measure. However, I can hold
my fingers on the device and not see any extra events due to jitter.
Simple inspection via evtest has convinced me.

-- Chase

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Henrik Rydberg]

On 08/31/2010 11:16 PM, Chase Douglas wrote:

> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
>>
>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
>>>>
>>>>> The Magic Mouse device is very precise. No driver filtering of input
>>>>> data needs to be performed.
>>>>>
>>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
>>>>> ---
>>>>
>>>>
>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
>>>> deal. A statement roughly how much data comes out of mtdev (which does the
>>>> filtering for type A devices) before and after this change would be reassuring.
>>>
>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
>>> the fuzz code ends up comparing in the MT case is between one touch and
>>> another touch, not between one touch's current location and its previous
>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
>>> non-slotted MT devices.
>>>
>>> In fact, the code in drivers/input/input.c around line 194 looks like it
>>> discards defuzzing in this case, so one could say this patch is making
>>> things more correct :).
>>
>>
>> For type A devices, the filtering is performed in userspace, in mtdev, in the
>> same manner as it would have been performed in the kernel in the MT slot case.
>> Therefore, knowing the amount of messages coming out of mtdev is a direct
>> measurement of the effect of filtering.
> 
> Yes, but we're only interested in the kernel driver when reviewing this
> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
> axes. On the other axes, such as the touch orientation, it's probably
> more harmful than good.


"We" are interested in knowing if this patch makes any sense, given that
filtering is in actuality performed in userspace, and thus modifying this code
changes the stream rate into userspace applications, thank you.

Henrik

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
> On 08/31/2010 11:16 PM, Chase Douglas wrote:
> 
> > On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
> >> On 08/31/2010 10:58 PM, Chase Douglas wrote:
> >>
> >>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> >>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> >>>>
> >>>>> The Magic Mouse device is very precise. No driver filtering of input
> >>>>> data needs to be performed.
> >>>>>
> >>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
> >>>>> ---
> >>>>
> >>>>
> >>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> >>>> deal. A statement roughly how much data comes out of mtdev (which does the
> >>>> filtering for type A devices) before and after this change would be reassuring.
> >>>
> >>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
> >>> the fuzz code ends up comparing in the MT case is between one touch and
> >>> another touch, not between one touch's current location and its previous
> >>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
> >>> non-slotted MT devices.
> >>>
> >>> In fact, the code in drivers/input/input.c around line 194 looks like it
> >>> discards defuzzing in this case, so one could say this patch is making
> >>> things more correct :).
> >>
> >>
> >> For type A devices, the filtering is performed in userspace, in mtdev, in the
> >> same manner as it would have been performed in the kernel in the MT slot case.
> >> Therefore, knowing the amount of messages coming out of mtdev is a direct
> >> measurement of the effect of filtering.
> > 
> > Yes, but we're only interested in the kernel driver when reviewing this
> > patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
> > axes. On the other axes, such as the touch orientation, it's probably
> > more harmful than good.
> 
> 
> "We" are interested in knowing if this patch makes any sense, given that
> filtering is in actuality performed in userspace, and thus modifying this code
> changes the stream rate into userspace applications, thank you.

Because in-kernel defuzzing is turned off for ABS_MT_* axes on
non-slotted MT devices, there will be no change in the number of events
sent to the userspace due to this patch.

Maybe I'm missing something more fundamental. In which case, I'll need
more details to get it through my dense skull :).

-- Chase

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Henrik Rydberg]

On 08/31/2010 11:27 PM, Chase Douglas wrote:

> On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
>> On 08/31/2010 11:16 PM, Chase Douglas wrote:
>>
>>> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
>>>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
>>>>
>>>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
>>>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
>>>>>>
>>>>>>> The Magic Mouse device is very precise. No driver filtering of input
>>>>>>> data needs to be performed.
>>>>>>>
>>>>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>>>>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
>>>>>>> ---
>>>>>>
>>>>>>
>>>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
>>>>>> deal. A statement roughly how much data comes out of mtdev (which does the
>>>>>> filtering for type A devices) before and after this change would be reassuring.
>>>>>
>>>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
>>>>> the fuzz code ends up comparing in the MT case is between one touch and
>>>>> another touch, not between one touch's current location and its previous
>>>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
>>>>> non-slotted MT devices.
>>>>>
>>>>> In fact, the code in drivers/input/input.c around line 194 looks like it
>>>>> discards defuzzing in this case, so one could say this patch is making
>>>>> things more correct :).
>>>>
>>>>
>>>> For type A devices, the filtering is performed in userspace, in mtdev, in the
>>>> same manner as it would have been performed in the kernel in the MT slot case.
>>>> Therefore, knowing the amount of messages coming out of mtdev is a direct
>>>> measurement of the effect of filtering.
>>>
>>> Yes, but we're only interested in the kernel driver when reviewing this
>>> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
>>> axes. On the other axes, such as the touch orientation, it's probably
>>> more harmful than good.
>>
>>
>> "We" are interested in knowing if this patch makes any sense, given that
>> filtering is in actuality performed in userspace, and thus modifying this code
>> changes the stream rate into userspace applications, thank you.
> 
> Because in-kernel defuzzing is turned off for ABS_MT_* axes on
> non-slotted MT devices, there will be no change in the number of events
> sent to the userspace due to this patch.
> 
> Maybe I'm missing something more fundamental. In which case, I'll need
> more details to get it through my dense skull :).


All events passes through to mtdev, yes, but mtdev filters a considerable amount
of events from passing through to X drivers like evdev. Thus, the fuzz values
reported in the kernel driver impacts the performance in userspace, even if
filtering is not done in the kernel.

Henrik

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

On Tue, 2010-08-31 at 23:39 +0200, Henrik Rydberg wrote:
> On 08/31/2010 11:27 PM, Chase Douglas wrote:
> 
> > On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
> >> On 08/31/2010 11:16 PM, Chase Douglas wrote:
> >>
> >>> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
> >>>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
> >>>>
> >>>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> >>>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> >>>>>>
> >>>>>>> The Magic Mouse device is very precise. No driver filtering of input
> >>>>>>> data needs to be performed.
> >>>>>>>
> >>>>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >>>>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
> >>>>>>> ---
> >>>>>>
> >>>>>>
> >>>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> >>>>>> deal. A statement roughly how much data comes out of mtdev (which does the
> >>>>>> filtering for type A devices) before and after this change would be reassuring.
> >>>>>
> >>>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
> >>>>> the fuzz code ends up comparing in the MT case is between one touch and
> >>>>> another touch, not between one touch's current location and its previous
> >>>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
> >>>>> non-slotted MT devices.
> >>>>>
> >>>>> In fact, the code in drivers/input/input.c around line 194 looks like it
> >>>>> discards defuzzing in this case, so one could say this patch is making
> >>>>> things more correct :).
> >>>>
> >>>>
> >>>> For type A devices, the filtering is performed in userspace, in mtdev, in the
> >>>> same manner as it would have been performed in the kernel in the MT slot case.
> >>>> Therefore, knowing the amount of messages coming out of mtdev is a direct
> >>>> measurement of the effect of filtering.
> >>>
> >>> Yes, but we're only interested in the kernel driver when reviewing this
> >>> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
> >>> axes. On the other axes, such as the touch orientation, it's probably
> >>> more harmful than good.
> >>
> >>
> >> "We" are interested in knowing if this patch makes any sense, given that
> >> filtering is in actuality performed in userspace, and thus modifying this code
> >> changes the stream rate into userspace applications, thank you.
> > 
> > Because in-kernel defuzzing is turned off for ABS_MT_* axes on
> > non-slotted MT devices, there will be no change in the number of events
> > sent to the userspace due to this patch.
> > 
> > Maybe I'm missing something more fundamental. In which case, I'll need
> > more details to get it through my dense skull :).
> 
> 
> All events passes through to mtdev, yes, but mtdev filters a considerable amount
> of events from passing through to X drivers like evdev. Thus, the fuzz values
> reported in the kernel driver impacts the performance in userspace, even if
> filtering is not done in the kernel.

My disconnect was that I didn't understand that the fuzz value in the
kernel is exported to userspace. I thought the defuzzing in mtdev was
independent of the defuzzing in the kernel.

Basically, I don't feel I have the time to do the analysis you want
right now. If you really want, I can just remove this change.

-- Chase

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

On Tue, 2010-08-31 at 17:51 -0400, Chase Douglas wrote:
> On Tue, 2010-08-31 at 23:39 +0200, Henrik Rydberg wrote:
> > On 08/31/2010 11:27 PM, Chase Douglas wrote:
> > 
> > > On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
> > >> On 08/31/2010 11:16 PM, Chase Douglas wrote:
> > >>
> > >>> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
> > >>>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
> > >>>>
> > >>>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> > >>>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> > >>>>>>
> > >>>>>>> The Magic Mouse device is very precise. No driver filtering of input
> > >>>>>>> data needs to be performed.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > >>>>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
> > >>>>>>> ---
> > >>>>>>
> > >>>>>>
> > >>>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> > >>>>>> deal. A statement roughly how much data comes out of mtdev (which does the
> > >>>>>> filtering for type A devices) before and after this change would be reassuring.
> > >>>>>
> > >>>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
> > >>>>> the fuzz code ends up comparing in the MT case is between one touch and
> > >>>>> another touch, not between one touch's current location and its previous
> > >>>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
> > >>>>> non-slotted MT devices.
> > >>>>>
> > >>>>> In fact, the code in drivers/input/input.c around line 194 looks like it
> > >>>>> discards defuzzing in this case, so one could say this patch is making
> > >>>>> things more correct :).
> > >>>>
> > >>>>
> > >>>> For type A devices, the filtering is performed in userspace, in mtdev, in the
> > >>>> same manner as it would have been performed in the kernel in the MT slot case.
> > >>>> Therefore, knowing the amount of messages coming out of mtdev is a direct
> > >>>> measurement of the effect of filtering.
> > >>>
> > >>> Yes, but we're only interested in the kernel driver when reviewing this
> > >>> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
> > >>> axes. On the other axes, such as the touch orientation, it's probably
> > >>> more harmful than good.
> > >>
> > >>
> > >> "We" are interested in knowing if this patch makes any sense, given that
> > >> filtering is in actuality performed in userspace, and thus modifying this code
> > >> changes the stream rate into userspace applications, thank you.
> > > 
> > > Because in-kernel defuzzing is turned off for ABS_MT_* axes on
> > > non-slotted MT devices, there will be no change in the number of events
> > > sent to the userspace due to this patch.
> > > 
> > > Maybe I'm missing something more fundamental. In which case, I'll need
> > > more details to get it through my dense skull :).
> > 
> > 
> > All events passes through to mtdev, yes, but mtdev filters a considerable amount
> > of events from passing through to X drivers like evdev. Thus, the fuzz values
> > reported in the kernel driver impacts the performance in userspace, even if
> > filtering is not done in the kernel.
> 
> My disconnect was that I didn't understand that the fuzz value in the
> kernel is exported to userspace. I thought the defuzzing in mtdev was
> independent of the defuzzing in the kernel.
> 
> Basically, I don't feel I have the time to do the analysis you want
> right now. If you really want, I can just remove this change.

On second thought, if there's no jitter from the device, the only reason
to filter is to save bandwidth. The magicmouse devices are very verbose
strictly because they have a higher resolution than most devices. If
that's an issue for userspace, then the filtering should be based on the
resolution. In fact, I see that's what you do in mtdev when the
in-kernel fuzz is 0.

This separates the kernel fuzz into a removal of jitter, and the mtdev
fuzz into a dampening of the number of events passed to clients. Since
magicmouse devices have no discernable jitter, shouldn't we set the
in-kernel fuzz to 0 and let mtdev do its thing as appropriate?

-- Chase

Re: [PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

[Henrik Rydberg]

On 08/31/2010 08:41 PM, Chase Douglas wrote:

> The trackpad speaks a similar, but different, protocol from the magic
> mouse. However, only small code tweaks here and there are needed to make
> basic multitouch work.
> 
> Extra logic is required for single-touch emulation of the touchpad. The
> changes made here take the approach that only one finger may emulate the
> single pointer when multiple fingers have touched the screen. Once that
> finger is raised, all touches must be raised before any further single
> touch events can be sent.
> 
> Sometimes the magic trackpad sends two distinct touch reports as one big
> report. Simply splitting the packet in two and resending them through
> magicmouse_raw_event ensures they are handled properly.
> 
> I also added myself to the copyright statement.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>


Thanks for this driver. Comments inline.

[...]

> @@ -166,15 +170,29 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
>  static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
>  {
>  	struct input_dev *input = msc->input;
> -	int id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> -	int x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> -	int y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> -	int size = tdata[5] & 0x3f;
> -	int orientation = (tdata[6] >> 2) - 32;
> -	int touch_major = tdata[3];
> -	int touch_minor = tdata[4];
> -	int state = tdata[7] & TOUCH_STATE_MASK;
> -	int down = state != TOUCH_STATE_NONE;
> +	int id, x, y, size, orientation, touch_major, touch_minor, state, down;
> +
> +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> +		id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> +		x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> +		y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> +		size = tdata[5] & 0x3f;
> +		orientation = (tdata[6] >> 2) - 32;
> +		touch_major = tdata[3];
> +		touch_minor = tdata[4];
> +		state = tdata[7] & TOUCH_STATE_MASK;
> +		down = state != TOUCH_STATE_NONE;
> +	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> +		id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
> +		x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> +		y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> +		size = tdata[6] & 0x3f;
> +		orientation = (tdata[7] >> 2) - 32;
> +		touch_major = tdata[4];
> +		touch_minor = tdata[5];
> +		state = tdata[8] & TOUCH_STATE_MASK;
> +		down = state != TOUCH_STATE_NONE;
> +	}
>  
>  	/* Store tracking ID and other fields. */
>  	msc->tracking_ids[raw_id] = id;
> @@ -225,10 +243,15 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  		}
>  	}
>  
> -	/* Generate the input events for this touch. */
> -	if (report_touches && down) {
> +	if (down) {
>  		msc->touches[id].down = 1;
> +		if (msc->single_touch_id == -1)
> +			msc->single_touch_id = id;
> +	} else if (msc->single_touch_id == id)
> +		msc->single_touch_id = -2;


The logic using single_touch_id seems complicated. Any chance you could get by
with less code here?


> +	/* Generate the input events for this touch. */
> +	if (report_touches && down) {
>  		input_report_abs(input, ABS_MT_TRACKING_ID, id);


The tracking id reported by the macpad is not ideal; it works more like a slot
that a MT protocol tracking id. Perhaps it is a slot? I would recommend looking
at the recent submissions for bamboo touch, 3m-pct and w8001 to see how the
tracking id and slots could be handled.

>  		input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
>  		input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
> @@ -236,8 +259,12 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  		input_report_abs(input, ABS_MT_POSITION_X, x);
>  		input_report_abs(input, ABS_MT_POSITION_Y, y);
>  
> -		if (report_undeciphered)
> -			input_event(input, EV_MSC, MSC_RAW, tdata[7]);
> +		if (report_undeciphered) {
> +			if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
> +				input_event(input, EV_MSC, MSC_RAW, tdata[7]);
> +			else /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> +				input_event(input, EV_MSC, MSC_RAW, tdata[8]);
> +		}
>  
>  		input_mt_sync(input);
>  	}
> @@ -248,7 +275,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  {
>  	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
>  	struct input_dev *input = msc->input;
> -	int x, y, ts, ii, clicks, last_up;
> +	int x = 0, y = 0, ts, ii, clicks = 0, last_up;
>  
>  	switch (data[0]) {
>  	case 0x10:
> @@ -258,7 +285,19 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		y = (__s16)(data[4] | data[5] << 8);
>  		clicks = data[1];
>  		break;
> -	case TOUCH_REPORT_ID:
> +	case TRACKPAD_REPORT_ID:
> +		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
> +		if (size < 4 || ((size - 4) % 9) != 0)
> +			return 0;
> +		ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
> +		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
> +		msc->last_timestamp = ts;


The delta_time and last_timestamp do not seem to be used anywhere?

> +		msc->ntouches = (size - 4) / 9;
> +		for (ii = 0; ii < msc->ntouches; ii++)
> +			magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
> +		clicks = data[1];
> +		break;
> +	case MOUSE_REPORT_ID:
>  		/* Expect six bytes of prefix, and N*8 bytes of touch data. */
>  		if (size < 6 || ((size - 6) % 8) != 0)
>  			return 0;
> @@ -269,19 +308,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		for (ii = 0; ii < msc->ntouches; ii++)
>  			magicmouse_emit_touch(msc, ii, data + ii * 8 + 6);
>  
> -		if (report_touches) {
> -			last_up = 1;
> -			for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> -				if (msc->touches[ii].down) {
> -					last_up = 0;
> -					msc->touches[ii].down = 0;
> -				}
> -			}
> -			if (last_up) {
> -				input_mt_sync(input);
> -			}
> -		}
> -
>  		/* When emulating three-button mode, it is important
>  		 * to have the current touch information before
>  		 * generating a click event.
> @@ -290,6 +316,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
>  		clicks = data[3];
>  		break;
> +	case DOUBLE_REPORT_ID:
> +		/* Sometimes the trackpad sends two touch reports in one
> +		 * packet.
> +		 */
> +		magicmouse_raw_event(hdev, report, data + 2, data[1]);
> +		magicmouse_raw_event(hdev, report, data + 2 + data[1],
> +			size - 2 - data[1]);
> +		break;


Nice find.

>  	case 0x20: /* Theoretically battery status (0-100), but I have
>  		    * never seen it -- maybe it is only upon request.
>  		    */
> @@ -301,9 +335,41 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		return 0;
>  	}
>  
> -	magicmouse_emit_buttons(msc, clicks & 3);
> -	input_report_rel(input, REL_X, x);
> -	input_report_rel(input, REL_Y, y);
> +	if ((data[0] == MOUSE_REPORT_ID || data[0] == TRACKPAD_REPORT_ID)) {
> +		last_up = 1;
> +		for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> +			if (msc->touches[ii].down) {
> +				last_up = 0;
> +				msc->touches[ii].down = 0;
> +			}
> +		}
> +		if (last_up) {
> +			msc->single_touch_id = -1;
> +			msc->ntouches = 0;
> +			if (report_touches)
> +				input_mt_sync(input);
> +		}


If the pointer emulation was handled differently, and the above was replaced
with something like

   if (!msc->ntouches)
  	input_mt_sync(input);

it would be short enough not to need to be broken out of the switch... Besides,
BTN_TOUCH is emitted for the trackpad case, so the above code is not strictly
needed.

> +	}
> +
> +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> +		magicmouse_emit_buttons(msc, clicks & 3);
> +		input_report_rel(input, REL_X, x);
> +		input_report_rel(input, REL_Y, y);
> +	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> +		input_report_key(input, BTN_MOUSE, clicks & 1);
> +		input_report_key(input, BTN_TOUCH, msc->ntouches > 0);
> +		input_report_key(input, BTN_TOOL_FINGER, msc->ntouches == 1);
> +		input_report_key(input, BTN_TOOL_DOUBLETAP, msc->ntouches == 2);
> +		input_report_key(input, BTN_TOOL_TRIPLETAP, msc->ntouches == 3);
> +		input_report_key(input, BTN_TOOL_QUADTAP, msc->ntouches == 4);
> +		if (msc->single_touch_id >= 0) {
> +			input_report_abs(input, ABS_X,
> +				msc->touches[msc->single_touch_id].x);
> +			input_report_abs(input, ABS_Y,
> +				msc->touches[msc->single_touch_id].y);
> +		}
> +	}
> +
>  	input_sync(input);
>  	return 1;
>  }
> @@ -339,18 +405,27 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  	input->dev.parent = hdev->dev.parent;
>  
>  	__set_bit(EV_KEY, input->evbit);
> -	__set_bit(BTN_LEFT, input->keybit);
> -	__set_bit(BTN_RIGHT, input->keybit);
> -	if (emulate_3button)
> -		__set_bit(BTN_MIDDLE, input->keybit);
> -	__set_bit(BTN_TOOL_FINGER, input->keybit);
> -
> -	__set_bit(EV_REL, input->evbit);
> -	__set_bit(REL_X, input->relbit);
> -	__set_bit(REL_Y, input->relbit);
> -	if (emulate_scroll_wheel) {
> -		__set_bit(REL_WHEEL, input->relbit);
> -		__set_bit(REL_HWHEEL, input->relbit);
> +
> +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> +		__set_bit(BTN_LEFT, input->keybit);
> +		__set_bit(BTN_RIGHT, input->keybit);
> +		if (emulate_3button)
> +			__set_bit(BTN_MIDDLE, input->keybit);
> +
> +		__set_bit(EV_REL, input->evbit);
> +		__set_bit(REL_X, input->relbit);
> +		__set_bit(REL_Y, input->relbit);
> +		if (emulate_scroll_wheel) {
> +			__set_bit(REL_WHEEL, input->relbit);
> +			__set_bit(REL_HWHEEL, input->relbit);
> +		}
> +	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> +		__set_bit(BTN_MOUSE, input->keybit);
> +		__set_bit(BTN_TOOL_FINGER, input->keybit);
> +		__set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
> +		__set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
> +		__set_bit(BTN_TOOL_QUADTAP, input->keybit);
> +		__set_bit(BTN_TOUCH, input->keybit);
>  	}
>  
>  	if (report_touches) {
> @@ -360,16 +435,26 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>  		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
>  		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
> -		input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
> -				0, 0);
> +
>  		/* Note: Touch Y position from the device is inverted relative
>  		 * to how pointer motion is reported (and relative to how USB
>  		 * HID recommends the coordinates work).  This driver keeps
>  		 * the origin at the same position, and just uses the additive
>  		 * inverse of the reported Y.
>  		 */
> -		input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
> -				0, 0);
> +		if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> +			input_set_abs_params(input, ABS_MT_POSITION_X, -1100,
> +				1358, 0, 0);
> +			input_set_abs_params(input, ABS_MT_POSITION_Y, -1589,
> +				2047, 0, 0);
> +		} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> +			input_set_abs_params(input, ABS_X, -2909, 3167, 0, 0);
> +			input_set_abs_params(input, ABS_Y, -2456, 2565, 0, 0);
> +			input_set_abs_params(input, ABS_MT_POSITION_X, -2909,
> +				3167, 0, 0);
> +			input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
> +				2565, 0, 0);
> +		}
>  	}
>  
>  	if (report_undeciphered) {
> @@ -388,12 +473,29 @@ static struct feature mouse_features[] = {
>  	{ { 0xf8, 0x01, 0x32 }, 3 }
>  };
>  
> +static struct feature trackpad_features[] = {
> +	{ { 0xf1, 0xdb }, 2 },
> +	{ { 0xf1, 0xdc }, 2 },
> +	{ { 0xf0, 0x00 }, 2 },
> +	{ { 0xf1, 0xdd }, 2 },
> +	{ { 0xf0, 0x02 }, 2 },
> +	{ { 0xf1, 0xc8 }, 2 },
> +	{ { 0xf0, 0x09 }, 2 },
> +	{ { 0xf1, 0xdc }, 2 },
> +	{ { 0xf0, 0x00 }, 2 },
> +	{ { 0xf1, 0xdd }, 2 },
> +	{ { 0xf0, 0x02 }, 2 },
> +	{ { 0xd7, 0x01 }, 2 },
> +};


As noted by Michael, this could likely be simplified. the "0x01" on the last
line could be the same coding as wellspring mode for bcm5974.

Thanks,
Henrik

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Henrik Rydberg]

On 08/31/2010 11:56 PM, Chase Douglas wrote:

> On Tue, 2010-08-31 at 17:51 -0400, Chase Douglas wrote:
>> On Tue, 2010-08-31 at 23:39 +0200, Henrik Rydberg wrote:
>>> On 08/31/2010 11:27 PM, Chase Douglas wrote:
>>>
>>>> On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
>>>>> On 08/31/2010 11:16 PM, Chase Douglas wrote:
>>>>>
>>>>>> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
>>>>>>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
>>>>>>>
>>>>>>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
>>>>>>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
>>>>>>>>>
>>>>>>>>>> The Magic Mouse device is very precise. No driver filtering of input
>>>>>>>>>> data needs to be performed.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>>>>>>>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
>>>>>>>>> deal. A statement roughly how much data comes out of mtdev (which does the
>>>>>>>>> filtering for type A devices) before and after this change would be reassuring.
>>>>>>>>
>>>>>>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
>>>>>>>> the fuzz code ends up comparing in the MT case is between one touch and
>>>>>>>> another touch, not between one touch's current location and its previous
>>>>>>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
>>>>>>>> non-slotted MT devices.
>>>>>>>>
>>>>>>>> In fact, the code in drivers/input/input.c around line 194 looks like it
>>>>>>>> discards defuzzing in this case, so one could say this patch is making
>>>>>>>> things more correct :).
>>>>>>>
>>>>>>>
>>>>>>> For type A devices, the filtering is performed in userspace, in mtdev, in the
>>>>>>> same manner as it would have been performed in the kernel in the MT slot case.
>>>>>>> Therefore, knowing the amount of messages coming out of mtdev is a direct
>>>>>>> measurement of the effect of filtering.
>>>>>>
>>>>>> Yes, but we're only interested in the kernel driver when reviewing this
>>>>>> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
>>>>>> axes. On the other axes, such as the touch orientation, it's probably
>>>>>> more harmful than good.
>>>>>
>>>>>
>>>>> "We" are interested in knowing if this patch makes any sense, given that
>>>>> filtering is in actuality performed in userspace, and thus modifying this code
>>>>> changes the stream rate into userspace applications, thank you.
>>>>
>>>> Because in-kernel defuzzing is turned off for ABS_MT_* axes on
>>>> non-slotted MT devices, there will be no change in the number of events
>>>> sent to the userspace due to this patch.
>>>>
>>>> Maybe I'm missing something more fundamental. In which case, I'll need
>>>> more details to get it through my dense skull :).
>>>
>>>
>>> All events passes through to mtdev, yes, but mtdev filters a considerable amount
>>> of events from passing through to X drivers like evdev. Thus, the fuzz values
>>> reported in the kernel driver impacts the performance in userspace, even if
>>> filtering is not done in the kernel.
>>
>> My disconnect was that I didn't understand that the fuzz value in the
>> kernel is exported to userspace. I thought the defuzzing in mtdev was
>> independent of the defuzzing in the kernel.
>>
>> Basically, I don't feel I have the time to do the analysis you want
>> right now. If you really want, I can just remove this change.
> 
> On second thought, if there's no jitter from the device, the only reason
> to filter is to save bandwidth. The magicmouse devices are very verbose
> strictly because they have a higher resolution than most devices. If
> that's an issue for userspace, then the filtering should be based on the
> resolution. In fact, I see that's what you do in mtdev when the
> in-kernel fuzz is 0.
> 
> This separates the kernel fuzz into a removal of jitter, and the mtdev
> fuzz into a dampening of the number of events passed to clients. Since
> magicmouse devices have no discernable jitter, shouldn't we set the
> in-kernel fuzz to 0 and let mtdev do its thing as appropriate?


Per-driver data is more precise than a generic value for all devices. This
thread has documented the details of why there is an interest in the fuzz
values. Skipping the patch until MT slots are implemented or measurements can be
done might be the most sensible thing to do.

Thanks,
Henrik

Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

[Chase Douglas]

On Wed, 2010-09-01 at 00:05 +0200, Henrik Rydberg wrote:
> On 08/31/2010 11:56 PM, Chase Douglas wrote:
> 
> > On Tue, 2010-08-31 at 17:51 -0400, Chase Douglas wrote:
> >> On Tue, 2010-08-31 at 23:39 +0200, Henrik Rydberg wrote:
> >>> On 08/31/2010 11:27 PM, Chase Douglas wrote:
> >>>
> >>>> On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
> >>>>> On 08/31/2010 11:16 PM, Chase Douglas wrote:
> >>>>>
> >>>>>> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
> >>>>>>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
> >>>>>>>
> >>>>>>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> >>>>>>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> >>>>>>>>>
> >>>>>>>>>> The Magic Mouse device is very precise. No driver filtering of input
> >>>>>>>>>> data needs to be performed.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >>>>>>>>>> Acked-by: Michael Poole <mdpoole@troilus.org>
> >>>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> >>>>>>>>> deal. A statement roughly how much data comes out of mtdev (which does the
> >>>>>>>>> filtering for type A devices) before and after this change would be reassuring.
> >>>>>>>>
> >>>>>>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
> >>>>>>>> the fuzz code ends up comparing in the MT case is between one touch and
> >>>>>>>> another touch, not between one touch's current location and its previous
> >>>>>>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
> >>>>>>>> non-slotted MT devices.
> >>>>>>>>
> >>>>>>>> In fact, the code in drivers/input/input.c around line 194 looks like it
> >>>>>>>> discards defuzzing in this case, so one could say this patch is making
> >>>>>>>> things more correct :).
> >>>>>>>
> >>>>>>>
> >>>>>>> For type A devices, the filtering is performed in userspace, in mtdev, in the
> >>>>>>> same manner as it would have been performed in the kernel in the MT slot case.
> >>>>>>> Therefore, knowing the amount of messages coming out of mtdev is a direct
> >>>>>>> measurement of the effect of filtering.
> >>>>>>
> >>>>>> Yes, but we're only interested in the kernel driver when reviewing this
> >>>>>> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
> >>>>>> axes. On the other axes, such as the touch orientation, it's probably
> >>>>>> more harmful than good.
> >>>>>
> >>>>>
> >>>>> "We" are interested in knowing if this patch makes any sense, given that
> >>>>> filtering is in actuality performed in userspace, and thus modifying this code
> >>>>> changes the stream rate into userspace applications, thank you.
> >>>>
> >>>> Because in-kernel defuzzing is turned off for ABS_MT_* axes on
> >>>> non-slotted MT devices, there will be no change in the number of events
> >>>> sent to the userspace due to this patch.
> >>>>
> >>>> Maybe I'm missing something more fundamental. In which case, I'll need
> >>>> more details to get it through my dense skull :).
> >>>
> >>>
> >>> All events passes through to mtdev, yes, but mtdev filters a considerable amount
> >>> of events from passing through to X drivers like evdev. Thus, the fuzz values
> >>> reported in the kernel driver impacts the performance in userspace, even if
> >>> filtering is not done in the kernel.
> >>
> >> My disconnect was that I didn't understand that the fuzz value in the
> >> kernel is exported to userspace. I thought the defuzzing in mtdev was
> >> independent of the defuzzing in the kernel.
> >>
> >> Basically, I don't feel I have the time to do the analysis you want
> >> right now. If you really want, I can just remove this change.
> > 
> > On second thought, if there's no jitter from the device, the only reason
> > to filter is to save bandwidth. The magicmouse devices are very verbose
> > strictly because they have a higher resolution than most devices. If
> > that's an issue for userspace, then the filtering should be based on the
> > resolution. In fact, I see that's what you do in mtdev when the
> > in-kernel fuzz is 0.
> > 
> > This separates the kernel fuzz into a removal of jitter, and the mtdev
> > fuzz into a dampening of the number of events passed to clients. Since
> > magicmouse devices have no discernable jitter, shouldn't we set the
> > in-kernel fuzz to 0 and let mtdev do its thing as appropriate?
> 
> 
> Per-driver data is more precise than a generic value for all devices. This
> thread has documented the details of why there is an interest in the fuzz
> values. Skipping the patch until MT slots are implemented or measurements can be
> done might be the most sensible thing to do.

Yes, there's still per-driver fuzz data. If a device is jittery it makes
sense to set the in-kernel fuzz factor to a reasonable value. If a
device is not jittery, I don't see a need to do any defuzzing of the
values in the driver itself. If we think it's necessary to reduce
bandwidth, then the kernel should be defuzzing uniformly across devices
to compensate. mtdev could do this defuzzing for bandwidth uniformly
too.

As it is, this driver just has an arbitrary value that is not used in
the kernel and likely hasn't ever been used anywhere outside of mtdev. I
think it makes sense to reset the fuzz to the baseline of 0 and fix it
later if we find it's necessary. Otherwise we're just codifying an
arbitrary value.

-- Chase

Re: [PATCH 1/6 v2] HID: magicmouse: don't allow hidinput to initialize the device

[Michael Poole]

On Tue, 2010-08-31 at 14:41 -0400, Chase Douglas wrote:
> From: Chase Douglas <chase.douglas@ubuntu.com>
> 
> The driver listens only for raw events from the device. If we allow
> the hidinput layer to initialize, we can hit NULL pointer dereferences
> in the hidinput layer because disconnecting only removes the hidinput
> devices from the hid device while leaving the hid fields configured.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
> Note that this mimics what the hid-picolcd module does.

Thanks, this approach makes sense to me.

Acked-by: Michael Poole <mdpoole@troilus.org>

>  drivers/hid/hid-magicmouse.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 319b0e5..d38b529 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -404,15 +404,20 @@ static int magicmouse_probe(struct hid_device *hdev,
>  		goto err_free;
>  	}
>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	/* When registering a hid device, one of hidinput, hidraw, or hiddev
> +	 * subsystems must claim the device. We are bypassing hidinput due to
> +	 * our raw event processing, and hidraw and hiddev may not claim the
> +	 * device. We get around this by telling hid_hw_start that input has
> +	 * claimed the device already, and then flipping the bit back.
> +	 */
> +	hdev->claimed = HID_CLAIMED_INPUT;
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDINPUT);
> +	hdev->claimed &= ~HID_CLAIMED_INPUT;
>  	if (ret) {
>  		dev_err(&hdev->dev, "magicmouse hw start failed\n");
>  		goto err_free;
>  	}
>  
> -	/* we are handling the input ourselves */
> -	hidinput_disconnect(hdev);
> -
>  	report = hid_register_report(hdev, HID_INPUT_REPORT, TOUCH_REPORT_ID);
>  	if (!report) {
>  		dev_err(&hdev->dev, "unable to register touch report\n");

Re: [PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

[Michael Poole]

On Tue, 2010-08-31 at 14:41 -0400, Chase Douglas wrote:
> The trackpad speaks a similar, but different, protocol from the magic
> mouse. However, only small code tweaks here and there are needed to make
> basic multitouch work.
> 
> Extra logic is required for single-touch emulation of the touchpad. The
> changes made here take the approach that only one finger may emulate the
> single pointer when multiple fingers have touched the screen. Once that
> finger is raised, all touches must be raised before any further single
> touch events can be sent.
> 
> Sometimes the magic trackpad sends two distinct touch reports as one big
> report. Simply splitting the packet in two and resending them through
> magicmouse_raw_event ensures they are handled properly.
> 
> I also added myself to the copyright statement.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/hid/hid-core.c       |    1 +
>  drivers/hid/hid-ids.h        |    1 +
>  drivers/hid/hid-magicmouse.c |  229 ++++++++++++++++++++++++++++++++----------
>  3 files changed, 176 insertions(+), 55 deletions(-)

Acked-by: Michael Poole <mdpoole@troilus.org>

One behavior that slightly surprised me -- which I believe is a quirk
due to userspace not expecting touchpads to have button switches -- is
that touches on the trackpad that do not close the switch can still be
interpreted by X as clicks.

Once the discussions about if/how to tweak this code settle down, I'll
put together a patch to change the "down" and "last_up" logic as I
suggested earlier.

Michael Poole

Re: [PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

[Chase Douglas]

On Wed, 2010-09-01 at 00:00 +0200, Henrik Rydberg wrote:
> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> 
> > The trackpad speaks a similar, but different, protocol from the magic
> > mouse. However, only small code tweaks here and there are needed to make
> > basic multitouch work.
> > 
> > Extra logic is required for single-touch emulation of the touchpad. The
> > changes made here take the approach that only one finger may emulate the
> > single pointer when multiple fingers have touched the screen. Once that
> > finger is raised, all touches must be raised before any further single
> > touch events can be sent.
> > 
> > Sometimes the magic trackpad sends two distinct touch reports as one big
> > report. Simply splitting the packet in two and resending them through
> > magicmouse_raw_event ensures they are handled properly.
> > 
> > I also added myself to the copyright statement.
> > 
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> 
> 
> Thanks for this driver. Comments inline.
> 
> [...]
> 
> > @@ -166,15 +170,29 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
> >  static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
> >  {
> >  	struct input_dev *input = msc->input;
> > -	int id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> > -	int x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> > -	int y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> > -	int size = tdata[5] & 0x3f;
> > -	int orientation = (tdata[6] >> 2) - 32;
> > -	int touch_major = tdata[3];
> > -	int touch_minor = tdata[4];
> > -	int state = tdata[7] & TOUCH_STATE_MASK;
> > -	int down = state != TOUCH_STATE_NONE;
> > +	int id, x, y, size, orientation, touch_major, touch_minor, state, down;
> > +
> > +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > +		id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> > +		x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> > +		y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> > +		size = tdata[5] & 0x3f;
> > +		orientation = (tdata[6] >> 2) - 32;
> > +		touch_major = tdata[3];
> > +		touch_minor = tdata[4];
> > +		state = tdata[7] & TOUCH_STATE_MASK;
> > +		down = state != TOUCH_STATE_NONE;
> > +	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > +		id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
> > +		x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> > +		y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> > +		size = tdata[6] & 0x3f;
> > +		orientation = (tdata[7] >> 2) - 32;
> > +		touch_major = tdata[4];
> > +		touch_minor = tdata[5];
> > +		state = tdata[8] & TOUCH_STATE_MASK;
> > +		down = state != TOUCH_STATE_NONE;
> > +	}
> >  
> >  	/* Store tracking ID and other fields. */
> >  	msc->tracking_ids[raw_id] = id;
> > @@ -225,10 +243,15 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
> >  		}
> >  	}
> >  
> > -	/* Generate the input events for this touch. */
> > -	if (report_touches && down) {
> > +	if (down) {
> >  		msc->touches[id].down = 1;
> > +		if (msc->single_touch_id == -1)
> > +			msc->single_touch_id = id;
> > +	} else if (msc->single_touch_id == id)
> > +		msc->single_touch_id = -2;
> 
> 
> The logic using single_touch_id seems complicated. Any chance you could get by
> with less code here?

The overall code for single touch handling is ~10 lines spread around
the driver. I've tried to condense the code as much as possible. Perhaps
someone could come up with something more elegant, but all this code
does is keep track of the touch that is tied to single touch emulation.

In my next submission I've added macros to make the -1 and -2 values
more obvious on inspection.

> > +	/* Generate the input events for this touch. */
> > +	if (report_touches && down) {
> >  		input_report_abs(input, ABS_MT_TRACKING_ID, id);
> 
> 
> The tracking id reported by the macpad is not ideal; it works more like a slot
> that a MT protocol tracking id. Perhaps it is a slot? I would recommend looking
> at the recent submissions for bamboo touch, 3m-pct and w8001 to see how the
> tracking id and slots could be handled.

Yes, I think it could be better handled. However, that handling probably
belongs in another patch when slots are implemented in this driver. I
just haven't had a chance to get to it yet :).

> >  		input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
> >  		input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
> > @@ -236,8 +259,12 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
> >  		input_report_abs(input, ABS_MT_POSITION_X, x);
> >  		input_report_abs(input, ABS_MT_POSITION_Y, y);
> >  
> > -		if (report_undeciphered)
> > -			input_event(input, EV_MSC, MSC_RAW, tdata[7]);
> > +		if (report_undeciphered) {
> > +			if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
> > +				input_event(input, EV_MSC, MSC_RAW, tdata[7]);
> > +			else /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > +				input_event(input, EV_MSC, MSC_RAW, tdata[8]);
> > +		}
> >  
> >  		input_mt_sync(input);
> >  	}
> > @@ -248,7 +275,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >  {
> >  	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
> >  	struct input_dev *input = msc->input;
> > -	int x, y, ts, ii, clicks, last_up;
> > +	int x = 0, y = 0, ts, ii, clicks = 0, last_up;
> >  
> >  	switch (data[0]) {
> >  	case 0x10:
> > @@ -258,7 +285,19 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >  		y = (__s16)(data[4] | data[5] << 8);
> >  		clicks = data[1];
> >  		break;
> > -	case TOUCH_REPORT_ID:
> > +	case TRACKPAD_REPORT_ID:
> > +		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
> > +		if (size < 4 || ((size - 4) % 9) != 0)
> > +			return 0;
> > +		ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
> > +		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
> > +		msc->last_timestamp = ts;
> 
> 
> The delta_time and last_timestamp do not seem to be used anywhere?

Good point. I've removed them in my next submission.

> > +		msc->ntouches = (size - 4) / 9;
> > +		for (ii = 0; ii < msc->ntouches; ii++)
> > +			magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
> > +		clicks = data[1];
> > +		break;
> > +	case MOUSE_REPORT_ID:
> >  		/* Expect six bytes of prefix, and N*8 bytes of touch data. */
> >  		if (size < 6 || ((size - 6) % 8) != 0)
> >  			return 0;
> > @@ -269,19 +308,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >  		for (ii = 0; ii < msc->ntouches; ii++)
> >  			magicmouse_emit_touch(msc, ii, data + ii * 8 + 6);
> >  
> > -		if (report_touches) {
> > -			last_up = 1;
> > -			for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> > -				if (msc->touches[ii].down) {
> > -					last_up = 0;
> > -					msc->touches[ii].down = 0;
> > -				}
> > -			}
> > -			if (last_up) {
> > -				input_mt_sync(input);
> > -			}
> > -		}
> > -
> >  		/* When emulating three-button mode, it is important
> >  		 * to have the current touch information before
> >  		 * generating a click event.
> > @@ -290,6 +316,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >  		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
> >  		clicks = data[3];
> >  		break;
> > +	case DOUBLE_REPORT_ID:
> > +		/* Sometimes the trackpad sends two touch reports in one
> > +		 * packet.
> > +		 */
> > +		magicmouse_raw_event(hdev, report, data + 2, data[1]);
> > +		magicmouse_raw_event(hdev, report, data + 2 + data[1],
> > +			size - 2 - data[1]);
> > +		break;
> 
> 
> Nice find.
> 
> >  	case 0x20: /* Theoretically battery status (0-100), but I have
> >  		    * never seen it -- maybe it is only upon request.
> >  		    */
> > @@ -301,9 +335,41 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >  		return 0;
> >  	}
> >  
> > -	magicmouse_emit_buttons(msc, clicks & 3);
> > -	input_report_rel(input, REL_X, x);
> > -	input_report_rel(input, REL_Y, y);
> > +	if ((data[0] == MOUSE_REPORT_ID || data[0] == TRACKPAD_REPORT_ID)) {
> > +		last_up = 1;
> > +		for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> > +			if (msc->touches[ii].down) {
> > +				last_up = 0;
> > +				msc->touches[ii].down = 0;
> > +			}
> > +		}
> > +		if (last_up) {
> > +			msc->single_touch_id = -1;
> > +			msc->ntouches = 0;
> > +			if (report_touches)
> > +				input_mt_sync(input);
> > +		}
> 
> 
> If the pointer emulation was handled differently, and the above was replaced
> with something like
> 
>    if (!msc->ntouches)
>   	input_mt_sync(input);
> 
> it would be short enough not to need to be broken out of the switch... Besides,
> BTN_TOUCH is emitted for the trackpad case, so the above code is not strictly
> needed.

I've rewritten the touch down logic, and this does get simple enough to
keep inside the switch scope.

> > +	}
> > +
> > +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > +		magicmouse_emit_buttons(msc, clicks & 3);
> > +		input_report_rel(input, REL_X, x);
> > +		input_report_rel(input, REL_Y, y);
> > +	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > +		input_report_key(input, BTN_MOUSE, clicks & 1);
> > +		input_report_key(input, BTN_TOUCH, msc->ntouches > 0);
> > +		input_report_key(input, BTN_TOOL_FINGER, msc->ntouches == 1);
> > +		input_report_key(input, BTN_TOOL_DOUBLETAP, msc->ntouches == 2);
> > +		input_report_key(input, BTN_TOOL_TRIPLETAP, msc->ntouches == 3);
> > +		input_report_key(input, BTN_TOOL_QUADTAP, msc->ntouches == 4);
> > +		if (msc->single_touch_id >= 0) {
> > +			input_report_abs(input, ABS_X,
> > +				msc->touches[msc->single_touch_id].x);
> > +			input_report_abs(input, ABS_Y,
> > +				msc->touches[msc->single_touch_id].y);
> > +		}
> > +	}
> > +
> >  	input_sync(input);
> >  	return 1;
> >  }
> > @@ -339,18 +405,27 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> >  	input->dev.parent = hdev->dev.parent;
> >  
> >  	__set_bit(EV_KEY, input->evbit);
> > -	__set_bit(BTN_LEFT, input->keybit);
> > -	__set_bit(BTN_RIGHT, input->keybit);
> > -	if (emulate_3button)
> > -		__set_bit(BTN_MIDDLE, input->keybit);
> > -	__set_bit(BTN_TOOL_FINGER, input->keybit);
> > -
> > -	__set_bit(EV_REL, input->evbit);
> > -	__set_bit(REL_X, input->relbit);
> > -	__set_bit(REL_Y, input->relbit);
> > -	if (emulate_scroll_wheel) {
> > -		__set_bit(REL_WHEEL, input->relbit);
> > -		__set_bit(REL_HWHEEL, input->relbit);
> > +
> > +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > +		__set_bit(BTN_LEFT, input->keybit);
> > +		__set_bit(BTN_RIGHT, input->keybit);
> > +		if (emulate_3button)
> > +			__set_bit(BTN_MIDDLE, input->keybit);
> > +
> > +		__set_bit(EV_REL, input->evbit);
> > +		__set_bit(REL_X, input->relbit);
> > +		__set_bit(REL_Y, input->relbit);
> > +		if (emulate_scroll_wheel) {
> > +			__set_bit(REL_WHEEL, input->relbit);
> > +			__set_bit(REL_HWHEEL, input->relbit);
> > +		}
> > +	} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > +		__set_bit(BTN_MOUSE, input->keybit);
> > +		__set_bit(BTN_TOOL_FINGER, input->keybit);
> > +		__set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
> > +		__set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
> > +		__set_bit(BTN_TOOL_QUADTAP, input->keybit);
> > +		__set_bit(BTN_TOUCH, input->keybit);
> >  	}
> >  
> >  	if (report_touches) {
> > @@ -360,16 +435,26 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> >  		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> >  		input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> >  		input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
> > -		input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
> > -				0, 0);
> > +
> >  		/* Note: Touch Y position from the device is inverted relative
> >  		 * to how pointer motion is reported (and relative to how USB
> >  		 * HID recommends the coordinates work).  This driver keeps
> >  		 * the origin at the same position, and just uses the additive
> >  		 * inverse of the reported Y.
> >  		 */
> > -		input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
> > -				0, 0);
> > +		if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > +			input_set_abs_params(input, ABS_MT_POSITION_X, -1100,
> > +				1358, 0, 0);
> > +			input_set_abs_params(input, ABS_MT_POSITION_Y, -1589,
> > +				2047, 0, 0);
> > +		} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > +			input_set_abs_params(input, ABS_X, -2909, 3167, 0, 0);
> > +			input_set_abs_params(input, ABS_Y, -2456, 2565, 0, 0);
> > +			input_set_abs_params(input, ABS_MT_POSITION_X, -2909,
> > +				3167, 0, 0);
> > +			input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
> > +				2565, 0, 0);
> > +		}
> >  	}
> >  
> >  	if (report_undeciphered) {
> > @@ -388,12 +473,29 @@ static struct feature mouse_features[] = {
> >  	{ { 0xf8, 0x01, 0x32 }, 3 }
> >  };
> >  
> > +static struct feature trackpad_features[] = {
> > +	{ { 0xf1, 0xdb }, 2 },
> > +	{ { 0xf1, 0xdc }, 2 },
> > +	{ { 0xf0, 0x00 }, 2 },
> > +	{ { 0xf1, 0xdd }, 2 },
> > +	{ { 0xf0, 0x02 }, 2 },
> > +	{ { 0xf1, 0xc8 }, 2 },
> > +	{ { 0xf0, 0x09 }, 2 },
> > +	{ { 0xf1, 0xdc }, 2 },
> > +	{ { 0xf0, 0x00 }, 2 },
> > +	{ { 0xf1, 0xdd }, 2 },
> > +	{ { 0xf0, 0x02 }, 2 },
> > +	{ { 0xd7, 0x01 }, 2 },
> > +};
> 
> 
> As noted by Michael, this could likely be simplified. the "0x01" on the last
> line could be the same coding as wellspring mode for bcm5974.

I've simplified this for the next submission.

-- Chase

Re: [PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

[Chase Douglas]

On Tue, 2010-08-31 at 20:08 -0400, Michael Poole wrote:
> One behavior that slightly surprised me -- which I believe is a quirk
> due to userspace not expecting touchpads to have button switches -- is
> that touches on the trackpad that do not close the switch can still be
> interpreted by X as clicks.

This is actually done by the X synaptics input module. It's the defacto
X touchpad driver and enables niceties like two finger scrolling and
whatnot. You can either use it as is, use xinput/xorg.conf to manipulate
its configuration, or switch to the X evdev input module which is more
"bare" support.

FYI, our gesture support in Maverick is based on a modified version of
the X evdev input module. However, because the whole stack of desktop
libs and toolkits won't support gestures in Maverick, you lose things
like two finger scroll. Thus, we're going to leave the default X input
module for the magic trackpad to synaptics in Ubuntu.

> Once the discussions about if/how to tweak this code settle down, I'll
> put together a patch to change the "down" and "last_up" logic as I
> suggested earlier.

I actually decided to tackle this to make the patches I'm writing
easier. I'll be sending a new batch shortly.

Thanks,

-- Chase