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 <rog...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
