converting appletouch to usb autosuspend again...

converting appletouch to usb autosuspend again...

[Soeren Sonnenburg]

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

Dear all,

I wonder whether anyone else tried to get appletouch usb autosuspend to
work. I've seen Oliver's patch but as it kept oopsing on me and I
couldn't parse the usb_mark_last_busy logic I came up with the attached
patch against (current git; at least didn't oops for me over the last
several days and suspend/resume cycles).

However things I don't understand, 

a) is autosuspend on the appletouch driver used at all (how can I find
out)

b) do I need dev->intf->needs_remote_wakeup = 1; ?

c) is line 33 in the patch safe to do ?

And while we are at it I am still seeing these X falls back to hid mouse
mode, only switching to console and back resolves this (looks like this
only worked in previous kernels as resume from s2ram took a lot longer
due to the IDE driver doing a couple of resets) - is there anything one
could do about it?

Soeren


[-- Attachment #2: appletouch_autosuspend.diff --]
[-- Type: text/x-patch, Size: 3921 bytes --]

diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
index 1f41ae9..19003c8 100644
--- a/drivers/input/mouse/appletouch.c
+++ b/drivers/input/mouse/appletouch.c
@@ -34,6 +34,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/usb/input.h>
+#include <linux/mutex.h>
 
 /* Type of touchpad */
 enum atp_touchpad_type {
@@ -140,10 +141,12 @@ MODULE_DEVICE_TABLE(usb, atp_table);
 struct atp {
 	char			phys[64];
 	struct usb_device	*udev;		/* usb device */
+	struct usb_interface	*intf;		/* our interface */
 	struct urb		*urb;		/* usb request block */
 	signed char		*data;		/* transferred data */
 	struct input_dev	*input;		/* input dev */
 	enum atp_touchpad_type	type;		/* type of touchpad */
+	struct mutex		pm_mutex;	/* serialize access to open/suspend */
 	bool			open;
 	bool			valid;		/* are the samples valid? */
 	bool			size_detect_done;
@@ -259,6 +262,11 @@ static void atp_reinit(struct work_struct *work)
 	if (retval)
 		err("atp_reinit: usb_submit_urb failed with error %d",
 		    retval);
+
+	/* device is idle and available for autosuspend */
+	mutex_lock(&dev->pm_mutex);
+	usb_autopm_disable(dev->intf);
+	mutex_unlock(&dev->pm_mutex);
 }
 
 static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
@@ -547,21 +555,37 @@ static void atp_complete(struct urb *urb)
 static int atp_open(struct input_dev *input)
 {
 	struct atp *dev = input_get_drvdata(input);
+	int error = usb_autopm_get_interface(dev->intf);
+	if (error)
+		return error;
 
-	if (usb_submit_urb(dev->urb, GFP_ATOMIC))
-		return -EIO;
+	mutex_lock(&dev->pm_mutex);
+	error=usb_submit_urb(dev->urb, GFP_ATOMIC);
 
-	dev->open = 1;
-	return 0;
+	if (!error)
+	    dev->open = 1;
+
+	mutex_unlock(&dev->pm_mutex);
+
+	if (error)
+		usb_autopm_put_interface(dev->intf);
+
+	return error;
 }
 
 static void atp_close(struct input_dev *input)
 {
 	struct atp *dev = input_get_drvdata(input);
 
+	mutex_lock(&dev->pm_mutex);
+
 	usb_kill_urb(dev->urb);
 	cancel_work_sync(&dev->work);
 	dev->open = 0;
+
+	mutex_unlock(&dev->pm_mutex);
+
+	usb_autopm_put_interface(dev->intf);
 }
 
 static int atp_handle_geyser(struct atp *dev)
@@ -615,9 +639,12 @@ static int atp_probe(struct usb_interface *iface,
 	}
 
 	dev->udev = udev;
+	dev->intf = iface;
 	dev->input = input_dev;
 	dev->type = id->driver_info;
 	dev->overflow_warned = false;
+	mutex_init(&dev->pm_mutex);
+
 	if (dev->type == ATP_FOUNTAIN || dev->type == ATP_GEYSER1)
 		dev->datalen = 81;
 	else
@@ -750,27 +777,41 @@ static int atp_suspend(struct usb_interface *iface, pm_message_t message)
 {
 	struct atp *dev = usb_get_intfdata(iface);
 
+	mutex_lock(&dev->pm_mutex);
+
 	usb_kill_urb(dev->urb);
 	dev->valid = false;
 
+	mutex_unlock(&dev->pm_mutex);
+
 	return 0;
 }
 
 static int atp_resume(struct usb_interface *iface)
 {
 	struct atp *dev = usb_get_intfdata(iface);
+	int error = 0;
 
-	if (dev->open && usb_submit_urb(dev->urb, GFP_ATOMIC))
-		return -EIO;
+	mutex_lock(&dev->pm_mutex);
 
-	return 0;
+	if (dev->open)
+	    error = usb_submit_urb(dev->urb, GFP_ATOMIC);
+
+	mutex_unlock(&dev->pm_mutex);
+
+	return error;
 }
 
 static int atp_reset_resume(struct usb_interface *iface)
 {
 	struct atp *dev = usb_get_intfdata(iface);
+	int error = 0;
 
-	return atp_recover(dev);
+	mutex_lock(&dev->pm_mutex);
+	error = atp_recover(dev);
+	mutex_unlock(&dev->pm_mutex);
+
+	return error;
 }
 
 static struct usb_driver atp_driver = {
@@ -781,6 +822,7 @@ static struct usb_driver atp_driver = {
 	.resume		= atp_resume,
 	.reset_resume	= atp_reset_resume,
 	.id_table	= atp_table,
+	.supports_autosuspend	= 1,
 };
 
 static int __init atp_init(void)
@@ -795,3 +837,4 @@ static void __exit atp_exit(void)
 
 module_init(atp_init);
 module_exit(atp_exit);
+

Re: converting appletouch to usb autosuspend again...

[Oliver Neukum]

Am Samstag 23 August 2008 23:00:18 schrieb Soeren Sonnenburg:
> Dear all,
> 
> I wonder whether anyone else tried to get appletouch usb autosuspend to
> work. I've seen Oliver's patch but as it kept oopsing on me and I

You might post the oops. I haven't seen it.

> couldn't parse the usb_mark_last_busy logic I came up with the attached
> patch against (current git; at least didn't oops for me over the last
> several days and suspend/resume cycles).
> 
> However things I don't understand, 
> 
> a) is autosuspend on the appletouch driver used at all (how can I find
> out)

Compile your kernel with CONFIG_USB_DEBUG and autosuspend will be logged.
You need to activate autosuspend via sysfs.

> b) do I need dev->intf->needs_remote_wakeup = 1; ?

Yes.

> c) is line 33 in the patch safe to do ?

