multi-touch-dev team mailing list archive

[Henrik Rydberg <rydberg@xxxxxxxxxxx>]

Hi Chase,

First patch is just fine. Some comments on the second patch below.

-#define TOUCH_REPORT_ID   0x29
+#define MT_TOUCH_REPORT_ID   0x28
+#define MM_TOUCH_REPORT_ID   0x29

It is very hard to read the difference between MT and MM like this. Different
names would clarify a lot.

+		touch_minor = tdata[4];
+		down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;
+	} else {
+		id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
+		x = (tdata[1] << 27 | tdata[0] << 19) >> 19;

This else, and several other in the code, could be "if MAGICTRACKPAD then..."
instead, for clarity.

+	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];
+		down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;

I *think* this code is equivalent to what was there, but it is really hard to
say for sure. Please do not modify existing code path like this, at least not in
the same patch.

-	/* Generate the input events for this touch. */
-	if (report_touches && down) {
-		int orientation = (misc >> 10) - 32;
-
+	if (down) {
 		msc->touches[id].down = 1;
+		if (msc->single_touch_id == -1)
+			msc->single_touch_id = id;
+	} else if (msc->single_touch_id == id)
+		msc->single_touch_id = -2;

+	/* Generate the input events for this touch. */
+	if (report_touches && down) {

Same here - use a separate patch for this if really needed. Just think of how to
differentiate your additions from existing code through a bisect.

-	int x, y, ts, ii, clicks, last_up;
+	int x = 0;
+	int y = 0;
+	int ts;
+	int ii;
+	int clicks = 0;
+	int last_up;

Why are those changed?

-	case TOUCH_REPORT_ID:
+	case MT_TOUCH_REPORT_ID:
+		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
+		if (size < 4 || ((size - 4) % 9) != 0)
+			return 0;

This code returns from a function which has a functional continuation - a
statement that this is indeed intended would be good.

-		if (report_touches) {
-			last_up = 1;
-			for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
-				if (msc->touches[ii].down) {

unrelated change, please split.

-	magicmouse_emit_buttons(msc, clicks & 3);
-	input_report_rel(input, REL_X, x);
-	input_report_rel(input, REL_Y, y);
+	if ((data[0] == MM_TOUCH_REPORT_ID || data[0] == MT_TOUCH_REPORT_ID)) {
+		last_up = 1;

ditto.


-static struct feature features[] = {
+static struct feature mm_features[] = {
 	{ { 0xd7, 0x01 }, 2 },
 	{ { 0xf8, 0x01, 0x32 }, 3 }
 };

why not do this part in the first patch? and name, please...

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

Non-functional change of unrelated code.

That was it for now. All in all, this is excellent work. With the comments
adressed, it seems ready for submission. Thanks!

Henrik