> -----Original Message----- > From: linux-input-owner@xxxxxxxxxxxxxxx [mailto:linux-input- > owner@xxxxxxxxxxxxxxx] On Behalf Of ext-phil.2.carmody@xxxxxxxxx > Sent: Tuesday, July 06, 2010 4:53 PM > To: mdpoole@xxxxxxxxxxx; chase.douglas@xxxxxxxxxxxxx > Cc: pinglinux@xxxxxxxxx; jkosina@xxxxxxx; linux-input@xxxxxxxxxxxxxxx > Subject: RE: [PATCH] hid-magicmouse: Correct parsing of large X and Y > motions. > > Arsebiscuits, forced to use webmail, sloppily prefixing every line with > '>' manually ... > > ________________________________________ > From: linux-input-owner@xxxxxxxxxxxxxxx [linux-input- > owner@xxxxxxxxxxxxxxx] On Behalf Of ext Michael Poole > [mdpoole@xxxxxxxxxxx] > Sent: Tuesday, July 06, 2010 12:08 AM > To: Chase Douglas > Cc: Ping Cheng; Jiri Kosina; linux-input@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] hid-magicmouse: Correct parsing of large X and Y > motions. > > >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); > > Beware - this relies on sane behaviour from the current and future > compilers, as the C standard doesn't mandate what should happen when > shortening out of range signed values. [n869.txt: 6.3.1.1.#3] Any reason of choosing __s32/ s32 ? > > >+ } > >+ return value & (1 << (n - 1)) ? value | (-1 << n) : value; > > That signed left shift, however, is good old fashioned undefined > behaviour. [n869.txt 6.5.7.#4] > > >+} > >+ > > 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