Why do you want to add it?

> And while we are at it I am still seeing these X falls back to hid mouse
> mode, only switching to console and back resolves this (looks like this
> only worked in previous kernels as resume from s2ram took a lot longer
> due to the IDE driver doing a couple of resets) - is there anything one
> could do about it?

Post a log.

	Regards
		Oliver

Re: converting appletouch to usb autosuspend again...

[Oliver Neukum]

Am Montag 25 August 2008 06:22:45 schrieb Justin Mattock:
> I've notice when letting the system rest for a few and then tapping the
> touchpad I see this in dmesg:
> appletouch: 17" model detected.
> could this be suspend working?

Yes.

> As for appletouch surving suspend
> I't seems to work i.g.
> one tap, two tap, side scroll,

Good.

> although with the latest git
> and the latest update from sid(of xserver);
> I'm not seeing these functions work....

Please post logs. Without logs nobody can help you.

	Regards
		Oliver

Re: converting appletouch to usb autosuspend again...

[Justin Mattock]

On Sun, Aug 24, 2008 at 11:40 PM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Montag 25 August 2008 06:22:45 schrieb Justin Mattock:
>> I've notice when letting the system rest for a few and then tapping the
>> touchpad I see this in dmesg:
>> appletouch: 17" model detected.
>> could this be suspend working?
>
> Yes.
>
>> As for appletouch surving suspend
>> I't seems to work i.g.
>> one tap, two tap, side scroll,
>
> Good.
>
>> although with the latest git
>> and the latest update from sid(of xserver);
>> I'm not seeing these functions work....
>
> Please post logs. Without logs nobody can help you.
>
>        Regards
>                Oliver
>

