[PATCH 0/7 v3] HID: magicmouse: fixes and support for the Magic Trackpad (v3)

[PATCH 0/7 v3] HID: magicmouse: fixes and support for the Magic Trackpad (v3)

[Chase Douglas]

The following patches are a reworking of my previous submission to
incorporate all the comments to date. The only patch of contention, removing
the fuzz factor, has been removed for now.

Thanks,

-- Chase

[PATCH 1/7 v3] 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>
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");
-- 
1.7.1

[PATCH 2/7 v3] HID: magicmouse: simplify multitouch feature request

[Chase Douglas]

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

Only the first feature request is required to put the Magic Mouse into
multitouch mode. This is also the case for the Magic Trackpad, for which
support will be added in a later commit.

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

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index d38b529..8a2fe78 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -380,8 +380,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 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 };
+	__u8 feature[] = { 0xd7, 0x01 };
 	struct input_dev *input;
 	struct magicmouse_sc *msc;
 	struct hid_report *report;
@@ -426,17 +425,10 @@ static int magicmouse_probe(struct hid_device *hdev,
 	}
 	report->size = 6;
 
-	ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
+	ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
 			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",
+	if (ret != sizeof(feature)) {
+		dev_err(&hdev->dev, "unable to request touch data (%d)\n",
 				ret);
 		goto err_stop_hw;
 	}
-- 
1.7.1

[PATCH 3/7 v3] 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 8a2fe78..2ee59a8 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/7 v3] HID: magicmouse: simplify touch down logic

[Chase Douglas]

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

For the MT protocol, we need to properly keep track of each down touch.
This change simplifies the logic, and should make things easier when
support for the Magic Trackpad is added.

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

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 2ee59a8..0fbca59 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -98,7 +98,6 @@ struct magicmouse_sc {
 		short scroll_x;
 		short scroll_y;
 		u8 size;
-		u8 down;
 	} touches[16];
 	int tracking_ids[16];
 };
@@ -227,8 +226,6 @@ 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) {
-		msc->touches[id].down = 1;
-
 		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);
@@ -241,6 +238,9 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 
 		input_mt_sync(input);
 	}
+
+	if (down)
+		msc->ntouches++;
 }
 
 static int magicmouse_raw_event(struct hid_device *hdev,
@@ -248,7 +248,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, y, ts, ii, clicks, npoints;
 
 	switch (data[0]) {
 	case 0x10:
@@ -265,22 +265,13 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
 		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
 		msc->last_timestamp = ts;
-		msc->ntouches = (size - 6) / 8;
-		for (ii = 0; ii < msc->ntouches; ii++)
+		npoints = (size - 6) / 8;
+		msc->ntouches = 0;
+		for (ii = 0; ii < npoints; 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);
-			}
-		}
+		if (report_touches && msc->ntouches == 0)
+			input_mt_sync(input);
 
 		/* When emulating three-button mode, it is important
 		 * to have the current touch information before
-- 
1.7.1

[PATCH 5/7 v3] HID: magicmouse: remove timestamp logic

[Chase Douglas]

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

The timestamps from the device are currently stored in the private data
structure. These aren't used, so remove them. I've left a comment
detailing the protocol for future reference.

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

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 0fbca59..9fc272f 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -71,11 +71,6 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
  * struct magicmouse_sc - Tracks Magic Mouse-specific data.
  * @input: Input device through which we report events.
  * @quirks: Currently unused.
- * @last_timestamp: Timestamp from most recent (18-bit) touch report
- *     (units of milliseconds over short windows, but seems to
- *     increase faster when there are no touches).
- * @delta_time: 18-bit difference between the two most recent touch
- *     reports from the mouse.
  * @ntouches: Number of touches in most recent touch report.
  * @scroll_accel: Number of consecutive scroll motions.
  * @scroll_jiffies: Time of last scroll motion.
@@ -86,8 +81,6 @@ struct magicmouse_sc {
 	struct input_dev *input;
 	unsigned long quirks;
 
-	int last_timestamp;
-	int delta_time;
 	int ntouches;
 	int scroll_accel;
 	unsigned long scroll_jiffies;
@@ -248,7 +241,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, npoints;
+	int x, y, ii, clicks, npoints;
 
 	switch (data[0]) {
 	case 0x10:
@@ -262,9 +255,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		/* Expect six bytes of prefix, and N*8 bytes of touch data. */
 		if (size < 6 || ((size - 6) % 8) != 0)
 			return 0;
-		ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
-		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
-		msc->last_timestamp = ts;
 		npoints = (size - 6) / 8;
 		msc->ntouches = 0;
 		for (ii = 0; ii < npoints; ii++)
@@ -280,6 +270,12 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
 		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
 		clicks = data[3];
+
+		/* The following bits provide a device specific timestamp. They
+		 * are unused here.
+		 *
+		 * ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
+		 */
 		break;
 	case 0x20: /* Theoretically battery status (0-100), but I have
 		    * never seen it -- maybe it is only upon request.
-- 
1.7.1

[PATCH 6/7 v3] 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>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
 drivers/hid/hid-core.c       |    1 +
 drivers/hid/hid-ids.h        |    1 +
 drivers/hid/hid-magicmouse.c |  192 +++++++++++++++++++++++++++++++++---------
 3 files changed, 155 insertions(+), 39 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 9fc272f..6f13e56 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,
@@ -67,6 +70,15 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
 
 #define SCROLL_ACCEL_DEFAULT 7
 
+/* Single touch emulation should only begin when no touches are currently down.
+ * This is true when single_touch_id is equal to NO_TOUCHES. If multiple touches
+ * are down and the touch providing for single touch emulation is lifted,
+ * single_touch_id is equal to SINGLE_TOUCH_UP. While single touch emulation is
+ * occuring, single_touch_id corresponds with the tracking id of the touch used.
+ */
+#define NO_TOUCHES -1
+#define SINGLE_TOUCH_UP -2
+
 /**
  * struct magicmouse_sc - Tracks Magic Mouse-specific data.
  * @input: Input device through which we report events.
@@ -93,6 +105,7 @@ struct magicmouse_sc {
 		u8 size;
 	} touches[16];
 	int tracking_ids[16];
+	int single_touch_id;
 };
 
 static int magicmouse_firm_touch(struct magicmouse_sc *msc)
@@ -158,15 +171,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;
@@ -217,6 +244,13 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		}
 	}
 
+	if (down) {
+		msc->ntouches++;
+		if (msc->single_touch_id == NO_TOUCHES)
+			msc->single_touch_id = id;
+	} else if (msc->single_touch_id == id)
+		msc->single_touch_id = SINGLE_TOUCH_UP;
+
 	/* Generate the input events for this touch. */
 	if (report_touches && down) {
 		input_report_abs(input, ABS_MT_TRACKING_ID, id);
@@ -226,14 +260,15 @@ 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);
 	}
