appletouch powersaving - please apply for 2.6.23-rc1

PATCH In review Soeren Sonnenburg 5 participants 9 messages Jul 17, 2007 – Jul 19, 2007

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Soeren Sonnenburg

author

2007-07-17 07:10 UTC #0 lore raw

PATCH 1/1 appletouch powersaving - please apply for 2.6.23-rc1

Hi,

the attached minimally intrusive patch is based on Matthew Garret's patch 'Make appletouch shut up when it has nothing to say' patches (e.g.
http://lkml.org/lkml/2007/5/13/117): Matthews description follows / second paragraph lists my additional changes.

The appletouch geyser3 devices found in the Intel Macs (and possibly some later PPC ones?) send a constant stream of packets after the first touch. This results in the kernel waking up around once every couple of milliseconds to process them, making it almost impossible to spend any significant period of time in C3 state on a dynamic HZ kernel. Sending the mode initialization code makes the device shut up until it's touched again. This patch does so after receiving 10 packets with no interesting content.

In addition it now empties the work queue via cancel_work_sync on module exit, keeps all error checking and only reports BTN_LEFT presses if bit 1 in the status byte (last byte in packet) is set. This fixes the random left clicks issue. Furthermore it invalidates touchpad data before the mode switch, which fixes the touchpad runs amok issue.

Credits:
Sven Anders found out that one should only check for bit 1 for BTN_LEFT. Matthew Garrett did the initial 'Make appletouch shut up when it has nothing to say' so I am adding him to the signed-off lines (hope that is the correct way).

Patch follows inline and attached.

Soeren.

Signed-off-by Soeren Sonnenburg <[email protected]>

Signed-off-by Matthew Garrett <[email protected]>

drivers/input/mouse/appletouch.c +79 ~~-35~~

index e321526..0426054 100644
@@ -155,6 +155,8 @@ struct atp {
 	int			xy_acc[ATP_XSENSORS + ATP_YSENSORS];
 	int			overflowwarn;	/* overflow warning printed? */
 	int			datalen;	/* size of an USB urb transfer */
+	int                     idlecount;      /* number of empty packets */
+	struct work_struct      work;
 };
 
 #define dbg_dump(msg, tab) \

@@ -208,6 +210,64 @@ static inline int atp_is_geyser_3(struct atp *dev)
 		(productId == GEYSER4_JIS_PRODUCT_ID);
 }
 
