* linu...@stefanringel.de wrote: > From: Stefan Ringel <linu...@stefanringel.de> > > Signed-off-by: Stefan Ringel <linu...@stefanringel.de>
Your commit message needs more details. Why do you think this is a bugfix?
Also this commit seems to effectively revert (and then partially reimplement)
a patch that I posted some months ago.
> ---
> drivers/media/video/tm6000/tm6000-core.c | 49
> -----------------------------
> drivers/media/video/tm6000/tm6000-video.c | 21 ++++++++++--
> 2 files changed, 17 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/media/video/tm6000/tm6000-core.c
> b/drivers/media/video/tm6000/tm6000-core.c
> index c007e6d..920299e 100644
> --- a/drivers/media/video/tm6000/tm6000-core.c
> +++ b/drivers/media/video/tm6000/tm6000-core.c
> @@ -599,55 +599,6 @@ int tm6000_init(struct tm6000_core *dev)
> return rc;
> }
>
> -int tm6000_reset(struct tm6000_core *dev)
> -{
> - int pipe;
> - int err;
> -
> - msleep(500);
> -
> - err = usb_set_interface(dev->udev, dev->isoc_in.bInterfaceNumber, 0);
> - if (err < 0) {
> - tm6000_err("failed to select interface %d, alt. setting 0\n",
> - dev->isoc_in.bInterfaceNumber);
> - return err;
> - }
> -
> - err = usb_reset_configuration(dev->udev);
> - if (err < 0) {
> - tm6000_err("failed to reset configuration\n");
> - return err;
> - }
> -
> - if ((dev->quirks & TM6000_QUIRK_NO_USB_DELAY) == 0)
> - msleep(5);
> -
> - /*
> - * Not all devices have int_in defined
> - */
> - if (!dev->int_in.endp)
> - return 0;
> -
> - err = usb_set_interface(dev->udev, dev->isoc_in.bInterfaceNumber, 2);
> - if (err < 0) {
> - tm6000_err("failed to select interface %d, alt. setting 2\n",
> - dev->isoc_in.bInterfaceNumber);
> - return err;
> - }
> -
> - msleep(5);
> -
> - pipe = usb_rcvintpipe(dev->udev,
> - dev->int_in.endp->desc.bEndpointAddress &
> USB_ENDPOINT_NUMBER_MASK);
> -
> - err = usb_clear_halt(dev->udev, pipe);
> - if (err < 0) {
> - tm6000_err("usb_clear_halt failed: %d\n", err);
> - return err;
> - }
> -
> - return 0;
> -}
>
> int tm6000_set_audio_bitrate(struct tm6000_core *dev, int bitrate)
> {
> diff --git a/drivers/media/video/tm6000/tm6000-video.c
> b/drivers/media/video/tm6000/tm6000-video.c
> index 1e5ace0..4db3535 100644
> --- a/drivers/media/video/tm6000/tm6000-video.c
> +++ b/drivers/media/video/tm6000/tm6000-video.c
> @@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
>
> tm6000_uninit_isoc(dev);
>
> + /* Stop interrupt USB pipe */
> + tm6000_ir_int_stop(dev);
> +
> + usb_reset_configuration(dev->udev);
> +
> + if (&dev->int_in)
This check is wrong, &dev->int_in will always be true.
> + usb_set_interface(dev->udev,
> + dev->isoc_in.bInterfaceNumber,
> + 2);
> + else
> + usb_set_interface(dev->udev,
> + dev->isoc_in.bInterfaceNumber,
> + 0);
This would need better indentation.
> +
> + /* Start interrupt USB pipe */
> + tm6000_ir_int_start(dev);
> +
Why do you restart the IR interrupt pipe when the device is being released?
> if (!fh->radio)
> videobuf_mmap_free(&fh->vb_vidq);
> -
> - err = tm6000_reset(dev);
> - if (err < 0)
> - dev_err(&vdev->dev, "reset failed: %d\n", err);
> }
>
> kfree(fh);
I think this whole patch should be much shorter. Something along the lines
of:
@@ -1609,12 +1609,25 @@ static int tm6000_release(struct file *file)
tm6000_uninit_isoc(dev);
+ /* Stop interrupt USB pipe */
+ tm6000_ir_int_stop(dev);
+
if (!fh->radio)
videobuf_mmap_free(&fh->vb_vidq);
Thierry
pgpMorYsK0ys3.pgp
Description: PGP signature
