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

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

[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 input
devices from the hid device while leaving the hid fields around.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-magicmouse.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index ee78787..2d8532d 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -404,15 +404,13 @@ static int magicmouse_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	/* we are handling the input ourselves */
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_HIDDEV);
 	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] 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>
---
 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 2d8532d..d758061 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);
@@ -419,19 +428,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] 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>
---
 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 d758061..f3ee6ab 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] 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>
---
 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 f3ee6ab..65ec2f8 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] 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 866e54e..79fc152 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1243,6 +1243,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 31601ee..631d902 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -61,6 +61,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 65ec2f8..687dde4 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 = (signed char)data[2];
 		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]);
+		return 1;
 	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");
@@ -421,7 +525,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;
@@ -429,10 +546,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);
@@ -475,8 +592,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] 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>
---
 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 687dde4..b051f7d 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 1/6] HID: magicmouse: don't allow hidinput to initialize

[Michael Poole]

On Mon, 2010-08-30 at 13:20 -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 input
> devices from the hid device while leaving the hid fields around.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/hid/hid-magicmouse.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index ee78787..2d8532d 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -404,15 +404,13 @@ static int magicmouse_probe(struct hid_device *hdev,
>  		goto err_free;
>  	}
>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	/* we are handling the input ourselves */
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_HIDDEV);
>  	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");

This effectively reverts commit 23d021167e.  Has the HID core changed so
that this won't cause problems when CONFIG_HIDRAW is disabled?

Michael Poole

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

[Michael Poole]

On Mon, 2010-08-30 at 13:20 -0400, Chase Douglas wrote:
> 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>
> ---
>  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 2d8532d..d758061 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 }
> +};
> +

While fiddling with my Magic Trackpad, I noticed that only the first
message is required to make it send multitouch reports.  The same turns
out to be true for the Magic Mouse.  I don't know what effect the second
message has.

>  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);
> @@ -419,19 +428,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();

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

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

[Michael Poole]

On Mon, 2010-08-30 at 13:20 -0400, Chase Douglas wrote:
> 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 d758061..f3ee6ab 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);

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

[Michael Poole]

On Mon, 2010-08-30 at 13:20 -0400, 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>

My original rationale for the fuzz setting was that the device is so
precise that it tends to spam applications, but that is probably too
policy-like for the kernel to enforce.

> ---
>  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 f3ee6ab..65ec2f8 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 5/6] HID: magicmouse: enable Magic Trackpad support

[Michael Poole]

On Mon, 2010-08-30 at 13:20 -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>

I have no technical concerns with the patch, just two questions.  (Once
I get the chance to test it, I expect to add my Acked-by.)

> ---
>  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 866e54e..79fc152 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1243,6 +1243,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 31601ee..631d902 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -61,6 +61,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 65ec2f8..687dde4 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);
> -			}
> -		}
> -

Maybe it is worth making magicmouse_emit_touch() return non-zero if the
touch is down, so that "last_up" can be accumulated in a single pass
(and we can remove the "down" field of the touch)?  I think that should
be a separate commit, if the idea makes sense to you.

>  		/* 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 = (signed char)data[2];
>  		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]);
> +		return 1;
>  	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 },
> +};
> +

As I mentioned in another email, only the last entry here is required to
turn on multitouch reports.  Do you know what the other entries do?

>  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");
> @@ -421,7 +525,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;
> @@ -429,10 +546,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);
> @@ -475,8 +592,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);

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

[Michael Poole]

On Mon, 2010-08-30 at 13:20 -0400, Chase Douglas wrote:
> 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>

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

I never did figure out a more elegant or robust way to figure out this
scaling factor.  Four seems like about the right number.

> ---
>  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 687dde4..b051f7d 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);

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

[Michael Poole]

On Tue, 2010-08-31 at 00:31 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 13:20 -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>
> 
> I have no technical concerns with the patch, just two questions.  (Once
> I get the chance to test it, I expect to add my Acked-by.)

Actually, one thing slipped my mind.  This patch does not apply cleanly
to Jiri's for-next tree, because the not-yet-upstream commit 7d876c05fa
changes the context at hid-magicmouse.c's line 290 before this patch.
Manually munging the context allows the commit to apply.  Could you
rebase this patch on a tree that contains that commit?

Michael

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

[Michael Poole]

On Mon, 2010-08-30 at 23:46 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 13:20 -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 input
> > devices from the hid device while leaving the hid fields around.
> > 
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > ---
> >  drivers/hid/hid-magicmouse.c |    6 ++----
> >  1 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> > index ee78787..2d8532d 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -404,15 +404,13 @@ static int magicmouse_probe(struct hid_device *hdev,
> >  		goto err_free;
> >  	}
> >  
> > -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > +	/* we are handling the input ourselves */
> > +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_HIDDEV);
> >  	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");
> 
> This effectively reverts commit 23d021167e.  Has the HID core changed so
> that this won't cause problems when CONFIG_HIDRAW is disabled?

To answer my own question, it has not changed: If CONFIG_HIDRAW is
turned off, the device will not get attached with this change, so the
driver does not get any input to process.  Turning CONFIG_HIDRAW on
restores the expected functionality.

Maybe hidinput_disconnect() should be modified instead, to clear the
fields that were causing null pointer dereferences?

Michael

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

[Chase Douglas]

