On 19:17 Sat 15 Dec 2007, Laurent Pinchart wrote:
> On Thursday 13 December 2007, Brandon Philips wrote:
> > This patch enables usb power management in the UVC driver. As cameras
> > are built into more and more modern laptops it is important that we get
> > power management for them working.
>
> Thanks for the patch. Comments inlined.
> >
> > Signed-off-by: Oliver Neukum <[EMAIL PROTECTED]>
> > Reviewed-by: Brandon Philips <[EMAIL PROTECTED]>
> >
> > ---
> > uvc_driver.c | 7 ++++++-
> > uvc_v4l2.c | 6 ++++++
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > Index: uvc/uvc_v4l2.c
> > ===================================================================
> > --- uvc.orig/uvc_v4l2.c
> > +++ uvc/uvc_v4l2.c
> > @@ -413,10 +413,15 @@ static int uvc_v4l2_open(struct inode *i
> > goto done;
> > }
> >
> > + ret = usb_autopm_get_interface(video->dev->intf);
> > + if (ret < 0)
> > + goto done;
> > +
> > /* Create the device handle. */
> > handle = kzalloc(sizeof *handle, GFP_KERNEL);
> > if (handle == NULL) {
> > ret = -ENOMEM;
> > + usb_autopm_put_interface(video->dev->intf);
> > goto done;
> > }
> >
> > @@ -455,6 +460,7 @@ static int uvc_v4l2_release(struct inode
> > kfree(handle);
> > file->private_data = NULL;
> >
> > + usb_autopm_put_interface(video->dev->intf);
> > kref_put(&video->dev->kref, uvc_delete);
> > return 0;
> > }
>
> Is autosuspend interface-based or device-based ? If I understand things
> correctly, a device won't be autosuspended as long as at least one of its
> interfaces have a PM counter greater than 0. In that case incrementing the PM
> counter for the control interface should be enough, there's no need to
> increment the streaming interface PM counter. Am I right ?
autosuspend won't happen until all interfaces on a device reach 0 (see
usb/core/driver.c: autosuspend_check()). So, incrementing just the one
interface is fine.
> > Index: uvc/uvc_driver.c
> > ===================================================================
> > --- uvc.orig/uvc_driver.c
> > +++ uvc/uvc_driver.c
> > @@ -678,8 +678,10 @@ static int uvc_parse_streaming(struct uv
> > format->frame = frame;
> > ret = uvc_parse_format(dev, streaming, format,
> > &interval, buffer, buflen);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + kfree(format);
> > return ret;
> > + }
>
> Are you sure about this ? format is freed in uvc_delete().
Yes, this is unnecessary. Do you need me to resubmit?
> > frame += format->nframes;
> > format++;
> > @@ -1605,6 +1607,8 @@ static int uvc_suspend(struct usb_interf
> > uvc_trace(UVC_TRACE_SUSPEND, "Suspending interface %u\n",
> > intf->cur_altsetting->desc.bInterfaceNumber);
> >
> > + usb_kill_urb(dev->int_urb);
> > +
>
> The URB should be resubmitted on resume. I'll take care of this.
Ok.
> > /* Controls are cached on the fly so they don't need to be saved. */
> > if (intf->cur_altsetting->desc.bInterfaceSubClass == SC_VIDEOCONTROL)
> > return 0;
> > @@ -1809,6 +1813,7 @@ struct uvc_driver uvc_driver = {
> > .suspend = uvc_suspend,
> > .resume = uvc_resume,
> > .id_table = uvc_ids,
> > + .supports_autosuspend = 1,
> > },
> > };
>
> What about devices that don't implement suspend properly ? Do I need some
> kind
> of quirk for them ?
I am not sure how broken devices are handled in this case. Oliver?
Cheers,
Brandon
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
