Re: PATCH [1/3] drivers/input/xpad.c: Improve Xbox 360 wireless support and add sysfs interface

[Linus Torvalds <torvalds@linux-foundation.org>]

On Mon, 2 Mar 2009, Mike Murphy wrote:
> 
> 3. The outer cast (__s16) converts the unsigned values to signed
> values, while the "~" inverts the y axes to make them function like a
> joystick instead of a flight simulator control.

You should do the ~ before the cast, or use - if you just want to reverse 
things. It probably doesn't much matter (the difference between ~ and - i 
just one), but still..

Also, quite frankly, it looks like your 'coords[]' array should just be of 
type 's16' (rather than 'int') to begin with. You seem to really never use 
it as an int anyway. That would get rid of the cast.

> Is there a cleaner way to accomplish the transition from 16-bit
> unsigned little endian to 16-bit signed host endian?

I think the code is fine, but I think you'd be better off if the "data" 
pointer was perhaps of type "le16 *" to begin with.

That obviously means that your "offset" addition should now be in 16-bit 
words rather than in bytes, so you'd need to divide the offsets by two to 
do that, but those are just numbers anyway. And quite frankly, it looks 
like the actual data is just offset differently - but with the same fixed 
offset between values - for the two cases, so you could just have _one_ 
offset (and even just add that into the 'data' pointer).

That would get rid of the second cast. You'd end up with just

	s16 coords[4];

	/* In words - so this is 12 vs 6 bytes into the data */
	data += (xpad->xtype == XTYPE_XBOX) ? 6 : 3;

	coords[0] = le16_to_cpup(data);
	coords[1] = ~le16_to_cpup(data + 1);
	coords[2] = le16_to_cpup(data + 2);
	coords[3] = ~le16_to_cpup(data + 3);
	..

which looks a bit shorter and avoids those casts. I dunno.

		Linus