On 11/21/2012 04:03 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Nov 21, 2012 at 02:36:48PM +0200, Roger Quadros wrote:
>>>> break;
>>>> default:
>>>> - dev_err(dev, "TLL version failed\n");
>>>> - ret = -ENODEV;
>>>> - goto err_ioremap;
>>>> + tll->nch = DEFAULT_TLL_CHANNEL_COUNT;
>>>> + dev_info(dev,
>>>> + "USB TLL Rev : 0x%x not recognized, assuming %d channels\n",
>>>> + ver, tll->nch);
>>>
>>> this hsould be dev_dbg().
>>>
>>
>> I think it should be more of a warning because it signals a problem.
>> There is another pr_info in the success path which could probably be a
>> dev_dbg.
>
> it's not a problem at all. It just means that a newer OMAP has come to
> market and perhaps we don't need extra code to support it. A revision
> detection should *never* be grounds to failure. When we can't understand
> the revision, we default to the highest possible value we know.
>
> that's not an error.
Agreed. I'm not treating it as an error. We still continue probe and the
driver should work for newer revisions. Just prints a line on the
console about the new revision. Thought it would be useful to us, but if
you think it is a problem I don't mind removing it :).
>
>>>> + struct clk *fck;
>>>> +
>>>> + sprintf(clk_name, "usb_tll_hs_usb_ch%d_clk", i);
>>>
>>> this will overflow if 'i' (for whatever reason) goes over 9.
>>
>> OK i'll add an extra character. Highly unlikely to go above 99 :)
>
> I'd stick to snprintf() though, or something safer.
OK.
>
>>>> + fck = clk_get(dev, clk_name);
>>>
>>> please use devm_clk_get().
>
> sidenote, it would be amazing to a patch at the top of this series
> converting to devm_* api ;-)
>
>>>> @@ -373,11 +385,17 @@ static int usbtll_runtime_resume(struct device *dev)
>>>>
>>>> spin_lock_irqsave(&tll->lock, flags);
>>>>
>>>> - if (is_ehci_tll_mode(pdata->port_mode[0]))
>>>> - clk_enable(tll->usbtll_p1_fck);
>>>> -
>>>> - if (is_ehci_tll_mode(pdata->port_mode[1]))
>>>> - clk_enable(tll->usbtll_p2_fck);
>>>> + for (i = 0; i < tll->nch; i++) {
>>>> + if (is_ehci_tll_mode(pdata->port_mode[i])) {
>>>> + int r;
>>>> + r = clk_enable(tll->ch_clk[i]);
>>>> + if (r) {
>>>> + dev_err(dev,
>>>> + "%s : Error enabling ch %d clock: %d\n",
>>>> + __func__, i, r);
>>>
>>> you don't need __func__.
>>>
>>
>> Thought it would be useful to point out where the message is coming
>> from. But it should be easy to grep too so I'll remove it.
>
> correct, if messages are unique, you don't need __func__ anyway ;-)
>
--
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
