Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

[Felipe Balbi <balbi@ti.com>]

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

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);

> >> @@ -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 ?

-- 
balbi

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