On 9/13/05, roy wood <[email protected]> wrote:
> This patch to the Interact joystick driver adds support for the
> "RaiderPro Digital" model of joystick from Interact. The patch is
> made against kernel version 2.6.13-1.
Cool, looks pretty nice.
> Also, apparently I need to send this directly to Linus to get this
> into the tree. Anyone care to tell me the best email address to use
> to do that? I promise not to foreward it to recruiters at MS. :-)
>
Actually that would be Vojtech Pavlik <[email protected]> - input system
maintainer.
> + *
> + * History:
> + * --------
> + * 2005-09-12: rrwood - Add support for RaiderPro
> */
>
We prefer keeping changelogs in SCM, when possible
> struct interact {
> - struct gameport *gameport;
> - struct input_dev dev;
> - int bads;
> - int reads;
> - unsigned char type;
> - unsigned char length;
> - char phys[32];
> + struct gameport *gameport; /* Kernel gameport struct ptr */
> + struct input_dev dev; /* Kernel input_dev struct ptr */
And I don't think we should add comments like these - they don't tell
you anything new. Especially when they wrong (dev is not a pointer
[yet])
>
> -static short interact_abs_hhfx[] =
> +
> +/* I think the original purpose of setting up lists of controller
> + * axes/buttons was to provide a single location to maintain such
> + * information. Although the table-based approach certainly makes
> + * the interact_connect() code below MUCH simpler and cleaner, the
> + * interact_poll() code ends up being very hard to read, unfortunately.
> + *
> + * I was tempted to either rewrite interact_poll() in a clearer fashion,
> + * or to implement a more comprehensive table-driven decoding approach
> + * (with values for offset, masking, shifting of each value). I'm a
> + * bit leery of making such massive change though, since I don't have the
> + * controllers to test the result. Instead, I'll just add support
> + * for the RaiderPro as clearly as I can.....
> + */
This is not a book, what one could have done but decided not to is not
very interesting unless the solution is outlined as a future TODO
item.
>
> if (interact_read_packet(interact->gameport, interact->length, data)
> < interact->length) {
> + /* Couldn't read a full packet, so update the bad-count,
> + * queue another read, and get out */
Yes, that is what that "if" statement said. Why also comment it?
> + if (INTERACT_MAX_LENGTH - interact->length > 0) {
> + /* If data packets are less than max length, shift them
> + * for easier processing below (ProPad goofiness) */
This one is a decent comment tough (IMHO).
> + /* Queue another read */
> input_sync(dev);
??? input_sync signals that the input event packet is complete. What
is it about queueing another read stuff?
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |