Re: PowerBook5,8 - TrackPad update

[Stelian Pop <stelian@popies.net>]

Le jeudi 01 décembre 2005 à 00:46 +0100, Michael Hanselmann a écrit :
> On Wed, Nov 30, 2005 at 11:39:17PM +0100, Michael Hanselmann wrote:
> > The patch is attached for easier use.
> 
> There was a mistake in it due to which the mouse button wouldn't work.
> Fixed in the now attached patch.

Is this version really working well on the new Powerbooks ? From what
I've seen in this thread there are still issues and it's still a work in
progress, so it may be too early to integrate the changes in the kernel.

Also, some other comments on the code itself:

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)                                                    
+#include <linux/relayfs_fs.h>
+#endif

While the relayfs code is ok for debugging, I'm wondering if it should be left in the final version at all.

+       int                     is0215;         /* is the device a 0x0215? */

No need for that, just use udev->descriptor.idProduct == 0x0215 (in a macro perhaps)

+       int                     overflowwarn;   /* overflow warning printed? */

I would use a static variable in the case -OVERFLOW: block here.

+               dev->xy_cur[i++] = dev->data[19];
+               dev->xy_cur[i++] = dev->data[20];
+               dev->xy_cur[i++] = dev->data[22];
+               dev->xy_cur[i++] = dev->data[23];

There is obviously a pattern here:

	for (i = 0; i < 15; i++)
		dev->xy_cur[i] = dev->data[ 19 + (i * 3) / 2 ]

I'm wondering if the same formula doesn't apply for more X and Y sensors (like 16 X
and 16 Y sensors on the old Powerbooks, 26 for the 17" models)

+#if 0
+               /* Some debug code */
+               for (i = 0; i < dev->urb->actual_length; i++) {
+                       printk("%2x,", (unsigned char)dev->data[i]);
+               }
+               printk("\n");
+#endif

Please dump that.

+               /* Prints the read values */
+               if (debug > 1) {
+                       printk("appletouch: X=");
+                       for (i = 0; i < 15; i++) {
+                               printk("%2x,", (unsigned char)dev->xy_cur[i]);
+                       }
+                       printk("  Y=");
+                       for (i = ATP_XSENSORS; i < (ATP_XSENSORS + (9 - 1)); i++) {
+                               printk("%2x,", (unsigned char)dev->xy_cur[i]);
+                       }
+                       printk("\n");
+               }

What is the point in doing this since the dbg_dump is called a few lines
later ? Best is to modify dbg_dump to know about the new number of
sensors...

+                       printk(KERN_INFO "appletouch: atp_probe found interrupt "
+                              "in endpoint: %d\n", int_in_endpointAddr);

Why is this useful to know ?

+       if (dev->is0215) {
+               dev->datalen = 64;
+       } else {
+               dev->datalen = 81;
+       }

Braces are not needed here.


PS: please inline the patch instead of attaching it to the mail, it's
much more easy to quote it that way.

Stelian.
-- 
Stelian Pop <stelian@popies.net>