| Message ID | 1392200333-28397-3-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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. It might have been clearer if you'd broken these two pieces of functionality changes into two different patches. In future it would be preferred. > CC: Lee Jones <lee.jones@linaro.org> > CC: Samuel Ortiz <sameo@linux.intel.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > drivers/mfd/omap-usb-host.c | 92 +++++++++++++++++++++++++++++++-------------- > 1 file changed, 64 insertions(+), 28 deletions(-) > > diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c > index 0c3c9a0..60a3bed 100644 > --- a/drivers/mfd/omap-usb-host.c > +++ b/drivers/mfd/omap-usb-host.c > @@ -665,22 +665,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(-ENODEV); > + omap->utmi_p1_gfclk = omap->utmi_p2_gfclk = ERR_PTR(-ENODEV); > + omap->xclk60mhsp1_ck = omap->xclk60mhsp2_ck = ERR_PTR(-ENODEV); > + For readability you should probably do these one per line. > 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(-ENODEV); > + omap->hsic480m_clk[i] = ERR_PTR(-ENODEV); > + omap->hsic60m_clk[i] = ERR_PTR(-ENODEV); > } > > - omap->ehci_logic_fck = ERR_PTR(-EINVAL); > - if (need_logic_fck) { > - omap->ehci_logic_fck = devm_clk_get(dev, "ehci_logic_fck"); Has this clock been renamed, or is it no longer required? Perhaps you should be explicit in the commit log as to which clocks you're removing. > - 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"); devm_clk_get()? <snip>
On 02/14/2014 12:07 PM, Lee Jones wrote: >> 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. > > It might have been clearer if you'd broken these two pieces of > functionality changes into two different patches. In future it would > be preferred. OK. > >> CC: Lee Jones <lee.jones@linaro.org> >> CC: Samuel Ortiz <sameo@linux.intel.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/mfd/omap-usb-host.c | 92 +++++++++++++++++++++++++++++++-------------- >> 1 file changed, 64 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index 0c3c9a0..60a3bed 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -665,22 +665,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(-ENODEV); >> + omap->utmi_p1_gfclk = omap->utmi_p2_gfclk = ERR_PTR(-ENODEV); >> + omap->xclk60mhsp1_ck = omap->xclk60mhsp2_ck = ERR_PTR(-ENODEV); >> + > > For readability you should probably do these one per line. OK. > >> 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(-ENODEV); >> + omap->hsic480m_clk[i] = ERR_PTR(-ENODEV); >> + omap->hsic60m_clk[i] = ERR_PTR(-ENODEV); >> } >> >> - omap->ehci_logic_fck = ERR_PTR(-EINVAL); >> - if (need_logic_fck) { >> - omap->ehci_logic_fck = devm_clk_get(dev, "ehci_logic_fck"); > > Has this clock been renamed, or is it no longer required? It is only for OMAP3 and it's actual name is "usbhost_120m_fck". "ehci_logic_fck" is just an alias. > > Perhaps you should be explicit in the commit log as to which clocks > you're removing. > >> - 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"); > > devm_clk_get()? Indeed. cheers, -roger
diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c index 0c3c9a0..60a3bed 100644 --- a/drivers/mfd/omap-usb-host.c +++ b/drivers/mfd/omap-usb-host.c @@ -665,22 +665,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(-ENODEV); + omap->utmi_p1_gfclk = omap->utmi_p2_gfclk = ERR_PTR(-ENODEV); + omap->xclk60mhsp1_ck = omap->xclk60mhsp2_ck = ERR_PTR(-ENODEV); + 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(-ENODEV); + omap->hsic480m_clk[i] = ERR_PTR(-ENODEV); + omap->hsic60m_clk[i] = ERR_PTR(-ENODEV); } - 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 +747,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])) { + ret = PTR_ERR(omap->hsic60m_clk[i]); + dev_err(dev, "Failed to get clock : %s : %d\n", + clkname, ret); + goto err_mem; + } } if (is_ehci_phy_mode(pdata->port_mode[0])) { - /* for OMAP3, clk_set_parent fails */ ret = clk_set_parent(omap->utmi_p1_gfclk, omap->xclk60mhsp1_ck); - if (ret != 0) - dev_dbg(dev, "xclk60mhsp1_ck set parent failed: %d\n", + if (ret != 0) { + dev_err(dev, "xclk60mhsp1_ck set parent failed: %d\n", ret); + goto err_mem; + } } else if (is_ehci_tll_mode(pdata->port_mode[0])) { ret = clk_set_parent(omap->utmi_p1_gfclk, omap->init_60m_fclk); - if (ret != 0) - dev_dbg(dev, "P0 init_60m_fclk set parent failed: %d\n", + if (ret != 0) { + dev_err(dev, "P0 init_60m_fclk set parent failed: %d\n", ret); + goto err_mem; + } } if (is_ehci_phy_mode(pdata->port_mode[1])) { ret = clk_set_parent(omap->utmi_p2_gfclk, omap->xclk60mhsp2_ck); - if (ret != 0) - dev_dbg(dev, "xclk60mhsp2_ck set parent failed: %d\n", + if (ret != 0) { + dev_err(dev, "xclk60mhsp2_ck set parent failed: %d\n", ret); + goto err_mem; + } } else if (is_ehci_tll_mode(pdata->port_mode[1])) { ret = clk_set_parent(omap->utmi_p2_gfclk, omap->init_60m_fclk); - if (ret != 0) - dev_dbg(dev, "P1 init_60m_fclk set parent failed: %d\n", + if (ret != 0) { + dev_err(dev, "P1 init_60m_fclk set parent failed: %d\n", ret); + goto err_mem; + } } +initialize: omap_usbhs_init(dev); if (dev->of_node) {
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.jones@linaro.org> CC: Samuel Ortiz <sameo@linux.intel.com> Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/mfd/omap-usb-host.c | 92 +++++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 28 deletions(-)