[PATCH 0/3] musb fixes and cleanups

[PATCH 0/3] musb fixes and cleanups

[Felipe Contreras]

Hi,

Here's a few cleanups and a possible fix. Nothing major.

Felipe Contreras (3):
  usb: musb: fix pm_runtime mismatches
  usb: musb: trivial cleanup
  usb: musb: remove a bit of indentation

 drivers/usb/musb/musb_gadget.c |    1 +
 drivers/usb/musb/omap2430.c    |   36 +++++++++++++++---------------------
 2 files changed, 16 insertions(+), 21 deletions(-)

-- 
1.7.8.rc1.14.g248db

[PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Contreras]

Properly call pm_runtime_put() afer pm_runttime_get() on errors.

Untested.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/usb/musb/musb_gadget.c |    1 +
 drivers/usb/musb/omap2430.c    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 922148f..95bfd2d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1960,6 +1960,7 @@ static int musb_gadget_start(struct usb_gadget *g,
 err2:
 	if (!is_otg_enabled(musb))
 		musb_stop(musb);
+	pm_runtime_put(musb->controller);
 err0:
 	return retval;
 }
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index ba85f27..4edfb91 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -385,6 +385,7 @@ static void omap2430_musb_disable(struct musb *musb)
 
 static int omap2430_musb_exit(struct musb *musb)
 {
+	pm_runtime_put(musb->controller);
 	del_timer_sync(&musb_idle_timer);
 
 	omap2430_low_level_exit(musb);
-- 
1.7.8.rc1.14.g248db

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Alan Stern]

On Fri, 16 Dec 2011, Felipe Contreras wrote:

> Properly call pm_runtime_put() afer pm_runttime_get() on errors.
> 
> Untested.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  drivers/usb/musb/musb_gadget.c |    1 +
>  drivers/usb/musb/omap2430.c    |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index 922148f..95bfd2d 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1960,6 +1960,7 @@ static int musb_gadget_start(struct usb_gadget *g,
>  err2:
>  	if (!is_otg_enabled(musb))
>  		musb_stop(musb);
> +	pm_runtime_put(musb->controller);
>  err0:
>  	return retval;
>  }
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index ba85f27..4edfb91 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -385,6 +385,7 @@ static void omap2430_musb_disable(struct musb *musb)
>  
>  static int omap2430_musb_exit(struct musb *musb)
>  {
> +	pm_runtime_put(musb->controller);
>  	del_timer_sync(&musb_idle_timer);
>  
>  	omap2430_low_level_exit(musb);

Should these calls be pm_runtime_put_sync() instead of 
pm_runtime_put()?

Alan Stern

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Contreras]

On Fri, Dec 16, 2011 at 1:22 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 16 Dec 2011, Felipe Contreras wrote:
>
>> Properly call pm_runtime_put() afer pm_runttime_get() on errors.
>>
>> Untested.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  drivers/usb/musb/musb_gadget.c |    1 +
>>  drivers/usb/musb/omap2430.c    |    1 +
>>  2 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
>> index 922148f..95bfd2d 100644
>> --- a/drivers/usb/musb/musb_gadget.c
>> +++ b/drivers/usb/musb/musb_gadget.c
>> @@ -1960,6 +1960,7 @@ static int musb_gadget_start(struct usb_gadget *g,
>>  err2:
>>       if (!is_otg_enabled(musb))
>>               musb_stop(musb);
>> +     pm_runtime_put(musb->controller);
>>  err0:
>>       return retval;
>>  }
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index ba85f27..4edfb91 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -385,6 +385,7 @@ static void omap2430_musb_disable(struct musb *musb)
>>
>>  static int omap2430_musb_exit(struct musb *musb)
>>  {
>> +     pm_runtime_put(musb->controller);
>>       del_timer_sync(&musb_idle_timer);
>>
>>       omap2430_low_level_exit(musb);
>
> Should these calls be pm_runtime_put_sync() instead of
> pm_runtime_put()?

I don't see why... The thing failed, it's not going to be used any
more so better let PM deactivate the device. But now that you point
this out, pm_runtime_put() on omap2430_musb_exit() should probably go
at the end of the function, after all the actions have been done.

I'll send an updated patch tomorrow, if there are no further comments.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Alan Stern]

On Fri, 16 Dec 2011, Felipe Contreras wrote:

> >> --- a/drivers/usb/musb/omap2430.c
> >> +++ b/drivers/usb/musb/omap2430.c
> >> @@ -385,6 +385,7 @@ static void omap2430_musb_disable(struct musb *musb)
> >>
> >>  static int omap2430_musb_exit(struct musb *musb)
> >>  {
> >> +     pm_runtime_put(musb->controller);
> >>       del_timer_sync(&musb_idle_timer);
> >>
> >>       omap2430_low_level_exit(musb);
> >
> > Should these calls be pm_runtime_put_sync() instead of
> > pm_runtime_put()?
> 
> I don't see why... The thing failed, it's not going to be used any
> more so better let PM deactivate the device.

It's not a big deal.  The difference is that pm_runtime_put_sync()
avoids an extra context switch when suspending the device.  A minor
optimization, that's all.

Alan Stern

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Contreras]

On Fri, Dec 16, 2011 at 5:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 16 Dec 2011, Felipe Contreras wrote:
>> > Should these calls be pm_runtime_put_sync() instead of
>> > pm_runtime_put()?
>>
>> I don't see why... The thing failed, it's not going to be used any
>> more so better let PM deactivate the device.
>
> It's not a big deal.  The difference is that pm_runtime_put_sync()
> avoids an extra context switch when suspending the device.  A minor
> optimization, that's all.

Ok, I used what most of the code was using, but I've trusted you and
updated the patches. It would probably make sense to do the same in
other places.

-- 
Felipe Contreras

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

On Fri, Dec 16, 2011 at 01:05:08AM +0200, Felipe Contreras wrote:
> Properly call pm_runtime_put() afer pm_runttime_get() on errors.
> 
> Untested.

sorry, but I'm not applying untested patches. I need someone to give a
Tested-by.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Contreras]

