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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> >> +		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.

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

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux