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

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

[Michael Poole]

The X and Y values have two more significant bits in the same byte
that contains click status.  Include these in the reported value.
Thanks to Iain Hibbert of NetBSD for pointing this out.

Signed-off-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 0b89c1c..7cdda23 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 		 * to have the current touch information before
 		 * generating a click event.
 		 */
-		x = (signed char)data[1];
-		y = (signed char)data[2];
+		x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
+		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
 		clicks = data[3];
 		break;
 	case 0x20: /* Theoretically battery status (0-100), but I have
-- 
1.7.1

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

[Chase Douglas]

On Mon, 2010-07-05 at 10:50 -0400, Michael Poole wrote:
> The X and Y values have two more significant bits in the same byte
> that contains click status.  Include these in the reported value.
> Thanks to Iain Hibbert of NetBSD for pointing this out.
> 
> Signed-off-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 0b89c1c..7cdda23 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>  		 * to have the current touch information before
>  		 * generating a click event.
>  		 */
> -		x = (signed char)data[1];
> -		y = (signed char)data[2];
> +		x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
> +		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
>  		clicks = data[3];
>  		break;
>  	case 0x20: /* Theoretically battery status (0-100), but I have

Looks right to me.

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

-- Chase

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

[Ping Cheng]

On Mon, Jul 5, 2010 at 10:50 PM, Michael Poole <mdpoole@troilus.org> wrote:
> The X and Y values have two more significant bits in the same byte
> that contains click status.  Include these in the reported value.
> Thanks to Iain Hibbert of NetBSD for pointing this out.
>
> Signed-off-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 0b89c1c..7cdda23 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>                 * to have the current touch information before
>                 * generating a click event.
>                 */
> -               x = (signed char)data[1];
> -               y = (signed char)data[2];
> +               x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
> +               y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;

Will the following give us the same result?

 +               x = (int)(((data[3] & 0x0c) << 6) | data[1]);
 +               y = (int)(((data[3] & 0x30) << 4) | data[2]);

Ping

>                clicks = data[3];
>                break;
>        case 0x20: /* Theoretically battery status (0-100), but I have
> --
> 1.7.1
> --
> 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
>
--
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] hid-magicmouse: Correct parsing of large X and Y motions.

[Chase Douglas]

On Tue, 2010-07-06 at 03:54 +0800, Ping Cheng wrote:
> On Mon, Jul 5, 2010 at 10:50 PM, Michael Poole <mdpoole@troilus.org> wrote:
> > The X and Y values have two more significant bits in the same byte
> > that contains click status.  Include these in the reported value.
> > Thanks to Iain Hibbert of NetBSD for pointing this out.
> >
> > Signed-off-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 0b89c1c..7cdda23 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >                 * to have the current touch information before
> >                 * generating a click event.
> >                 */
> > -               x = (signed char)data[1];
> > -               y = (signed char)data[2];
> > +               x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
> > +               y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
> 
> Will the following give us the same result?
> 
>  +               x = (int)(((data[3] & 0x0c) << 6) | data[1]);
>  +               y = (int)(((data[3] & 0x30) << 4) | data[2]);

I thought about this too, but there's a sign extension issue. If the X
coordinate is -1 (these are relative coordinates), then you will end up
with (int)(1023), which is no longer -1.

When you shift to the right, the bits are sign-extended. Thus, shifting
everything to the most significant bit of an integer, and then shifting
everything back to the least significant will properly sign extend.

(I actually wrote a test program just to verify this)

That's just a reason for the shifts. There very well could be a more
elegant solution.

-- Chase

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

[Michael Poole]

Chase Douglas writes:

> On Tue, 2010-07-06 at 03:54 +0800, Ping Cheng wrote:
>> On Mon, Jul 5, 2010 at 10:50 PM, Michael Poole <mdpoole@troilus.org> wrote:
>> > The X and Y values have two more significant bits in the same byte
>> > that contains click status.  Include these in the reported value.
>> > Thanks to Iain Hibbert of NetBSD for pointing this out.
>> >
>> > Signed-off-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 0b89c1c..7cdda23 100644
>> > --- a/drivers/hid/hid-magicmouse.c
>> > +++ b/drivers/hid/hid-magicmouse.c
>> > @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
>> >                 * to have the current touch information before
>> >                 * generating a click event.
>> >                 */
>> > -               x = (signed char)data[1];
>> > -               y = (signed char)data[2];
>> > +               x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
>> > +               y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
>> 
>> Will the following give us the same result?
>> 
>>  +               x = (int)(((data[3] & 0x0c) << 6) | data[1]);
>>  +               y = (int)(((data[3] & 0x30) << 4) | data[2]);
>
> I thought about this too, but there's a sign extension issue. If the X
> coordinate is -1 (these are relative coordinates), then you will end up
> with (int)(1023), which is no longer -1.
>
> When you shift to the right, the bits are sign-extended. Thus, shifting
> everything to the most significant bit of an integer, and then shifting
> everything back to the least significant will properly sign extend.
>
> (I actually wrote a test program just to verify this)
>
> That's just a reason for the shifts. There very well could be a more
> elegant solution.

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;

Which do people find more readable?  Is the more portable behavior of
this idiom preferred over requiring sign extension by the compiler?

Michael Poole

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

[Chase Douglas]

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.

-- Chase

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

[Michael Poole]

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

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

[Chase Douglas]

On Mon, 2010-07-05 at 18:08 -0400, Michael Poole wrote:
> Chase Douglas writes:
> > 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. */

Instead of having this intermediate x_y that's shifted 8 bits to the
left, why not do:

int x = snto32((tdata[1] << 8 | tdata[0]) & 0xFFF, 12);
int y = snto32(tdata[2] << 4 | tdata[1] >> 4, 12);

P.S.: I much prefer hex when doing masks because it's easier to count
the bits.

-- Chase

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

[Dmitry Torokhov]

On Jul 5, 2010, at 12:54 PM, Ping Cheng <pinglinux@gmail.com> wrote:

> On Mon, Jul 5, 2010 at 10:50 PM, Michael Poole <mdpoole@troilus.org>  
> wrote:
>> The X and Y values have two more significant bits in the same byte
>> that contains click status.  Include these in the reported value.
>> Thanks to Iain Hibbert of NetBSD for pointing this out.
>>
>> Signed-off-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 0b89c1c..7cdda23 100644
>> --- a/drivers/hid/hid-magicmouse.c
>> +++ b/drivers/hid/hid-magicmouse.c
>> @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct  
>> hid_device *hdev,
>>                 * to have the current touch information before
>>                 * generating a click event.
>>                 */
>> -               x = (signed char)data[1];
>> -               y = (signed char)data[2];
>> +               x = (int)(((data[3] & 0x0c) << 28) | (data[1] <<  
>> 22)) >> 22;
>> +               y = (int)(((data[3] & 0x30) << 26) | (data[2] <<  
>> 22)) >> 22;
>
> Will the following give us the same result?
>
> +               x = (int)(((data[3] & 0x0c) << 6) | data[1]);
> +               y = (int)(((data[3] & 0x30) << 4) | data[2]);

I'd think you'd run into endianness issues.

-- 
Dmitry

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

[ext-phil.2.carmody]

Arsebiscuits, forced to use webmail, sloppily prefixing every line with '>' manually ...

________________________________________
From: linux-input-owner@vger.kernel.org [linux-input-owner@vger.kernel.org] On Behalf Of ext Michael Poole [mdpoole@troilus.org]
Sent: Tuesday, July 06, 2010 12:08 AM
To: Chase Douglas
Cc: Ping Cheng; Jiri Kosina; linux-input@vger.kernel.org
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]

>+       }
>+       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

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

[Datta, Shubhrajyoti]

> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-
> owner@vger.kernel.org] On Behalf Of ext-phil.2.carmody@nokia.com
> Sent: Tuesday, July 06, 2010 4:53 PM
> To: mdpoole@troilus.org; chase.douglas@canonical.com
> Cc: pinglinux@gmail.com; jkosina@suse.cz; linux-input@vger.kernel.org
> 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@vger.kernel.org [linux-input-
> owner@vger.kernel.org] On Behalf Of ext Michael Poole
> [mdpoole@troilus.org]
> Sent: Tuesday, July 06, 2010 12:08 AM
> To: Chase Douglas
> Cc: Ping Cheng; Jiri Kosina; linux-input@vger.kernel.org
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Michael Poole]

ext-phil.2.carmody@nokia.com writes:

[snip]
>>+/* 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]

That's good to know.  There's probably some much more complicated, but
portable, way to write the operation -- I suspect we'll continue to rely
on the compiler to be sane.

>>+       }
>>+       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]

Which version is that from?  The copy of C99 I have says the resulting
value is -1 * (2 ** n) if that value can be represented.  (A 6 May 2005
committee draft of TC2 says the same thing.)  Until n > 31, the behavior
should be standard-defined.

Michael

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

[ext-phil.2.carmody]

From: ext Michael Poole [mdpoole@troilus.org]
>ext-phil.2.carmody@nokia.com writes:
>
>[snip]
>>>+/* 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]
>
>That's good to know.  There's probably some much more complicated, but
>portable, way to write the operation -- I suspect we'll continue to rely
>on the compiler to be sane.
>
>>>+       }
>>>+       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]
>
>Which version is that from?

A text version of n869, which looks like it suffered a little in conversion to not-quite-ASCII.

>  The copy of C99 I have says the resulting
>value is -1 * (2 ** n) if that value can be represented.  (A 6 May 2005
>committee draft of TC2 says the same thing.)  Until n > 31, the behavior
>should be standard-defined.

Looking at the pdfs, agreed, My mistake. I shall have to 'fix' the non-ASCII in my text version, as I much prefer greppable text to pdfs.

Phil

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

[Jiri Kosina]

On Mon, 5 Jul 2010, Chase Douglas wrote:

> On Mon, 2010-07-05 at 10:50 -0400, Michael Poole wrote:
> > The X and Y values have two more significant bits in the same byte
> > that contains click status.  Include these in the reported value.
> > Thanks to Iain Hibbert of NetBSD for pointing this out.
> > 
> > Signed-off-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 0b89c1c..7cdda23 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -267,8 +267,8 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> >  		 * to have the current touch information before
> >  		 * generating a click event.
> >  		 */
> > -		x = (signed char)data[1];
> > -		y = (signed char)data[2];
> > +		x = (int)(((data[3] & 0x0c) << 28) | (data[1] << 22)) >> 22;
> > +		y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
> >  		clicks = data[3];
> >  		break;
> >  	case 0x20: /* Theoretically battery status (0-100), but I have
> 
> Looks right to me.
> 
> Acked-by: Chase Douglas <chase.douglas@canonical.com>

Thanks. Applied to magicmouse branch now.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.