On Sat, Dec 3, 2016 at 12:17 AM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote: > On 12/01/2016 12:42 AM, Vivek Gautam wrote: >> On Tue, Nov 29, 2016 at 4:44 AM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote: >>> On 11/22, Vivek Gautam wrote: >>>> + } >>>> + >>>> + /* >>>> + * we need to read only one byte here, since the required >>>> + * parameter value fits in one nibble >>>> + */ >>>> + val = (u8 *)nvmem_cell_read(cell, &len); >>> Shouldn't need the cast here. Also it would be nice if >>> nvmem_cell_read() didn't require a second argument if we don't >>> care for it. We should update the API to allow NULL there. >> Will remove the u8 pointer cast. >> >> Correct, it makes sense to allow the length pointer to be passed as NULL. >> We don't care about this length. Will update the nvmem API, to allow this. >> >> Also, we should add a check for 'cell' as well. This pointer can be >> NULL, and the first thing that nvmem_cell_read does is - deference >> the pointer 'cell' > > It would be pretty stupid to read a cell and pass NULL as the first > argument. I imagine things would blow up there like we want and we would > see a nice big stacktrace. Right, reading a 'NULL' cell doesn't make a sense at all. >>>> + } else { >>>> + reset_val |= CLK_REF_SEL; >>>> + } >>>> + >>>> + writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST); >>>> + >>>> + /* Make sure that above write is completed to get PLL source clock */ >>>> + wmb(); >>>> + >>>> + /* Required to get PHY PLL lock successfully */ >>>> + usleep_range(100, 110); >>>> + >>>> + if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) & >>>> + PLL_LOCKED)) { >>>> + dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n", >>>> + readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS)); >>> Would be pretty funny if this was locked now when the error >>> printk runs. Are there other bits in there that are helpful? >> This is the only bit that's there to check the PLL locking status. >> Should we rather poll ? >> > > I'm just saying that the printk may have the "correct" status but the > check would have failed earlier making the printk confusing. Perhaps > just save the register value from the first read and print it instead of > reading it again? Polling would probably be a better design anyway? > Hopefully the status bit isn't toggling back and forth during those > 100-100us though, which may be the case here and that would explain why > it's not a polling design. Okay, will save the register value. Will also stick to just checking the status after the delay, like we have in downstream kernel. Regards Vivek -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html