Re: [PATCH] drivers/input/joystick/interact.c ; Linux 2.6.13-1

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On 9/13/05, roy wood <roy.wood@gmail.com> 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 <vojtech@suse.cz> - 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