On Thu, 2008-04-24 at 10:05 +0200, Sven Anders wrote:
> I hereby request inclusion of the attached patch to the Linux 2.6.26 development branch.
> If this patch is added, I will be able to commit further enhancements in the future.
> This mainly adds support for the status bits on Geyser 3/4 touchpads.
> It had been tested for serveral months by the MacTel community.
> - { ATP_DEVICE(GEYSER_ANSI_PRODUCT_ID) },
> + { ATP_DEVICE(GEYSER2_ANSI_PRODUCT_ID) },
Why this rename? Is that backed by the OSX file? I don't care much, but
it seems useless.
> - signed char * data; /* transferred data */
> + u8 * data; /* transferred data */
That I can understand,
> int xy_acc[ATP_XSENSORS + ATP_YSENSORS];
> int datalen; /* size of an USB urb transfer */
> - int idlecount; /* number of empty packets */
> + int idle_counter; /* number of empty packets */
But not that. Why?
> #define dbg_dump(msg, tab) \
> @@ -182,8 +198,12 @@
> if (debug) printk(KERN_DEBUG format, ##a); \
> } while (0)
>
> -MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Michael Hanselmann");
> -MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
> +MODULE_AUTHOR("Johannes Berg");
> +MODULE_AUTHOR("Stelian Pop");
> +MODULE_AUTHOR("Frank Arnold");
> +MODULE_AUTHOR("Michael Hanselmann");
> +MODULE_AUTHOR("Sven Anders");
> +MODULE_DESCRIPTION("Apple PowerBook and MacBook USB touchpad driver");
Ack.
> -static int debug = 1;
> +static int debug;
> module_param(debug, int, 0644);
> MODULE_PARM_DESC(debug, "Activate debugging output");
Also ack.
> @@ -240,7 +260,7 @@
> {
> char data[8];
> int size;
> -
> +
No introducing leading whitespace please.
> if (size != 8) {
> + if (debug)
> + {
CodingStyle.
> if (size != 8) {
> + if (debug)
> + {
Ditto.
> + /* Just update the base values (i.e. touchpad in untouched state) */
> + if (dev->data[dev->datalen-1] & ATP_STATUS_BIT_BASE_UPDATE)
> + {
Ditto.
> for (i = 0; i < ATP_XSENSORS + ATP_YSENSORS; i++) {
> - /* accumulate the change */
> - signed char change = dev->xy_old[i] - dev->xy_cur[i];
> - dev->xy_acc[i] -= change;
> + /* calculate the change */
> + dev->xy_acc[i] = dev->xy_cur[i] - dev->xy_old[i];
> +
> + /* this is a round-robin value, so couple with that */
> + if (dev->xy_acc[i] > 127)
> + dev->xy_acc[i] -= 256;
> +
> + if (dev->xy_acc[i] < -127)
> + dev->xy_acc[i] += 256;
> +
> + /* Needed for the older Geyser */
> + if (!atp_is_geyser_3_4(dev))
> + {
Ditto.
> + /* store new 'untouched' value, if any new */
> + if (dev->xy_acc[i] < -1)
> + dev->xy_old[i] = dev->xy_cur[i];
> + }
You need more rationale for the algorithm change.
> - key = dev->data[dev->datalen - 1] & 1;
> -
> + key = dev->data[dev->datalen - 1] & ATP_STATUS_BIT_BUTTON;
> +
Shouldn't this depend on the new format appletouch?
> @@ -549,16 +610,28 @@
> * work on Fountain touchpads.
> */
> if (!atp_is_fountain(dev)) {
> +
> + /* Button must not be pressed when entering suspend,
> + otherwise we will never release the button. */
Useless whitespace, CodingStyle for comments looks differently.
[stopped here]
Please read CodingStyle, then split your patch at least into the
renames/code changes that don't really change anything and the actual
algorithm changes. Also, if you're feeling motivated, it would be good
if the "is_atp_xxx" foo was replaced by the driver_info in struct
usb_device_id.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