-
-	if (down)
-		msc->ntouches++;
 }
 
 static int magicmouse_raw_event(struct hid_device *hdev,
@@ -241,7 +276,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, ii, clicks, npoints;
+	int x = 0, y = 0, ii, clicks = 0, npoints;
 
 	switch (data[0]) {
 	case 0x10:
@@ -251,7 +286,30 @@ 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;
+		npoints = (size - 4) / 9;
+		msc->ntouches = 0;
+		for (ii = 0; ii < npoints; ii++)
+			magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
+
+		/* We don't need an MT sync here because trackpad emits a
+		 * BTN_TOUCH event in a new frame when all touches are released.
+		 */
+		if (msc->ntouches == 0)
+			msc->single_touch_id = NO_TOUCHES;
+
+		clicks = data[1];
+
+		/* The following bits provide a device specific timestamp. They
+		 * are unused here.
+		 *
+		 * ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
+		 */
+		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;
@@ -277,6 +335,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		 * ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
 		 */
 		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.
 		    */
@@ -288,9 +354,25 @@ 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 (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;
 }
@@ -326,18 +408,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) {
@@ -347,16 +438,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, 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_POSITION_X, -1100, 1358,
-				4, 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,
-				4, 0);
+		if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+			input_set_abs_params(input, ABS_MT_POSITION_X, -1100,
+				1358, 4, 0);
+			input_set_abs_params(input, ABS_MT_POSITION_Y, -1589,
+				2047, 4, 0);
+		} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+			input_set_abs_params(input, ABS_X, -2909, 3167, 4, 0);
+			input_set_abs_params(input, ABS_Y, -2456, 2565, 4, 0);
+			input_set_abs_params(input, ABS_MT_POSITION_X, -2909,
+				3167, 4, 0);
+			input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
+				2565, 4, 0);
+		}
 	}
 
 	if (report_undeciphered) {
@@ -385,6 +486,8 @@ static int magicmouse_probe(struct hid_device *hdev,
 	msc->quirks = id->driver_data;
 	hid_set_drvdata(hdev, msc);
 
+	msc->single_touch_id = NO_TOUCHES;
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		dev_err(&hdev->dev, "magicmouse hid parse failed\n");
@@ -405,7 +508,16 @@ 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);
+	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);
+	}
+
 	if (!report) {
 		dev_err(&hdev->dev, "unable to register touch report\n");
 		ret = -ENOMEM;
@@ -456,8 +568,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 7/7 v3] 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 6f13e56..0792b16 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -254,8 +254,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/7 v3] HID: magicmouse: simplify touch down logic

[Michael Poole]

On Tue, 2010-08-31 at 21:56 -0400, Chase Douglas wrote:
> From: Chase Douglas <chase.douglas@ubuntu.com>
> 
> For the MT protocol, we need to properly keep track of each down touch.
> This change simplifies the logic, and should make things easier when
> support for the Magic Trackpad is added.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>

Thanks, this looks slightly cleaner than what I had in mind.

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

