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

[Roger Quadros <rogerq@ti.com>]

Felipe,

Thanks for reviewing.

On 11/21/2012 01:55 PM, Felipe Balbi wrote:
> On Thu, Nov 15, 2012 at 04:34:00PM +0200, Roger Quadros wrote:
>> Every channel has a functional clock that is similarly named.
>> It makes sense to use a for loop to manage these clocks as OMAPs
>> can come with upto 3 channels.
> 
> s/upto/up to
> 
> BTW, this patch is doing a lot more than "cleaning up clock handling".
> This needs to be split into smaller patches.
> 
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/mfd/omap-usb-tll.c |  130 +++++++++++++++++++++++++-------------------
>>  1 files changed, 74 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
>> index d1750a4..943ac14 100644
>> --- a/drivers/mfd/omap-usb-tll.c
>> +++ b/drivers/mfd/omap-usb-tll.c
>> @@ -82,8 +82,12 @@
>>  #define	OMAP_TLL_ULPI_DEBUG(num)			(0x815 + 0x100 * num)
>>  #define	OMAP_TLL_ULPI_SCRATCH_REGISTER(num)		(0x816 + 0x100 * num)
>>  
>> -#define OMAP_REV2_TLL_CHANNEL_COUNT			2
>> -#define OMAP_TLL_CHANNEL_COUNT				3
>> +#define REV2_TLL_CHANNEL_COUNT				2
> 
> why are you dropping the OMAP_ prefix ? You shouldn't do that.
> 
>> +#define DEFAULT_TLL_CHANNEL_COUNT			3
> 
> Add OMAP_ prefix here.
> 
>> +
>> +/* Update if any chip has more */
>> +#define MAX_TLL_CHANNEL_COUNT				3
> 
> can't you figure this one out in runtime ? If this isn't in any
> registers (and looks like it's not), you can pass this information to
> the driver via DT or just use driver_data field on struct
> platform_driver.
> 
>>  #define OMAP_TLL_CHANNEL_1_EN_MASK			(1 << 0)
>>  #define OMAP_TLL_CHANNEL_2_EN_MASK			(1 << 1)
>>  #define OMAP_TLL_CHANNEL_3_EN_MASK			(1 << 2)
>> @@ -96,8 +100,9 @@
>>  #define is_ehci_tll_mode(x)	(x == OMAP_EHCI_PORT_MODE_TLL)
>>  
>>  struct usbtll_omap {
>> -	struct clk				*usbtll_p1_fck;
>> -	struct clk				*usbtll_p2_fck;
>> +	void __iomem				*base;
>> +	int					nch;	/* number of channels */
>> +	struct clk				*ch_clk[MAX_TLL_CHANNEL_COUNT];
> 
> should be allocated dynamically.
> 
>>  	struct usbtll_omap_platform_data	*pdata;
>>  	/* secure the register updates */
>>  	spinlock_t				lock;
>> @@ -210,49 +215,35 @@ static int __devinit usbtll_omap_probe(struct platform_device *pdev)
>>  	unsigned				reg;
>>  	unsigned long				flags;
>>  	int					ret = 0;
>> -	int					i, ver, count;
>> +	int					i, ver;
>>  
>>  	dev_dbg(dev, "starting TI HSUSB TLL Controller\n");
>>  
>>  	tll = kzalloc(sizeof(struct usbtll_omap), GFP_KERNEL);
>>  	if (!tll) {
>>  		dev_err(dev, "Memory allocation failed\n");
>> -		ret = -ENOMEM;
>> -		goto end;
>> +		return -ENOMEM;
>>  	}
>>  
>>  	spin_lock_init(&tll->lock);
>>  
>>  	tll->pdata = pdata;
>>  
>> -	tll->usbtll_p1_fck = clk_get(dev, "usb_tll_hs_usb_ch0_clk");
>> -	if (IS_ERR(tll->usbtll_p1_fck)) {
>> -		ret = PTR_ERR(tll->usbtll_p1_fck);
>> -		dev_err(dev, "usbtll_p1_fck failed error:%d\n", ret);
>> -		goto err_tll;
>> -	}
>> -
>> -	tll->usbtll_p2_fck = clk_get(dev, "usb_tll_hs_usb_ch1_clk");
>> -	if (IS_ERR(tll->usbtll_p2_fck)) {
>> -		ret = PTR_ERR(tll->usbtll_p2_fck);
>> -		dev_err(dev, "usbtll_p2_fck failed error:%d\n", ret);
>> -		goto err_usbtll_p1_fck;
>> -	}
>> -
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	if (!res) {
>>  		dev_err(dev, "usb tll get resource failed\n");
>>  		ret = -ENODEV;
>> -		goto err_usbtll_p2_fck;
>> +		goto err_mem;
>>  	}
>>  
>>  	base = ioremap(res->start, resource_size(res));
>>  	if (!base) {
>>  		dev_err(dev, "TLL ioremap failed\n");
>>  		ret = -ENOMEM;
>> -		goto err_usbtll_p2_fck;
>> +		goto err_mem;
>>  	}
>>  
>> +	tll->base = base;
>>  	platform_set_drvdata(pdev, tll);
>>  	pm_runtime_enable(dev);
>>  	pm_runtime_get_sync(dev);
>> @@ -262,16 +253,33 @@ static int __devinit usbtll_omap_probe(struct platform_device *pdev)
>>  	ver =  usbtll_read(base, OMAP_USBTLL_REVISION);
>>  	switch (ver) {
>>  	case OMAP_USBTLL_REV1:
>> -	case OMAP_USBTLL_REV2:
>> -		count = OMAP_TLL_CHANNEL_COUNT;
>> +		tll->nch = DEFAULT_TLL_CHANNEL_COUNT;
>>  		break;
>> +	case OMAP_USBTLL_REV2:
>>  	case OMAP_USBTLL_REV3:
>> -		count = OMAP_REV2_TLL_CHANNEL_COUNT;
>> +		tll->nch = REV2_TLL_CHANNEL_COUNT;
> 
> nice, you *can* figure that out based on the revision... In that case,
> you shouldn't even define MAX_TLL_CHANNEL_COUNT, just allocate the array
> dynamically for the exact size you need.
> 

OK.

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

>> +		break;
>> +	}
>> +
>> +	for (i = 0; i < tll->nch; i++) {
>> +		char clk_name[] = "usb_tll_hs_usb_chx_clk";
> 
> just lazyness of counting the amount of letters ? :-p

;)

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

> 
>> +		fck = clk_get(dev, clk_name);
> 
> please use devm_clk_get().
> 
>> +		if (IS_ERR(fck)) {
>> +			dev_err(dev, "can't get clock : %s\n", clk_name);
>> +			ret = PTR_ERR(fck);
>> +			goto err_clk;
>> +		}
>> +		tll->ch_clk[i] = fck;
>>  	}
>>  
>>  	if (is_ehci_tll_mode(pdata->port_mode[0]) ||
>> @@ -291,7 +299,7 @@ static int __devinit usbtll_omap_probe(struct platform_device *pdev)
>>  		usbtll_write(base, OMAP_TLL_SHARED_CONF, reg);
>>  
>>  		/* Enable channels now */
>> -		for (i = 0; i < count; i++) {
>> +		for (i = 0; i < tll->nch; i++) {
>>  			reg = usbtll_read(base,	OMAP_TLL_CHANNEL_CONF(i));
>>  
>>  			if (is_ohci_port(pdata->port_mode[i])) {
>> @@ -319,25 +327,25 @@ static int __devinit usbtll_omap_probe(struct platform_device *pdev)
>>  		}
>>  	}
>>  
>> -err_ioremap:
>> -	spin_unlock_irqrestore(&tll->lock, flags);
>> -	iounmap(base);
>> -	pm_runtime_put_sync(dev);
>>  	tll_pdev = pdev;
>> -	if (!ret)
>> -		goto end;
>> -	pm_runtime_disable(dev);
>>  
>> -err_usbtll_p2_fck:
>> -	clk_put(tll->usbtll_p2_fck);
>> +err_clk:
>> +	for (--i; i >= 0 ; i--)
>> +		clk_put(tll->ch_clk[i]);
> 
> is clk_put(NULL) safe ??
> 
>> -err_usbtll_p1_fck:
>> -	clk_put(tll->usbtll_p1_fck);
>> +	spin_unlock_irqrestore(&tll->lock, flags);
>> +	pm_runtime_put_sync(dev);
>> +	if (ret == 0) {
>> +		pr_info("OMAP USB TLL : revision 0x%x, channels %d\n",
>> +				ver, tll->nch);
>> +		return 0;
>> +	}
> 
> the whole thing is quite confusing. Please while cleaning up the driver,
> also try to clean up the error path.
> 
>> -err_tll:
>> -	kfree(tll);
>> +	iounmap(base);
> 
> could be using devm_request_and_ioremap()
> 
>> +	pm_runtime_disable(dev);
>>  
>> -end:
>> +err_mem:
>> +	kfree(tll);
> 
> could be using devm_kzalloc()
> 
>>  	return ret;
>>  }
>>  
>> @@ -350,9 +358,12 @@ end:
>>  static int __devexit usbtll_omap_remove(struct platform_device *pdev)
>>  {
>>  	struct usbtll_omap *tll = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	iounmap(tll->base);
>> +	for (i = 0; i < tll->nch; i++)
>> +		clk_put(tll->ch_clk[i]);
>>  
>> -	clk_put(tll->usbtll_p2_fck);
>> -	clk_put(tll->usbtll_p1_fck);
>>  	pm_runtime_disable(&pdev->dev);
>>  	kfree(tll);
>>  	return 0;
>> @@ -363,6 +374,7 @@ static int usbtll_runtime_resume(struct device *dev)
>>  	struct usbtll_omap			*tll = dev_get_drvdata(dev);
>>  	struct usbtll_omap_platform_data	*pdata = tll->pdata;
>>  	unsigned long				flags;
>> +	int i;
>>  
>>  	dev_dbg(dev, "usbtll_runtime_resume\n");
>>  
>> @@ -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.

cheers,
-roger