On Mon, Dec 19, 2011 at 10:11 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Dec 16, 2011 at 01:05:08AM +0200, Felipe Contreras wrote:
>> Properly call pm_runtime_put() afer pm_runttime_get() on errors.
>>
>> Untested.
>
> sorry, but I'm not applying untested patches. I need someone to give a
> Tested-by.

I'm pretty sure most people don't test the exit paths, but fine, don't
apply them. Who needs power management anyway.

-- 
Felipe Contreras

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Mon, Dec 19, 2011 at 10:20:09PM +0200, Felipe Contreras wrote:
> On Mon, Dec 19, 2011 at 10:11 AM, Felipe Balbi <balbi@ti.com> wrote:
> > On Fri, Dec 16, 2011 at 01:05:08AM +0200, Felipe Contreras wrote:
> >> Properly call pm_runtime_put() afer pm_runttime_get() on errors.
> >>
> >> Untested.
> >
> > sorry, but I'm not applying untested patches. I need someone to give a
> > Tested-by.
> 
> I'm pretty sure most people don't test the exit paths, but fine, don't
> apply them. Who needs power management anyway.

if it wasn't tested you can't claim $SUBJECT makes PM magically work.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] usb: musb: fix pm_runtime mismatches

[Felipe Contreras]

On Tue, Dec 20, 2011 at 1:19 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Dec 19, 2011 at 10:20:09PM +0200, Felipe Contreras wrote:
>> On Mon, Dec 19, 2011 at 10:11 AM, Felipe Balbi <balbi@ti.com> wrote:
>> > On Fri, Dec 16, 2011 at 01:05:08AM +0200, Felipe Contreras wrote:
>> >> Properly call pm_runtime_put() afer pm_runttime_get() on errors.
>> >>
>> >> Untested.
>> >
>> > sorry, but I'm not applying untested patches. I need someone to give a
>> > Tested-by.
>>
>> I'm pretty sure most people don't test the exit paths, but fine, don't
>> apply them. Who needs power management anyway.
>
> if it wasn't tested you can't claim $SUBJECT makes PM magically work.

It's clear from the code, no need to test to see that pm_runtime_get()
is called but not pm_runtime_put(), thus the device remains active,
thus PM would not work after that.

-- 
Felipe Contreras

[PATCH 2/3] usb: musb: trivial cleanup

[Felipe Contreras]

enabled && driver || !enabled can be simplified to !enabled || driver.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/usb/musb/omap2430.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 4edfb91..a615538 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -236,13 +236,7 @@ static int musb_otg_notifications(struct notifier_block *nb,
 	case USB_EVENT_ID:
 		dev_dbg(musb->controller, "ID GND\n");
 
-		if (is_otg_enabled(musb)) {
-			if (musb->gadget_driver) {
-				pm_runtime_get_sync(musb->controller);
-				otg_init(musb->xceiv);
-				omap2430_musb_set_vbus(musb, 1);
-			}
-		} else {
+		if (!is_otg_enabled(musb) || musb->gadget_driver) {
 			pm_runtime_get_sync(musb->controller);
 			otg_init(musb->xceiv);
 			omap2430_musb_set_vbus(musb, 1);
-- 
1.7.8.rc1.14.g248db

[PATCH 3/3] usb: musb: remove a bit of indentation

[Felipe Contreras]

And use dev instead of musb->controller.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/usb/musb/omap2430.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index a615538..31fc8a2 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -344,20 +344,19 @@ static void omap2430_musb_enable(struct musb *musb)
 
 	case USB_EVENT_ID:
 		otg_init(musb->xceiv);
-		if (data->interface_type == MUSB_INTERFACE_UTMI) {
-			devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
-			/* start the session */
-			devctl |= MUSB_DEVCTL_SESSION;
-			musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
-			while (musb_readb(musb->mregs, MUSB_DEVCTL) &
-						MUSB_DEVCTL_BDEVICE) {
-				cpu_relax();
-
-				if (time_after(jiffies, timeout)) {
-					dev_err(musb->controller,
-					"configured as A device timeout");
-					break;
-				}
+		if (data->interface_type != MUSB_INTERFACE_UTMI)
+			break;
+		devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
+		/* start the session */
+		devctl |= MUSB_DEVCTL_SESSION;
+		musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
+		while (musb_readb(musb->mregs, MUSB_DEVCTL) &
+				MUSB_DEVCTL_BDEVICE) {
+			cpu_relax();
+
+			if (time_after(jiffies, timeout)) {
+				dev_err(dev, "configured as A device timeout");
+				break;
 			}
 		}
 		break;
-- 
1.7.8.rc1.14.g248db