On Tue, 2010-08-31 at 07:30 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 23:46 -0400, Michael Poole wrote:
> > On Mon, 2010-08-30 at 13:20 -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 input
> > > devices from the hid device while leaving the hid fields around.
> > > 
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > ---
> > >  drivers/hid/hid-magicmouse.c |    6 ++----
> > >  1 files changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> > > index ee78787..2d8532d 100644
> > > --- a/drivers/hid/hid-magicmouse.c
> > > +++ b/drivers/hid/hid-magicmouse.c
> > > @@ -404,15 +404,13 @@ static int magicmouse_probe(struct hid_device *hdev,
> > >  		goto err_free;
> > >  	}
> > >  
> > > -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > > +	/* we are handling the input ourselves */
> > > +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_HIDDEV);
> > >  	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");
> > 
> > This effectively reverts commit 23d021167e.  Has the HID core changed so
> > that this won't cause problems when CONFIG_HIDRAW is disabled?
> 
> To answer my own question, it has not changed: If CONFIG_HIDRAW is
> turned off, the device will not get attached with this change, so the
> driver does not get any input to process.  Turning CONFIG_HIDRAW on
> restores the expected functionality.
> 
> Maybe hidinput_disconnect() should be modified instead, to clear the
> fields that were causing null pointer dereferences?

Ahh, I should have looked at the git log of the driver :). I'll look
into the hidinput stack to figure out the best way of handling this.

Thanks,

-- Chase

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

[Chase Douglas]

On Tue, 2010-08-31 at 07:30 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 23:46 -0400, Michael Poole wrote:
> > On Mon, 2010-08-30 at 13:20 -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 input
> > > devices from the hid device while leaving the hid fields around.
> > > 
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > ---
> > >  drivers/hid/hid-magicmouse.c |    6 ++----
> > >  1 files changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> > > index ee78787..2d8532d 100644
> > > --- a/drivers/hid/hid-magicmouse.c
> > > +++ b/drivers/hid/hid-magicmouse.c
> > > @@ -404,15 +404,13 @@ static int magicmouse_probe(struct hid_device *hdev,
> > >  		goto err_free;
> > >  	}
> > >  
> > > -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > > +	/* we are handling the input ourselves */
> > > +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_HIDDEV);
> > >  	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");
> > 
> > This effectively reverts commit 23d021167e.  Has the HID core changed so
> > that this won't cause problems when CONFIG_HIDRAW is disabled?
> 
> To answer my own question, it has not changed: If CONFIG_HIDRAW is
> turned off, the device will not get attached with this change, so the
> driver does not get any input to process.  Turning CONFIG_HIDRAW on
> restores the expected functionality.
> 
> Maybe hidinput_disconnect() should be modified instead, to clear the
> fields that were causing null pointer dereferences?

That may be correct, but I don't really know the hid layer well enough
to be sure.

I found that the hid-picolcd driver fakes out hid_hw_start by setting
the hdev->claimed field to HID_CLAIMED_INPUT before calling the
function. It then resets the bit after the function call. This ensures
that the device is initialized.

Even if the hidinput layer is fixed so it doesn't panic after a hidinput
device is disconnected, I like this change because it means we aren't
creating a device and then deleting it immediately. I think this is what
I noticed in my Xorg.0.log file. It's really strange to see a device pop
in and out.

I'll be sending a new set of patches with this change.

Thanks,

-- Chase

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

[Chase Douglas]

On Tue, 2010-08-31 at 00:26 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 13:20 -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>
> 
> I have no technical concerns with the patch, just two questions.  (Once
> I get the chance to test it, I expect to add my Acked-by.)

<snip>
  
> > -		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);
> > -			}
> > -		}
> > -
> 
> Maybe it is worth making magicmouse_emit_touch() return non-zero if the
> touch is down, so that "last_up" can be accumulated in a single pass
> (and we can remove the "down" field of the touch)?  I think that should
> be a separate commit, if the idea makes sense to you.

That makes sense to me without diving into the code. I don't really have
the time to work on it right now though, so is it ok if these patches
are applied first?

<snip>

> > @@ -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 I mentioned in another email, only the last entry here is required to
> turn on multitouch reports.  Do you know what the other entries do?

No clue. I just copied what OS X was sending to the device. It works for
myself and others, so I haven't messed with it.

Thanks,

-- Chase

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

[Chase Douglas]

On Tue, 2010-08-31 at 00:36 -0400, Michael Poole wrote:
> On Tue, 2010-08-31 at 00:31 -0400, Michael Poole wrote:
> > On Mon, 2010-08-30 at 13:20 -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>
> > 
> > I have no technical concerns with the patch, just two questions.  (Once
> > I get the chance to test it, I expect to add my Acked-by.)
> 
> Actually, one thing slipped my mind.  This patch does not apply cleanly
> to Jiri's for-next tree, because the not-yet-upstream commit 7d876c05fa
> changes the context at hid-magicmouse.c's line 290 before this patch.
> Manually munging the context allows the commit to apply.  Could you
> rebase this patch on a tree that contains that commit?

An oversight on my part. I'll rebase onto Jiri's for-next tree and
resubmit.

Thanks,

-- Chase

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

[Chase Douglas]

On Mon, 2010-08-30 at 23:59 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 13:20 -0400, 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>
> 
> My original rationale for the fuzz setting was that the device is so
> precise that it tends to spam applications, but that is probably too
> policy-like for the kernel to enforce.

This driver should be converted to the new slotted MT protocol as well,
which will reduce the amount of data sent for each event. Hopefully I
can get to that at some point in the near future, if no one beats me to
it :).

Thanks,

-- Chase