Re: [rft]power management for usbtouch

["Ville Syrjälä" <syrjala@sci.fi>]

On Thu, Jun 26, 2008 at 03:57:17PM +0200, Oliver Neukum wrote:
> 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.

> +	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 :)

> +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.

> +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.

>  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.

> +	.pre_reset	= usbtouch_pre_reset,
> +	.post_reset	= usbtouch_post_reset,
>  	.id_table	= usbtouch_devices,
> +	.supports_autosuspend = 1,
>  };
>  
>  static int __init usbtouch_init(void)

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html