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

[Mike Murphy <mamurph@cs.clemson.edu>]

On Mon, Mar 2, 2009 at 4:04 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> +static void xpad_process_sticks(struct usb_xpad *xpad, unsigned char *data)
>> +{
>> +     struct input_dev *dev = xpad->dev;
>> +     int coords[4];    /* x, y, rx, ry */
>> +     int x_offset, y_offset, rx_offset, ry_offset;
>> +     int c;
>> +     int range;
>> +     int abs_magnitude, adjusted_magnitude, difference, scale_fraction;
>> +     int dead_zone[2], stick_limit[2];
>> +
>> +     dead_zone[0] = xpad->left_dead_zone;
>> +     dead_zone[1] = xpad->right_dead_zone;
>> +     stick_limit[0] = xpad->left_stick_limit;
>> +     stick_limit[1] = xpad->right_stick_limit;
>> +
>> +     if (xpad->xtype == XTYPE_XBOX) {
>> +             x_offset = 12;
>> +             y_offset = 14;
>> +             rx_offset = 16;
>> +             ry_offset = 18;
>> +     } else {
>> +             x_offset = 6;
>> +             y_offset = 8;
>> +             rx_offset = 10;
>> +             ry_offset = 12;
>> +     }
>> +
>> +     coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
>> +     coords[1] = ~(__s16) le16_to_cpup((__le16 *)(data + y_offset));
>> +     coords[2] = (__s16) le16_to_cpup((__le16 *)(data + rx_offset));
>> +     coords[3] = ~(__s16) le16_to_cpup((__le16 *)(data + ry_offset));
>
> We don't need the first typecast here and if `data' were to have type
> `void *', we could do away with the second cast as well.
>

gcc 4.3.3 refused to compile with data set to type void *'. I also
tried removing the casts and changing to le16_to_cpu, but the result
was that stick inputs were lost. After further testing, I reverted to
the original code, which was taken from the driver in the stable tree.

The typecasting follows this logic:

1. The stick axis inputs are 16-bit *unsigned* little endian (0 -
65535), which need to map onto the *signed* axis (-32767 to +32767)
inside the input subsystem.

2. The innermost typecast (__le16 *) tells gcc to treat the (unsigned
char *) address as a pointer to an unsigned little-endian value, which
is converted to a pointer of host endiannes -- still unsigned -- by
le16_to_cpup.

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.

Is there a cleaner way to accomplish the transition from 16-bit
unsigned little endian to 16-bit signed host endian? If so, I can
change it... if not, I can comment this code to explain why it looks
like it does. I don't have enough experience with the Linux internal
types system to have a better solution.

I've fixed the other problems (and maybe created new ones :)...
revision of this part of the patch to follow.

Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph
--
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