> ---
>  drivers/hid/hid-magicmouse.c |   27 +++++++++------------------
>  1 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 2ee59a8..0fbca59 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -98,7 +98,6 @@ struct magicmouse_sc {
>  		short scroll_x;
>  		short scroll_y;
>  		u8 size;
> -		u8 down;
>  	} touches[16];
>  	int tracking_ids[16];
>  };
> @@ -227,8 +226,6 @@ 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) {
> -		msc->touches[id].down = 1;
> -
>  		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);
> @@ -241,6 +238,9 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  
>  		input_mt_sync(input);
>  	}
> +
> +	if (down)
> +		msc->ntouches++;
>  }
>  
>  static int magicmouse_raw_event(struct hid_device *hdev,
> @@ -248,7 +248,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, y, ts, ii, clicks, npoints;
>  
>  	switch (data[0]) {
>  	case 0x10:
> @@ -265,22 +265,13 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
>  		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
>  		msc->last_timestamp = ts;
> -		msc->ntouches = (size - 6) / 8;
> -		for (ii = 0; ii < msc->ntouches; ii++)
> +		npoints = (size - 6) / 8;
> +		msc->ntouches = 0;
> +		for (ii = 0; ii < npoints; 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);
> -			}
> -		}
> +		if (report_touches && msc->ntouches == 0)
> +			input_mt_sync(input);
>  
>  		/* When emulating three-button mode, it is important
>  		 * to have the current touch information before

Re: [PATCH 5/7 v3] HID: magicmouse: remove timestamp logic

[Michael Poole]

On Tue, 2010-08-31 at 21:56 -0400, Chase Douglas wrote:
> From: Chase Douglas <chase.douglas@ubuntu.com>
> 
> The timestamps from the device are currently stored in the private data
> structure. These aren't used, so remove them. I've left a comment
> detailing the protocol for future reference.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>

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

> ---
>  drivers/hid/hid-magicmouse.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 0fbca59..9fc272f 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -71,11 +71,6 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
>   * struct magicmouse_sc - Tracks Magic Mouse-specific data.
>   * @input: Input device through which we report events.
>   * @quirks: Currently unused.
> - * @last_timestamp: Timestamp from most recent (18-bit) touch report
> - *     (units of milliseconds over short windows, but seems to
> - *     increase faster when there are no touches).
> - * @delta_time: 18-bit difference between the two most recent touch
> - *     reports from the mouse.
>   * @ntouches: Number of touches in most recent touch report.
>   * @scroll_accel: Number of consecutive scroll motions.
>   * @scroll_jiffies: Time of last scroll motion.
> @@ -86,8 +81,6 @@ struct magicmouse_sc {
>  	struct input_dev *input;
>  	unsigned long quirks;
>  
> -	int last_timestamp;
> -	int delta_time;
>  	int ntouches;
>  	int scroll_accel;
>  	unsigned long scroll_jiffies;
> @@ -248,7 +241,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, npoints;
> +	int x, y, ii, clicks, npoints;
>  
>  	switch (data[0]) {
>  	case 0x10:
> @@ -262,9 +255,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		/* Expect six bytes of prefix, and N*8 bytes of touch data. */
>  		if (size < 6 || ((size - 6) % 8) != 0)
>  			return 0;
> -		ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
> -		msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
> -		msc->last_timestamp = ts;
>  		npoints = (size - 6) / 8;
>  		msc->ntouches = 0;
>  		for (ii = 0; ii < npoints; ii++)
> @@ -280,6 +270,12 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
>  		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
>  		clicks = data[3];
> +
> +		/* The following bits provide a device specific timestamp. They
> +		 * are unused here.
> +		 *
> +		 * ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
> +		 */
>  		break;
>  	case 0x20: /* Theoretically battery status (0-100), but I have
>  		    * never seen it -- maybe it is only upon request.

Re: [PATCH 2/7 v3] HID: magicmouse: simplify multitouch feature request

[Henrik Rydberg]

On 09/01/2010 03:56 AM, Chase Douglas wrote:

> From: Chase Douglas <chase.douglas@ubuntu.com>
> 
> Only the first feature request is required to put the Magic Mouse into
> multitouch mode. This is also the case for the Magic Trackpad, for which
> support will be added in a later commit.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> Acked-by: Michael Poole <mdpoole@troilus.org>
> ---


Since this is starting to look awfully similar to the mode switch found in
appletouch and bcm5974, I wonder what happens to the stream from the device when
the connection to hid is closed. It could be that the mode switch should be
moved to open/close instead.

Henrik

>  drivers/hid/hid-magicmouse.c |   16 ++++------------
>  1 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index d38b529..8a2fe78 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -380,8 +380,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  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 };
> +	__u8 feature[] = { 0xd7, 0x01 };
>  	struct input_dev *input;
>  	struct magicmouse_sc *msc;
>  	struct hid_report *report;
> @@ -426,17 +425,10 @@ static int magicmouse_probe(struct hid_device *hdev,
>  	}
>  	report->size = 6;
>  
> -	ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
> +	ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
>  			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",
> +	if (ret != sizeof(feature)) {
> +		dev_err(&hdev->dev, "unable to request touch data (%d)\n",
>  				ret);
>  		goto err_stop_hw;
>  	}

Re: [PATCH 2/7 v3] HID: magicmouse: simplify multitouch feature request

[Chase Douglas]

On Wed, 2010-09-01 at 09:43 +0200, Henrik Rydberg wrote:
> On 09/01/2010 03:56 AM, Chase Douglas wrote:
> 
> > From: Chase Douglas <chase.douglas@ubuntu.com>
> > 
> > Only the first feature request is required to put the Magic Mouse into
> > multitouch mode. This is also the case for the Magic Trackpad, for which
> > support will be added in a later commit.
> > 
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > Acked-by: Michael Poole <mdpoole@troilus.org>
> > ---
> 
> 
> Since this is starting to look awfully similar to the mode switch found in
> appletouch and bcm5974, I wonder what happens to the stream from the device when
> the connection to hid is closed. It could be that the mode switch should be
> moved to open/close instead.

I don't really understand what you are proposing. It sounds like maybe
you want the device to continue to function in a non-multitouch way
after hid disconnection, but the device needs hid to function at all.

The closest thing I can think to what you are saying is to do whatever
it takes on module removal so that it goes back to functioning in the
non-multitouch mode. That may be possible.

> >  drivers/hid/hid-magicmouse.c |   16 ++++------------
> >  1 files changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> > index d38b529..8a2fe78 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -380,8 +380,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> >  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 };
> > +	__u8 feature[] = { 0xd7, 0x01 };
> >  	struct input_dev *input;
> >  	struct magicmouse_sc *msc;
> >  	struct hid_report *report;
> > @@ -426,17 +425,10 @@ static int magicmouse_probe(struct hid_device *hdev,
> >  	}
> >  	report->size = 6;
> >  
> > -	ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
> > +	ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
> >  			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",
> > +	if (ret != sizeof(feature)) {
> > +		dev_err(&hdev->dev, "unable to request touch data (%d)\n",
> >  				ret);
> >  		goto err_stop_hw;
> >  	}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/7 v3] HID: magicmouse: simplify multitouch feature request

[Henrik Rydberg]

On 09/01/2010 02:34 PM, Chase Douglas wrote:

> On Wed, 2010-09-01 at 09:43 +0200, Henrik Rydberg wrote:
>> On 09/01/2010 03:56 AM, Chase Douglas wrote:
>>
>>> From: Chase Douglas <chase.douglas@ubuntu.com>
>>>
>>> Only the first feature request is required to put the Magic Mouse into
>>> multitouch mode. This is also the case for the Magic Trackpad, for which
>>> support will be added in a later commit.
>>>
>>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>>> Acked-by: Michael Poole <mdpoole@troilus.org>
>>> ---
>>
>>
>> Since this is starting to look awfully similar to the mode switch found in
>> appletouch and bcm5974, I wonder what happens to the stream from the device when
>> the connection to hid is closed. It could be that the mode switch should be
>> moved to open/close instead.
> 
> I don't really understand what you are proposing. It sounds like maybe
> you want the device to continue to function in a non-multitouch way
> after hid disconnection, but the device needs hid to function at all.


Suspend/resume, broken bt connections, etc. There was an issue with the bcm5974
macbook trackpads not functioning properly after a close unless returned to
normal mode. The mode switch code was moved to the open/close functions for that
reason.


> The closest thing I can think to what you are saying is to do whatever
> it takes on module removal so that it goes back to functioning in the
> non-multitouch mode. That may be possible.


Sounds like a good start. Doing a suspend test just now I could not get the
device up again, for whatever reason.

Henrik

Re: [PATCH 2/7 v3] HID: magicmouse: simplify multitouch feature request

[Chase Douglas]

On Wed, 2010-09-01 at 15:14 +0200, Henrik Rydberg wrote:
> On 09/01/2010 02:34 PM, Chase Douglas wrote:
> 
> > On Wed, 2010-09-01 at 09:43 +0200, Henrik Rydberg wrote:
> >> On 09/01/2010 03:56 AM, Chase Douglas wrote:
> >>
> >>> From: Chase Douglas <chase.douglas@ubuntu.com>
> >>>
> >>> Only the first feature request is required to put the Magic Mouse into
> >>> multitouch mode. This is also the case for the Magic Trackpad, for which
> >>> support will be added in a later commit.
> >>>
> >>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >>> Acked-by: Michael Poole <mdpoole@troilus.org>
> >>> ---
> >>
> >>
> >> Since this is starting to look awfully similar to the mode switch found in
> >> appletouch and bcm5974, I wonder what happens to the stream from the device when
> >> the connection to hid is closed. It could be that the mode switch should be
> >> moved to open/close instead.
> > 
> > I don't really understand what you are proposing. It sounds like maybe
> > you want the device to continue to function in a non-multitouch way
> > after hid disconnection, but the device needs hid to function at all.
> 
> 
> Suspend/resume, broken bt connections, etc. There was an issue with the bcm5974
> macbook trackpads not functioning properly after a close unless returned to
> normal mode. The mode switch code was moved to the open/close functions for that
> reason.

I'm not sure how applicable this is to a bluetooth device though. On
suspend, the device is disconnected. On resume, the device should
reconnect through the hid layer again.

> > The closest thing I can think to what you are saying is to do whatever
> > it takes on module removal so that it goes back to functioning in the
> > non-multitouch mode. That may be possible.
> 
> 
> Sounds like a good start. Doing a suspend test just now I could not get the
> device up again, for whatever reason.

I'm having some sporadic issues too, but when I hit issues and I use
ftrace I see that the hid layer isn't doing everything it should. I'm
not sure how much is the fault of hid vs the driver.

-- Chase

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

[Jiri Slaby]

On 09/01/2010 03:56 AM, 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>
> 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;

This looks weird. Is there any past discussion about why you cannot use
hidinput and you have to do all the input bits yourself while cheating
this very weird way?

thanks,
-- 
js

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

[Michael Poole]

Jiri Slaby wrote:
> This looks weird. Is there any past discussion about why you cannot use
> hidinput and you have to do all the input bits yourself while cheating
> this very weird way?

(Resending because I didn't notice Gmail was in "rich text" mode, and
that kept my earlier message from reaching the vger lists.  Apologies
to the individuals who get a second copy of this message.)

The Magic Mouse and Magic Trackpad do not publish HID descriptors for
the multitouch reports.  Given the variable-length report packets --
and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
packets -- it would be non-trivial to write accurate descriptors that
the HID core can use.  (Someone wrote a patch to try that a few months
ago.  It ended up adding significantly more lines to hid-magicmouse.c
than it removed, and it was not obvious to me that it got the Report
Count fields correct.)

Michael Poole

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

[Jiri Slaby]

On 09/02/2010 01:57 AM, Michael Poole wrote:
> Jiri Slaby wrote:
>> This looks weird. Is there any past discussion about why you cannot use
>> hidinput and you have to do all the input bits yourself while cheating
>> this very weird way?
> 
> The Magic Mouse and Magic Trackpad do not publish HID descriptors for
> the multitouch reports.  Given the variable-length report packets --
> and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
> packets -- it would be non-trivial to write accurate descriptors that
> the HID core can use.  (Someone wrote a patch to try that a few months
> ago.  It ended up adding significantly more lines to hid-magicmouse.c
> than it removed, and it was not obvious to me that it got the Report
> Count fields correct.)

Ok, makes sense. The proper solution is to call a driver hook in
hidinput_connect to do the mapping instead of report parsing done there,
right? (And not setting [gs]etkeycode in that case.)

Then the part of magicmouse_setup_input starting at the first __set_bit
will be exactly the hook.

Otherwise this hack looks like a hard cross-layer breakage.

/me wonders how HID issuers are creative in breaking standards.

regards,
-- 
js

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

[Michael Poole]

On Thu, Sep 2, 2010 at 5:28 AM, Jiri Slaby wrote:
> On 09/02/2010 01:57 AM, Michael Poole wrote:
>> Jiri Slaby wrote:
>>> This looks weird. Is there any past discussion about why you cannot use
>>> hidinput and you have to do all the input bits yourself while cheating
>>> this very weird way?
>>
>> The Magic Mouse and Magic Trackpad do not publish HID descriptors for
>> the multitouch reports.  Given the variable-length report packets --
>> and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
>> packets -- it would be non-trivial to write accurate descriptors that
>> the HID core can use.  (Someone wrote a patch to try that a few months
>> ago.  It ended up adding significantly more lines to hid-magicmouse.c
>> than it removed, and it was not obvious to me that it got the Report
>> Count fields correct.)
>
> Ok, makes sense. The proper solution is to call a driver hook in
> hidinput_connect to do the mapping instead of report parsing done there,
> right? (And not setting [gs]etkeycode in that case.)

I do not know how this would work with the current HID core; could you
elaborate?  For the Magic Mouse, there is a valid descriptor for the
0x10 (traditional mouse motion and clicks) report, and a pretty
trivial descriptor in the vendor-defined usage region (which I assume
Apple drivers use to identify the ensemble of multi-touch,
mouse-up/down, laser status and battery level reports).  How would the
driver's input_mapping() hook, or any other hook, map that single
descriptor into fields for each of the parts of a multi-touch report,
or support the other reports?  How would hid-magicmouse describe the
report so that hid-core handles the variable-length multi-touch
reports properly?  As far as I can tell, hid_report_raw_event() and
its callees assume each report has fixed length.

> Then the part of magicmouse_setup_input starting at the first __set_bit
> will be exactly the hook.
>
> Otherwise this hack looks like a hard cross-layer breakage.
>
> /me wonders how HID issuers are creative in breaking standards.

In Apple's case, apparently fairly creative.  Devices from other
sources seem to generate one report per touch, rather than cram as
many touches as possible into a single report.

Michael Poole

Re: [PATCH 4/7 v3] HID: magicmouse: simplify touch down logic

[Jiri Kosina]

On Tue, 31 Aug 2010, Michael Poole wrote:

> On Tue, 2010-08-31 at 21:56 -0400, Chase Douglas wrote:
> > From: Chase Douglas <chase.douglas@ubuntu.com>
> > 
> > For the MT protocol, we need to properly keep track of each down touch.
> > This change simplifies the logic, and should make things easier when
> > support for the Magic Trackpad is added.
> > 
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> 
> Thanks, this looks slightly cleaner than what I had in mind.
> 
> Acked-by: Michael Poole <mdpoole@troilus.org>

Applied ('magicmouse' branch). Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 5/7 v3] HID: magicmouse: remove timestamp logic

[Jiri Kosina]

On Tue, 31 Aug 2010, Michael Poole wrote:

> On Tue, 2010-08-31 at 21:56 -0400, Chase Douglas wrote:
> > From: Chase Douglas <chase.douglas@ubuntu.com>
> > 
> > The timestamps from the device are currently stored in the private data
> > structure. These aren't used, so remove them. I've left a comment
> > detailing the protocol for future reference.
> > 
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> 
> Acked-by: Michael Poole <mdpoole@troilus.org>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Kosina]

On Tue, 31 Aug 2010, 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>
> 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 6f13e56..0792b16 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -254,8 +254,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);

Care to fold this into "[PATCH 3/7 v3] HID: magicmouse: simplify touch 
data bit manipulation", which is actually adding this?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Slaby]

On 09/02/2010 02:06 PM, Michael Poole wrote:
> On Thu, Sep 2, 2010 at 5:28 AM, Jiri Slaby wrote:
>> On 09/02/2010 01:57 AM, Michael Poole wrote:
>>> Jiri Slaby wrote:
>>>> This looks weird. Is there any past discussion about why you cannot use
>>>> hidinput and you have to do all the input bits yourself while cheating
>>>> this very weird way?
>>>
>>> The Magic Mouse and Magic Trackpad do not publish HID descriptors for
>>> the multitouch reports.  Given the variable-length report packets --
>>> and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
>>> packets -- it would be non-trivial to write accurate descriptors that
>>> the HID core can use.  (Someone wrote a patch to try that a few months
>>> ago.  It ended up adding significantly more lines to hid-magicmouse.c
>>> than it removed, and it was not obvious to me that it got the Report
>>> Count fields correct.)
>>
>> Ok, makes sense. The proper solution is to call a driver hook in
>> hidinput_connect to do the mapping instead of report parsing done there,
>> right? (And not setting [gs]etkeycode in that case.)
> 
> I do not know how this would work with the current HID core; could you
> elaborate?

No way. You would have to implement that.

>  For the Magic Mouse, there is a valid descriptor for the
> 0x10 (traditional mouse motion and clicks) report, and a pretty
> trivial descriptor in the vendor-defined usage region (which I assume
> Apple drivers use to identify the ensemble of multi-touch,
> mouse-up/down, laser status and battery level reports).  How would the
> driver's input_mapping() hook, or any other hook, map that single
> descriptor into fields for each of the parts of a multi-touch report,
> or support the other reports?  How would hid-magicmouse describe the
> report so that hid-core handles the variable-length multi-touch
> reports properly?  As far as I can tell, hid_report_raw_event() and
> its callees assume each report has fixed length.

If I understand correctly, you have 2 reports. One report is standard
HID and one is very specific and broken.
1) I don't understand where events from the standard report are going
now while no output is claimed. In other words, why you return from
raw_event in the default case 0, there is nobody to eat the data, right?
2) You handle events from the broken (or missing) report in
magicmouse_raw_event and it will remain as is.


So in the end there will be some .parse_reports hook called from inside
hidinput_connect or somewhere instead of parsing and you will do the job
you currently do in magicmouse_setup_input except the input structure
setup (this will be done generically in hid core).

The approach you have now is prone to errors, because somebody in the
future may add a new claimant without bearing in mind to update these
drivers.

regards,
-- 
js

Re: [PATCH 2/7 v3] HID: magicmouse: simplify multitouch feature request

[Jiri Kosina]

On Tue, 31 Aug 2010, Chase Douglas wrote:

> From: Chase Douglas <chase.douglas@ubuntu.com>
> 
> Only the first feature request is required to put the Magic Mouse into
> multitouch mode. This is also the case for the Magic Trackpad, for which
> support will be added in a later commit.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> Acked-by: Michael Poole <mdpoole@troilus.org>

Applied, thanks.

> ---
>  drivers/hid/hid-magicmouse.c |   16 ++++------------
>  1 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index d38b529..8a2fe78 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -380,8 +380,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>  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 };
> +	__u8 feature[] = { 0xd7, 0x01 };
>  	struct input_dev *input;
>  	struct magicmouse_sc *msc;
>  	struct hid_report *report;
> @@ -426,17 +425,10 @@ static int magicmouse_probe(struct hid_device *hdev,
>  	}
>  	report->size = 6;
>  
> -	ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
> +	ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
>  			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",
> +	if (ret != sizeof(feature)) {
> +		dev_err(&hdev->dev, "unable to request touch data (%d)\n",
>  				ret);
>  		goto err_stop_hw;
>  	}
> -- 
> 1.7.1
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Kosina]

On Tue, 31 Aug 2010, 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>
> Acked-by: Michael Poole <mdpoole@troilus.org>

Applied, thank you.

> ---
>  drivers/hid/hid-core.c       |    1 +
>  drivers/hid/hid-ids.h        |    1 +
>  drivers/hid/hid-magicmouse.c |  192 +++++++++++++++++++++++++++++++++---------
>  3 files changed, 155 insertions(+), 39 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 9fc272f..6f13e56 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,
> @@ -67,6 +70,15 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
>  
>  #define SCROLL_ACCEL_DEFAULT 7
>  
> +/* Single touch emulation should only begin when no touches are currently down.
> + * This is true when single_touch_id is equal to NO_TOUCHES. If multiple touches
> + * are down and the touch providing for single touch emulation is lifted,
> + * single_touch_id is equal to SINGLE_TOUCH_UP. While single touch emulation is
> + * occuring, single_touch_id corresponds with the tracking id of the touch used.
> + */
> +#define NO_TOUCHES -1
> +#define SINGLE_TOUCH_UP -2
> +
>  /**
>   * struct magicmouse_sc - Tracks Magic Mouse-specific data.
>   * @input: Input device through which we report events.
> @@ -93,6 +105,7 @@ struct magicmouse_sc {
>  		u8 size;
>  	} touches[16];
>  	int tracking_ids[16];
> +	int single_touch_id;
>  };
>  
>  static int magicmouse_firm_touch(struct magicmouse_sc *msc)
> @@ -158,15 +171,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;
> @@ -217,6 +244,13 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  		}
>  	}
>  
> +	if (down) {
> +		msc->ntouches++;
> +		if (msc->single_touch_id == NO_TOUCHES)
> +			msc->single_touch_id = id;
> +	} else if (msc->single_touch_id == id)
> +		msc->single_touch_id = SINGLE_TOUCH_UP;
> +
>  	/* Generate the input events for this touch. */
>  	if (report_touches && down) {
>  		input_report_abs(input, ABS_MT_TRACKING_ID, id);
> @@ -226,14 +260,15 @@ 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);
>  	}
> -
> -	if (down)
> -		msc->ntouches++;
>  }
>  
>  static int magicmouse_raw_event(struct hid_device *hdev,
> @@ -241,7 +276,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, ii, clicks, npoints;
> +	int x = 0, y = 0, ii, clicks = 0, npoints;
>  
>  	switch (data[0]) {
>  	case 0x10:
> @@ -251,7 +286,30 @@ 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;
> +		npoints = (size - 4) / 9;
> +		msc->ntouches = 0;
> +		for (ii = 0; ii < npoints; ii++)
> +			magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
> +
> +		/* We don't need an MT sync here because trackpad emits a
> +		 * BTN_TOUCH event in a new frame when all touches are released.
> +		 */
> +		if (msc->ntouches == 0)
> +			msc->single_touch_id = NO_TOUCHES;
> +
> +		clicks = data[1];
> +
> +		/* The following bits provide a device specific timestamp. They
> +		 * are unused here.
> +		 *
> +		 * ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
> +		 */
> +		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;
> @@ -277,6 +335,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		 * ts = data[3] >> 6 | data[4] << 2 | data[5] << 10;
>  		 */
>  		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.
>  		    */
> @@ -288,9 +354,25 @@ 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 (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;
>  }
> @@ -326,18 +408,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) {
> @@ -347,16 +438,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, 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_POSITION_X, -1100, 1358,
> -				4, 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,
> -				4, 0);
> +		if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> +			input_set_abs_params(input, ABS_MT_POSITION_X, -1100,
> +				1358, 4, 0);
> +			input_set_abs_params(input, ABS_MT_POSITION_Y, -1589,
> +				2047, 4, 0);
> +		} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> +			input_set_abs_params(input, ABS_X, -2909, 3167, 4, 0);
> +			input_set_abs_params(input, ABS_Y, -2456, 2565, 4, 0);
> +			input_set_abs_params(input, ABS_MT_POSITION_X, -2909,
> +				3167, 4, 0);
> +			input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
> +				2565, 4, 0);
> +		}
>  	}
>  
>  	if (report_undeciphered) {
> @@ -385,6 +486,8 @@ static int magicmouse_probe(struct hid_device *hdev,
>  	msc->quirks = id->driver_data;
>  	hid_set_drvdata(hdev, msc);
>  
> +	msc->single_touch_id = NO_TOUCHES;
> +
>  	ret = hid_parse(hdev);
>  	if (ret) {
>  		dev_err(&hdev->dev, "magicmouse hid parse failed\n");
> @@ -405,7 +508,16 @@ 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);
> +	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);
> +	}
> +
>  	if (!report) {
>  		dev_err(&hdev->dev, "unable to register touch report\n");
>  		ret = -ENOMEM;
> @@ -456,8 +568,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
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Kosina]

