Hello,

On Thu, Jul 2, 2009 at 10:07 PM, Oliver Neukum<oliver@neukum.org> wrote:
quoted text
> Hi,
>
> this patch should cleanup suspend/resume support and add autosuspend
> support. Could you test this?
>
>        Regards
>                Oliver
>
> --
>
> commit 57375ad73388fd944d7447ddb3e1e56eb48ba421
> Author: Oliver Neukum <oneukum@linux-d698.(none)>
> Date:   Thu Jul 2 20:03:02 2009 +0200
>
>    usb: autosuspend for radio-mr800 driver
>
> diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
> index 837467f..496c9cf 100644
> --- a/drivers/media/radio/radio-mr800.c
> +++ b/drivers/media/radio/radio-mr800.c
> @@ -123,11 +123,13 @@ static int usb_amradio_close(struct file *file);
>  static int usb_amradio_suspend(struct usb_interface *intf,
>                                pm_message_t message);
>  static int usb_amradio_resume(struct usb_interface *intf);
> +static int usb_amradio_reset_resume(struct usb_interface *intf);
>
>  /* Data for one (physical) device */
>  struct amradio_device {
>        /* reference to USB and video device */
>        struct usb_device *usbdev;
> +       struct usb_interface *intf;
>        struct video_device *videodev;
>        struct v4l2_device v4l2_dev;
>
> @@ -156,9 +158,9 @@ static struct usb_driver usb_amradio_driver = {
>        .disconnect             = usb_amradio_disconnect,
>        .suspend                = usb_amradio_suspend,
>        .resume                 = usb_amradio_resume,
> -       .reset_resume           = usb_amradio_resume,
> +       .reset_resume           = usb_amradio_reset_resume,
>        .id_table               = usb_amradio_device_table,
> -       .supports_autosuspend   = 0,
> +       .supports_autosuspend   = 1,
>  };
>
>  /* switch on/off the radio. Send 8 bytes to device */
> @@ -541,13 +543,15 @@ static int usb_amradio_open(struct file *file)
>        radio->users = 1;
>        radio->muted = 1;
>
> +       retval = usb_autopm_get_interface(radio->intf);
> +       if (retval < 0)
> +               goto err_out;
> +
>        retval = amradio_set_mute(radio, AMRADIO_START);
>        if (retval < 0) {
>                amradio_dev_warn(&radio->videodev->dev,
> -                       "radio did not start up properly\n");
> -               radio->users = 0;
> -               unlock_kernel();
> -               return -EIO;
> +                       "radio did not wake up\n");
> +               goto err_out_pm;
>        }
>
>        retval = amradio_set_stereo(radio, WANT_STEREO);
> @@ -562,6 +566,15 @@ static int usb_amradio_open(struct file *file)
>
>        unlock_kernel();
>        return 0;
> +
> +err_out_pm:
> +       amradio_dev_warn(&radio->videodev->dev,
> +               "radio did not start up properly\n");
> +       usb_autopm_put_interface(radio->intf);
> +err_out:
> +       radio->users = 0;
> +       unlock_kernel();
> +       return -EIO;
>  }
>
>  /*close device */
> @@ -582,6 +595,7 @@ static int usb_amradio_close(struct file *file)
>                if (retval < 0)
>                        amradio_dev_warn(&radio->videodev->dev,
>                                "amradio_stop failed\n");
> +               usb_autopm_put_interface(radio->intf);
>        }
>
>        return 0;
> @@ -591,30 +605,45 @@ static int usb_amradio_close(struct file *file)
>  static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>        struct amradio_device *radio = usb_get_intfdata(intf);
> -       int retval;
> +       int retval = 0;
>
>        retval = amradio_set_mute(radio, AMRADIO_STOP);
>        if (retval < 0)
>                dev_warn(&intf->dev, "amradio_stop failed\n");
> +       else
> +               dev_info(&intf->dev, "going into suspend..\n");
>
> -       dev_info(&intf->dev, "going into suspend..\n");
> -
> -       return 0;
> +       return retval;
>  }
>
> -/* Resume device - start device. Need to be checked and fixed */
>  static int usb_amradio_resume(struct usb_interface *intf)
>  {
>        struct amradio_device *radio = usb_get_intfdata(intf);
> -       int retval;
> +       int retval = 0;
>
> -       retval = amradio_set_mute(radio, AMRADIO_START);
> -       if (retval < 0)
> -               dev_warn(&intf->dev, "amradio_start failed\n");
> +       if (radio->users) {
> +               retval = amradio_set_mute(radio, AMRADIO_START);
> +               if (retval < 0)
> +                       dev_warn(&intf->dev, "amradio_start failed\n");
> +       }
>
>        dev_info(&intf->dev, "coming out of suspend..\n");
>
> -       return 0;
> +       return retval;
> +}
> +
> +static int usb_amradio_reset_resume(struct usb_interface *intf)
> +{
> +       struct amradio_device *radio = usb_get_intfdata(intf);
> +       int retval = 0;
> +
> +       if (radio->users) {
> +               retval = amradio_set_stereo(radio, WANT_STEREO);
> +               if (!retval)
> +                       retval = usb_amradio_resume(intf);
> +       }
> +
> +       return retval;
>  }
>
>  /* File system interface */
> @@ -669,6 +698,7 @@ static int usb_amradio_probe(struct usb_interface *intf,
>                return -ENOMEM;
>        }
>
> +       radio->intf = intf;
>        radio->buffer = kmalloc(BUFFER_LENGTH, GFP_KERNEL);
>
>        if (!radio->buffer) {
>

I wanna say thank you for spending time for mr800 driver.
I'm in timeline when i'm preparing 6 or 7 patches for this driver(not
posted yet to maillist). And my patches will conflict with your patch.
Generally because of removing usb_amradio_open/close functions and
radio->users counter. The main reason of removing is needless in them.
And suspend/resume will be realized in other way..

So because you are experienced developer i should i ask you what is
the better option here. Is you patch goes first and i need to change
mine patchset or other way round?

-- 
Best regards, Klimov Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html