Re: [rft]power management for usbtouch

Am Donnerstag 26 Juni 2008 16:45:45 schrieb Ville Syrjälä:

On Thu, Jun 26, 2008 at 03:57:17PM +0200, Oliver Neukum wrote:

quoted

Hi,

this is power management for usbtouch. Could somebody test it?

	Regards
		Oliver

---

--- linux-2.6.26-sierra/drivers/input/touchscreen/usbtouchscreen.c.alt2	2008-06-26 15:23:38.000000000 +0200
+++ linux-2.6.26-sierra/drivers/input/touchscreen/usbtouchscreen.c	2008-06-26 15:48:44.000000000 +0200

@@ -89,13 +89,17 @@ struct usbtouch_usb {
 	int buf_len;
 	struct urb *irq;
 	struct usb_device *udev;
+	struct usb_interface *intf;
 	struct input_dev *input;
 	struct usbtouch_device_info *type;
+	struct mutex lock;
 	char name[128];
 	char phys[64];
 
 	int x, y;
 	int touch, press;
+
+	char open:1;
 };
 
 

@@ -820,13 +824,23 @@ exit:
 static int usbtouch_open(struct input_dev *input)
 {
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int rv;
 
+	rv = usb_autopm_get_interface(usbtouch->intf);
+	if (rv < 0)
+		goto bail;
+	mutex_lock(&usbtouch->lock);
 	usbtouch->irq->dev = usbtouch->udev;
 
-	if (usb_submit_urb(usbtouch->irq, GFP_KERNEL))
-		return -EIO;
-
-	return 0;
+	if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
+		rv = -EIO;
+		usb_autopm_put_interface(usbtouch->intf);
+	} else {
+		usbtouch->open = 1;
+	}
+	mutex_unlock(&usbtouch->lock);
+bail:
+	return rv;
 }
 
 static void usbtouch_close(struct input_dev *input)

@@ -834,6 +848,10 @@ static void usbtouch_close(struct input_
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
 
 	usb_kill_urb(usbtouch->irq);

If usbtouch_resume() happens here it will resubmit the urb.

Thanks. That's a bug.

quoted

+	mutex_lock(&usbtouch->lock);
+	usbtouch->open = 0;
+	mutex_unlock(&usbtouch->lock);
+	usb_autopm_put_interface(usbtouch->intf);
 }
 
 

@@ -889,6 +907,8 @@ static int usbtouch_probe(struct usb_int
 
 	usbtouch->udev = udev;
 	usbtouch->input = input_dev;
+	mutex_init(&usbtouch->lock);
+	usbtouch->intf = intf;
 
 	if (udev->manufacturer)
 		strlcpy(usbtouch->name, udev->manufacturer, sizeof(usbtouch->name));

@@ -973,20 +993,76 @@ static void usbtouch_disconnect(struct u
 
 	dbg("%s - usbtouch is initialized, cleaning up", __FUNCTION__);
 	usb_set_intfdata(intf, NULL);
-	input_unregister_device(usbtouch->input);
 	usb_kill_urb(usbtouch->irq);
+	input_unregister_device(usbtouch->input);
 	usb_free_urb(usbtouch->irq);
 	usbtouch_free_buffers(interface_to_usbdev(intf), usbtouch);
 	kfree(usbtouch);
 }

Already discussed :)

OK.

quoted

+static int usbtouch_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
+
+	dbg("%s - called", __FUNCTION__);
+
+	mutex_lock(&usbtouch->lock);
+	usb_kill_urb(usbtouch->irq);
+	mutex_unlock(&usbtouch->lock);
+	return 0;
+}

Shouldn't all the usb_kill_urb() callers check the usbtouch->open flag
first? Perhaps it's not considered an error to kill an unsubmitted urb
though.

It's no problem to call usb_kill_urb() on an unsubmitted urb.
That must be the case as the urb can always finish.

quoted

+static int usbtouch_resume(struct usb_interface *intf)
+{
+	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
+	int rv = 0;
+
+	dbg("%s - called", __FUNCTION__);
+
+	mutex_lock(&usbtouch->lock);
+	if (usbtouch->open)
+		rv = usb_submit_urb(usbtouch->irq, GFP_NOIO);
+	mutex_unlock(&usbtouch->lock);
+	return rv;
+}
+
+static int usbtouch_pre_reset(struct usb_interface *intf)
+{
+	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
+
+	dbg("%s - called", __FUNCTION__);
+
+	mutex_lock(&usbtouch->lock);
+	usb_kill_urb(usbtouch->irq);
+	return 0;
+}
+
+static int usbtouch_post_reset(struct usb_interface *intf)
+{
+	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
+	int rv = 0;
+
+	dbg("%s - called", __FUNCTION__);
+
+	if (usbtouch->open)
+		rv = usb_submit_urb(usbtouch->irq, GFP_NOIO);
+	mutex_unlock(&usbtouch->lock);
+	return rv;
+}

Hmm. Are pre_reset and post_reset actually required in such simple cases?
AFAICS device reset is already protected against pm and autopm. Or is
resubmitting urbs necessary after a reset? I'm mainly interested since
I didn't implement these callbacks in my ati_remote2 patch.

If somebody resets the device you can assume it was in a bad shape.
It's much safer to cleanly resubmit.

quoted

 MODULE_DEVICE_TABLE(usb, usbtouch_devices);
 
 static struct usb_driver usbtouch_driver = {
 	.name		= "usbtouchscreen",
 	.probe		= usbtouch_probe,
 	.disconnect	= usbtouch_disconnect,
+	.suspend	= usbtouch_suspend,
+	.resume		= usbtouch_resume,
+	.reset_resume	= usbtouch_resume,

What is the purpose of providing identical resume and reset_resume?
AFAICS reset_resume is only called if USB_QUIRK_RESET_RESUME is
enabled.

It is also called if you resume from STD if power was lost.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html