Re: [PATCH v5 2/9] mfd: omap-usb-host: Get clocks based on hardware revision

2014-01-20 Thread Roger Quadros 
On 01/10/2014 04:26 PM, Lee Jones wrote:
 On Fri, 10 Jan 2014, Arnd Bergmann wrote:
 
 On Friday 10 January 2014, Lee Jones wrote:
  
 - need_logic_fck = false;
 + /* Set all clocks as invalid to begin with */
 + omap-ehci_logic_fck = omap-init_60m_fclk = ERR_PTR(-EINVAL);
 + omap-utmi_p1_gfclk = omap-utmi_p2_gfclk = ERR_PTR(-EINVAL);
 + omap-xclk60mhsp1_ck = omap-xclk60mhsp2_ck = ERR_PTR(-EINVAL);

 I don't think this is the correct error code.

 -EINVAL means 'invalid parameter'.

 You probably want -ENODEV or -ENOSYS ('function not implemented'
 probably isn't ideal either tbh, but you get the idea). Perhaps you
 can set them as NULL and check for IS_ERR_OR_NULL() instead?

 I think ENODEV is ok here, I'd much prefer this over IS_ERR_OR_NULL().
 
 Sounds good to me.
 
OK. Will fix.

cheers,
-roger
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/9] mfd: omap-usb-host: Get clocks based on hardware revision

2014-01-10 Thread Lee Jones 
 Not all revisions have all the clocks so get the necessary clocks
 based on hardware revision.
 
 This should avoid un-necessary clk_get failure messages that were
 observed earlier.
 
 Be more strict and always fail on clk_get() error.
 
 CC: Lee Jones lee.jo...@linaro.org
 CC: Samuel Ortiz sa...@linux.intel.com
 Signed-off-by: Roger Quadros rog...@ti.com
 ---
  drivers/mfd/omap-usb-host.c | 93 
 +++--
  1 file changed, 64 insertions(+), 29 deletions(-)
 
 diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
 index 1c9bca2..7202cc6 100644
 --- a/drivers/mfd/omap-usb-host.c
 +++ b/drivers/mfd/omap-usb-host.c
 @@ -92,7 +92,6 @@
  #define is_ehci_tll_mode(x)  (x == OMAP_EHCI_PORT_MODE_TLL)
  #define is_ehci_hsic_mode(x) (x == OMAP_EHCI_PORT_MODE_HSIC)
  
 -

Sneaky! ;)

  struct usbhs_hcd_omap {
   int nports;
   struct clk  **utmi_clk;
 @@ -665,22 +664,41 @@ static int usbhs_omap_probe(struct platform_device 
 *pdev)
   goto err_mem;
   }
  
 - need_logic_fck = false;
 + /* Set all clocks as invalid to begin with */
 + omap-ehci_logic_fck = omap-init_60m_fclk = ERR_PTR(-EINVAL);
 + omap-utmi_p1_gfclk = omap-utmi_p2_gfclk = ERR_PTR(-EINVAL);
 + omap-xclk60mhsp1_ck = omap-xclk60mhsp2_ck = ERR_PTR(-EINVAL);

I don't think this is the correct error code.

-EINVAL means 'invalid parameter'.

You probably want -ENODEV or -ENOSYS ('function not implemented'
probably isn't ideal either tbh, but you get the idea). Perhaps you
can set them as NULL and check for IS_ERR_OR_NULL() instead?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/9] mfd: omap-usb-host: Get clocks based on hardware revision

2014-01-10 Thread Arnd Bergmann 
On Friday 10 January 2014, Lee Jones wrote:
   
  - need_logic_fck = false;
  + /* Set all clocks as invalid to begin with */
  + omap-ehci_logic_fck = omap-init_60m_fclk = ERR_PTR(-EINVAL);
  + omap-utmi_p1_gfclk = omap-utmi_p2_gfclk = ERR_PTR(-EINVAL);
  + omap-xclk60mhsp1_ck = omap-xclk60mhsp2_ck = ERR_PTR(-EINVAL);
 
 I don't think this is the correct error code.
 
 -EINVAL means 'invalid parameter'.
 
 You probably want -ENODEV or -ENOSYS ('function not implemented'
 probably isn't ideal either tbh, but you get the idea). Perhaps you
 can set them as NULL and check for IS_ERR_OR_NULL() instead?

I think ENODEV is ok here, I'd much prefer this over IS_ERR_OR_NULL().

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/9] mfd: omap-usb-host: Get clocks based on hardware revision

2014-01-10 Thread Lee Jones 
On Fri, 10 Jan 2014, Arnd Bergmann wrote:

 On Friday 10 January 2014, Lee Jones wrote:

   - need_logic_fck = false;
   + /* Set all clocks as invalid to begin with */
   + omap-ehci_logic_fck = omap-init_60m_fclk = ERR_PTR(-EINVAL);
   + omap-utmi_p1_gfclk = omap-utmi_p2_gfclk = ERR_PTR(-EINVAL);
   + omap-xclk60mhsp1_ck = omap-xclk60mhsp2_ck = ERR_PTR(-EINVAL);
  
  I don't think this is the correct error code.
  
  -EINVAL means 'invalid parameter'.
  
  You probably want -ENODEV or -ENOSYS ('function not implemented'
  probably isn't ideal either tbh, but you get the idea). Perhaps you
  can set them as NULL and check for IS_ERR_OR_NULL() instead?
 
 I think ENODEV is ok here, I'd much prefer this over IS_ERR_OR_NULL().

Sounds good to me.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/9] mfd: omap-usb-host: Get clocks based on hardware revision

