Re: [linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-15 Thread Soeren Sonnenburg 
On Sun, 2007-05-13 at 18:58 -0700, Pete Zaitcev wrote:
 On Sun, 13 May 2007 20:57:25 +0100, Matthew Garrett [EMAIL PROTECTED] wrote:
 
  Ok, I've tidied this up a little. [...]
 
 Looks fine here... well, almost. Did you try rmmod (I don't even know if
 it's applicable, sorry)? Usually, when schedule_work is involved, you want
 to make sure that a scheduled work won't be run when the module is gone.
 More often, a device removal is the issue, but as I take it, such is not
 possible for a built-in device :-) . In most cases, all it takes is a
 strategically placed flush_scheduled_work().

I was using this patch for some days now and I realized that - from time
to time - the touchpad runs amok, i.e. I more or less unable to control
the mouse when that happens.

Then a rmmod appletouch (+ reload) fixes this, as well as a sleep/resume
cycle.

As I had to rmmod appletouch a lot and did not see crashes I think it
works... though this problem persists...

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

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-15 Thread Matthew Garrett 
On Tue, May 15, 2007 at 08:34:55PM +, Soeren Sonnenburg wrote:

 I was using this patch for some days now and I realized that - from time
 to time - the touchpad runs amok, i.e. I more or less unable to control
 the mouse when that happens.

Hmm. Just seen this. I'll see if I can work around that.

-- 
Matthew Garrett | [EMAIL PROTECTED]

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-13 Thread Matthew Garrett 
The appletouch 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 
initialisation 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: Matthew Garrett [EMAIL PROTECTED]

diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
index e321526..7931a76 100644
--- a/drivers/input/mouse/appletouch.c
+++ b/drivers/input/mouse/appletouch.c
@@ -155,8 +155,12 @@ 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;
 };
 
+static struct workqueue_struct *appletouch_wq;
+
 #define dbg_dump(msg, tab) \
if (debug  1) {\
int i;  \
@@ -208,6 +212,46 @@ static inline int atp_is_geyser_3(struct atp *dev)
(productId == GEYSER4_JIS_PRODUCT_ID);
 }
 
