On 10/31/2014 12:42 PM, Felipe Balbi wrote: > Hi, > > On Fri, Oct 31, 2014 at 10:20:06AM -0500, Dinh Nguyen wrote: >>>> @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) >>>> } >>>> /* Change to L0 state */ >>>> hsotg->lx_state = DWC2_L0; >>>> - call_gadget(hsotg, resume); >>>> + if (!IS_ERR(hsotg->clk)) >>>> + call_gadget(hsotg, resume); >>> >>> instead of exposing the clock detail to the entire driver, add IS_ERR() >>> checks to resume and suspend instead. In fact, NULL is a valid clock, so >>> you might as well: >>> >>> clk = clk_get(foo, bar); >>> if (IS_ERR(clk)) >>> dwc->clk = NULL; >>> else >>> dwc->clk = clk; >>> >>> Then you don't need any IS_ERR() checks sprinkled around the driver. >> >> But we would still need to check for the clock before accessing gadget >> functionality right? >> >> if (dwc2->clk) >> call_gadget(); > > Read my comment again. "NULL is a valid clock". Look at what > clk_enable() does when a NULL pointer is passed: > > static int __clk_enable(struct clk *clk) > { > int ret = 0; > > if (!clk) > return 0; > > if (WARN_ON(clk->prepare_count == 0)) > return -ESHUTDOWN; > > if (clk->enable_count == 0) { > ret = __clk_enable(clk->parent); > > if (ret) > return ret; > > if (clk->ops->enable) { > ret = clk->ops->enable(clk->hw); > if (ret) { > __clk_disable(clk->parent); > return ret; > } > } > } > > clk->enable_count++; > return 0; > } > > int clk_enable(struct clk *clk) > { > unsigned long flags; > int ret; > > flags = clk_enable_lock(); > ret = __clk_enable(clk); > clk_enable_unlock(flags); > > return ret; > } > EXPORT_SYMBOL_GPL(clk_enable); Ah yes, thanks for the explanation. So if clk=NULL, it just return 0. But what I'm saying is that if the driver is configured for dual-role mode, and no clock is specified, then the driver should not be accessing any gadget functionality. So as the patch series stands right now, if I swap out an A connector to a B-connector, then I get a connect_id_status change interrupt. The status would show a device and I would initialize the gadget portion of the driver. diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 44c609f..96810f7 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct work_struct *work) hsotg->op_state = OTG_STATE_B_PERIPHERAL; dwc2_core_init(hsotg, false, -1); dwc2_enable_global_interrupts(hsotg); - s3c_hsotg_core_init(hsotg); + if (hsotg->clk) + s3c_hsotg_core_init(hsotg); So if I don't have a valid clock, I'll be accessing the peripheral portion of the IP. But I guess not having the check for the valid clock here should be fine as I don't see a case where there can be 2 different clocks for host and peripheral? > >>>> @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg) >>>> "DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n", >>>> !!(dsts & DSTS_SUSPSTS), >>>> hsotg->hw_params.power_optimized); >>>> - call_gadget(hsotg, suspend); >>>> + if (!IS_ERR(hsotg->clk)) >>>> + call_gadget(hsotg, suspend); >>>> } else { >>>> if (hsotg->op_state == OTG_STATE_A_PERIPHERAL) { >>>> dev_dbg(hsotg->dev, "a_peripheral->a_host\n"); >>>> @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev) >>>> spin_lock(&hsotg->lock); >>>> >>>> if (dwc2_is_device_mode(hsotg)) >>>> - retval = s3c_hsotg_irq(irq, dev); >>>> + if (!IS_ERR(hsotg->clk)) >>>> + retval = s3c_hsotg_irq(irq, dev); >>> >>> wait a minute, if there is no clock we don't call the gadget interrupt >>> handler ? Why ? Who will disable the IRQ line ? >> >> This portion is no static int __clk_enable(struct clk *clk) > > huh ? What I mean is that this has the potential of leaving that IRQ > line enabled. Imagine you don't have a clock and s3c_hsotg_irq() isn't > called, then a peripheral IRQ fires, since the handler isn't called, who > will clear the interrupt ? > Yes, right. This portion of the code is no longer needed when I switched to use a shared IRQ. Dinh -- 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