Re: Appletouch patch

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]