+/* 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;
+   char data[8];
+   int size;
+
+   if (!atp_is_geyser_3(dev))
+   return;
+
+   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;
+   }
+
+   /* 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;
+   }
+}
+
 static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
 int *z, int *fingers)
 {
@@ -419,6 +463,7 @@ static void atp_complete(struct urb* urb)
  ATP_YFACT, y_z, y_f);
 
if (x  y) {
+   dev-idlecount = 0;
if (dev-x_old != -1) {
x = (dev-x_old * 3 + x)  2;
y = (dev-y_old * 3 + y)  2;
@@ -441,7 +486,7 @@ static void atp_complete(struct urb* urb)
dev-y_old = y;
}
else if (!x  !y) {
-
+   dev-idlecount++;
dev-x_old = dev-y_old = -1;
input_report_key(dev-input, BTN_TOUCH, 0);
input_report_abs(dev-input, ABS_PRESSURE, 0);
@@ -449,6 +494,13 @@ static void atp_complete(struct urb* urb)
 
/* reset the accumulator on release */
memset(dev-xy_acc, 0, sizeof(dev-xy_acc));
+
+   /* 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 (dev-idlecount = 10)
+   queue_work (appletouch_wq, dev-work);
}
 
input_report_key(dev-input, BTN_LEFT,
@@ -636,6 +688,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:
@@ -694,12 +748,18 @@ static struct usb_driver atp_driver = {
 
 static int __init atp_init(void)
 {
+   appletouch_wq = create_singlethread_workqueue(kappletouch);
+   if (!appletouch_wq) {
+   printk(KERN_ERR appletouch: failed to create workqueue\n);
+   return -ENOMEM;
+   }
return

Re: [linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-13 Thread Pete Zaitcev 
On Sun, 13 May 2007 18:20:53 +0100, Matthew Garrett [EMAIL PROTECTED] wrote:

 +static void atp_reinit(struct work_struct *work)
 +{
 + char data[8];
 + 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);

Generally speaking, DMA-ing from stack is not allowed. Maybe you want
to add a comment saying buffer on stack because this is Mac-only driver.
I would not ask for kmalloc here which is entirely pointless.

 +
 + /* 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 (dev-idlecount = 10)
 + queue_work (appletouch_wq, dev-work);

So, why don't you check for Geyser 3 or whatnot here instead of doing
it inside the work function? Why poking workqueue on other systems?

 + appletouch_wq = create_singlethread_workqueue(kappletouch);
 + if (!appletouch_wq) {
 + printk(KERN_ERR appletouch: failed to create workqueue\n);
 + return -ENOMEM;
 + }
   return usb_register(atp_driver);

I'm wondering about the wisdom of this too. Why won't the stock
workqueue work? We have way too many daemons already. Plus, making
one more unconditionally regardless of hardware present seems like
stepping over the line to me.

Overall this seems like a good patch to me, only needs to be less
intrusive for owners of other hardware.

-- Pete

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-13 Thread Matthew Garrett 
Ok, I've tidied this up a little. I've separated the actual mode init 
code into a separate function in order to avoid code duplication, and no 
longer creating a new workqueue. The only other change is something that 
I /think/ is actually a bug in the driver to begin with, but I'd like 
some more feedback on that first - the first packet sent after the mode 
change has 0x20 in the final byte. This seems to be interpreted as a 
left mouse button press. As a result, moving the touchpad sends a false 
press after every reinitialisation, or (approximately) every time the 
pointer is moved. As far as I can tell this also happens with the 
existing code, but is probably not noticable there because it won't 
appear again after the first touch on the pad. Just skipping that case 
seems to work fine.

diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
index e321526..f126de8 100644
--- a/drivers/input/mouse/appletouch.c
+++ b/drivers/input/mouse/appletouch.c
@@ -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,51 @@ static inline int atp_is_geyser_3(struct atp *dev)
(productId == GEYSER4_JIS_PRODUCT_ID);
 }
 
+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)
 {
@@ -441,7 +488,6 @@ static void atp_complete(struct urb* urb)
dev-y_old = y;
}
else if (!x  !y) {
-
dev-x_old = dev-y_old = -1;
input_report_key(dev-input, BTN_TOUCH, 0);
input_report_abs(dev-input, ABS_PRESSURE, 0);
@@ -449,10 +495,21 @@ static void atp_complete(struct urb* urb)
 
/* reset the accumulator on release */
memset(dev-xy_acc, 0, sizeof(dev-xy_acc));
+
+   /* 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)
+   schedule_work (dev-work);
+   }
}
 
-   input_report_key(dev-input, BTN_LEFT,
-!!dev-data[dev-datalen - 1]);
+   if (dev-data[dev-datalen-1] != 20) 
+   input_report_key(dev-input, BTN_LEFT,
+!!dev-data[dev-datalen - 1]);
 
input_sync(dev-input);
 
@@ -533,35 +590,9 @@ static int atp_probe(struct usb_interface *iface, const 
struct usb_device_id *id
 * 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,
-

Re: [linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-13 Thread Pete Zaitcev 
On Sun, 13 May 2007 20:57:25 +0100, Matthew Garrett [EMAIL PROTECTED] wrote:

 Ok, I've tidied this up a little. [...]

Looks fine here... well, almost. Did you try rmmod (I don't even know if
it's applicable, sorry)? Usually, when schedule_work is involved, you want
to make sure that a scheduled work won't be run when the module is gone.
More often, a device removal is the issue, but as I take it, such is not
possible for a built-in device :-) . In most cases, all it takes is a
strategically placed flush_scheduled_work().

-- Pete

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] [PATCH] Make appletouch shut up when it has nothing to say

2007-05-13 Thread Soeren Sonnenburg 
On Sun, 2007-05-13 at 20:57 +0100, Matthew Garrett wrote:
 Ok, I've tidied this up a little. I've separated the actual mode init 
 code into a separate function in order to avoid code duplication, and no 
 longer creating a new workqueue. The only other change is something that 
 I /think/ is actually a bug in the driver to begin with, but I'd like 
 some more feedback on that first - the first packet sent after the mode 
 change has 0x20 in the final byte. This seems to be interpreted as a 
 left mouse button press. As a result, moving the touchpad sends a false 
 press after every reinitialisation, or (approximately) every time the 
 pointer is moved. As far as I can tell this also happens with the 
 existing code, but is probably not noticable there because it won't 
 appear again after the first touch on the pad. Just skipping that case 
 seems to work fine.

This patch indeed fixes the problem and I have yet to observe problems
with it... However I don't know whether a re-init is the intended way of
dealing with it...

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

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel