Re: [PATCH] hid-magicmouse: Correct parsing of large X and Y motions.

[Michael Poole <mdpoole@troilus.org>]

Chase Douglas writes:

> On Mon, 2010-07-05 at 16:33 -0400, Michael Poole wrote:
>> C99 says that the result of right-shifting a negative value is
>> compiler-defined.  gcc documents that it ensures sign extension.  Other
>> parts of hid-magicmouse.c use this idiom already.  The corresponding
>> idiom in hid-core.c (see the snto32() function) would look something
>> like this:
>> 
>>       x = ((data[3] & 0x0c) << 6) | data[1];
>>       x |= (x & (1 << 9)) ? (-1 << 10) : 0;
>
> snto32() seems like something we should be using in hid-magicmouse.c? On
> further thought, it actually seems like something that should be a macro
> in linux/kernel.h. I would think there could be utility for it in many
> places of the kernel.

Probably so.  The patch below is a proof of concept, which probably is
not worth including until we decide the right place to put snto32(), but
I think does show that using snto32() is more readable than the current
code.

Note the bitwise-and for the ABS_MT_POSITION_X value: I missed that the
first time I tried this, to rather undesirable effect.  I am not sure
whether snto32() should automatically do that kind of masking: It is
only needed in one out of the four sites here, but it is a fairly easy
mistake to make.

Michael Poole

--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -163,14 +163,26 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
 		msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
 }
 
+/* Function shamelessly borrowed from hid-core.c. */
+
+static s32 snto32(__u32 value, unsigned n)
+{
+	switch (n) {
+	case 8:  return ((__s8)value);
+	case 16: return ((__s16)value);
+	case 32: return ((__s32)value);
+	}
+	return value & (1 << (n - 1)) ? value | (-1 << n) : value;
+}
+
 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 x = snto32((x_y >> 8) & 4095, 12);
+	int y = snto32(x_y >> 20, 12);
 	int down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;
 
 	/* Store tracking ID and other fields. */
@@ -285,8 +297,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		 * to have the current touch information before
 		 * generating a click event.
 		 */
-		x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
-		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
+		x = snto32(((data[3] & 0x0c) << 6) | data[1], 10);
+		y = snto32(((data[3] & 0x30) << 4) | data[2], 10);
 		clicks = data[3];
 		break;
 	case 0x20: /* Theoretically battery status (0-100), but I have