Re: [PATCH 02/16] mfd: omap-usb-tll: Clean up clock handling

[Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html