+/*
+ * By default Geyser 3 device sends standard USB HID mouse
+ * packets (Report ID 2). This code changes device mode, so it
+ * sends raw sensor reports (Report ID 5).
+ */
+static int atp_geyser3_init(struct usb_device *udev)
+{
+	char data[8];
+	int size;
+	int i;
+
+	size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+			       ATP_GEYSER3_MODE_READ_REQUEST_ID,
+			       USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			       ATP_GEYSER3_MODE_REQUEST_VALUE,
+			       ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+
+	if (size != 8) {
+		printk("appletouch atp_geyser3_init READ error\n");
+		for (i=0; i<8; i++)
+			printk("appletouch[%d]: %d\n", i, (int) data[i]);
+
+		err("Could not do mode read request from device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+
+	/* Apply the mode switch */
+	data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
+
+	size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			       ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
+			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			       ATP_GEYSER3_MODE_REQUEST_VALUE,
+			       ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+	
+	if (size != 8) {
+		printk("appletouch atp_geyser3_init WRITE error\n");
+		for (i=0; i<8; i++)
+			printk("appletouch[%d]: %d\n", i, (int) data[i]);
+		err("Could not do mode write request to device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+	return 0;
+}
+
+/* Reinitialise the device if it's a geyser 3 */
+static void atp_reinit(struct work_struct *work)
+{
+	struct atp *dev = container_of(work, struct atp, work);
+	struct usb_device *udev = dev->udev;
+
+	dev->idlecount = 0;
+	atp_geyser3_init(udev);
+}
+
 static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
 			     int *z, int *fingers)
 {

@@ -418,6 +478,8 @@ static void atp_complete(struct urb* urb)
 	y = atp_calculate_abs(dev->xy_acc + ATP_XSENSORS, ATP_YSENSORS,
 			      ATP_YFACT, &y_z, &y_f);
 
+	input_report_key(dev->input, BTN_LEFT, dev->data[dev->datalen-1] & 1);
+
 	if (x && y) {
 		if (dev->x_old != -1) {
 			x = (dev->x_old * 3 + x) >> 2;

@@ -449,10 +511,19 @@ static void atp_complete(struct urb* urb)
 
 		/* reset the accumulator on release */
 		memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
-	}
 
-	input_report_key(dev->input, BTN_LEFT,
-			 !!dev->data[dev->datalen - 1]);
+		/* Geyser 3 will continue to send packets continually after
+		   the first touch unless reinitialised. Do so if it's been
+		   idle for a while in order to avoid waking the kernel up
+		   several hundred times a second */
+		if (atp_is_geyser_3(dev)) {
+			dev->idlecount++;
+			if (dev->idlecount == 10) {
+				dev->valid=0;
+				schedule_work (&dev->work);
+			}
+		}
+	}
 
 	input_sync(dev->input);

@@ -528,40 +599,10 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 		dev->datalen = 81;
 
 	if (atp_is_geyser_3(dev)) {
-		/*
-		 * By default Geyser 3 device sends standard USB HID mouse
-		 * packets (Report ID 2). This code changes device mode, so it
-		 * sends raw sensor reports (Report ID 5).
-		 */
-		char data[8];
-		int size;
-
-		size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_READ_REQUEST_ID,
-			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
-
-		if (size != 8) {
-			err("Could not do mode read request from device"
-							" (Geyser 3 mode)");
+		/* switch to raw sensor mode */
+		if (atp_geyser3_init(udev))
 			goto err_free_devs;
-		}
-
-		/* Apply the mode switch */
-		data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
 
-		size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
-			USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
-
-		if (size != 8) {
-			err("Could not do mode write request to device"
-							" (Geyser 3 mode)");
-			goto err_free_devs;
-		}
 		printk("appletouch Geyser 3 inited.\n");
 	}

@@ -636,6 +677,8 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 	/* save our data pointer in this interface device */
 	usb_set_intfdata(iface, dev);
 
+	INIT_WORK(&dev->work, atp_reinit);
+
 	return 0;
 
  err_free_buffer:

@@ -656,6 +699,7 @@ static void atp_disconnect(struct usb_interface *iface)
 
 	usb_set_intfdata(iface, NULL);
 	if (dev) {
+		cancel_work_sync(&dev->work);
 		usb_kill_urb(dev->urb);
 		input_unregister_device(dev->input);
 		usb_buffer_free(dev->udev, dev->datalen,

Signature · 2 lines

Sometimes, there's a moment as you're waking, when you become aware of
the real world around you, but you're still dreaming.

drivers/input/mouse/appletouch.c +79 ~~-35~~

159 lines not on this page · raw message

Johannes Berg

2007-07-17 13:03 UTC #1 lore raw

Hi,

Good stuff :)

Review

Quoted 1 line

(quoted diff) +1 -0

+	int                     idlecount;      /* number of empty packets */

should probably use tabs here.

drivers/input/mouse/appletouch.c static inline int atp_is_geyser_3(struct atp *dev)

Quoted 6 lines

drivers/input/mouse/appletouch.c +6 -0

+	size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			       ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
+			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			       ATP_GEYSER3_MODE_REQUEST_VALUE,
+			       ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+	<eol>

trailing whitespace.

drivers/input/mouse/appletouch.c static void atp_complete(struct urb* urb) · line 455

Quoted 12 lines

drivers/input/mouse/appletouch.c +2 -3

+	input_report_key(dev->input, BTN_LEFT, dev->data[dev->datalen-1] & 1);
+

@@ -449,10 +511,19 @@ static void atp_complete(struct urb* urb)
 
 		/* reset the accumulator on release */
 		memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
-	}
 
-	input_report_key(dev->input, BTN_LEFT,
-			 !!dev->data[dev->datalen - 1]);

Any hint as to why you move this? The different test, yes, ok, you explained that, but moving it?

johannes

Soeren Sonnenburg

author

2007-07-17 16:14 UTC #2 lore raw

Quoted 7 lines · Johannes Berg

Hi,
Good stuff :)
Review
Quoted 1 line
(quoted diff) +1 -0
+	int                     idlecount;      /* number of empty packets */
should probably use tabs here.

fixed.

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			       ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
+			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			       ATP_GEYSER3_MODE_REQUEST_VALUE,
+			       ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+	<eol>

trailing whitespace.

fixed

Quoted 10 lines

(quoted diff) +2 -1

+	input_report_key(dev->input, BTN_LEFT, dev->data[dev->datalen-1] & 1);
+

@@ -449,10 +511,19 @@ static void atp_complete(struct urb* urb)
 
 		/* reset the accumulator on release */
 		memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
-	}

Quoted 5 lines

Review static void atp_complete(struct urb* urb) · line 455
Quoted 2 lines
(quoted diff) +0 -2
-	input_report_key(dev->input, BTN_LEFT,
-			 !!dev->data[dev->datalen - 1]);
Any hint as to why you move this? The different test, yes, ok, you explained that, but moving it?

OK, Sven Anders also asked why the move... and well the reason was that when I was trying to figure out what goes wrong I memset everything including dev->data to zero which required the move ...

Anyway as there is no goto/return inbetween I fail to see that this will make any difference. So I moved the code back down where it was.

The new patch containing these cleanups is attached.

Best,
Soeren

Signature · 2 lines

Sometimes, there's a moment as you're waking, when you become aware of
the real world around you, but you're still dreaming.

drivers/input/mouse/appletouch.c +77 ~~-35~~

index e321526..34c1eca 100644
@@ -155,6 +155,8 @@ struct atp {
 	int			xy_acc[ATP_XSENSORS + ATP_YSENSORS];
 	int			overflowwarn;	/* overflow warning printed? */
 	int			datalen;	/* size of an USB urb transfer */
+	int			idlecount;      /* number of empty packets */
+	struct work_struct      work;
 };
 
 #define dbg_dump(msg, tab) \

@@ -208,6 +210,63 @@ static inline int atp_is_geyser_3(struct atp *dev)
 		(productId == GEYSER4_JIS_PRODUCT_ID);
 }
 
+/*
+ * By default Geyser 3 device sends standard USB HID mouse
+ * packets (Report ID 2). This code changes device mode, so it
+ * sends raw sensor reports (Report ID 5).
+ */
+static int atp_geyser3_init(struct usb_device *udev)
+{
+	char data[8];
+	int size;
+	int i;
+
+	size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				ATP_GEYSER3_MODE_READ_REQUEST_ID,
+				USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+				ATP_GEYSER3_MODE_REQUEST_VALUE,
+				ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+	if (size != 8) {
+		printk("appletouch atp_geyser3_init READ error\n");
+		for (i=0; i<8; i++)
+			printk("appletouch[%d]: %d\n", i, (int) data[i]);
+
+		err("Could not do mode read request from device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+
+	/* Apply the mode switch */
+	data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
+
+	size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+				ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
+				USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+				ATP_GEYSER3_MODE_REQUEST_VALUE,
+				ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+	if (size != 8) {
+		printk("appletouch atp_geyser3_init WRITE error\n");
+		for (i=0; i<8; i++)
+			printk("appletouch[%d]: %d\n", i, (int) data[i]);
+		err("Could not do mode write request to device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+	return 0;
+}
+
+/* Reinitialise the device if it's a geyser 3 */
+static void atp_reinit(struct work_struct *work)
+{
+	struct atp *dev = container_of(work, struct atp, work);
+	struct usb_device *udev = dev->udev;
+
+	dev->idlecount = 0;
+	atp_geyser3_init(udev);
+}
+
 static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
 			     int *z, int *fingers)
 {

@@ -449,11 +508,21 @@ static void atp_complete(struct urb* urb)
 
 		/* reset the accumulator on release */
 		memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
-	}
 
-	input_report_key(dev->input, BTN_LEFT,
-			 !!dev->data[dev->datalen - 1]);
+		/* Geyser 3 will continue to send packets continually after
+		   the first touch unless reinitialised. Do so if it's been
+		   idle for a while in order to avoid waking the kernel up
+		   several hundred times a second */
+		if (atp_is_geyser_3(dev)) {
+			dev->idlecount++;
+			if (dev->idlecount == 10) {
+				dev->valid=0;
+				schedule_work (&dev->work);
+			}
+		}
+	}
 
+	input_report_key(dev->input, BTN_LEFT, dev->data[dev->datalen-1] & 1);
 	input_sync(dev->input);
 
 exit:

@@ -528,40 +597,10 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 		dev->datalen = 81;
 
 	if (atp_is_geyser_3(dev)) {
-		/*
-		 * By default Geyser 3 device sends standard USB HID mouse
-		 * packets (Report ID 2). This code changes device mode, so it
-		 * sends raw sensor reports (Report ID 5).
-		 */
-		char data[8];
-		int size;
-
-		size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_READ_REQUEST_ID,
-			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
-
-		if (size != 8) {
-			err("Could not do mode read request from device"
-							" (Geyser 3 mode)");
+		/* switch to raw sensor mode */
+		if (atp_geyser3_init(udev))
 			goto err_free_devs;
-		}
-
-		/* Apply the mode switch */
-		data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
-
-		size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
-			USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
 
-		if (size != 8) {
-			err("Could not do mode write request to device"
-							" (Geyser 3 mode)");
-			goto err_free_devs;
-		}
 		printk("appletouch Geyser 3 inited.\n");
 	}

@@ -636,6 +675,8 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 	/* save our data pointer in this interface device */
 	usb_set_intfdata(iface, dev);
 
+	INIT_WORK(&dev->work, atp_reinit);
+
 	return 0;
 
  err_free_buffer:

@@ -656,6 +697,7 @@ static void atp_disconnect(struct usb_interface *iface)
 
 	usb_set_intfdata(iface, NULL);
 	if (dev) {
+		cancel_work_sync(&dev->work);
 		usb_kill_urb(dev->urb);
 		input_unregister_device(dev->input);
 		usb_buffer_free(dev->udev, dev->datalen,

Dmitry Torokhov

maintainer

2007-07-17 15:01 UTC #3 lore raw

In reply to Soeren Sonnenburg

Hi,

drivers/input/mouse/appletouch.c static void atp_disconnect(struct usb_interface *iface) · line 705

Quoted 11 lines · Soeren Sonnenburg

err_free_buffer:

drivers/input/mouse/appletouch.c +1 -0

@@ -656,6 +699,7 @@ static void atp_disconnect(struct usb_interface *iface)
 
       usb_set_intfdata(iface, NULL);
       if (dev) {
+               cancel_work_sync(&dev->work);
               usb_kill_urb(dev->urb);
               input_unregister_device(dev->input);
               usb_buffer_free(dev->udev, dev->datalen,

This should go into atp_close() and I think you need to do cancel_work_sync after calling usb_kill_urb() otherwise you risk it being submitted while you gettingto kill the urb.

How many boxes did you try this patch on?

Signature · 1 line

Dmitry

Soeren Sonnenburg

author

2007-07-17 18:16 UTC #4 lore raw

Re: [PATCH] appletouch powersaving - please apply for 2.6.23-rc1 take #3

Quoted 7 lines · Dmitry Torokhov

Hi,
Quoted 4 lines · Soeren Sonnenburg
err_free_buffer:
(quoted diff) +0 -0
@@ -656,6 +699,7 @@ static void atp_disconnect(struct usb_interface *iface)
 

Quoted 11 lines

Review static void atp_disconnect(struct usb_interface *iface) · line 705
Quoted 7 lines
(quoted diff) +1 -0
       usb_set_intfdata(iface, NULL);
       if (dev) {
+               cancel_work_sync(&dev->work);
               usb_kill_urb(dev->urb);
               input_unregister_device(dev->input);
               usb_buffer_free(dev->udev, dev->datalen,
This should go into atp_close() and I think you need to do cancel_work_sync after calling usb_kill_urb() otherwise you risk it being submitted while you gettingto kill the urb.

good catch. modified patch accordingly+attached.

Quoted 1 line

How many boxes did you try this patch on?

Mine plus 1 other. However please note that Matthews patch has been (which is what this patch is based on) is in the mactel-patches repository for quite some time now and that the not-yet-cleanup up variant of this patch was posted to mactel-devel...

So the modeswitch part should work...

Soeren

Signature · 2 lines

Sometimes, there's a moment as you're waking, when you become aware of
the real world around you, but you're still dreaming.

drivers/input/mouse/appletouch.c +77 ~~-35~~

index e321526..7f180f7 100644
@@ -155,6 +155,8 @@ struct atp {
 	int			xy_acc[ATP_XSENSORS + ATP_YSENSORS];
 	int			overflowwarn;	/* overflow warning printed? */
 	int			datalen;	/* size of an USB urb transfer */
+	int			idlecount;      /* number of empty packets */
+	struct work_struct      work;
 };
 
 #define dbg_dump(msg, tab) \

@@ -208,6 +210,63 @@ static inline int atp_is_geyser_3(struct atp *dev)
 		(productId == GEYSER4_JIS_PRODUCT_ID);
 }
 
+/*
+ * By default Geyser 3 device sends standard USB HID mouse
+ * packets (Report ID 2). This code changes device mode, so it
+ * sends raw sensor reports (Report ID 5).
+ */
+static int atp_geyser3_init(struct usb_device *udev)
+{
+	char data[8];
+	int size;
+	int i;
+
+	size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				ATP_GEYSER3_MODE_READ_REQUEST_ID,
+				USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+				ATP_GEYSER3_MODE_REQUEST_VALUE,
+				ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+	if (size != 8) {
+		printk("appletouch atp_geyser3_init READ error\n");
+		for (i=0; i<8; i++)
+			printk("appletouch[%d]: %d\n", i, (int) data[i]);
+
+		err("Could not do mode read request from device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+
+	/* Apply the mode switch */
+	data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
+
+	size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+				ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
+				USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+				ATP_GEYSER3_MODE_REQUEST_VALUE,
+				ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+	if (size != 8) {
+		printk("appletouch atp_geyser3_init WRITE error\n");
+		for (i=0; i<8; i++)
+			printk("appletouch[%d]: %d\n", i, (int) data[i]);
+		err("Could not do mode write request to device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+	return 0;
+}
+
+/* Reinitialise the device if it's a geyser 3 */
+static void atp_reinit(struct work_struct *work)
+{
+	struct atp *dev = container_of(work, struct atp, work);
+	struct usb_device *udev = dev->udev;
+
+	dev->idlecount = 0;
+	atp_geyser3_init(udev);
+}
+
 static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
 			     int *z, int *fingers)
 {

@@ -449,11 +508,21 @@ static void atp_complete(struct urb* urb)
 
 		/* reset the accumulator on release */
 		memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
-	}
 
-	input_report_key(dev->input, BTN_LEFT,
-			 !!dev->data[dev->datalen - 1]);
+		/* Geyser 3 will continue to send packets continually after
+		   the first touch unless reinitialised. Do so if it's been
+		   idle for a while in order to avoid waking the kernel up
+		   several hundred times a second */
+		if (atp_is_geyser_3(dev)) {
+			dev->idlecount++;
+			if (dev->idlecount == 10) {
+				dev->valid=0;
+				schedule_work (&dev->work);
+			}
+		}
+	}
 
+	input_report_key(dev->input, BTN_LEFT, dev->data[dev->datalen-1] & 1);
 	input_sync(dev->input);
 
 exit:

@@ -480,6 +549,7 @@ static void atp_close(struct input_dev *input)
 	struct atp *dev = input_get_drvdata(input);
 
 	usb_kill_urb(dev->urb);
+	cancel_work_sync(&dev->work);
 	dev->open = 0;
 }

@@ -528,40 +598,10 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 		dev->datalen = 81;
 
 	if (atp_is_geyser_3(dev)) {
-		/*
-		 * By default Geyser 3 device sends standard USB HID mouse
-		 * packets (Report ID 2). This code changes device mode, so it
-		 * sends raw sensor reports (Report ID 5).
-		 */
-		char data[8];
-		int size;
-
-		size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_READ_REQUEST_ID,
-			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
-
-		if (size != 8) {
-			err("Could not do mode read request from device"
-							" (Geyser 3 mode)");
+		/* switch to raw sensor mode */
+		if (atp_geyser3_init(udev))
 			goto err_free_devs;
-		}
-
-		/* Apply the mode switch */
-		data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
-
-		size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
-			USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
 
-		if (size != 8) {
-			err("Could not do mode write request to device"
-							" (Geyser 3 mode)");
-			goto err_free_devs;
-		}
 		printk("appletouch Geyser 3 inited.\n");
 	}

@@ -636,6 +676,8 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 	/* save our data pointer in this interface device */
 	usb_set_intfdata(iface, dev);
 
+	INIT_WORK(&dev->work, atp_reinit);
+
 	return 0;
 
  err_free_buffer:

Dmitry Torokhov

2007-07-18 01:48 UTC #5 lore raw

Re: [PATCH] appletouch powersaving - please apply for 2.6.23-rc1 take #3

Quoted 22 lines · Soeren Sonnenburg

Quoted 7 lines · Dmitry Torokhov
Hi,
Quoted 4 lines · Soeren Sonnenburg
err_free_buffer:
(quoted diff) +0 -0
@@ -656,6 +699,7 @@ static void atp_disconnect(struct usb_interface *iface)
 
Quoted 11 lines
Review static void atp_disconnect(struct usb_interface *iface) · line 705
Quoted 7 lines
(quoted diff) +1 -0
       usb_set_intfdata(iface, NULL);
       if (dev) {
+               cancel_work_sync(&dev->work);
               usb_kill_urb(dev->urb);
               input_unregister_device(dev->input);
               usb_buffer_free(dev->udev, dev->datalen,
This should go into atp_close() and I think you need to do cancel_work_sync after calling usb_kill_urb() otherwise you risk it being submitted while you gettingto kill the urb.
good catch. modified patch accordingly+attached.

Quoted 9 lines

Quoted 1 line
How many boxes did you try this patch on?
Mine plus 1 other. However please note that Matthews patch has been (which is what this patch is based on) is in the mactel-patches repository for quite some time now and that the not-yet-cleanup up variant of this patch was posted to mactel-devel...
So the modeswitch part should work...

OK, can I please get signed-off-bys for the latest version so I can apply it?

Thanks!

Signature · 1 line

Dmitry

Soeren Sonnenburg

author

2007-07-18 05:32 UTC #6 lore raw

Re: [PATCH] appletouch powersaving - please apply for 2.6.23-rc1 take #3

Quoted 10 lines · Dmitry Torokhov

Quoted 1 line · Soeren Sonnenburg
On Tue, 2007-07-17 at 11:01 -0400, Dmitry Torokhov wrote:
[...]
Quoted 7 lines
Quoted 1 line
How many boxes did you try this patch on?
Mine plus 1 other. However please note that Matthews patch has been (which is what this patch is based on) is in the mactel-patches repository for quite some time now and that the not-yet-cleanup up variant of this patch was posted to mactel-devel...

Quoted 5 lines

Quoted 2 lines
So the modeswitch part should work...
OK, can I please get signed-off-bys for the latest version so I can apply it?

They are the same as in the initial patch:

Signed-off-by Soeren Sonnenburg <[email protected]>

Signed-off-by Matthew Garrett <[email protected]>

Soeren

Signature · 2 lines

Sometimes, there's a moment as you're waking, when you become aware of
the real world around you, but you're still dreaming.

Andrew Morton

2007-07-18 22:57 UTC #7 lore raw

In reply to Soeren Sonnenburg

Quoted 14 lines · On Tue, 17 Jul 2007 09:10:18 +0200 Soeren Sonnenburg <[email protected]> wrote:

the attached minimally intrusive patch is based on Matthew Garret's patch 'Make appletouch shut up when it has nothing to say' patches (e.g.
http://lkml.org/lkml/2007/5/13/117): Matthews description follows / second paragraph lists my additional changes.
The appletouch geyser3 devices found in the Intel Macs (and possibly some later PPC ones?) send a constant stream of packets after the first touch. This results in the kernel waking up around once every couple of milliseconds to process them, making it almost impossible to spend any significant period of time in C3 state on a dynamic HZ kernel. Sending the mode initialization code makes the device shut up until it's touched again. This patch does so after receiving 10 packets with no interesting content.

Quoted 5 lines

In addition it now empties the work queue via cancel_work_sync on module exit, keeps all error checking and only reports BTN_LEFT presses if bit 1 in the status byte (last byte in packet) is set. This fixes the random left clicks issue. Furthermore it invalidates touchpad data before the mode switch, which fixes the touchpad runs amok issue.

Please feed this through scripts/checkpatch.pl and consider addressing all the things which it reports.

Soeren Sonnenburg

author

2007-07-19 03:35 UTC #8 lore raw

Re: [PATCH] appletouch powersaving - please apply for 2.6.23-rc1 - take #4

Quoted 17 lines · Andrew Morton

Quoted 14 lines · On Tue, 17 Jul 2007 09:10:18 +0200 Soeren Sonnenburg <[email protected]> wrote:
the attached minimally intrusive patch is based on Matthew Garret's patch 'Make appletouch shut up when it has nothing to say' patches (e.g.
http://lkml.org/lkml/2007/5/13/117): Matthews description follows / second paragraph lists my additional changes.
The appletouch geyser3 devices found in the Intel Macs (and possibly some later PPC ones?) send a constant stream of packets after the first touch. This results in the kernel waking up around once every couple of milliseconds to process them, making it almost impossible to spend any significant period of time in C3 state on a dynamic HZ kernel. Sending the mode initialization code makes the device shut up until it's touched again. This patch does so after receiving 10 packets with no interesting content.

Quoted 8 lines

Quoted 5 lines
In addition it now empties the work queue via cancel_work_sync on module exit, keeps all error checking and only reports BTN_LEFT presses if bit 1 in the status byte (last byte in packet) is set. This fixes the random left clicks issue. Furthermore it invalidates touchpad data before the mode switch, which fixes the touchpad runs amok issue.
Please feed this through scripts/checkpatch.pl and consider addressing all the things which it reports.

So I did. Updated patch which differs in

dev->valid = 0; (note the space around =) and further removed debug code (which never got triggered but checkpatch.pl complained about)

attached.

Soeren

Signature · 2 lines

Sometimes, there's a moment as you're waking, when you become aware of
the real world around you, but you're still dreaming.

The attached patch is based on Matthew Garret's patch 'Make appletouch shut up when it has nothing to say' patches (e.g.
http://lkml.org/lkml/2007/5/13/117): Matthews description follows / second paragraph lists my additional changes.

The appletouch geyser3 devices found in the Intel Macs (and possibly some later PPC ones?) send a constant stream of packets after the first touch. This results in the kernel waking up around once every couple of milliseconds to process them, making it almost impossible to spend any significant period of time in C3 state on a dynamic HZ kernel. Sending the mode initialization code makes the device shut up until it's touched again.
This patch does so after receiving 10 packets with no interesting content.

Signed-off-by Soeren Sonnenburg <[email protected]>

Signed-off-by Matthew Garrett <[email protected]>

Cc Nicolas Boichat <[email protected]>

Cc Michael Hanselmann <[email protected]>

Cc Peter Osterlund <[email protected]>

Cc Frank Arnold <[email protected]>

Cc Stelian Pop <[email protected]>

Cc Johannes Berg <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

drivers/input/mouse/appletouch.c +69 ~~-35~~

index e321526..f336b7b 100644
@@ -155,6 +155,8 @@ struct atp {
 	int			xy_acc[ATP_XSENSORS + ATP_YSENSORS];
 	int			overflowwarn;	/* overflow warning printed? */
 	int			datalen;	/* size of an USB urb transfer */
+	int			idlecount;      /* number of empty packets */
+	struct work_struct      work;
 };
 
 #define dbg_dump(msg, tab) \

@@ -208,6 +210,55 @@ static inline int atp_is_geyser_3(struct atp *dev)
 		(productId == GEYSER4_JIS_PRODUCT_ID);
 }
 
+/*
+ * By default Geyser 3 device sends standard USB HID mouse
+ * packets (Report ID 2). This code changes device mode, so it
+ * sends raw sensor reports (Report ID 5).
+ */
+static int atp_geyser3_init(struct usb_device *udev)
+{
+	char data[8];
+	int size;
+
+	size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+			ATP_GEYSER3_MODE_READ_REQUEST_ID,
+			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			ATP_GEYSER3_MODE_REQUEST_VALUE,
+			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+	if (size != 8) {
+		err("Could not do mode read request from device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+
+	/* Apply the mode switch */
+	data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
+
+	size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
+			USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			ATP_GEYSER3_MODE_REQUEST_VALUE,
+			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
+
+	if (size != 8) {
+		err("Could not do mode write request to device"
+		    " (Geyser 3 mode)");
+		return -EIO;
+	}
+	return 0;
+}
+
+/* Reinitialise the device if it's a geyser 3 */
+static void atp_reinit(struct work_struct *work)
+{
+	struct atp *dev = container_of(work, struct atp, work);
+	struct usb_device *udev = dev->udev;
+
+	dev->idlecount = 0;
+	atp_geyser3_init(udev);
+}
+
 static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
 			     int *z, int *fingers)
 {

@@ -449,11 +500,21 @@ static void atp_complete(struct urb* urb)
 
 		/* reset the accumulator on release */
 		memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
-	}
 
-	input_report_key(dev->input, BTN_LEFT,
-			 !!dev->data[dev->datalen - 1]);
+		/* Geyser 3 will continue to send packets continually after
+		   the first touch unless reinitialised. Do so if it's been
+		   idle for a while in order to avoid waking the kernel up
+		   several hundred times a second */
+		if (atp_is_geyser_3(dev)) {
+			dev->idlecount++;
+			if (dev->idlecount == 10) {
+				dev->valid = 0;
+				schedule_work (&dev->work);
+			}
+		}
+	}
 
+	input_report_key(dev->input, BTN_LEFT, dev->data[dev->datalen-1] & 1);
 	input_sync(dev->input);
 
 exit:

@@ -480,6 +541,7 @@ static void atp_close(struct input_dev *input)
 	struct atp *dev = input_get_drvdata(input);
 
 	usb_kill_urb(dev->urb);
+	cancel_work_sync(&dev->work);
 	dev->open = 0;
 }

@@ -528,40 +590,10 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 		dev->datalen = 81;
 
 	if (atp_is_geyser_3(dev)) {
-		/*
-		 * By default Geyser 3 device sends standard USB HID mouse
-		 * packets (Report ID 2). This code changes device mode, so it
-		 * sends raw sensor reports (Report ID 5).
-		 */
-		char data[8];
-		int size;
-
-		size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_READ_REQUEST_ID,
-			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
-
-		if (size != 8) {
-			err("Could not do mode read request from device"
-							" (Geyser 3 mode)");
+		/* switch to raw sensor mode */
+		if (atp_geyser3_init(udev))
 			goto err_free_devs;
-		}
-
-		/* Apply the mode switch */
-		data[0] = ATP_GEYSER3_MODE_VENDOR_VALUE;
-
-		size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			ATP_GEYSER3_MODE_WRITE_REQUEST_ID,
-			USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			ATP_GEYSER3_MODE_REQUEST_VALUE,
-			ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);
 
-		if (size != 8) {
-			err("Could not do mode write request to device"
-							" (Geyser 3 mode)");
-			goto err_free_devs;
-		}
 		printk("appletouch Geyser 3 inited.\n");
 	}

@@ -636,6 +668,8 @@ static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id
 	/* save our data pointer in this interface device */
 	usb_set_intfdata(iface, dev);
 
+	INIT_WORK(&dev->work, atp_reinit);
+
 	return 0;
 
  err_free_buffer: