Hi Matthew,
> The bcm5974 code takes a USB autosuspend reference on device open and
> releases it on device close. This means that the hardware won't sleep
> when anything holds it open. This is sensible for input devices that
> don't support remote wakeups on normal use (like most mice), but this
> hardware trigger wakeups on touch and so can suspend transparently to
> the user. Doing so allows the USB host controller to sleep when the
> machine is idle, giving measurable power savings. The downside is a small
> amount of additional latency when the device wakes up, and as a result
> this functionality isn't enabled by default.
With the previous version of this patch, the machine would not wake up
on a normal tap. Is that what you refer to with the "small amount of
additional latency"? By not enabled by default, you mean in the usb
subsystem?
Last time I checked, it seemed the first keystroke would sometimes
disappear at boot. As far as I can see, this version modifies the
needs_remote_wakeup variable to only be true while open, which might
remove that particular problem.
Still, I am concerned with the user experience. On this machine
(MBA31), turning on autopm with this patch included feels like a
regression.
>
> [rydberg@xxxxxxxxxxx: wakeup before mode switch]
> Signed-off-by: Matthew Garrett <mjg@xxxxxxxxxx>
> Signed-off-by: Henrik Rydberg <rydberg@xxxxxxxxxxx>
> ---
> drivers/input/mouse/bcm5974.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
> index 927e479..aaf158c 100644
> --- a/drivers/input/mouse/bcm5974.c
> +++ b/drivers/input/mouse/bcm5974.c
> @@ -634,6 +634,7 @@ static void bcm5974_irq_button(struct urb *urb)
>
> switch (urb->status) {
> case 0:
> + usb_mark_last_busy(dev->udev);
> break;
> case -EOVERFLOW:
> case -ECONNRESET:
> @@ -663,6 +664,7 @@ static void bcm5974_irq_trackpad(struct urb *urb)
>
> switch (urb->status) {
> case 0:
> + usb_mark_last_busy(dev->udev);
> break;
> case -EOVERFLOW:
> case -ECONNRESET:
> @@ -735,10 +737,15 @@ err_out:
> return error;
> }
>
> -static void bcm5974_pause_traffic(struct bcm5974 *dev)
> +static void bcm5974_kill_urbs(struct bcm5974 *dev)
> {
> usb_kill_urb(dev->tp_urb);
> usb_kill_urb(dev->bt_urb);
> +}
> +
> +static void bcm5974_pause_traffic(struct bcm5974 *dev)
> +{
> + bcm5974_kill_urbs(dev);
> bcm5974_wellspring_mode(dev, false);
> }
>
> @@ -759,6 +766,9 @@ static int bcm5974_open(struct input_dev *input)
> if (error)
> return error;
>
> + /* Required for autosuspend */
> + dev->intf->needs_remote_wakeup = 1;
> +
> mutex_lock(&dev->pm_mutex);
>
> error = bcm5974_start_traffic(dev);
> @@ -767,8 +777,10 @@ static int bcm5974_open(struct input_dev *input)
>
> mutex_unlock(&dev->pm_mutex);
>
> + usb_autopm_put_interface(dev->intf);
> +
> if (error)
> - usb_autopm_put_interface(dev->intf);
> + dev->intf->needs_remote_wakeup = 0;
>
> return error;
> }
> @@ -776,15 +788,25 @@ static int bcm5974_open(struct input_dev *input)
> static void bcm5974_close(struct input_dev *input)
> {
> struct bcm5974 *dev = input_get_drvdata(input);
> + int error;
> +
> + dev->intf->needs_remote_wakeup = 0;
> +
> + error = usb_autopm_get_interface(dev->intf);
>
> mutex_lock(&dev->pm_mutex);
>
> - bcm5974_pause_traffic(dev);
> + bcm5974_kill_urbs(dev);
> dev->opened = 0;
> + if (error)
> + err("bcm5974: wakeup failed during close: %d\n", error);
> + else
> + bcm5974_wellspring_mode(dev, false);
>
> mutex_unlock(&dev->pm_mutex);
>
> - usb_autopm_put_interface(dev->intf);
> + if (!error)
> + usb_autopm_put_interface(dev->intf);
> }
>
> static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)
> --
> 1.7.9.3
>
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