2014-01-09 Thread Roger Quadros 
Not all revisions have all the clocks so get the necessary clocks
based on hardware revision.

This should avoid un-necessary clk_get failure messages that were
observed earlier.

Be more strict and always fail on clk_get() error.

CC: Lee Jones lee.jo...@linaro.org
CC: Samuel Ortiz sa...@linux.intel.com
Signed-off-by: Roger Quadros rog...@ti.com
---
 drivers/mfd/omap-usb-host.c | 93 +++--
 1 file changed, 64 insertions(+), 29 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 1c9bca2..7202cc6 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -92,7 +92,6 @@
 #define is_ehci_tll_mode(x)(x == OMAP_EHCI_PORT_MODE_TLL)
 #define is_ehci_hsic_mode(x)   (x == OMAP_EHCI_PORT_MODE_HSIC)
 
-
 struct usbhs_hcd_omap {
int nports;
struct clk  **utmi_clk;
@@ -665,22 +664,41 @@ static int usbhs_omap_probe(struct platform_device *pdev)
goto err_mem;
}
 
-   need_logic_fck = false;
+   /* Set all clocks as invalid to begin with */
+   omap-ehci_logic_fck = omap-init_60m_fclk = ERR_PTR(-EINVAL);
+   omap-utmi_p1_gfclk = omap-utmi_p2_gfclk = ERR_PTR(-EINVAL);
+   omap-xclk60mhsp1_ck = omap-xclk60mhsp2_ck = ERR_PTR(-EINVAL);
+
for (i = 0; i  omap-nports; i++) {
-   if (is_ehci_phy_mode(i) || is_ehci_tll_mode(i) ||
-   is_ehci_hsic_mode(i))
-   need_logic_fck |= true;
+   omap-utmi_clk[i] = ERR_PTR(-EINVAL);
+   omap-hsic480m_clk[i] = ERR_PTR(-EINVAL);
+   omap-hsic60m_clk[i] = ERR_PTR(-EINVAL);
}
 
-   omap-ehci_logic_fck = ERR_PTR(-EINVAL);
-   if (need_logic_fck) {
-   omap-ehci_logic_fck = devm_clk_get(dev, ehci_logic_fck);
-   if (IS_ERR(omap-ehci_logic_fck)) {
-   ret = PTR_ERR(omap-ehci_logic_fck);
-   dev_dbg(dev, ehci_logic_fck failed:%d\n, ret);
+   /* for OMAP3 i.e. USBHS REV1 */
+   if (omap-usbhs_rev == OMAP_USBHS_REV1) {
+   need_logic_fck = false;
+   for (i = 0; i  omap-nports; i++) {
+   if (is_ehci_phy_mode(pdata-port_mode[i]) ||
+   is_ehci_tll_mode(pdata-port_mode[i]) ||
+   is_ehci_hsic_mode(pdata-port_mode[i]))
+
+   need_logic_fck |= true;
+   }
+
+   if (need_logic_fck) {
+   omap-ehci_logic_fck = clk_get(dev, usbhost_120m_fck);
+   if (IS_ERR(omap-ehci_logic_fck)) {
+   ret = PTR_ERR(omap-ehci_logic_fck);
+   dev_err(dev, usbhost_120m_fck failed:%d\n,
+   ret);
+   goto err_mem;
+   }
}
+   goto initialize;
}
 
+   /* for OMAP4+ i.e. USBHS REV2+ */
omap-utmi_p1_gfclk = devm_clk_get(dev, utmi_p1_gfclk);
if (IS_ERR(omap-utmi_p1_gfclk)) {
ret = PTR_ERR(omap-utmi_p1_gfclk);
@@ -728,54 +746,71 @@ static int usbhs_omap_probe(struct platform_device *pdev)
 * them
 */
omap-utmi_clk[i] = devm_clk_get(dev, clkname);
-   if (IS_ERR(omap-utmi_clk[i]))
-   dev_dbg(dev, Failed to get clock : %s : %ld\n,
-   clkname, PTR_ERR(omap-utmi_clk[i]));
+   if (IS_ERR(omap-utmi_clk[i])) {
+   ret = PTR_ERR(omap-utmi_clk[i]);
+   dev_err(dev, Failed to get clock : %s : %d\n,
+   clkname, ret);
+   goto err_mem;
+   }
 
snprintf(clkname, sizeof(clkname),
usb_host_hs_hsic480m_p%d_clk, i + 1);
omap-hsic480m_clk[i] = devm_clk_get(dev, clkname);
-   if (IS_ERR(omap-hsic480m_clk[i]))
-   dev_dbg(dev, Failed to get clock : %s : %ld\n,
-   clkname, PTR_ERR(omap-hsic480m_clk[i]));
+   if (IS_ERR(omap-hsic480m_clk[i])) {
+   ret = PTR_ERR(omap-hsic480m_clk[i]);
+   dev_err(dev, Failed to get clock : %s : %d\n,
+   clkname, ret);
+   goto err_mem;
+   }
 
snprintf(clkname, sizeof(clkname),
usb_host_hs_hsic60m_p%d_clk, i + 1);
omap-hsic60m_clk[i] = devm_clk_get(dev, clkname);
-   if (IS_ERR(omap-hsic60m_clk[i]))
-   dev_dbg(dev, Failed to get clock : %s : %ld\n,
-   clkname, PTR_ERR(omap-hsic60m_clk[i]));
+   if (IS_ERR(omap-hsic60m_clk[i])) {
+