Hello,
On Thu, Jul 2, 2009 at 10:07 PM, Oliver Neukum<oliver@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