Alright; as for the appletouch
not working upon wakeup, I just tried it to see,
and it is working...(can't explain why it wasn't, maybe a quirk somwhere);
Although; I am receiving an error, so maybe this has something to
do with it(heres the thread):
http://marc.info/?l=linux-kernel&m=121926203029380&q=raw
from what I remember I issued echo mem > /sys/power/state
and then left the laptop in sleep for a few hours.
Anyways I'll try and grab as much logs as possible.
and thanks for the info and help.

regards;

-- 
Justin P. Mattock

Re: converting appletouch to usb autosuspend again...

[Oliver Neukum]

Am Montag 25 August 2008 09:08:49 schrieb Justin Mattock:
> Alright; as for the appletouch
> not working upon wakeup, I just tried it to see,
> and it is working...(can't explain why it wasn't, maybe a quirk somwhere);
> Although; I am receiving an error, so maybe this has something to
> do with it(heres the thread):
> http://marc.info/?l=linux-kernel&m=121926203029380&q=raw

That's bluetooth. Totally unrelated.

So to summarize: suspend/resume has been tested and works.
What about autosuspend? Did you test that?

	Regards
		Oliver

Re: converting appletouch to usb autosuspend again...

[Justin Mattock]

On Mon, Aug 25, 2008 at 12:32 AM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Montag 25 August 2008 09:08:49 schrieb Justin Mattock:
>> Alright; as for the appletouch
>> not working upon wakeup, I just tried it to see,
>> and it is working...(can't explain why it wasn't, maybe a quirk somwhere);
>> Although; I am receiving an error, so maybe this has something to
>> do with it(heres the thread):
>> http://marc.info/?l=linux-kernel&m=121926203029380&q=raw
>
> That's bluetooth. Totally unrelated.
>
> So to summarize: suspend/resume has been tested and works.
> What about autosuspend? Did you test that?
>
>        Regards
>                Oliver
>
>

autosuspend, meaning when leaving the system
idle, then after a few minuetes tapping the touchpad!
if this is what you are referring to, then yes I see appletouch working
in that area, but if there is something else I'm missing, then yeah I
can look into
it and see what I find.


-- 
Justin P. Mattock

Re: converting appletouch to usb autosuspend again...

[Soeren Sonnenburg]

On Sun, 2008-08-24 at 20:39 +0200, Oliver Neukum wrote:
> Am Samstag 23 August 2008 23:00:18 schrieb Soeren Sonnenburg:
> > Dear all,
> > 
> > I wonder whether anyone else tried to get appletouch usb autosuspend to
> > work. I've seen Oliver's patch but as it kept oopsing on me and I
> 
> You might post the oops. I haven't seen it.

Do you have an updated version of your patch that applies to
git-current? I will give your patch another try then.

> > couldn't parse the usb_mark_last_busy logic I came up with the attached
> > patch against (current git; at least didn't oops for me over the last
> > several days and suspend/resume cycles).
> > 
> > However things I don't understand, 
> > 
> > a) is autosuspend on the appletouch driver used at all (how can I find
> > out)
> 
> Compile your kernel with CONFIG_USB_DEBUG and autosuspend will be logged.
> You need to activate autosuspend via sysfs.

OK, indeed it never autosuspends... I guess this is this due to the
keyboard and the mice being on the same usb port and as the kbd part
does not support autosuspend it never suspends?!

> > b) do I need dev->intf->needs_remote_wakeup = 1; ?
> 
> Yes.
> 
> > c) is line 33 in the patch safe to do ?
> 
> Why do you want to add it?

Well reaching this line the touchpad is idling (hasn't been touched+no
button was pressed) - I somehow wanted to force it to suspend in this
case.

> > And while we are at it I am still seeing these X falls back to hid mouse
> > mode, only switching to console and back resolves this (looks like this
> > only worked in previous kernels as resume from s2ram took a lot longer
> > due to the IDE driver doing a couple of resets) - is there anything one
> > could do about it?
> 
> Post a log.

My fault. I was still rmmod'ing appletouch before suspending. It works
perfectly + reliably the way it is.

Thanks,
Soeren