On Tue, 31 Aug 2010, 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>

Applied, thanks.

> ---
>  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 8a2fe78..2ee59a8 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
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Michael Poole]

On Fri, Sep 3, 2010 at 7:25 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/02/2010 02:06 PM, Michael Poole wrote:
>> On Thu, Sep 2, 2010 at 5:28 AM, Jiri Slaby wrote:
>>> On 09/02/2010 01:57 AM, Michael Poole wrote:
>>>> Jiri Slaby wrote:
>>>>> This looks weird. Is there any past discussion about why you cannot use
>>>>> hidinput and you have to do all the input bits yourself while cheating
>>>>> this very weird way?
>>>>
>>>> The Magic Mouse and Magic Trackpad do not publish HID descriptors for
>>>> the multitouch reports.  Given the variable-length report packets --
>>>> and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
>>>> packets -- it would be non-trivial to write accurate descriptors that
>>>> the HID core can use.  (Someone wrote a patch to try that a few months
>>>> ago.  It ended up adding significantly more lines to hid-magicmouse.c
>>>> than it removed, and it was not obvious to me that it got the Report
>>>> Count fields correct.)
>>>
>>> Ok, makes sense. The proper solution is to call a driver hook in
>>> hidinput_connect to do the mapping instead of report parsing done there,
>>> right? (And not setting [gs]etkeycode in that case.)
>>
>> I do not know how this would work with the current HID core; could you
>> elaborate?
>
> No way. You would have to implement that.
>
>>  For the Magic Mouse, there is a valid descriptor for the
>> 0x10 (traditional mouse motion and clicks) report, and a pretty
>> trivial descriptor in the vendor-defined usage region (which I assume
>> Apple drivers use to identify the ensemble of multi-touch,
>> mouse-up/down, laser status and battery level reports).  How would the
>> driver's input_mapping() hook, or any other hook, map that single
>> descriptor into fields for each of the parts of a multi-touch report,
>> or support the other reports?  How would hid-magicmouse describe the
>> report so that hid-core handles the variable-length multi-touch
>> reports properly?  As far as I can tell, hid_report_raw_event() and
>> its callees assume each report has fixed length.
>
> If I understand correctly, you have 2 reports. One report is standard
> HID and one is very specific and broken.
> 1) I don't understand where events from the standard report are going
> now while no output is claimed. In other words, why you return from
> raw_event in the default case 0, there is nobody to eat the data, right?

