diff mbox

USB: OTG: Use work_queue in set_vbus for TWL6030 transciever

Message ID 1308928443-15352-1-git-send-email-m-sonasath@ti.com (mailing list archive)
State New, archived
Delegated to: Felipe Balbi
Headers show

Commit Message

Sonasath, Moiz June 24, 2011, 3:14 p.m. UTC 
From: Moiz Sonasath <m-sonasath@ti.com>

With this commit: cccad6d4b103e53fb3d1fc1467f654ecb572d047
usb: otg: notifier: switch to atomic notifier

Following dumps are observed on attach/detach for MUSB HOST
mode and on a detach for MUSB Device mode.

BUG: sleeping function called from invalid context at kernel/mutex.c:85
where, the source is:
twl6030_usb_irq
->atomic_notifier_call_chain
 ->musb_otg_notifications
  ->twl6030_set_vbus
   ->twl_i2c_write_u8
    ->mutex_lock

This patch moves the i2c writes in set_vbus function to a
work-queue thereby avoiding I2C writes in atomic context.

Tested HOST and Device mode functionality on OMAP4460

Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
---
 drivers/usb/otg/twl6030-usb.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

Comments

Todd Poynor June 25, 2011, 6:31 a.m. UTC | #1 
On Fri, Jun 24, 2011 at 10:14:03AM -0500, Moiz Sonasath wrote:
...  
> +	if (enabled)
> +		twl->vbus_enable = 1;
> +	else
> +		twl->vbus_enable = 0;
> +

Suggest twl->vbus_enable = enabled;

>  	/*
>  	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
>  	 * register. This enables boost mode.
>  	 */
> -	if (enabled)
> -		twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x40,
> -						CHARGERUSB_CTRL1);
> -	 else
> -		twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x00,
> -						CHARGERUSB_CTRL1);
> +
> +	schedule_work(&twl->set_vbus_work);
> +

Suggest also moving the comments together with the new location of
the code to write CHARGERUSB_CTRL1.

Add a cancel_work_sync(&twl->set_vbus_work) at twl6030_usb_remove(),
prior to kfree(twl).
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sonasath, Moiz June 27, 2011, 3:06 p.m. UTC | #2 
Todd,

On Sat, Jun 25, 2011 at 1:31 AM, Todd Poynor <toddpoynor@google.com> wrote:
> On Fri, Jun 24, 2011 at 10:14:03AM -0500, Moiz Sonasath wrote:
> ...
>> +     if (enabled)
>> +             twl->vbus_enable = 1;
>> +     else
>> +             twl->vbus_enable = 0;
>> +
>
> Suggest twl->vbus_enable = enabled;
>
>>       /*
>>        * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
>>        * register. This enables boost mode.
>>        */
>> -     if (enabled)
>> -             twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x40,
>> -                                             CHARGERUSB_CTRL1);
>> -      else
>> -             twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x00,
>> -                                             CHARGERUSB_CTRL1);
>> +
>> +     schedule_work(&twl->set_vbus_work);
>> +
>
> Suggest also moving the comments together with the new location of
> the code to write CHARGERUSB_CTRL1.
>
> Add a cancel_work_sync(&twl->set_vbus_work) at twl6030_usb_remove(),
> prior to kfree(twl).
>

Thank you for the feedback, have posted a V2 of the patch.
diff mbox

Patch

diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index cfb5aa7..857de79 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -95,11 +95,15 @@  struct twl6030_usb {
 
 	struct regulator		*usb3v3;
 
+	/* used to set vbus, in atomic path */
+	struct work_struct	set_vbus_work;
+
 	int			irq1;
 	int			irq2;
 	u8			linkstat;
 	u8			asleep;
 	bool			irq_enabled;
+	bool			vbus_enable;
 	unsigned long		features;
 };
 
@@ -370,20 +374,35 @@  static int twl6030_enable_irq(struct otg_transceiver *x)
 	return 0;
 }
 
+static void otg_set_vbus_work(struct work_struct *data)
+{
+	struct twl6030_usb *twl = container_of(data, struct twl6030_usb,
+								set_vbus_work);
+
+	if (twl->vbus_enable)
+		twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x40,
+							CHARGERUSB_CTRL1);
+	else
+		twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x00,
+							CHARGERUSB_CTRL1);
+}
+
 static int twl6030_set_vbus(struct otg_transceiver *x, bool enabled)
 {
 	struct twl6030_usb *twl = xceiv_to_twl(x);
 
+	if (enabled)
+		twl->vbus_enable = 1;
+	else
+		twl->vbus_enable = 0;
+
 	/*
 	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
 	 * register. This enables boost mode.
 	 */
-	if (enabled)
-		twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x40,
-						CHARGERUSB_CTRL1);
-	 else
-		twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x00,
-						CHARGERUSB_CTRL1);
+
+	schedule_work(&twl->set_vbus_work);
+
 	return 0;
 }
 
@@ -444,6 +463,8 @@  static int __devinit twl6030_usb_probe(struct platform_device *pdev)
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&twl->otg.notifier);
 
+	INIT_WORK(&twl->set_vbus_work, otg_set_vbus_work);
+
 	twl->irq_enabled = true;
 	status = request_threaded_irq(twl->irq1, NULL, twl6030_usbotg_irq,
 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,