From: Johannes Berg <johannes@sipsolutions.net> Date: 2007-10-24 10:44:07
The patch 46249ea60fbb61a72ee6929b831b1f3e6865f024 was obviously done
without testing on a Geyser 1, and I'm a very annoyed that it was
applied. It causes appletouch to continuously printk:
drivers/input/mouse/appletouch.c: Could not do mode read request from device (Geyser 3 mode)
because the Geyser 1 doesn't respond to that. The patch description also
states:
if we see 10 empty packets the touchpad needs to be reset; good
touchpads should not send empty packets anyway.
which is *TOTALLY* bogus since Geyser 1 touchpads have no notion of
empty packets, the simply continuously send measurements. One look at
the specification would have confirmed that.
This reverts the clueless commit.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Hi Johannes,
On 10/24/07, Johannes Berg [off-list ref] wrote:
The patch 46249ea60fbb61a72ee6929b831b1f3e6865f024 was obviously done
without testing on a Geyser 1,
My fault, sorry. However Anton's device has product ID of 90x30B which
is Geyser 1 as far as I understand... But yes, we should not expect
other geysers respond to Geyser 3-specific commands.
and I'm a very annoyed that it was
applied. It causes appletouch to continuously printk:
drivers/input/mouse/appletouch.c: Could not do mode read request from device (Geyser 3 mode)
because the Geyser 1 doesn't respond to that. The patch description also
states:
quoted
if we see 10 empty packets the touchpad needs to be reset; good
touchpads should not send empty packets anyway.
which is *TOTALLY* bogus since Geyser 1 touchpads have no notion of
empty packets, the simply continuously send measurements. One look at
the specification would have confirmed that.
Is there a way to "plug" these Geysers? Waking up the kernel
continuously is not nice.
--
Dmitry
From: Johannes Berg <johannes@sipsolutions.net> Date: 2007-10-24 13:07:26
On Wed, 2007-10-24 at 12:44 +0200, Johannes Berg wrote:
The patch 46249ea60fbb61a72ee6929b831b1f3e6865f024 was obviously done
without testing on a Geyser 1, and I'm a very annoyed that it was
applied. It causes appletouch to continuously printk:
I spoke too soon, I don't have a Geyser 1 but rather a Fountain touchpad
on my powerbook.
johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: 2007-10-24 13:10:34
The patch 46249ea60fbb61a72ee6929b831b1f3e6865f024 was obviously done
without testing on a fountain touchpad. It causes appletouch to
continuously printk:
drivers/input/mouse/appletouch.c: Could not do mode read request from device (Geyser 3 mode)
because the fountain touchpad doesn't respond to that. The patch description
also states:
if we see 10 empty packets the touchpad needs to be reset; good
touchpads should not send empty packets anyway.
which is *TOTALLY* bogus since fountain touchpads have no notion of
empty packets, the simply continuously send measurements. One look at
the specification would have confirmed that.
This reverts the clueless commit, a better solution for geyser 1
touchpads must be found.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
What I'd advocate for 2.6.25 is to split appletouch into two drivers:
"appletouch" for fountain touchpads and maybe "appletouch2" for geyser
touchpads, this will get rid of many of the huge if statements in the
packet processing path and make sure that the macbook crowd will no
longer have to workaround the powerbook touchpads seeing that we seem to
hardly talk to each other.
Or maybe Soeren Sonnenburg's rewrite could be used for Geyser touchpads.
drivers/input/mouse/appletouch.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
From: Johannes Berg <johannes@sipsolutions.net> Date: 2007-10-24 13:35:38
On Wed, 2007-10-24 at 09:34 -0400, Dmitry Torokhov wrote:
Well, but what about fountains then? Regardless of the model, if there
is a way to stop "empty" meaurements, we should do it.
There is no way on fountains though. We could check the measurement
ourselves and if no finger is detected decrease the polling frequency or
something, but there's no hw support.
johannes
On Wed, 2007-10-24 at 09:34 -0400, Dmitry Torokhov wrote:
quoted
Well, but what about fountains then? Regardless of the model, if there
is a way to stop "empty" meaurements, we should do it.
There is no way on fountains though. We could check the measurement
ourselves and if no finger is detected decrease the polling frequency or
something, but there's no hw support.
Do yo know who has powerbooks with older geyser models (0x214, 215,
216)? It would be nice to know if they send the data continiously and
whether the geyser 3 reset hack works on them.
--
Dmitry
From: Benjamin Berg <hidden> Date: 2007-10-25 19:11:04
On Thu, 2007-25-10 at 15:23 +0200, Johannes Berg wrote:
On Wed, 2007-10-24 at 10:29 -0400, Dmitry Torokhov wrote:
quoted
Do yo know who has powerbooks with older geyser models (0x214, 215,
216)?
Not sure, Benjamin? We're talking about the touchpad, just lsusb should
be enough.
lsusb says I have a 0x215
(Bus 003 Device 002: ID 05ac:0215 Apple Computer, Inc.)
Everything is working fine with a 2.6.24-rc1 kernel (ie. no bogus
messages).
quoted
It would be nice to know if they send the data continiously and
whether the geyser 3 reset hack works on them.
That'd be good, but for fountains it definitely doesn't work.
On Thu, 2007-25-10 at 15:23 +0200, Johannes Berg wrote:
quoted
On Wed, 2007-10-24 at 10:29 -0400, Dmitry Torokhov wrote:
quoted
Do yo know who has powerbooks with older geyser models (0x214, 215,
216)?
Not sure, Benjamin? We're talking about the touchpad, just lsusb should
be enough.
lsusb says I have a 0x215
(Bus 003 Device 002: ID 05ac:0215 Apple Computer, Inc.)
Everything is working fine with a 2.6.24-rc1 kernel (ie. no bogus
messages).
quoted
quoted
It would be nice to know if they send the data continiously and
whether the geyser 3 reset hack works on them.
That'd be good, but for fountains it definitely doesn't work.
Benjamin
Johannes, and what is product ID for your touchpad?
--
Dmitry
On Friday 26 October 2007 16:31, Johannes Berg wrote:
quoted
Johannes, and what is product ID for your touchpad?
It's 0x20e, listed as 'fountain'
OK, then maybe instead of reverting the change outright we could try the
patch below?
--
Dmitry
Input: appletouch - idle reset logic broke older Fountains
Older models of fountains do not support change mode request and
therefore shoudl be excluded from idle reset attempts.
Signed-off-by: Dmitry Torokhov <redacted>
---
drivers/input/mouse/appletouch.c | 83 +++++++++++++++++++++++----------------
1 file changed, 49 insertions(+), 34 deletions(-)
Index: work/drivers/input/mouse/appletouch.c
===================================================================
@@ -130,11 +130,11 @@ MODULE_DEVICE_TABLE (usb, atp_table);#define ATP_THRESHOLD 5/* MacBook Pro (Geyser 3 & 4) initialization constants */-#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1-#define ATP_GEYSER3_MODE_WRITE_REQUEST_ID 9-#define ATP_GEYSER3_MODE_REQUEST_VALUE 0x300-#define ATP_GEYSER3_MODE_REQUEST_INDEX 0-#define ATP_GEYSER3_MODE_VENDOR_VALUE 0x04+#define ATP_GEYSER_MODE_READ_REQUEST_ID 1+#define ATP_GEYSER_MODE_WRITE_REQUEST_ID 9+#define ATP_GEYSER_MODE_REQUEST_VALUE 0x300+#define ATP_GEYSER_MODE_REQUEST_INDEX 0+#define ATP_GEYSER_MODE_VENDOR_VALUE 0x04/* Structure to hold all of our device specific stuff */structatp{
@@ -188,6 +188,14 @@ static int debug = 1;module_param(debug,int,0644);MODULE_PARM_DESC(debug,"Activate debugging output");+staticinlineintatp_is_old_fountain(structatp*dev)+{+u16productId=le16_to_cpu(dev->udev->descriptor.idProduct);++returnproductId==FOUNTAIN_ANSI_PRODUCT_ID||+productId==FOUNTAIN_ISO_PRODUCT_ID;+}+/* Checks if the device a Geyser 2 (ANSI, ISO, JIS) */staticinlineintatp_is_geyser_2(structatp*dev){
@@ -211,52 +219,55 @@ static inline int atp_is_geyser_3(struct}/*-*BydefaultGeyser3devicesendsstandardUSBHIDmouse+*BydefaultnewerGeyserdevicessendstandardUSBHIDmouse*packets(ReportID2).Thiscodechangesdevicemode,soit*sendsrawsensorreports(ReportID5).*/-staticintatp_geyser3_init(structusb_device*udev)+staticintatp_geyser_init(structusb_device*udev){chardata[8];intsize;size=usb_control_msg(udev,usb_rcvctrlpipe(udev,0),-ATP_GEYSER3_MODE_READ_REQUEST_ID,+ATP_GEYSER_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);+ATP_GEYSER_MODE_REQUEST_VALUE,+ATP_GEYSER_MODE_REQUEST_INDEX,&data,8,5000);if(size!=8){err("Could not do mode read request from device"-" (Geyser 3 mode)");+" (Geyser Raw mode)");return-EIO;}/* Apply the mode switch */-data[0]=ATP_GEYSER3_MODE_VENDOR_VALUE;+data[0]=ATP_GEYSER_MODE_VENDOR_VALUE;size=usb_control_msg(udev,usb_sndctrlpipe(udev,0),-ATP_GEYSER3_MODE_WRITE_REQUEST_ID,+ATP_GEYSER_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);+ATP_GEYSER_MODE_REQUEST_VALUE,+ATP_GEYSER_MODE_REQUEST_INDEX,&data,8,5000);if(size!=8){err("Could not do mode write request to device"-" (Geyser 3 mode)");+" (Geyser Raw mode)");return-EIO;}return0;}-/* Reinitialise the device if it's a geyser 3 */+/*+*Reinitialisethedevice.Thisusuallystopsstreamofemptypackets+*comingformit.+*/staticvoidatp_reinit(structwork_struct*work){structatp*dev=container_of(work,structatp,work);structusb_device*udev=dev->udev;dev->idlecount=0;-atp_geyser3_init(udev);+atp_geyser_init(udev);}staticintatp_calculate_abs(int*xy_sensors,intnb_sensors,intfact,
@@ -507,19 +518,23 @@ static void atp_complete(struct urb* urbinput_report_key(dev->input,BTN_LEFT,key);input_sync(dev->input);-/* Many Geysers will continue to send packets continually after-thefirsttouchunlessreinitialised.Dosoifit'sbeen-idleforawhileinordertoavoidwakingthekernelup-severalhundredtimesasecond*/--if(!x&&!y&&!key){-dev->idlecount++;-if(dev->idlecount==10){-dev->valid=0;-schedule_work(&dev->work);-}-}else-dev->idlecount=0;+/*+*ManyGeyserswillcontinuetosendpacketscontinuallyafter+*thefirsttouchunlessreinitialised.Dosoifit'sbeen+*idleforawhileinordertoavoidwakingthekernelup+*severalhundredtimesasecond.Re-initializationdoesnot+*workonolderversionsofFountaintouchpads.+*/+if(!atp_is_old_fountain(dev)){+if(!x&&!y&&!key){+dev->idlecount++;+if(dev->idlecount==10){+dev->valid=0;+schedule_work(&dev->work);+}+}else+dev->idlecount=0;+}exit:retval=usb_submit_urb(dev->urb,GFP_ATOMIC);
@@ -593,12 +608,12 @@ static int atp_probe(struct usb_interfacelsedev->datalen=81;-if(atp_is_geyser_3(dev)){+if(!atp_is_old_fountain(dev)){/* switch to raw sensor mode */-if(atp_geyser3_init(udev))+if(atp_geyser_init(udev))gotoerr_free_devs;-printk("appletouch Geyser 3 inited.\n");+printk(KERN_INFO"appletouch: Geyser mode initialized.\n");}dev->urb=usb_alloc_urb(0,GFP_KERNEL);
OK, then maybe instead of reverting the change outright we could try the
patch below?
That patch works,
Any chance Benjamin could also test it? The behaviour is different
from 2.6.24-rc1 since we call atp_geyser_init for all geysers now.
minor comments:
quoted
Older models of fountains do not support change mode request and
I think there's only one fountain model.
I was hoping that FOUNTAIN_TP_ONLY_PRODUCT_ID (0x30A) behaves similar
to Geyser in this regard. If you know that this assumption is incorrect
then we need to rename atp_is_older_fountain() to atp_is_fountain()
anf add this product ID to it.
quoted
therefore shoudl be excluded from idle reset attempts.
typo
OK
quoted
/* MacBook Pro (Geyser 3 & 4) initialization constants */
That comment is no longer correct, you should change it.
OK
quoted
-#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1
quoted
+ * Reinitialise the device. This usually stops stream of empty packets
+ * coming form it.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2007-10-28 15:06:55
I was hoping that FOUNTAIN_TP_ONLY_PRODUCT_ID (0x30A) behaves similar
to Geyser in this regard. If you know that this assumption is incorrect
then we need to rename atp_is_older_fountain() to atp_is_fountain()
anf add this product ID to it.
Ah ok, I forgot about that one. If I were to venture a guess I'd say
that they renamed it to geyser because of this idle behaviour
"technology upgrade", but I can't say, nor do I know anybody with such a
version, sorry.
johannes
On Sunday 28 October 2007 11:08, Johannes Berg wrote:
quoted
I was hoping that FOUNTAIN_TP_ONLY_PRODUCT_ID (0x30A) behaves similar
to Geyser in this regard. If you know that this assumption is incorrect
then we need to rename atp_is_older_fountain() to atp_is_fountain()
anf add this product ID to it.
Ah ok, I forgot about that one. If I were to venture a guess I'd say
that they renamed it to geyser because of this idle behaviour
"technology upgrade", but I can't say, nor do I know anybody with such a
version, sorry.
OK then let's play safe and don't touch fountains at all. How about the
patch below?
--
Dmitry
Input: appletouch - idle reset logic broke older Fountains
Fountains do not support change mode request and therefore
should be excluded from idle reset attempts.
Also:
- do not re-submit URB when we decide that toucvhpad needs to be
reinicialized
- do not repeat size detection when reinitializing the touchpad
- Add missing KERN_* prefixes to messages
Signed-off-by: Dmitry Torokhov <redacted>
---
drivers/input/mouse/appletouch.c | 125 ++++++++++++++++++++++++---------------
1 file changed, 77 insertions(+), 48 deletions(-)
Index: work/drivers/input/mouse/appletouch.c
===================================================================
@@ -129,12 +129,12 @@ MODULE_DEVICE_TABLE (usb, atp_table);*/#define ATP_THRESHOLD 5-/* MacBook Pro (Geyser 3 & 4) initialization constants */-#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1-#define ATP_GEYSER3_MODE_WRITE_REQUEST_ID 9-#define ATP_GEYSER3_MODE_REQUEST_VALUE 0x300-#define ATP_GEYSER3_MODE_REQUEST_INDEX 0-#define ATP_GEYSER3_MODE_VENDOR_VALUE 0x04+/* Geyser initialization constants */+#define ATP_GEYSER_MODE_READ_REQUEST_ID 1+#define ATP_GEYSER_MODE_WRITE_REQUEST_ID 9+#define ATP_GEYSER_MODE_REQUEST_VALUE 0x300+#define ATP_GEYSER_MODE_REQUEST_INDEX 0+#define ATP_GEYSER_MODE_VENDOR_VALUE 0x04/* Structure to hold all of our device specific stuff */structatp{
@@ -142,9 +142,11 @@ struct atp {structusb_device*udev;/* usb device */structurb*urb;/* usb request block */signedchar*data;/* transferred data */-intopen;/* non-zero if opened */-structinput_dev*input;/* input dev */-intvalid;/* are the sensors valid ? */+structinput_dev*input;/* input dev */+unsignedcharopen;/* non-zero if opened */+unsignedcharvalid;/* are the sensors valid ? */+unsignedcharsize_detect_done;+unsignedcharoverflowwarn;/* overflow warning printed? */intx_old;/* last reported x/y, */inty_old;/* used for smoothing *//* current value of the sensors */
@@ -153,7 +155,6 @@ struct atp {signedcharxy_old[ATP_XSENSORS+ATP_YSENSORS];/* accumulated sensors */intxy_acc[ATP_XSENSORS+ATP_YSENSORS];-intoverflowwarn;/* overflow warning printed? */intdatalen;/* size of an USB urb transfer */intidlecount;/* number of empty packets */structwork_structwork;
@@ -170,7 +171,7 @@ struct atp {#define dprintk(format, a...) \do{\-if(debug)printk(format,##a);\+if(debug)printk(KERN_DEBUGformat,##a);\}while(0)MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Michael Hanselmann");
@@ -188,6 +189,15 @@ static int debug = 1;module_param(debug,int,0644);MODULE_PARM_DESC(debug,"Activate debugging output");+staticinlineintatp_is_fountain(structatp*dev)+{+u16productId=le16_to_cpu(dev->udev->descriptor.idProduct);++returnproductId==FOUNTAIN_ANSI_PRODUCT_ID||+productId==FOUNTAIN_ISO_PRODUCT_ID||+productId==FOUNTAIN_TP_ONLY_PRODUCT_ID;+}+/* Checks if the device a Geyser 2 (ANSI, ISO, JIS) */staticinlineintatp_is_geyser_2(structatp*dev){
@@ -211,52 +221,63 @@ static inline int atp_is_geyser_3(struct}/*-*BydefaultGeyser3devicesendsstandardUSBHIDmouse+*BydefaultnewerGeyserdevicessendstandardUSBHIDmouse*packets(ReportID2).Thiscodechangesdevicemode,soit*sendsrawsensorreports(ReportID5).*/-staticintatp_geyser3_init(structusb_device*udev)+staticintatp_geyser_init(structusb_device*udev){chardata[8];intsize;size=usb_control_msg(udev,usb_rcvctrlpipe(udev,0),-ATP_GEYSER3_MODE_READ_REQUEST_ID,+ATP_GEYSER_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);+ATP_GEYSER_MODE_REQUEST_VALUE,+ATP_GEYSER_MODE_REQUEST_INDEX,&data,8,5000);if(size!=8){err("Could not do mode read request from device"-" (Geyser 3 mode)");+" (Geyser Raw mode)");return-EIO;}/* Apply the mode switch */-data[0]=ATP_GEYSER3_MODE_VENDOR_VALUE;+data[0]=ATP_GEYSER_MODE_VENDOR_VALUE;size=usb_control_msg(udev,usb_sndctrlpipe(udev,0),-ATP_GEYSER3_MODE_WRITE_REQUEST_ID,+ATP_GEYSER_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);+ATP_GEYSER_MODE_REQUEST_VALUE,+ATP_GEYSER_MODE_REQUEST_INDEX,&data,8,5000);if(size!=8){err("Could not do mode write request to device"-" (Geyser 3 mode)");+" (Geyser Raw mode)");return-EIO;}return0;}-/* Reinitialise the device if it's a geyser 3 */+/*+*Reinitialisethedevice.Thisusuallystopsstreamofemptypackets+*comingfromit.+*/staticvoidatp_reinit(structwork_struct*work){structatp*dev=container_of(work,structatp,work);structusb_device*udev=dev->udev;+intretval;dev->idlecount=0;-atp_geyser3_init(udev);++atp_geyser_init(udev);++retval=usb_submit_urb(dev->urb,GFP_ATOMIC);+if(retval){+err("%s - usb_submit_urb failed with result %d",+__FUNCTION__,retval);+}}staticintatp_calculate_abs(int*xy_sensors,intnb_sensors,intfact,
@@ -337,7 +358,7 @@ static void atp_complete(struct urb* urbbreak;case-EOVERFLOW:if(!dev->overflowwarn){-printk("appletouch: OVERFLOW with data "+printk(KERN_WARNING"appletouch: OVERFLOW with data ""length %d, actual length is %d\n",dev->datalen,dev->urb->actual_length);dev->overflowwarn=1;
@@ -426,15 +447,17 @@ static void atp_complete(struct urb* urbdev->x_old=dev->y_old=-1;memcpy(dev->xy_old,dev->xy_cur,sizeof(dev->xy_old));-if(atp_is_geyser_3(dev))/* No 17" Macbooks (yet) */+if(dev->size_detect_done||+atp_is_geyser_3(dev))/* No 17" Macbooks (yet) */gotoexit;/* 17" Powerbooks have extra X sensors */-for(i=(atp_is_geyser_2(dev)?15:16);i<ATP_XSENSORS;i++){-if(!dev->xy_cur[i])continue;+for(i=(atp_is_geyser_2(dev)?15:16);i<ATP_XSENSORS;i++){+if(!dev->xy_cur[i])+continue;-printk("appletouch: 17\" model detected.\n");-if(atp_is_geyser_2(dev))+printk(KERN_INFO"appletouch: 17\" model detected.\n");+if(atp_is_geyser_2(dev))input_set_abs_params(dev->input,ABS_X,0,(20-1)*ATP_XFACT-1,
From: Johannes Berg <johannes@sipsolutions.net> Date: 2007-10-29 08:10:54
OK then let's play safe and don't touch fountains at all. How about the
patch below?
Looks fine to me, works with my fountain touchpad and should fix
Joseph's error too.
Input: appletouch - idle reset logic broke older Fountains
Fountains do not support change mode request and therefore
should be excluded from idle reset attempts.
Also:
- do not re-submit URB when we decide that toucvhpad needs to be
reinicialized
- do not repeat size detection when reinitializing the touchpad
- Add missing KERN_* prefixes to messages
Signed-off-by: Dmitry Torokhov <redacted>
Acked-by: Johannes Berg <johannes@sipsolutions.net>