hid-magicmouse.c parses the standard-compliant report.  It is the
"case 0x10:" handler.

> 2) You handle events from the broken (or missing) report in
> magicmouse_raw_event and it will remain as is.
>
> So in the end there will be some .parse_reports hook called from inside
> hidinput_connect or somewhere instead of parsing and you will do the job
> you currently do in magicmouse_setup_input except the input structure
> setup (this will be done generically in hid core).
>
> The approach you have now is prone to errors, because somebody in the
> future may add a new claimant without bearing in mind to update these
> drivers.

What input_dev should this use for delivering events from the
non-standard report?  The only way I saw (or see) for hid-magicmouse
to get the (hid-input-created) input_dev for the mouse is to look at
hid_device->report_enum[HID_INPUT_REPORT].report_list to get a struct
hid_report*, then use hid_report->field[0]->hidinput->input.  That
seemed more fragile and abstraction-breaking than the existing
approach.

Right now, the device-specific driver's raw_event hook gets the first
shot at parsing any report; hid-core and hid-input process reports for
which raw_event returns 0.  If someone adds a new kind of claimant
that gets higher priority than both of those categories, by definition
it wouldn't be an error for it to supersede hid-magicmouse's handler.

Michael Poole

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

[Michael Poole]

On Fri, Sep 3, 2010 at 7:25 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/02/2010 02:06 PM, Michael Poole wrote:
>> On Thu, Sep 2, 2010 at 5:28 AM, Jiri Slaby wrote:
>>> On 09/02/2010 01:57 AM, Michael Poole wrote:
>>>> Jiri Slaby wrote:
>>>>> This looks weird. Is there any past discussion about why you cannot use
>>>>> hidinput and you have to do all the input bits yourself while cheating
>>>>> this very weird way?
>>>>
>>>> The Magic Mouse and Magic Trackpad do not publish HID descriptors for
>>>> the multitouch reports.  Given the variable-length report packets --
>>>> and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
>>>> packets -- it would be non-trivial to write accurate descriptors that
>>>> the HID core can use.  (Someone wrote a patch to try that a few months
>>>> ago.  It ended up adding significantly more lines to hid-magicmouse.c
>>>> than it removed, and it was not obvious to me that it got the Report
>>>> Count fields correct.)
>>>
>>> Ok, makes sense. The proper solution is to call a driver hook in
>>> hidinput_connect to do the mapping instead of report parsing done there,
>>> right? (And not setting [gs]etkeycode in that case.)
>>
>> I do not know how this would work with the current HID core; could you
>> elaborate?
>
> No way. You would have to implement that.
>
>>  For the Magic Mouse, there is a valid descriptor for the
>> 0x10 (traditional mouse motion and clicks) report, and a pretty
>> trivial descriptor in the vendor-defined usage region (which I assume
>> Apple drivers use to identify the ensemble of multi-touch,
>> mouse-up/down, laser status and battery level reports).  How would the
>> driver's input_mapping() hook, or any other hook, map that single
>> descriptor into fields for each of the parts of a multi-touch report,
>> or support the other reports?  How would hid-magicmouse describe the
>> report so that hid-core handles the variable-length multi-touch
>> reports properly?  As far as I can tell, hid_report_raw_event() and
>> its callees assume each report has fixed length.
>
> If I understand correctly, you have 2 reports. One report is standard
> HID and one is very specific and broken.
> 1) I don't understand where events from the standard report are going
> now while no output is claimed. In other words, why you return from
> raw_event in the default case 0, there is nobody to eat the data, right?

hid-magicmouse.c parses the standard-compliant report.  It is the
"case 0x10:" handler.

> 2) You handle events from the broken (or missing) report in
> magicmouse_raw_event and it will remain as is.
>
> So in the end there will be some .parse_reports hook called from inside
> hidinput_connect or somewhere instead of parsing and you will do the job
> you currently do in magicmouse_setup_input except the input structure
> setup (this will be done generically in hid core).
>
> The approach you have now is prone to errors, because somebody in the
> future may add a new claimant without bearing in mind to update these
> drivers.

What input_dev should this use for delivering events from the
non-standard report?  The only way I saw (or see) for hid-magicmouse
to get the (hid-input-created) input_dev for the mouse is to look at
hid_device->report_enum[HID_INPUT_REPORT].report_list to get a struct
hid_report*, then use hid_report->field[0]->hidinput->input.  That
seemed more fragile and abstraction-breaking than the existing
approach.

Right now, the device-specific driver's raw_event hook gets the first
shot at parsing any report; hid-core and hid-input process reports for
which raw_event returns 0.  If someone adds a new kind of claimant
that gets higher priority than both of those categories, by definition
it wouldn't be an error for it to supersede hid-magicmouse's handler.

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

[RFC PATCH] hid-magicmouse: Map inputs rather than munging input devices

[Michael Poole]

Let the HID core handle input device setup and HID-compliant reports.
This driver then only has to worry about the non-standard reports.

Signed-off-by: Michael Poole <mdpoole@troilus.org>
---
Jiri (Slaby), is this the kind of change you had in mind for getting rid
of the input device setup for hid-magicmouse?

(This applies on top of commit a462230e16 from the magicmouse branch at
git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git.)

 drivers/hid/hid-magicmouse.c |   87
+++++++++++------------------------------
 1 files changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 8791a08..3778f9b 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -278,14 +278,12 @@ static int magicmouse_raw_event(struct hid_device
*hdev,
 	struct input_dev *input = msc->input;
 	int x = 0, y = 0, ii, clicks = 0, npoints;
 
+	/* Slightly paranoid, but "input" only gets set when our
+	 * input_mapping sees the right field. */
+	if (!input)
+		return 0;
+
 	switch (data[0]) {
-	case 0x10:
-		if (size != 6)
-			return 0;
-		x = (__s16)(data[2] | data[3] << 8);
-		y = (__s16)(data[4] | data[5] << 8);
-		clicks = data[1];
-		break;
 	case TRACKPAD_REPORT_ID:
 		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
 		if (size < 4 || ((size - 4) % 9) != 0)
@@ -343,13 +341,6 @@ static int magicmouse_raw_event(struct hid_device
*hdev,
 		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.
-		    */
-	case 0x60: /* Unknown, maybe laser on/off. */
-	case 0x61: /* Laser reflection status change.
-		    * data[1]: 0 = spotted, 1 = lost
-		    */
 	default:
 		return 0;
 	}
@@ -377,36 +368,8 @@ static int magicmouse_raw_event(struct hid_device
*hdev,
 	return 1;
 }
 
-static int magicmouse_input_open(struct input_dev *dev)
-{
-	struct hid_device *hid = input_get_drvdata(dev);
-
-	return hid->ll_driver->open(hid);
-}
-
-static void magicmouse_input_close(struct input_dev *dev)
-{
-	struct hid_device *hid = input_get_drvdata(dev);
-
-	hid->ll_driver->close(hid);
-}
-
 static void magicmouse_setup_input(struct input_dev *input, struct
hid_device *hdev)
 {
-	input_set_drvdata(input, hdev);
-	input->event = hdev->ll_driver->hidinput_input_event;
-	input->open = magicmouse_input_open;
-	input->close = magicmouse_input_close;
-
-	input->name = hdev->name;
-	input->phys = hdev->phys;
-	input->uniq = hdev->uniq;
-	input->id.bustype = hdev->bus;
-	input->id.vendor = hdev->vendor;
-	input->id.product = hdev->product;
-	input->id.version = hdev->version;
-	input->dev.parent = hdev->dev.parent;
-
 	__set_bit(EV_KEY, input->evbit);
 
 	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
@@ -466,11 +429,22 @@ static void magicmouse_setup_input(struct
input_dev *input, struct hid_device *h
 	}
 }
 
+static int magicmouse_input_mapping(struct hid_device *hdev,
+		struct hid_input *hi, struct hid_field *field,
+		struct hid_usage *usage, unsigned long **bit, int *max)
+{
+	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
+
+	if (!msc->input)
+		msc->input = hi->input;
+
+	return 0;
+}
+
 static int magicmouse_probe(struct hid_device *hdev,
 	const struct hid_device_id *id)
 {
 	__u8 feature[] = { 0xd7, 0x01 };
-	struct input_dev *input;
 	struct magicmouse_sc *msc;
 	struct hid_report *report;
 	int ret;
@@ -500,8 +474,12 @@ static int magicmouse_probe(struct hid_device
*hdev,
 		goto err_free;
 	}
 
-	/* we are handling the input ourselves */
-	hidinput_disconnect(hdev);
+	/* We do this after HID maps its inputs so that the
+	 * Magic Mouse's HID-compliant report gets the right
+	 * button and axis IDs.
+	 */
+	if (msc->input)
+		magicmouse_setup_input(msc->input, hdev);
 
 	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
@@ -528,24 +506,7 @@ static int magicmouse_probe(struct hid_device
*hdev,
 		goto err_stop_hw;
 	}
 
-	input = input_allocate_device();
-	if (!input) {
-		dev_err(&hdev->dev, "can't alloc input device\n");
-		ret = -ENOMEM;
-		goto err_stop_hw;
-	}
-	magicmouse_setup_input(input, hdev);
-
-	ret = input_register_device(input);
-	if (ret) {
-		dev_err(&hdev->dev, "input device registration failed\n");
-		goto err_input;
-	}
-	msc->input = input;
-
 	return 0;
-err_input:
-	input_free_device(input);
 err_stop_hw:
 	hid_hw_stop(hdev);
 err_free:
@@ -558,7 +519,6 @@ static void magicmouse_remove(struct hid_device
*hdev)
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
 
 	hid_hw_stop(hdev);
-	input_unregister_device(msc->input);
 	kfree(msc);
 }
 
@@ -577,6 +537,7 @@ static struct hid_driver magicmouse_driver = {
 	.probe = magicmouse_probe,
 	.remove = magicmouse_remove,
 	.raw_event = magicmouse_raw_event,
+	.input_mapping = magicmouse_input_mapping,
 };
 
 static int __init magicmouse_init(void)
-- 
1.7.2.2

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

[Chase Douglas]

On Thu, 2010-09-02 at 16:55 +0200, Jiri Kosina wrote:
> On Tue, 31 Aug 2010, 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>
> > 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 6f13e56..0792b16 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -254,8 +254,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);
> 
> Care to fold this into "[PATCH 3/7 v3] HID: magicmouse: simplify touch 
> data bit manipulation", which is actually adding this?

Patch 3/7 isn't actually adding this, it just modifies the lines to
simplify touch data bit manipulation. Keeping this as a separate commit
ensures we maintain independent changes separately. As it is, Patch 3/7
doesn't actually change any functionality. If this were folded in, it
would change the functionality in an otherwise nop commit.

Thanks,

-- Chase

Re: [RFC PATCH] hid-magicmouse: Map inputs rather than munging input devices

[Chase Douglas]

On Sun, 2010-09-05 at 11:16 -0400, Michael Poole wrote:
> Let the HID core handle input device setup and HID-compliant reports.
> This driver then only has to worry about the non-standard reports.
> 
> Signed-off-by: Michael Poole <mdpoole@troilus.org>

I have tested this patch and it does prevent the magic trackpad from
panicking the machine on device removal. I am acking this patch because
it resolves the issue and the changes are reasonable when looking at the
driver. I do not know much about the internal HID layer, so I have no
comment on the overall approach taken.

Acked-by: Chase Douglas <chase.douglas@canonical.com>

When pushing to the input tree, I suggest rebasing the magic trackpad
enablement patch after this patch so someone bisecting commits won't hit
the panic.

Thanks Michael!

> ---
> Jiri (Slaby), is this the kind of change you had in mind for getting rid
> of the input device setup for hid-magicmouse?
> 
> (This applies on top of commit a462230e16 from the magicmouse branch at
> git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git.)
> 
>  drivers/hid/hid-magicmouse.c |   87
> +++++++++++------------------------------
>  1 files changed, 24 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 8791a08..3778f9b 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -278,14 +278,12 @@ static int magicmouse_raw_event(struct hid_device
> *hdev,
>  	struct input_dev *input = msc->input;
>  	int x = 0, y = 0, ii, clicks = 0, npoints;
>  
> +	/* Slightly paranoid, but "input" only gets set when our
> +	 * input_mapping sees the right field. */
> +	if (!input)
> +		return 0;
> +
>  	switch (data[0]) {
> -	case 0x10:
> -		if (size != 6)
> -			return 0;
> -		x = (__s16)(data[2] | data[3] << 8);
> -		y = (__s16)(data[4] | data[5] << 8);
> -		clicks = data[1];
> -		break;
>  	case TRACKPAD_REPORT_ID:
>  		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
>  		if (size < 4 || ((size - 4) % 9) != 0)
> @@ -343,13 +341,6 @@ static int magicmouse_raw_event(struct hid_device
> *hdev,
>  		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.
> -		    */
> -	case 0x60: /* Unknown, maybe laser on/off. */
> -	case 0x61: /* Laser reflection status change.
> -		    * data[1]: 0 = spotted, 1 = lost
> -		    */
>  	default:
>  		return 0;
>  	}
> @@ -377,36 +368,8 @@ static int magicmouse_raw_event(struct hid_device
> *hdev,
>  	return 1;
>  }
>  
> -static int magicmouse_input_open(struct input_dev *dev)
> -{
> -	struct hid_device *hid = input_get_drvdata(dev);
> -
> -	return hid->ll_driver->open(hid);
> -}
> -
> -static void magicmouse_input_close(struct input_dev *dev)
> -{
> -	struct hid_device *hid = input_get_drvdata(dev);
> -
> -	hid->ll_driver->close(hid);
> -}
> -
>  static void magicmouse_setup_input(struct input_dev *input, struct
> hid_device *hdev)
>  {
> -	input_set_drvdata(input, hdev);
> -	input->event = hdev->ll_driver->hidinput_input_event;
> -	input->open = magicmouse_input_open;
> -	input->close = magicmouse_input_close;
> -
> -	input->name = hdev->name;
> -	input->phys = hdev->phys;
> -	input->uniq = hdev->uniq;
> -	input->id.bustype = hdev->bus;
> -	input->id.vendor = hdev->vendor;
> -	input->id.product = hdev->product;
> -	input->id.version = hdev->version;
> -	input->dev.parent = hdev->dev.parent;
> -
>  	__set_bit(EV_KEY, input->evbit);
>  
>  	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> @@ -466,11 +429,22 @@ static void magicmouse_setup_input(struct
> input_dev *input, struct hid_device *h
>  	}
>  }
>  
> +static int magicmouse_input_mapping(struct hid_device *hdev,
> +		struct hid_input *hi, struct hid_field *field,
> +		struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> +	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
> +
> +	if (!msc->input)
> +		msc->input = hi->input;
> +
> +	return 0;
> +}
> +
>  static int magicmouse_probe(struct hid_device *hdev,
>  	const struct hid_device_id *id)
>  {
>  	__u8 feature[] = { 0xd7, 0x01 };
> -	struct input_dev *input;
>  	struct magicmouse_sc *msc;
>  	struct hid_report *report;
>  	int ret;
> @@ -500,8 +474,12 @@ static int magicmouse_probe(struct hid_device
> *hdev,
>  		goto err_free;
>  	}
>  
> -	/* we are handling the input ourselves */
> -	hidinput_disconnect(hdev);
> +	/* We do this after HID maps its inputs so that the
> +	 * Magic Mouse's HID-compliant report gets the right
> +	 * button and axis IDs.
> +	 */
> +	if (msc->input)
> +		magicmouse_setup_input(msc->input, hdev);
>  
>  	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
>  		report = hid_register_report(hdev, HID_INPUT_REPORT,
> @@ -528,24 +506,7 @@ static int magicmouse_probe(struct hid_device
> *hdev,
>  		goto err_stop_hw;
>  	}
>  
> -	input = input_allocate_device();
> -	if (!input) {
> -		dev_err(&hdev->dev, "can't alloc input device\n");
> -		ret = -ENOMEM;
> -		goto err_stop_hw;
> -	}
> -	magicmouse_setup_input(input, hdev);
> -
> -	ret = input_register_device(input);
> -	if (ret) {
> -		dev_err(&hdev->dev, "input device registration failed\n");
> -		goto err_input;
> -	}
> -	msc->input = input;
> -
>  	return 0;
> -err_input:
> -	input_free_device(input);
>  err_stop_hw:
>  	hid_hw_stop(hdev);
>  err_free:
> @@ -558,7 +519,6 @@ static void magicmouse_remove(struct hid_device
> *hdev)
>  	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
>  
>  	hid_hw_stop(hdev);
> -	input_unregister_device(msc->input);
>  	kfree(msc);
>  }
>  
> @@ -577,6 +537,7 @@ static struct hid_driver magicmouse_driver = {
>  	.probe = magicmouse_probe,
>  	.remove = magicmouse_remove,
>  	.raw_event = magicmouse_raw_event,
> +	.input_mapping = magicmouse_input_mapping,
>  };
>  
>  static int __init magicmouse_init(void)