lkml.org 
[lkml]   [2014]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2014-10-31 19:21    [W:2.197 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site