[PATCH v2 1/4] usb: ohci-exynos: Use struct device instead of platform_device

[PATCH v2 1/4] usb: ohci-exynos: Use struct device instead of platform_device

[Vivek Gautam]

Change to use struct device instead of struct platform_device
for some static functions.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---

Changes from v1:
 - none

 drivers/usb/host/ohci-exynos.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 9cf80cb..05f00e3 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -39,18 +39,18 @@ struct exynos_ohci_hcd {
 	struct usb_otg *otg;
 };
 
-static void exynos_ohci_phy_enable(struct platform_device *pdev)
+static void exynos_ohci_phy_enable(struct device *dev)
 {
-	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
 
 	if (exynos_ohci->phy)
 		usb_phy_init(exynos_ohci->phy);
 }
 
-static void exynos_ohci_phy_disable(struct platform_device *pdev)
+static void exynos_ohci_phy_disable(struct device *dev)
 {
-	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
 
 	if (exynos_ohci->phy)
@@ -139,7 +139,7 @@ skip_phy:
 
 	platform_set_drvdata(pdev, hcd);
 
-	exynos_ohci_phy_enable(pdev);
+	exynos_ohci_phy_enable(&pdev->dev);
 
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (err) {
@@ -150,7 +150,7 @@ skip_phy:
 	return 0;
 
 fail_add_hcd:
-	exynos_ohci_phy_disable(pdev);
+	exynos_ohci_phy_disable(&pdev->dev);
 fail_io:
 	clk_disable_unprepare(exynos_ohci->clk);
 fail_clk:
@@ -168,7 +168,7 @@ static int exynos_ohci_remove(struct platform_device *pdev)
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_disable(pdev);
+	exynos_ohci_phy_disable(&pdev->dev);
 
 	clk_disable_unprepare(exynos_ohci->clk);
 
@@ -190,7 +190,6 @@ static int exynos_ohci_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
-	struct platform_device *pdev = to_platform_device(dev);
 	bool do_wakeup = device_may_wakeup(dev);
 	int rc = ohci_suspend(hcd, do_wakeup);
 
@@ -200,7 +199,7 @@ static int exynos_ohci_suspend(struct device *dev)
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_disable(pdev);
+	exynos_ohci_phy_disable(dev);
 
 	clk_disable_unprepare(exynos_ohci->clk);
 
@@ -211,14 +210,13 @@ static int exynos_ohci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd			= dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci	= to_exynos_ohci(hcd);
-	struct platform_device *pdev		= to_platform_device(dev);
 
 	clk_prepare_enable(exynos_ohci->clk);
 
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_enable(pdev);
+	exynos_ohci_phy_enable(dev);
 
 	ohci_resume(hcd, false);
 
-- 
1.7.10.4

[PATCH v2 1/4] usb: ohci-exynos: Use struct device instead of platform_device

[Vivek Gautam]

Change to use struct device instead of struct platform_device
for some static functions.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---

Changes from v1:
 - none

 drivers/usb/host/ohci-exynos.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 9cf80cb..05f00e3 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -39,18 +39,18 @@ struct exynos_ohci_hcd {
 	struct usb_otg *otg;
 };
 
-static void exynos_ohci_phy_enable(struct platform_device *pdev)
+static void exynos_ohci_phy_enable(struct device *dev)
 {
-	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
 
 	if (exynos_ohci->phy)
 		usb_phy_init(exynos_ohci->phy);
 }
 
-static void exynos_ohci_phy_disable(struct platform_device *pdev)
+static void exynos_ohci_phy_disable(struct device *dev)
 {
-	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
 
 	if (exynos_ohci->phy)
@@ -139,7 +139,7 @@ skip_phy:
 
 	platform_set_drvdata(pdev, hcd);
 
-	exynos_ohci_phy_enable(pdev);
+	exynos_ohci_phy_enable(&pdev->dev);
 
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (err) {
@@ -150,7 +150,7 @@ skip_phy:
 	return 0;
 
 fail_add_hcd:
-	exynos_ohci_phy_disable(pdev);
+	exynos_ohci_phy_disable(&pdev->dev);
 fail_io:
 	clk_disable_unprepare(exynos_ohci->clk);
 fail_clk:
@@ -168,7 +168,7 @@ static int exynos_ohci_remove(struct platform_device *pdev)
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_disable(pdev);
+	exynos_ohci_phy_disable(&pdev->dev);
 
 	clk_disable_unprepare(exynos_ohci->clk);
 
@@ -190,7 +190,6 @@ static int exynos_ohci_suspend(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
-	struct platform_device *pdev = to_platform_device(dev);
 	bool do_wakeup = device_may_wakeup(dev);
 	int rc = ohci_suspend(hcd, do_wakeup);
 
@@ -200,7 +199,7 @@ static int exynos_ohci_suspend(struct device *dev)
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_disable(pdev);
+	exynos_ohci_phy_disable(dev);
 
 	clk_disable_unprepare(exynos_ohci->clk);
 
@@ -211,14 +210,13 @@ static int exynos_ohci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd			= dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci	= to_exynos_ohci(hcd);
-	struct platform_device *pdev		= to_platform_device(dev);
 
 	clk_prepare_enable(exynos_ohci->clk);
 
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_enable(pdev);
+	exynos_ohci_phy_enable(dev);
 
 	ohci_resume(hcd, false);
 
-- 
1.7.10.4

[PATCH v2 2/4] usb: ehci-exynos: Use struct device instead of platform_device

[Vivek Gautam]

Change to use struct device instead of struct platform_device
for some static functions.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---

Changes from v1:
 - none

 drivers/usb/host/ehci-exynos.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index 7f425ac..4d763dc 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -50,9 +50,8 @@ struct exynos_ehci_hcd {
 
 #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
 
-static void exynos_setup_vbus_gpio(struct platform_device *pdev)
+static void exynos_setup_vbus_gpio(struct device *dev)
 {
-	struct device *dev = &pdev->dev;
 	int err;
 	int gpio;
 
@@ -88,7 +87,7 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	exynos_setup_vbus_gpio(pdev);
+	exynos_setup_vbus_gpio(&pdev->dev);
 
 	hcd = usb_create_hcd(&exynos_ehci_hc_driver,
 			     &pdev->dev, dev_name(&pdev->dev));
-- 
1.7.10.4

[PATCH v2 2/4] usb: ehci-exynos: Use struct device instead of platform_device

[Vivek Gautam]

Change to use struct device instead of struct platform_device
for some static functions.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---

Changes from v1:
 - none

 drivers/usb/host/ehci-exynos.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index 7f425ac..4d763dc 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -50,9 +50,8 @@ struct exynos_ehci_hcd {
 
 #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
 
-static void exynos_setup_vbus_gpio(struct platform_device *pdev)
+static void exynos_setup_vbus_gpio(struct device *dev)
 {
-	struct device *dev = &pdev->dev;
 	int err;
 	int gpio;
 
@@ -88,7 +87,7 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	exynos_setup_vbus_gpio(pdev);
+	exynos_setup_vbus_gpio(&pdev->dev);
 
 	hcd = usb_create_hcd(&exynos_ehci_hc_driver,
 			     &pdev->dev, dev_name(&pdev->dev));
-- 
1.7.10.4

[PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

Add support to consume phy provided by Generic phy framework.
Keeping the support for older usb-phy intact right now, in order
to prevent any functionality break in absence of relevant
device tree side change for ohci-exynos.
Once we move to new phy in the device nodes for ohci, we can
remove the support for older phys.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
---

Changes from v3:
 - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
 - Made exynos_ohci_phy_disable() return void, since its return value
   did not serve any purpose.
 - Calling clk_disable_unprepare() in exynos_ohci_resume() when
   exynos_ohci_phy_enable() is failed.

 .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
 drivers/usb/host/ohci-exynos.c                     |  128 +++++++++++++++++---
 2 files changed, 132 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index d967ba1..a90c973 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -38,6 +38,15 @@ Required properties:
  - interrupts: interrupt number to the cpu.
  - clocks: from common clock binding: handle to usb clock.
  - clock-names: from common clock binding: Shall be "usbhost".
+ - port: if in the SoC there are OHCI phys, they should be listed here.
+   One phy per port. Each port should have following entries:
+	- reg: port number on OHCI controller, e.g
+	       On Exynos5250, port 0 is USB2.0 otg phy
+			      port 1 is HSIC phy0
+			      port 2 is HSIC phy1
+	- phys: from the *Generic PHY* bindings, specifying phy used by port.
+	- phy-names: from the *Generic PHY* bindings, specifying name of phy
+		     used by the port.
 
 Example:
 	usb@12120000 {
@@ -47,6 +56,16 @@ Example:
 
 		clocks = <&clock 285>;
 		clock-names = "usbhost";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port@0 {
+		    reg = <0>;
+		    phys = <&usb2phy 1>;
+		    phy-names = "host";
+		    status = "disabled";
+		};
+
 	};
 
 DWC3
diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 05f00e3..f90bf9a 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/phy/phy.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/samsung_usb_phy.h>
 #include <linux/usb.h>
@@ -33,28 +34,122 @@ static struct hc_driver __read_mostly exynos_ohci_hc_driver;
 
 #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
 
+#define PHY_NUMBER 3
 struct exynos_ohci_hcd {
 	struct clk *clk;
 	struct usb_phy *phy;
 	struct usb_otg *otg;
+	struct phy *phy_g[PHY_NUMBER];
 };
 
-static void exynos_ohci_phy_enable(struct device *dev)
+static int exynos_ohci_get_phy(struct device *dev,
+				struct exynos_ohci_hcd *exynos_ohci)
+{
+	struct device_node *child;
+	struct phy *phy;
+	int phy_number;
+	int ret = 0;
+
+	exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+	if (IS_ERR(exynos_ohci->phy)) {
+		ret = PTR_ERR(exynos_ohci->phy);
+		/* This is the case when PHY config is disabled */
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dev_dbg(dev, "Failed to get usb2 phy\n");
+			exynos_ohci->phy = NULL;
+			ret = 0;
+		} else if (ret == -EPROBE_DEFER) {
+			goto fail_phy;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			goto fail_phy;
+		}
+	} else {
+		exynos_ohci->otg = exynos_ohci->phy->otg;
+	}
+
+	/* Getting generic phy:
+	 * We are keeping both types of phys as a part of transiting OHCI
+	 * to generic phy framework, so that in absence of supporting dts
+	 * changes the functionality doesn't break.
+	 * Once we move the ohci dt nodes to use new generic phys,
+	 * we can remove support for older PHY in this driver.
+	 */
+	for_each_available_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &phy_number);
+		if (ret) {
+			dev_err(dev, "Failed to parse device tree\n");
+			of_node_put(child);
+			goto fail_phy;
+		}
+		if (phy_number >= PHY_NUMBER) {
+			dev_err(dev, "Invalid number of PHYs\n");
+			of_node_put(child);
+			ret = -EINVAL;
+			goto fail_phy;
+		}
+		phy = devm_of_phy_get(dev, child, 0);
+		of_node_put(child);
+		if (IS_ERR(phy)) {
+			ret = PTR_ERR(phy);
+			/* This is the case when PHY config is disabled */
+			if (ret == -ENOSYS || ret == -ENODEV) {
+				dev_dbg(dev, "Failed to get usb2 phy\n");
+				phy = NULL;
+				ret = 0;
+			} else if (ret == -EPROBE_DEFER) {
+				goto fail_phy;
+			} else {
+				dev_err(dev, "no usb2 phy configured\n");
+				goto fail_phy;
+			}
+		}
+		exynos_ohci->phy_g[phy_number] = phy;
+	}
+
+fail_phy:
+	return ret;
+}
+
+static int exynos_ohci_phy_enable(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
+	int i;
+	int ret = 0;
 
-	if (exynos_ohci->phy)
-		usb_phy_init(exynos_ohci->phy);
+	if (exynos_ohci->phy) {
+		ret = usb_phy_init(exynos_ohci->phy);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
+		if (exynos_ohci->phy_g[i])
+			ret = phy_power_on(exynos_ohci->phy_g[i]);
+	if (ret) {
+		for (i--; i >= 0; i--)
+			if (exynos_ohci->phy_g[i])
+				phy_power_off(exynos_ohci->phy_g[i]);
+		if (exynos_ohci->phy)
+			usb_phy_shutdown(exynos_ohci->phy);
+	}
+
+	return ret;
 }
 
 static void exynos_ohci_phy_disable(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
+	int i;
 
 	if (exynos_ohci->phy)
 		usb_phy_shutdown(exynos_ohci->phy);
+
+	for (i = 0; i < PHY_NUMBER; i++)
+		if (exynos_ohci->phy_g[i])
+			phy_power_off(exynos_ohci->phy_g[i]);
 }
 
 static int exynos_ohci_probe(struct platform_device *pdev)
@@ -62,7 +157,6 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 	struct exynos_ohci_hcd *exynos_ohci;
 	struct usb_hcd *hcd;
 	struct resource *res;
-	struct usb_phy *phy;
 	int irq;
 	int err;
 
@@ -88,15 +182,9 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 					"samsung,exynos5440-ohci"))
 		goto skip_phy;
 
-	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
-	if (IS_ERR(phy)) {
-		usb_put_hcd(hcd);
-		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
-		return -EPROBE_DEFER;
-	} else {
-		exynos_ohci->phy = phy;
-		exynos_ohci->otg = phy->otg;
-	}
+	err = exynos_ohci_get_phy(&pdev->dev, exynos_ohci);
+	if (err)
+		goto fail_clk;
 
 skip_phy:
 	exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
@@ -139,7 +227,11 @@ skip_phy:
 
 	platform_set_drvdata(pdev, hcd);
 
-	exynos_ohci_phy_enable(&pdev->dev);
+	err = exynos_ohci_phy_enable(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable USB phy\n");
+		goto fail_io;
+	}
 
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (err) {
@@ -210,13 +302,19 @@ static int exynos_ohci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd			= dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci	= to_exynos_ohci(hcd);
+	int ret;
 
 	clk_prepare_enable(exynos_ohci->clk);
 
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_enable(dev);
+	ret = exynos_ohci_phy_enable(dev);
+	if (ret) {
+		dev_err(dev, "Failed to enable USB phy\n");
+		clk_disable_unprepare(exynos_ohci->clk);
+		return ret;
+	}
 
 	ohci_resume(hcd, false);
 
-- 
1.7.10.4

[PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

Add support to consume phy provided by Generic phy framework.
Keeping the support for older usb-phy intact right now, in order
to prevent any functionality break in absence of relevant
device tree side change for ohci-exynos.
Once we move to new phy in the device nodes for ohci, we can
remove the support for older phys.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
---

Changes from v3:
 - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
 - Made exynos_ohci_phy_disable() return void, since its return value
   did not serve any purpose.
 - Calling clk_disable_unprepare() in exynos_ohci_resume() when
   exynos_ohci_phy_enable() is failed.

 .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
 drivers/usb/host/ohci-exynos.c                     |  128 +++++++++++++++++---
 2 files changed, 132 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index d967ba1..a90c973 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -38,6 +38,15 @@ Required properties:
  - interrupts: interrupt number to the cpu.
  - clocks: from common clock binding: handle to usb clock.
  - clock-names: from common clock binding: Shall be "usbhost".
+ - port: if in the SoC there are OHCI phys, they should be listed here.
+   One phy per port. Each port should have following entries:
+	- reg: port number on OHCI controller, e.g
+	       On Exynos5250, port 0 is USB2.0 otg phy
+			      port 1 is HSIC phy0
+			      port 2 is HSIC phy1
+	- phys: from the *Generic PHY* bindings, specifying phy used by port.
+	- phy-names: from the *Generic PHY* bindings, specifying name of phy
+		     used by the port.
 
 Example:
 	usb at 12120000 {
@@ -47,6 +56,16 @@ Example:
 
 		clocks = <&clock 285>;
 		clock-names = "usbhost";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port at 0 {
+		    reg = <0>;
+		    phys = <&usb2phy 1>;
+		    phy-names = "host";
+		    status = "disabled";
+		};
+
 	};
 
 DWC3
diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 05f00e3..f90bf9a 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/phy/phy.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/samsung_usb_phy.h>
 #include <linux/usb.h>
@@ -33,28 +34,122 @@ static struct hc_driver __read_mostly exynos_ohci_hc_driver;
 
 #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
 
+#define PHY_NUMBER 3
 struct exynos_ohci_hcd {
 	struct clk *clk;
 	struct usb_phy *phy;
 	struct usb_otg *otg;
+	struct phy *phy_g[PHY_NUMBER];
 };
 
-static void exynos_ohci_phy_enable(struct device *dev)
+static int exynos_ohci_get_phy(struct device *dev,
+				struct exynos_ohci_hcd *exynos_ohci)
+{
+	struct device_node *child;
+	struct phy *phy;
+	int phy_number;
+	int ret = 0;
+
+	exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+	if (IS_ERR(exynos_ohci->phy)) {
+		ret = PTR_ERR(exynos_ohci->phy);
+		/* This is the case when PHY config is disabled */
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dev_dbg(dev, "Failed to get usb2 phy\n");
+			exynos_ohci->phy = NULL;
+			ret = 0;
+		} else if (ret == -EPROBE_DEFER) {
+			goto fail_phy;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			goto fail_phy;
+		}
+	} else {
+		exynos_ohci->otg = exynos_ohci->phy->otg;
+	}
+
+	/* Getting generic phy:
+	 * We are keeping both types of phys as a part of transiting OHCI
+	 * to generic phy framework, so that in absence of supporting dts
+	 * changes the functionality doesn't break.
+	 * Once we move the ohci dt nodes to use new generic phys,
+	 * we can remove support for older PHY in this driver.
+	 */
+	for_each_available_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &phy_number);
+		if (ret) {
+			dev_err(dev, "Failed to parse device tree\n");
+			of_node_put(child);
+			goto fail_phy;
+		}
+		if (phy_number >= PHY_NUMBER) {
+			dev_err(dev, "Invalid number of PHYs\n");
+			of_node_put(child);
+			ret = -EINVAL;
+			goto fail_phy;
+		}
+		phy = devm_of_phy_get(dev, child, 0);
+		of_node_put(child);
+		if (IS_ERR(phy)) {
+			ret = PTR_ERR(phy);
+			/* This is the case when PHY config is disabled */
+			if (ret == -ENOSYS || ret == -ENODEV) {
+				dev_dbg(dev, "Failed to get usb2 phy\n");
+				phy = NULL;
+				ret = 0;
+			} else if (ret == -EPROBE_DEFER) {
+				goto fail_phy;
+			} else {
+				dev_err(dev, "no usb2 phy configured\n");
+				goto fail_phy;
+			}
+		}
+		exynos_ohci->phy_g[phy_number] = phy;
+	}
+
+fail_phy:
+	return ret;
+}
+
+static int exynos_ohci_phy_enable(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
+	int i;
+	int ret = 0;
 
-	if (exynos_ohci->phy)
-		usb_phy_init(exynos_ohci->phy);
+	if (exynos_ohci->phy) {
+		ret = usb_phy_init(exynos_ohci->phy);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
+		if (exynos_ohci->phy_g[i])
+			ret = phy_power_on(exynos_ohci->phy_g[i]);
+	if (ret) {
+		for (i--; i >= 0; i--)
+			if (exynos_ohci->phy_g[i])
+				phy_power_off(exynos_ohci->phy_g[i]);
+		if (exynos_ohci->phy)
+			usb_phy_shutdown(exynos_ohci->phy);
+	}
+
+	return ret;
 }
 
 static void exynos_ohci_phy_disable(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
+	int i;
 
 	if (exynos_ohci->phy)
 		usb_phy_shutdown(exynos_ohci->phy);
+
+	for (i = 0; i < PHY_NUMBER; i++)
+		if (exynos_ohci->phy_g[i])
+			phy_power_off(exynos_ohci->phy_g[i]);
 }
 
 static int exynos_ohci_probe(struct platform_device *pdev)
@@ -62,7 +157,6 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 	struct exynos_ohci_hcd *exynos_ohci;
 	struct usb_hcd *hcd;
 	struct resource *res;
-	struct usb_phy *phy;
 	int irq;
 	int err;
 
@@ -88,15 +182,9 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 					"samsung,exynos5440-ohci"))
 		goto skip_phy;
 
-	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
-	if (IS_ERR(phy)) {
-		usb_put_hcd(hcd);
-		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
-		return -EPROBE_DEFER;
-	} else {
-		exynos_ohci->phy = phy;
-		exynos_ohci->otg = phy->otg;
-	}
+	err = exynos_ohci_get_phy(&pdev->dev, exynos_ohci);
+	if (err)
+		goto fail_clk;
 
 skip_phy:
 	exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
@@ -139,7 +227,11 @@ skip_phy:
 
 	platform_set_drvdata(pdev, hcd);
 
-	exynos_ohci_phy_enable(&pdev->dev);
+	err = exynos_ohci_phy_enable(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable USB phy\n");
+		goto fail_io;
+	}
 
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (err) {
@@ -210,13 +302,19 @@ static int exynos_ohci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd			= dev_get_drvdata(dev);
 	struct exynos_ohci_hcd *exynos_ohci	= to_exynos_ohci(hcd);
+	int ret;
 
 	clk_prepare_enable(exynos_ohci->clk);
 
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
-	exynos_ohci_phy_enable(dev);
+	ret = exynos_ohci_phy_enable(dev);
+	if (ret) {
+		dev_err(dev, "Failed to enable USB phy\n");
+		clk_disable_unprepare(exynos_ohci->clk);
+		return ret;
+	}
 
 	ohci_resume(hcd, false);
 
-- 
1.7.10.4

[PATCH v10 4/4] usb: ehci-exynos: Change to use phy provided by the generic phy framework

[Vivek Gautam]

From: Kamil Debski <k.debski@samsung.com>

Add the phy provider, supplied by new Exynos-usb2phy using
Generic phy framework.
Keeping the support for older USB phy intact right now, in order
to prevent any functionality break in absence of relevant
device tree side change for ehci-exynos.
Once we move to new phy in the device nodes for ehci, we can
remove the support for older phys.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
[gautam.vivek@samsung.com: Addressed review comments from mailing list]
[gautam.vivek@samsung.com: Kept the code for old usb-phy, and just
added support for new exynos5-usb2phy in generic phy framework]
[gautam.vivek@samsung.com: Edited the commit message]
Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
---

Changes from v9:
 - Calling usb_phy_shutdown() when exynos_ehci_phy_enable() is failing.
 - Made exynos_ehci_phy_disable() return void, since its return value
   did not serve any purpose.
 - Calling clk_disable_unprepare() in exynos_ehci_resume() when
   exynos_ehci_phy_enable() is failed.

 .../devicetree/bindings/usb/exynos-usb.txt         |   18 +++
 drivers/usb/host/ehci-exynos.c                     |  144 +++++++++++++++++---
 2 files changed, 142 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index a90c973..126a7a9 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -12,6 +12,15 @@ Required properties:
  - interrupts: interrupt number to the cpu.
  - clocks: from common clock binding: handle to usb clock.
  - clock-names: from common clock binding: Shall be "usbhost".
+ - port: if in the SoC there are EHCI phys, they should be listed here.
+   One phy per port. Each port should have following entries:
+	- reg: port number on EHCI controller, e.g
+	       On Exynos5250, port 0 is USB2.0 otg phy
+			      port 1 is HSIC phy0
+			      port 2 is HSIC phy1
+	- phys: from the *Generic PHY* bindings; specifying phy used by port.
+	- phy-names: from the *Generic PHY* bindings; specifying name of phy
+		     used by the port.
 
 Optional properties:
  - samsung,vbus-gpio:  if present, specifies the GPIO that
@@ -27,6 +36,15 @@ Example:
 
 		clocks = <&clock 285>;
 		clock-names = "usbhost";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port@0 {
+		    reg = <0>;
+		    phys = <&usb2phy 1>;
+		    phy-names = "host";
+		    status = "disabled";
+		};
 	};
 
 OHCI
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index 4d763dc..931cfc8 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/samsung_usb_phy.h>
@@ -42,14 +43,119 @@
 static const char hcd_name[] = "ehci-exynos";
 static struct hc_driver __read_mostly exynos_ehci_hc_driver;
 
+#define PHY_NUMBER 3
 struct exynos_ehci_hcd {
 	struct clk *clk;
 	struct usb_phy *phy;
 	struct usb_otg *otg;
+	struct phy *phy_g[PHY_NUMBER];
 };
 
 #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
 
+static int exynos_ehci_get_phy(struct device *dev,
+				struct exynos_ehci_hcd *exynos_ehci)
+{
+	struct device_node *child;
+	struct phy *phy;
+	int phy_number;
+	int ret = 0;
+
+	exynos_ehci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+	if (IS_ERR(exynos_ehci->phy)) {
+		ret = PTR_ERR(exynos_ehci->phy);
+		/* This is the case when PHY config is disabled */
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dev_dbg(dev, "Failed to get usb2 phy\n");
+			exynos_ehci->phy = NULL;
+			ret = 0;
+		} else if (ret == -EPROBE_DEFER) {
+			goto fail_phy;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			goto fail_phy;
+		}
+	} else {
+		exynos_ehci->otg = exynos_ehci->phy->otg;
+	}
+
+	for_each_available_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &phy_number);
+		if (ret) {
+			dev_err(dev, "Failed to parse device tree\n");
+			of_node_put(child);
+			goto fail_phy;
+		}
+		if (phy_number >= PHY_NUMBER) {
+			dev_err(dev, "Invalid number of PHYs\n");
+			of_node_put(child);
+			ret = -EINVAL;
+			goto fail_phy;
+		}
+		phy = devm_of_phy_get(dev, child, 0);
+		of_node_put(child);
+		if (IS_ERR(phy)) {
+			ret = PTR_ERR(phy);
+			/* This is the case when PHY config is disabled */
+			if (ret == -ENOSYS || ret == -ENODEV) {
+				dev_dbg(dev, "Failed to get usb2 phy\n");
+				phy = NULL;
+				ret = 0;
+			} else if (ret == -EPROBE_DEFER) {
+				goto fail_phy;
+			} else {
+				dev_err(dev, "no usb2 phy configured\n");
+				goto fail_phy;
+			}
+		}
+		exynos_ehci->phy_g[phy_number] = phy;
+	}
+
+fail_phy:
+	return ret;
+}
+
+static int exynos_ehci_phy_enable(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int i;
+	int ret = 0;
+
+	if (exynos_ehci->phy) {
+		ret = usb_phy_init(exynos_ehci->phy);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
+		if (exynos_ehci->phy_g[i])
+			ret = phy_power_on(exynos_ehci->phy_g[i]);
+	if (ret) {
+		for (i--; i >= 0; i--)
+			if (exynos_ehci->phy_g[i])
+				phy_power_off(exynos_ehci->phy_g[i]);
+		if (exynos_ehci->phy)
+			usb_phy_shutdown(exynos_ehci->phy);
+	}
+
+	return ret;
+}
+
+static void exynos_ehci_phy_disable(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int i;
+
+	if (exynos_ehci->phy)
+		usb_phy_shutdown(exynos_ehci->phy);
+
+	for (i = 0; i < PHY_NUMBER; i++)
+		if (exynos_ehci->phy_g[i])
+			phy_power_off(exynos_ehci->phy_g[i]);
+}
+
 static void exynos_setup_vbus_gpio(struct device *dev)
 {
 	int err;
@@ -74,7 +180,6 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 	struct usb_hcd *hcd;
 	struct ehci_hcd *ehci;
 	struct resource *res;
-	struct usb_phy *phy;
 	int irq;
 	int err;
 
@@ -101,15 +206,9 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 					"samsung,exynos5440-ehci"))
 		goto skip_phy;
 
-	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
-	if (IS_ERR(phy)) {
-		usb_put_hcd(hcd);
-		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
-		return -EPROBE_DEFER;
-	} else {
-		exynos_ehci->phy = phy;
-		exynos_ehci->otg = phy->otg;
-	}
+	err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
+	if (err)
+		goto fail_clk;
 
 skip_phy:
 
@@ -151,8 +250,11 @@ skip_phy:
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_init(exynos_ehci->phy);
+	err = exynos_ehci_phy_enable(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable USB phy\n");
+		goto fail_io;
+	}
 
 	ehci = hcd_to_ehci(hcd);
 	ehci->caps = hcd->regs;
@@ -172,8 +274,7 @@ skip_phy:
 	return 0;
 
 fail_add_hcd:
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(&pdev->dev);
 fail_io:
 	clk_disable_unprepare(exynos_ehci->clk);
 fail_clk:
@@ -191,8 +292,7 @@ static int exynos_ehci_remove(struct platform_device *pdev)
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(&pdev->dev);
 
 	clk_disable_unprepare(exynos_ehci->clk);
 
@@ -217,8 +317,7 @@ static int exynos_ehci_suspend(struct device *dev)
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(dev);
 
 	clk_disable_unprepare(exynos_ehci->clk);
 
@@ -229,14 +328,19 @@ static int exynos_ehci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int ret;
 
 	clk_prepare_enable(exynos_ehci->clk);
 
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_init(exynos_ehci->phy);
+	ret = exynos_ehci_phy_enable(dev);
+	if (ret) {
+		dev_err(dev, "Failed to enable USB phy\n");
+		clk_disable_unprepare(exynos_ehci->clk);
+		return ret;
+	}
 
 	/* DMA burst Enable */
 	writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
-- 
1.7.10.4

[PATCH v10 4/4] usb: ehci-exynos: Change to use phy provided by the generic phy framework

[Vivek Gautam]

From: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Add the phy provider, supplied by new Exynos-usb2phy using
Generic phy framework.
Keeping the support for older USB phy intact right now, in order
to prevent any functionality break in absence of relevant
device tree side change for ehci-exynos.
Once we move to new phy in the device nodes for ehci, we can
remove the support for older phys.

Signed-off-by: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
[gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org: Addressed review comments from mailing list]
[gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org: Kept the code for old usb-phy, and just
added support for new exynos5-usb2phy in generic phy framework]
[gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org: Edited the commit message]
Signed-off-by: Vivek Gautam <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
---

Changes from v9:
 - Calling usb_phy_shutdown() when exynos_ehci_phy_enable() is failing.
 - Made exynos_ehci_phy_disable() return void, since its return value
   did not serve any purpose.
 - Calling clk_disable_unprepare() in exynos_ehci_resume() when
   exynos_ehci_phy_enable() is failed.

 .../devicetree/bindings/usb/exynos-usb.txt         |   18 +++
 drivers/usb/host/ehci-exynos.c                     |  144 +++++++++++++++++---
 2 files changed, 142 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index a90c973..126a7a9 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -12,6 +12,15 @@ Required properties:
  - interrupts: interrupt number to the cpu.
  - clocks: from common clock binding: handle to usb clock.
  - clock-names: from common clock binding: Shall be "usbhost".
+ - port: if in the SoC there are EHCI phys, they should be listed here.
+   One phy per port. Each port should have following entries:
+	- reg: port number on EHCI controller, e.g
+	       On Exynos5250, port 0 is USB2.0 otg phy
+			      port 1 is HSIC phy0
+			      port 2 is HSIC phy1
+	- phys: from the *Generic PHY* bindings; specifying phy used by port.
+	- phy-names: from the *Generic PHY* bindings; specifying name of phy
+		     used by the port.
 
 Optional properties:
  - samsung,vbus-gpio:  if present, specifies the GPIO that
@@ -27,6 +36,15 @@ Example:
 
 		clocks = <&clock 285>;
 		clock-names = "usbhost";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port@0 {
+		    reg = <0>;
+		    phys = <&usb2phy 1>;
+		    phy-names = "host";
+		    status = "disabled";
+		};
 	};
 
 OHCI
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index 4d763dc..931cfc8 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/samsung_usb_phy.h>
@@ -42,14 +43,119 @@
 static const char hcd_name[] = "ehci-exynos";
 static struct hc_driver __read_mostly exynos_ehci_hc_driver;
 
+#define PHY_NUMBER 3
 struct exynos_ehci_hcd {
 	struct clk *clk;
 	struct usb_phy *phy;
 	struct usb_otg *otg;
+	struct phy *phy_g[PHY_NUMBER];
 };
 
 #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
 
+static int exynos_ehci_get_phy(struct device *dev,
+				struct exynos_ehci_hcd *exynos_ehci)
+{
+	struct device_node *child;
+	struct phy *phy;
+	int phy_number;
+	int ret = 0;
+
+	exynos_ehci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+	if (IS_ERR(exynos_ehci->phy)) {
+		ret = PTR_ERR(exynos_ehci->phy);
+		/* This is the case when PHY config is disabled */
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dev_dbg(dev, "Failed to get usb2 phy\n");
+			exynos_ehci->phy = NULL;
+			ret = 0;
+		} else if (ret == -EPROBE_DEFER) {
+			goto fail_phy;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			goto fail_phy;
+		}
+	} else {
+		exynos_ehci->otg = exynos_ehci->phy->otg;
+	}
+
+	for_each_available_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &phy_number);
+		if (ret) {
+			dev_err(dev, "Failed to parse device tree\n");
+			of_node_put(child);
+			goto fail_phy;
+		}
+		if (phy_number >= PHY_NUMBER) {
+			dev_err(dev, "Invalid number of PHYs\n");
+			of_node_put(child);
+			ret = -EINVAL;
+			goto fail_phy;
+		}
+		phy = devm_of_phy_get(dev, child, 0);
+		of_node_put(child);
+		if (IS_ERR(phy)) {
+			ret = PTR_ERR(phy);
+			/* This is the case when PHY config is disabled */
+			if (ret == -ENOSYS || ret == -ENODEV) {
+				dev_dbg(dev, "Failed to get usb2 phy\n");
+				phy = NULL;
+				ret = 0;
+			} else if (ret == -EPROBE_DEFER) {
+				goto fail_phy;
+			} else {
+				dev_err(dev, "no usb2 phy configured\n");
+				goto fail_phy;
+			}
+		}
+		exynos_ehci->phy_g[phy_number] = phy;
+	}
+
+fail_phy:
+	return ret;
+}
+
+static int exynos_ehci_phy_enable(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int i;
+	int ret = 0;
+
+	if (exynos_ehci->phy) {
+		ret = usb_phy_init(exynos_ehci->phy);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
+		if (exynos_ehci->phy_g[i])
+			ret = phy_power_on(exynos_ehci->phy_g[i]);
+	if (ret) {
+		for (i--; i >= 0; i--)
+			if (exynos_ehci->phy_g[i])
+				phy_power_off(exynos_ehci->phy_g[i]);
+		if (exynos_ehci->phy)
+			usb_phy_shutdown(exynos_ehci->phy);
+	}
+
+	return ret;
+}
+
+static void exynos_ehci_phy_disable(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int i;
+
+	if (exynos_ehci->phy)
+		usb_phy_shutdown(exynos_ehci->phy);
+
+	for (i = 0; i < PHY_NUMBER; i++)
+		if (exynos_ehci->phy_g[i])
+			phy_power_off(exynos_ehci->phy_g[i]);
+}
+
 static void exynos_setup_vbus_gpio(struct device *dev)
 {
 	int err;
@@ -74,7 +180,6 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 	struct usb_hcd *hcd;
 	struct ehci_hcd *ehci;
 	struct resource *res;
-	struct usb_phy *phy;
 	int irq;
 	int err;
 
@@ -101,15 +206,9 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 					"samsung,exynos5440-ehci"))
 		goto skip_phy;
 
-	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
-	if (IS_ERR(phy)) {
-		usb_put_hcd(hcd);
-		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
-		return -EPROBE_DEFER;
-	} else {
-		exynos_ehci->phy = phy;
-		exynos_ehci->otg = phy->otg;
-	}
+	err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
+	if (err)
+		goto fail_clk;
 
 skip_phy:
 
@@ -151,8 +250,11 @@ skip_phy:
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_init(exynos_ehci->phy);
+	err = exynos_ehci_phy_enable(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable USB phy\n");
+		goto fail_io;
+	}
 
 	ehci = hcd_to_ehci(hcd);
 	ehci->caps = hcd->regs;
@@ -172,8 +274,7 @@ skip_phy:
 	return 0;
 
 fail_add_hcd:
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(&pdev->dev);
 fail_io:
 	clk_disable_unprepare(exynos_ehci->clk);
 fail_clk:
@@ -191,8 +292,7 @@ static int exynos_ehci_remove(struct platform_device *pdev)
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(&pdev->dev);
 
 	clk_disable_unprepare(exynos_ehci->clk);
 
@@ -217,8 +317,7 @@ static int exynos_ehci_suspend(struct device *dev)
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(dev);
 
 	clk_disable_unprepare(exynos_ehci->clk);
 
@@ -229,14 +328,19 @@ static int exynos_ehci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int ret;
 
 	clk_prepare_enable(exynos_ehci->clk);
 
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_init(exynos_ehci->phy);
+	ret = exynos_ehci_phy_enable(dev);
+	if (ret) {
+		dev_err(dev, "Failed to enable USB phy\n");
+		clk_disable_unprepare(exynos_ehci->clk);
+		return ret;
+	}
 
 	/* DMA burst Enable */
 	writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v10 4/4] usb: ehci-exynos: Change to use phy provided by the generic phy framework

[Vivek Gautam]

From: Kamil Debski <k.debski@samsung.com>

Add the phy provider, supplied by new Exynos-usb2phy using
Generic phy framework.
Keeping the support for older USB phy intact right now, in order
to prevent any functionality break in absence of relevant
device tree side change for ehci-exynos.
Once we move to new phy in the device nodes for ehci, we can
remove the support for older phys.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
[gautam.vivek at samsung.com: Addressed review comments from mailing list]
[gautam.vivek at samsung.com: Kept the code for old usb-phy, and just
added support for new exynos5-usb2phy in generic phy framework]
[gautam.vivek at samsung.com: Edited the commit message]
Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
---

Changes from v9:
 - Calling usb_phy_shutdown() when exynos_ehci_phy_enable() is failing.
 - Made exynos_ehci_phy_disable() return void, since its return value
   did not serve any purpose.
 - Calling clk_disable_unprepare() in exynos_ehci_resume() when
   exynos_ehci_phy_enable() is failed.

 .../devicetree/bindings/usb/exynos-usb.txt         |   18 +++
 drivers/usb/host/ehci-exynos.c                     |  144 +++++++++++++++++---
 2 files changed, 142 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index a90c973..126a7a9 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -12,6 +12,15 @@ Required properties:
  - interrupts: interrupt number to the cpu.
  - clocks: from common clock binding: handle to usb clock.
  - clock-names: from common clock binding: Shall be "usbhost".
+ - port: if in the SoC there are EHCI phys, they should be listed here.
+   One phy per port. Each port should have following entries:
+	- reg: port number on EHCI controller, e.g
+	       On Exynos5250, port 0 is USB2.0 otg phy
+			      port 1 is HSIC phy0
+			      port 2 is HSIC phy1
+	- phys: from the *Generic PHY* bindings; specifying phy used by port.
+	- phy-names: from the *Generic PHY* bindings; specifying name of phy
+		     used by the port.
 
 Optional properties:
  - samsung,vbus-gpio:  if present, specifies the GPIO that
@@ -27,6 +36,15 @@ Example:
 
 		clocks = <&clock 285>;
 		clock-names = "usbhost";
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port at 0 {
+		    reg = <0>;
+		    phys = <&usb2phy 1>;
+		    phy-names = "host";
+		    status = "disabled";
+		};
 	};
 
 OHCI
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index 4d763dc..931cfc8 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/samsung_usb_phy.h>
@@ -42,14 +43,119 @@
 static const char hcd_name[] = "ehci-exynos";
 static struct hc_driver __read_mostly exynos_ehci_hc_driver;
 
+#define PHY_NUMBER 3
 struct exynos_ehci_hcd {
 	struct clk *clk;
 	struct usb_phy *phy;
 	struct usb_otg *otg;
+	struct phy *phy_g[PHY_NUMBER];
 };
 
 #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
 
+static int exynos_ehci_get_phy(struct device *dev,
+				struct exynos_ehci_hcd *exynos_ehci)
+{
+	struct device_node *child;
+	struct phy *phy;
+	int phy_number;
+	int ret = 0;
+
+	exynos_ehci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+	if (IS_ERR(exynos_ehci->phy)) {
+		ret = PTR_ERR(exynos_ehci->phy);
+		/* This is the case when PHY config is disabled */
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dev_dbg(dev, "Failed to get usb2 phy\n");
+			exynos_ehci->phy = NULL;
+			ret = 0;
+		} else if (ret == -EPROBE_DEFER) {
+			goto fail_phy;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			goto fail_phy;
+		}
+	} else {
+		exynos_ehci->otg = exynos_ehci->phy->otg;
+	}
+
+	for_each_available_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &phy_number);
+		if (ret) {
+			dev_err(dev, "Failed to parse device tree\n");
+			of_node_put(child);
+			goto fail_phy;
+		}
+		if (phy_number >= PHY_NUMBER) {
+			dev_err(dev, "Invalid number of PHYs\n");
+			of_node_put(child);
+			ret = -EINVAL;
+			goto fail_phy;
+		}
+		phy = devm_of_phy_get(dev, child, 0);
+		of_node_put(child);
+		if (IS_ERR(phy)) {
+			ret = PTR_ERR(phy);
+			/* This is the case when PHY config is disabled */
+			if (ret == -ENOSYS || ret == -ENODEV) {
+				dev_dbg(dev, "Failed to get usb2 phy\n");
+				phy = NULL;
+				ret = 0;
+			} else if (ret == -EPROBE_DEFER) {
+				goto fail_phy;
+			} else {
+				dev_err(dev, "no usb2 phy configured\n");
+				goto fail_phy;
+			}
+		}
+		exynos_ehci->phy_g[phy_number] = phy;
+	}
+
+fail_phy:
+	return ret;
+}
+
+static int exynos_ehci_phy_enable(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int i;
+	int ret = 0;
+
+	if (exynos_ehci->phy) {
+		ret = usb_phy_init(exynos_ehci->phy);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
+		if (exynos_ehci->phy_g[i])
+			ret = phy_power_on(exynos_ehci->phy_g[i]);
+	if (ret) {
+		for (i--; i >= 0; i--)
+			if (exynos_ehci->phy_g[i])
+				phy_power_off(exynos_ehci->phy_g[i]);
+		if (exynos_ehci->phy)
+			usb_phy_shutdown(exynos_ehci->phy);
+	}
+
+	return ret;
+}
+
+static void exynos_ehci_phy_disable(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int i;
+
+	if (exynos_ehci->phy)
+		usb_phy_shutdown(exynos_ehci->phy);
+
+	for (i = 0; i < PHY_NUMBER; i++)
+		if (exynos_ehci->phy_g[i])
+			phy_power_off(exynos_ehci->phy_g[i]);
+}
+
 static void exynos_setup_vbus_gpio(struct device *dev)
 {
 	int err;
@@ -74,7 +180,6 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 	struct usb_hcd *hcd;
 	struct ehci_hcd *ehci;
 	struct resource *res;
-	struct usb_phy *phy;
 	int irq;
 	int err;
 
@@ -101,15 +206,9 @@ static int exynos_ehci_probe(struct platform_device *pdev)
 					"samsung,exynos5440-ehci"))
 		goto skip_phy;
 
-	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
-	if (IS_ERR(phy)) {
-		usb_put_hcd(hcd);
-		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
-		return -EPROBE_DEFER;
-	} else {
-		exynos_ehci->phy = phy;
-		exynos_ehci->otg = phy->otg;
-	}
+	err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
+	if (err)
+		goto fail_clk;
 
 skip_phy:
 
@@ -151,8 +250,11 @@ skip_phy:
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_init(exynos_ehci->phy);
+	err = exynos_ehci_phy_enable(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to enable USB phy\n");
+		goto fail_io;
+	}
 
 	ehci = hcd_to_ehci(hcd);
 	ehci->caps = hcd->regs;
@@ -172,8 +274,7 @@ skip_phy:
 	return 0;
 
 fail_add_hcd:
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(&pdev->dev);
 fail_io:
 	clk_disable_unprepare(exynos_ehci->clk);
 fail_clk:
@@ -191,8 +292,7 @@ static int exynos_ehci_remove(struct platform_device *pdev)
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(&pdev->dev);
 
 	clk_disable_unprepare(exynos_ehci->clk);
 
@@ -217,8 +317,7 @@ static int exynos_ehci_suspend(struct device *dev)
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_shutdown(exynos_ehci->phy);
+	exynos_ehci_phy_disable(dev);
 
 	clk_disable_unprepare(exynos_ehci->clk);
 
@@ -229,14 +328,19 @@ static int exynos_ehci_resume(struct device *dev)
 {
 	struct usb_hcd *hcd = dev_get_drvdata(dev);
 	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
+	int ret;
 
 	clk_prepare_enable(exynos_ehci->clk);
 
 	if (exynos_ehci->otg)
 		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
 
-	if (exynos_ehci->phy)
-		usb_phy_init(exynos_ehci->phy);
+	ret = exynos_ehci_phy_enable(dev);
+	if (ret) {
+		dev_err(dev, "Failed to enable USB phy\n");
+		clk_disable_unprepare(exynos_ehci->clk);
+		return ret;
+	}
 
 	/* DMA burst Enable */
 	writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
-- 
1.7.10.4

Re: [PATCH v2 1/4] usb: ohci-exynos: Use struct device instead of platform_device

[Alan Stern]

On Wed, 30 Apr 2014, Vivek Gautam wrote:

> Change to use struct device instead of struct platform_device
> for some static functions.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---

For all four patches in this series:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v2 1/4] usb: ohci-exynos: Use struct device instead of platform_device

[Alan Stern]

On Wed, 30 Apr 2014, Vivek Gautam wrote:

> Change to use struct device instead of struct platform_device
> for some static functions.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---

For all four patches in this series:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

[PATCH v2 1/4] usb: ohci-exynos: Use struct device instead of platform_device

[Alan Stern]

On Wed, 30 Apr 2014, Vivek Gautam wrote:

> Change to use struct device instead of struct platform_device
> for some static functions.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---

For all four patches in this series:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Tomasz Figa]

Hi Vivek,

Please see my comments inline.

On 30.04.2014 07:19, Vivek Gautam wrote:
> Add support to consume phy provided by Generic phy framework.
> Keeping the support for older usb-phy intact right now, in order
> to prevent any functionality break in absence of relevant
> device tree side change for ohci-exynos.
> Once we move to new phy in the device nodes for ohci, we can
> remove the support for older phys.
>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
>
> Changes from v3:
>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>   - Made exynos_ohci_phy_disable() return void, since its return value
>     did not serve any purpose.
>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>     exynos_ohci_phy_enable() is failed.
>
>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>   drivers/usb/host/ohci-exynos.c                     |  128 +++++++++++++++++---
>   2 files changed, 132 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> index d967ba1..a90c973 100644
> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> @@ -38,6 +38,15 @@ Required properties:
>    - interrupts: interrupt number to the cpu.
>    - clocks: from common clock binding: handle to usb clock.
>    - clock-names: from common clock binding: Shall be "usbhost".
> + - port: if in the SoC there are OHCI phys, they should be listed here.
> +   One phy per port. Each port should have following entries:
> +	- reg: port number on OHCI controller, e.g
> +	       On Exynos5250, port 0 is USB2.0 otg phy
> +			      port 1 is HSIC phy0
> +			      port 2 is HSIC phy1
> +	- phys: from the *Generic PHY* bindings, specifying phy used by port.
> +	- phy-names: from the *Generic PHY* bindings, specifying name of phy
> +		     used by the port.

I think you don't need this property for this binding, as the PHYs are 
being requested by indices (in fact only index 0 is used).

>
>   Example:
>   	usb@12120000 {
> @@ -47,6 +56,16 @@ Example:
>
>   		clocks = <&clock 285>;
>   		clock-names = "usbhost";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port@0 {
> +		    reg = <0>;
> +		    phys = <&usb2phy 1>;
> +		    phy-names = "host";

Ditto.

> +		    status = "disabled";
> +		};
> +
>   	};
>
>   DWC3
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 05f00e3..f90bf9a 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -18,6 +18,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
>   #include <linux/usb/phy.h>
>   #include <linux/usb/samsung_usb_phy.h>
>   #include <linux/usb.h>
> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly exynos_ohci_hc_driver;
>
>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
>
> +#define PHY_NUMBER 3

nit: A blank line would be nice here to separate from the struct below.

By the way, doesn't the number of PHY ports depend on platform? Of 
course right now the driver supports only Exynos >= 4210 SoCs with the 
generic PHY interface, so it might be changed in further patches.

>   struct exynos_ohci_hcd {
>   	struct clk *clk;
>   	struct usb_phy *phy;
>   	struct usb_otg *otg;
> +	struct phy *phy_g[PHY_NUMBER];
>   };
>
> -static void exynos_ohci_phy_enable(struct device *dev)
> +static int exynos_ohci_get_phy(struct device *dev,
> +				struct exynos_ohci_hcd *exynos_ohci)
> +{
> +	struct device_node *child;
> +	struct phy *phy;
> +	int phy_number;
> +	int ret = 0;
> +
> +	exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +	if (IS_ERR(exynos_ohci->phy)) {
> +		ret = PTR_ERR(exynos_ohci->phy);
> +		/* This is the case when PHY config is disabled */
> +		if (ret == -ENXIO || ret == -ENODEV) {
> +			dev_dbg(dev, "Failed to get usb2 phy\n");
> +			exynos_ohci->phy = NULL;

I think you should keep this an ERR_PTR() and just use IS_ERR() check 
further in the driver instead of checking for NULL.

> +			ret = 0;

Do you need to set ret to 0 here? The code for getting generic PHYs will 
either leave it unchanged when there are no port nodes defined or 
overwrite it with value returned by of_property_read_u32(). In the first 
case, an error code should be returned, not zero, as the driver was 
unable to get any PHY.

> +		} else if (ret == -EPROBE_DEFER) {

I think you could merge this case with the else clause below, as most 
driver do. Moreover, since the only thing done after the fail_phy label 
is returning the error code, you could just immediately return from 
here. So the whole block of code would end up like this:

	if (IS_ERR(exynos_ohci->phy)) {
		ret = PTR_ERR(exynos_ohci->phy);
		if (ret != -ENXIO && ret != -ENODEV) {
			dev_err(dev, "no usb2 phy configured\n");
			return ret;
		}
		dev_dbg(dev, "Failed to get usb2 phy\n");
		exynos_ohci->phy = NULL;
	} else {
		exynos_ohci->otg = exynos_ohci->phy->otg;
	}

> +			goto fail_phy;
> +		} else {
> +			dev_err(dev, "no usb2 phy configured\n");
> +			goto fail_phy;
> +		}
> +	} else {
> +		exynos_ohci->otg = exynos_ohci->phy->otg;
> +	}
> +
> +	/* Getting generic phy:

CodingStyle: Multi-line comments should begin and end with a single 
empty line:

	/*
	 * Getting generic phy:
	 * ...
	 */

also nit: s/phy/PHY/

> +	 * We are keeping both types of phys as a part of transiting OHCI
> +	 * to generic phy framework, so that in absence of supporting dts
> +	 * changes the functionality doesn't break.
> +	 * Once we move the ohci dt nodes to use new generic phys,
> +	 * we can remove support for older PHY in this driver.

Well, this is not entirely true. The problem here is not caused by 
existing DTS files, but rather a chance that there are existing devices 
using DTB files built from them. So to remove the support for old 
bindings, we need to make sure that such devices have their DTBs updated 
to ones built from new DTS.

> +	 */
> +	for_each_available_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &phy_number);
> +		if (ret) {
> +			dev_err(dev, "Failed to parse device tree\n");
> +			of_node_put(child);
> +			goto fail_phy;

Why not just return ret here?

> +		}

nit: Blank line here would be nice.

> +		if (phy_number >= PHY_NUMBER) {
> +			dev_err(dev, "Invalid number of PHYs\n");
> +			of_node_put(child);
> +			ret = -EINVAL;
> +			goto fail_phy;

What about just return -EINVAL;

> +		}

nit: Here too.

> +		phy = devm_of_phy_get(dev, child, 0);
> +		of_node_put(child);
> +		if (IS_ERR(phy)) {
> +			ret = PTR_ERR(phy);
> +			/* This is the case when PHY config is disabled */
> +			if (ret == -ENOSYS || ret == -ENODEV) {
> +				dev_dbg(dev, "Failed to get usb2 phy\n");
> +				phy = NCould you lULL;
> +				ret = 0;
> +			} else if (ret == -EPROBE_DEFER) {
> +				goto fail_phy;
> +			} else {
> +				dev_err(dev, "no usb2 phy configured\n");
> +				goto fail_phy;
> +			}
> +		}

Similar comments to this block apply as for the block getting legacy USB 
PHY.

> +		exynos_ohci->phy_g[phy_number] = phy;
> +	}
> +
> +fail_phy:
> +	return ret;
> +}
> +
> +static int exynos_ohci_phy_enable(struct device *dev)
>   {
>   	struct usb_hcd *hcd = dev_get_drvdata(dev);
>   	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> +	int i;
> +	int ret = 0;
>
> -	if (exynos_ohci->phy)
> -		usb_phy_init(exynos_ohci->phy);
> +	if (exynos_ohci->phy) {

!IS_ERR() should be used to check for validity.

> +		ret = usb_phy_init(exynos_ohci->phy);
> +		if (ret)
> +			return ret;

IMHO a simple return usb_phy_init(...) could be used here, if we are 
using the legacy PHY interface.

> +	}
> +
> +	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> +		if (exynos_ohci->phy_g[i])

!IS_ERR(). Just make sure that the array is initialized with an 
ERR_PTR() with some error code, (-ENODEV) probably

> +			ret = phy_power_on(exynos_ohci->phy_g[i]);
> +	if (ret) {
> +		for (i--; i >= 0; i--)
> +			if (exynos_ohci->phy_g[i])

Ditto.

> +				phy_power_off(exynos_ohci->phy_g[i]);
> +		if (exynos_ohci->phy)
> +			usb_phy_shutdown(exynos_ohci->phy);

I don't think handling of legacy PHY is needed here. I don't think we 
can have both legacy and generic PHY used at the same time.

> +	}
> +
> +	return ret;
>   }
>
>   static void exynos_ohci_phy_disable(struct device *dev)
>   {
>   	struct usb_hcd *hcd = dev_get_drvdata(dev);
>   	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> +	int i;
>
>   	if (exynos_ohci->phy)
>   		usb_phy_shutdown(exynos_ohci->phy);
> +
> +	for (i = 0; i < PHY_NUMBER; i++)
> +		if (exynos_ohci->phy_g[i])

!IS_ERR()

Best regards,
Tomasz

[PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Tomasz Figa]

Hi Vivek,

Please see my comments inline.

On 30.04.2014 07:19, Vivek Gautam wrote:
> Add support to consume phy provided by Generic phy framework.
> Keeping the support for older usb-phy intact right now, in order
> to prevent any functionality break in absence of relevant
> device tree side change for ohci-exynos.
> Once we move to new phy in the device nodes for ohci, we can
> remove the support for older phys.
>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
>
> Changes from v3:
>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>   - Made exynos_ohci_phy_disable() return void, since its return value
>     did not serve any purpose.
>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>     exynos_ohci_phy_enable() is failed.
>
>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>   drivers/usb/host/ohci-exynos.c                     |  128 +++++++++++++++++---
>   2 files changed, 132 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> index d967ba1..a90c973 100644
> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> @@ -38,6 +38,15 @@ Required properties:
>    - interrupts: interrupt number to the cpu.
>    - clocks: from common clock binding: handle to usb clock.
>    - clock-names: from common clock binding: Shall be "usbhost".
> + - port: if in the SoC there are OHCI phys, they should be listed here.
> +   One phy per port. Each port should have following entries:
> +	- reg: port number on OHCI controller, e.g
> +	       On Exynos5250, port 0 is USB2.0 otg phy
> +			      port 1 is HSIC phy0
> +			      port 2 is HSIC phy1
> +	- phys: from the *Generic PHY* bindings, specifying phy used by port.
> +	- phy-names: from the *Generic PHY* bindings, specifying name of phy
> +		     used by the port.

I think you don't need this property for this binding, as the PHYs are 
being requested by indices (in fact only index 0 is used).

>
>   Example:
>   	usb at 12120000 {
> @@ -47,6 +56,16 @@ Example:
>
>   		clocks = <&clock 285>;
>   		clock-names = "usbhost";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port at 0 {
> +		    reg = <0>;
> +		    phys = <&usb2phy 1>;
> +		    phy-names = "host";

Ditto.

> +		    status = "disabled";
> +		};
> +
>   	};
>
>   DWC3
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 05f00e3..f90bf9a 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -18,6 +18,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
>   #include <linux/usb/phy.h>
>   #include <linux/usb/samsung_usb_phy.h>
>   #include <linux/usb.h>
> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly exynos_ohci_hc_driver;
>
>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
>
> +#define PHY_NUMBER 3

nit: A blank line would be nice here to separate from the struct below.

By the way, doesn't the number of PHY ports depend on platform? Of 
course right now the driver supports only Exynos >= 4210 SoCs with the 
generic PHY interface, so it might be changed in further patches.

>   struct exynos_ohci_hcd {
>   	struct clk *clk;
>   	struct usb_phy *phy;
>   	struct usb_otg *otg;
> +	struct phy *phy_g[PHY_NUMBER];
>   };
>
> -static void exynos_ohci_phy_enable(struct device *dev)
> +static int exynos_ohci_get_phy(struct device *dev,
> +				struct exynos_ohci_hcd *exynos_ohci)
> +{
> +	struct device_node *child;
> +	struct phy *phy;
> +	int phy_number;
> +	int ret = 0;
> +
> +	exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +	if (IS_ERR(exynos_ohci->phy)) {
> +		ret = PTR_ERR(exynos_ohci->phy);
> +		/* This is the case when PHY config is disabled */
> +		if (ret == -ENXIO || ret == -ENODEV) {
> +			dev_dbg(dev, "Failed to get usb2 phy\n");
> +			exynos_ohci->phy = NULL;

I think you should keep this an ERR_PTR() and just use IS_ERR() check 
further in the driver instead of checking for NULL.

> +			ret = 0;

Do you need to set ret to 0 here? The code for getting generic PHYs will 
either leave it unchanged when there are no port nodes defined or 
overwrite it with value returned by of_property_read_u32(). In the first 
case, an error code should be returned, not zero, as the driver was 
unable to get any PHY.

> +		} else if (ret == -EPROBE_DEFER) {

I think you could merge this case with the else clause below, as most 
driver do. Moreover, since the only thing done after the fail_phy label 
is returning the error code, you could just immediately return from 
here. So the whole block of code would end up like this:

	if (IS_ERR(exynos_ohci->phy)) {
		ret = PTR_ERR(exynos_ohci->phy);
		if (ret != -ENXIO && ret != -ENODEV) {
			dev_err(dev, "no usb2 phy configured\n");
			return ret;
		}
		dev_dbg(dev, "Failed to get usb2 phy\n");
		exynos_ohci->phy = NULL;
	} else {
		exynos_ohci->otg = exynos_ohci->phy->otg;
	}

> +			goto fail_phy;
> +		} else {
> +			dev_err(dev, "no usb2 phy configured\n");
> +			goto fail_phy;
> +		}
> +	} else {
> +		exynos_ohci->otg = exynos_ohci->phy->otg;
> +	}
> +
> +	/* Getting generic phy:

CodingStyle: Multi-line comments should begin and end with a single 
empty line:

	/*
	 * Getting generic phy:
	 * ...
	 */

also nit: s/phy/PHY/

> +	 * We are keeping both types of phys as a part of transiting OHCI
> +	 * to generic phy framework, so that in absence of supporting dts
> +	 * changes the functionality doesn't break.
> +	 * Once we move the ohci dt nodes to use new generic phys,
> +	 * we can remove support for older PHY in this driver.

Well, this is not entirely true. The problem here is not caused by 
existing DTS files, but rather a chance that there are existing devices 
using DTB files built from them. So to remove the support for old 
bindings, we need to make sure that such devices have their DTBs updated 
to ones built from new DTS.

> +	 */
> +	for_each_available_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &phy_number);
> +		if (ret) {
> +			dev_err(dev, "Failed to parse device tree\n");
> +			of_node_put(child);
> +			goto fail_phy;

Why not just return ret here?

> +		}

nit: Blank line here would be nice.

> +		if (phy_number >= PHY_NUMBER) {
> +			dev_err(dev, "Invalid number of PHYs\n");
> +			of_node_put(child);
> +			ret = -EINVAL;
> +			goto fail_phy;

What about just return -EINVAL;

> +		}

nit: Here too.

> +		phy = devm_of_phy_get(dev, child, 0);
> +		of_node_put(child);
> +		if (IS_ERR(phy)) {
> +			ret = PTR_ERR(phy);
> +			/* This is the case when PHY config is disabled */
> +			if (ret == -ENOSYS || ret == -ENODEV) {
> +				dev_dbg(dev, "Failed to get usb2 phy\n");
> +				phy = NCould you lULL;
> +				ret = 0;
> +			} else if (ret == -EPROBE_DEFER) {
> +				goto fail_phy;
> +			} else {
> +				dev_err(dev, "no usb2 phy configured\n");
> +				goto fail_phy;
> +			}
> +		}

Similar comments to this block apply as for the block getting legacy USB 
PHY.

> +		exynos_ohci->phy_g[phy_number] = phy;
> +	}
> +
> +fail_phy:
> +	return ret;
> +}
> +
> +static int exynos_ohci_phy_enable(struct device *dev)
>   {
>   	struct usb_hcd *hcd = dev_get_drvdata(dev);
>   	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> +	int i;
> +	int ret = 0;
>
> -	if (exynos_ohci->phy)
> -		usb_phy_init(exynos_ohci->phy);
> +	if (exynos_ohci->phy) {

!IS_ERR() should be used to check for validity.

> +		ret = usb_phy_init(exynos_ohci->phy);
> +		if (ret)
> +			return ret;

IMHO a simple return usb_phy_init(...) could be used here, if we are 
using the legacy PHY interface.

> +	}
> +
> +	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> +		if (exynos_ohci->phy_g[i])

!IS_ERR(). Just make sure that the array is initialized with an 
ERR_PTR() with some error code, (-ENODEV) probably

> +			ret = phy_power_on(exynos_ohci->phy_g[i]);
> +	if (ret) {
> +		for (i--; i >= 0; i--)
> +			if (exynos_ohci->phy_g[i])

Ditto.

> +				phy_power_off(exynos_ohci->phy_g[i]);
> +		if (exynos_ohci->phy)
> +			usb_phy_shutdown(exynos_ohci->phy);

I don't think handling of legacy PHY is needed here. I don't think we 
can have both legacy and generic PHY used at the same time.

> +	}
> +
> +	return ret;
>   }
>
>   static void exynos_ohci_phy_disable(struct device *dev)
>   {
>   	struct usb_hcd *hcd = dev_get_drvdata(dev);
>   	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
> +	int i;
>
>   	if (exynos_ohci->phy)
>   		usb_phy_shutdown(exynos_ohci->phy);
> +
> +	for (i = 0; i < PHY_NUMBER; i++)
> +		if (exynos_ohci->phy_g[i])

!IS_ERR()

Best regards,
Tomasz

Re: [PATCH v10 4/4] usb: ehci-exynos: Change to use phy provided by the generic phy framework

[Tomasz Figa]

Hi Vivek,

I believe the same comments as for the patch for ohci-exynos apply for 
this patch as well.

Best regards,
Tomasz

On 30.04.2014 07:19, Vivek Gautam wrote:
> From: Kamil Debski <k.debski@samsung.com>
>
> Add the phy provider, supplied by new Exynos-usb2phy using
> Generic phy framework.
> Keeping the support for older USB phy intact right now, in order
> to prevent any functionality break in absence of relevant
> device tree side change for ehci-exynos.
> Once we move to new phy in the device nodes for ehci, we can
> remove the support for older phys.
>
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> [gautam.vivek@samsung.com: Addressed review comments from mailing list]
> [gautam.vivek@samsung.com: Kept the code for old usb-phy, and just
> added support for new exynos5-usb2phy in generic phy framework]
> [gautam.vivek@samsung.com: Edited the commit message]
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
>
> Changes from v9:
>   - Calling usb_phy_shutdown() when exynos_ehci_phy_enable() is failing.
>   - Made exynos_ehci_phy_disable() return void, since its return value
>     did not serve any purpose.
>   - Calling clk_disable_unprepare() in exynos_ehci_resume() when
>     exynos_ehci_phy_enable() is failed.
>
>   .../devicetree/bindings/usb/exynos-usb.txt         |   18 +++
>   drivers/usb/host/ehci-exynos.c                     |  144 +++++++++++++++++---
>   2 files changed, 142 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> index a90c973..126a7a9 100644
> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> @@ -12,6 +12,15 @@ Required properties:
>    - interrupts: interrupt number to the cpu.
>    - clocks: from common clock binding: handle to usb clock.
>    - clock-names: from common clock binding: Shall be "usbhost".
> + - port: if in the SoC there are EHCI phys, they should be listed here.
> +   One phy per port. Each port should have following entries:
> +	- reg: port number on EHCI controller, e.g
> +	       On Exynos5250, port 0 is USB2.0 otg phy
> +			      port 1 is HSIC phy0
> +			      port 2 is HSIC phy1
> +	- phys: from the *Generic PHY* bindings; specifying phy used by port.
> +	- phy-names: from the *Generic PHY* bindings; specifying name of phy
> +		     used by the port.
>
>   Optional properties:
>    - samsung,vbus-gpio:  if present, specifies the GPIO that
> @@ -27,6 +36,15 @@ Example:
>
>   		clocks = <&clock 285>;
>   		clock-names = "usbhost";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port@0 {
> +		    reg = <0>;
> +		    phys = <&usb2phy 1>;
> +		    phy-names = "host";
> +		    status = "disabled";
> +		};
>   	};
>
>   OHCI
> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> index 4d763dc..931cfc8 100644
> --- a/drivers/usb/host/ehci-exynos.c
> +++ b/drivers/usb/host/ehci-exynos.c
> @@ -19,6 +19,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_gpio.h>
> +#include <linux/phy/phy.h>
>   #include <linux/platform_device.h>
>   #include <linux/usb/phy.h>
>   #include <linux/usb/samsung_usb_phy.h>
> @@ -42,14 +43,119 @@
>   static const char hcd_name[] = "ehci-exynos";
>   static struct hc_driver __read_mostly exynos_ehci_hc_driver;
>
> +#define PHY_NUMBER 3
>   struct exynos_ehci_hcd {
>   	struct clk *clk;
>   	struct usb_phy *phy;
>   	struct usb_otg *otg;
> +	struct phy *phy_g[PHY_NUMBER];
>   };
>
>   #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
>
> +static int exynos_ehci_get_phy(struct device *dev,
> +				struct exynos_ehci_hcd *exynos_ehci)
> +{
> +	struct device_node *child;
> +	struct phy *phy;
> +	int phy_number;
> +	int ret = 0;
> +
> +	exynos_ehci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +	if (IS_ERR(exynos_ehci->phy)) {
> +		ret = PTR_ERR(exynos_ehci->phy);
> +		/* This is the case when PHY config is disabled */
> +		if (ret == -ENXIO || ret == -ENODEV) {
> +			dev_dbg(dev, "Failed to get usb2 phy\n");
> +			exynos_ehci->phy = NULL;
> +			ret = 0;
> +		} else if (ret == -EPROBE_DEFER) {
> +			goto fail_phy;
> +		} else {
> +			dev_err(dev, "no usb2 phy configured\n");
> +			goto fail_phy;
> +		}
> +	} else {
> +		exynos_ehci->otg = exynos_ehci->phy->otg;
> +	}
> +
> +	for_each_available_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &phy_number);
> +		if (ret) {
> +			dev_err(dev, "Failed to parse device tree\n");
> +			of_node_put(child);
> +			goto fail_phy;
> +		}
> +		if (phy_number >= PHY_NUMBER) {
> +			dev_err(dev, "Invalid number of PHYs\n");
> +			of_node_put(child);
> +			ret = -EINVAL;
> +			goto fail_phy;
> +		}
> +		phy = devm_of_phy_get(dev, child, 0);
> +		of_node_put(child);
> +		if (IS_ERR(phy)) {
> +			ret = PTR_ERR(phy);
> +			/* This is the case when PHY config is disabled */
> +			if (ret == -ENOSYS || ret == -ENODEV) {
> +				dev_dbg(dev, "Failed to get usb2 phy\n");
> +				phy = NULL;
> +				ret = 0;
> +			} else if (ret == -EPROBE_DEFER) {
> +				goto fail_phy;
> +			} else {
> +				dev_err(dev, "no usb2 phy configured\n");
> +				goto fail_phy;
> +			}
> +		}
> +		exynos_ehci->phy_g[phy_number] = phy;
> +	}
> +
> +fail_phy:
> +	return ret;
> +}
> +
> +static int exynos_ehci_phy_enable(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> +	int i;
> +	int ret = 0;
> +
> +	if (exynos_ehci->phy) {
> +		ret = usb_phy_init(exynos_ehci->phy);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> +		if (exynos_ehci->phy_g[i])
> +			ret = phy_power_on(exynos_ehci->phy_g[i]);
> +	if (ret) {
> +		for (i--; i >= 0; i--)
> +			if (exynos_ehci->phy_g[i])
> +				phy_power_off(exynos_ehci->phy_g[i]);
> +		if (exynos_ehci->phy)
> +			usb_phy_shutdown(exynos_ehci->phy);
> +	}
> +
> +	return ret;
> +}
> +
> +static void exynos_ehci_phy_disable(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> +	int i;
> +
> +	if (exynos_ehci->phy)
> +		usb_phy_shutdown(exynos_ehci->phy);
> +
> +	for (i = 0; i < PHY_NUMBER; i++)
> +		if (exynos_ehci->phy_g[i])
> +			phy_power_off(exynos_ehci->phy_g[i]);
> +}
> +
>   static void exynos_setup_vbus_gpio(struct device *dev)
>   {
>   	int err;
> @@ -74,7 +180,6 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>   	struct usb_hcd *hcd;
>   	struct ehci_hcd *ehci;
>   	struct resource *res;
> -	struct usb_phy *phy;
>   	int irq;
>   	int err;
>
> @@ -101,15 +206,9 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>   					"samsung,exynos5440-ehci"))
>   		goto skip_phy;
>
> -	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
> -	if (IS_ERR(phy)) {
> -		usb_put_hcd(hcd);
> -		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
> -		return -EPROBE_DEFER;
> -	} else {
> -		exynos_ehci->phy = phy;
> -		exynos_ehci->otg = phy->otg;
> -	}
> +	err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
> +	if (err)
> +		goto fail_clk;
>
>   skip_phy:
>
> @@ -151,8 +250,11 @@ skip_phy:
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_init(exynos_ehci->phy);
> +	err = exynos_ehci_phy_enable(&pdev->dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to enable USB phy\n");
> +		goto fail_io;
> +	}
>
>   	ehci = hcd_to_ehci(hcd);
>   	ehci->caps = hcd->regs;
> @@ -172,8 +274,7 @@ skip_phy:
>   	return 0;
>
>   fail_add_hcd:
> -	if (exynos_ehci->phy)
> -		usb_phy_shutdown(exynos_ehci->phy);
> +	exynos_ehci_phy_disable(&pdev->dev);
>   fail_io:
>   	clk_disable_unprepare(exynos_ehci->clk);
>   fail_clk:
> @@ -191,8 +292,7 @@ static int exynos_ehci_remove(struct platform_device *pdev)
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_shutdown(exynos_ehci->phy);
> +	exynos_ehci_phy_disable(&pdev->dev);
>
>   	clk_disable_unprepare(exynos_ehci->clk);
>
> @@ -217,8 +317,7 @@ static int exynos_ehci_suspend(struct device *dev)
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_shutdown(exynos_ehci->phy);
> +	exynos_ehci_phy_disable(dev);
>
>   	clk_disable_unprepare(exynos_ehci->clk);
>
> @@ -229,14 +328,19 @@ static int exynos_ehci_resume(struct device *dev)
>   {
>   	struct usb_hcd *hcd = dev_get_drvdata(dev);
>   	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> +	int ret;
>
>   	clk_prepare_enable(exynos_ehci->clk);
>
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_init(exynos_ehci->phy);
> +	ret = exynos_ehci_phy_enable(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable USB phy\n");
> +		clk_disable_unprepare(exynos_ehci->clk);
> +		return ret;
> +	}
>
>   	/* DMA burst Enable */
>   	writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
>

[PATCH v10 4/4] usb: ehci-exynos: Change to use phy provided by the generic phy framework

[Tomasz Figa]

Hi Vivek,

I believe the same comments as for the patch for ohci-exynos apply for 
this patch as well.

Best regards,
Tomasz

On 30.04.2014 07:19, Vivek Gautam wrote:
> From: Kamil Debski <k.debski@samsung.com>
>
> Add the phy provider, supplied by new Exynos-usb2phy using
> Generic phy framework.
> Keeping the support for older USB phy intact right now, in order
> to prevent any functionality break in absence of relevant
> device tree side change for ehci-exynos.
> Once we move to new phy in the device nodes for ehci, we can
> remove the support for older phys.
>
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> [gautam.vivek at samsung.com: Addressed review comments from mailing list]
> [gautam.vivek at samsung.com: Kept the code for old usb-phy, and just
> added support for new exynos5-usb2phy in generic phy framework]
> [gautam.vivek at samsung.com: Edited the commit message]
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
>
> Changes from v9:
>   - Calling usb_phy_shutdown() when exynos_ehci_phy_enable() is failing.
>   - Made exynos_ehci_phy_disable() return void, since its return value
>     did not serve any purpose.
>   - Calling clk_disable_unprepare() in exynos_ehci_resume() when
>     exynos_ehci_phy_enable() is failed.
>
>   .../devicetree/bindings/usb/exynos-usb.txt         |   18 +++
>   drivers/usb/host/ehci-exynos.c                     |  144 +++++++++++++++++---
>   2 files changed, 142 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> index a90c973..126a7a9 100644
> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> @@ -12,6 +12,15 @@ Required properties:
>    - interrupts: interrupt number to the cpu.
>    - clocks: from common clock binding: handle to usb clock.
>    - clock-names: from common clock binding: Shall be "usbhost".
> + - port: if in the SoC there are EHCI phys, they should be listed here.
> +   One phy per port. Each port should have following entries:
> +	- reg: port number on EHCI controller, e.g
> +	       On Exynos5250, port 0 is USB2.0 otg phy
> +			      port 1 is HSIC phy0
> +			      port 2 is HSIC phy1
> +	- phys: from the *Generic PHY* bindings; specifying phy used by port.
> +	- phy-names: from the *Generic PHY* bindings; specifying name of phy
> +		     used by the port.
>
>   Optional properties:
>    - samsung,vbus-gpio:  if present, specifies the GPIO that
> @@ -27,6 +36,15 @@ Example:
>
>   		clocks = <&clock 285>;
>   		clock-names = "usbhost";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port at 0 {
> +		    reg = <0>;
> +		    phys = <&usb2phy 1>;
> +		    phy-names = "host";
> +		    status = "disabled";
> +		};
>   	};
>
>   OHCI
> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> index 4d763dc..931cfc8 100644
> --- a/drivers/usb/host/ehci-exynos.c
> +++ b/drivers/usb/host/ehci-exynos.c
> @@ -19,6 +19,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_gpio.h>
> +#include <linux/phy/phy.h>
>   #include <linux/platform_device.h>
>   #include <linux/usb/phy.h>
>   #include <linux/usb/samsung_usb_phy.h>
> @@ -42,14 +43,119 @@
>   static const char hcd_name[] = "ehci-exynos";
>   static struct hc_driver __read_mostly exynos_ehci_hc_driver;
>
> +#define PHY_NUMBER 3
>   struct exynos_ehci_hcd {
>   	struct clk *clk;
>   	struct usb_phy *phy;
>   	struct usb_otg *otg;
> +	struct phy *phy_g[PHY_NUMBER];
>   };
>
>   #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
>
> +static int exynos_ehci_get_phy(struct device *dev,
> +				struct exynos_ehci_hcd *exynos_ehci)
> +{
> +	struct device_node *child;
> +	struct phy *phy;
> +	int phy_number;
> +	int ret = 0;
> +
> +	exynos_ehci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +	if (IS_ERR(exynos_ehci->phy)) {
> +		ret = PTR_ERR(exynos_ehci->phy);
> +		/* This is the case when PHY config is disabled */
> +		if (ret == -ENXIO || ret == -ENODEV) {
> +			dev_dbg(dev, "Failed to get usb2 phy\n");
> +			exynos_ehci->phy = NULL;
> +			ret = 0;
> +		} else if (ret == -EPROBE_DEFER) {
> +			goto fail_phy;
> +		} else {
> +			dev_err(dev, "no usb2 phy configured\n");
> +			goto fail_phy;
> +		}
> +	} else {
> +		exynos_ehci->otg = exynos_ehci->phy->otg;
> +	}
> +
> +	for_each_available_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &phy_number);
> +		if (ret) {
> +			dev_err(dev, "Failed to parse device tree\n");
> +			of_node_put(child);
> +			goto fail_phy;
> +		}
> +		if (phy_number >= PHY_NUMBER) {
> +			dev_err(dev, "Invalid number of PHYs\n");
> +			of_node_put(child);
> +			ret = -EINVAL;
> +			goto fail_phy;
> +		}
> +		phy = devm_of_phy_get(dev, child, 0);
> +		of_node_put(child);
> +		if (IS_ERR(phy)) {
> +			ret = PTR_ERR(phy);
> +			/* This is the case when PHY config is disabled */
> +			if (ret == -ENOSYS || ret == -ENODEV) {
> +				dev_dbg(dev, "Failed to get usb2 phy\n");
> +				phy = NULL;
> +				ret = 0;
> +			} else if (ret == -EPROBE_DEFER) {
> +				goto fail_phy;
> +			} else {
> +				dev_err(dev, "no usb2 phy configured\n");
> +				goto fail_phy;
> +			}
> +		}
> +		exynos_ehci->phy_g[phy_number] = phy;
> +	}
> +
> +fail_phy:
> +	return ret;
> +}
> +
> +static int exynos_ehci_phy_enable(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> +	int i;
> +	int ret = 0;
> +
> +	if (exynos_ehci->phy) {
> +		ret = usb_phy_init(exynos_ehci->phy);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> +		if (exynos_ehci->phy_g[i])
> +			ret = phy_power_on(exynos_ehci->phy_g[i]);
> +	if (ret) {
> +		for (i--; i >= 0; i--)
> +			if (exynos_ehci->phy_g[i])
> +				phy_power_off(exynos_ehci->phy_g[i]);
> +		if (exynos_ehci->phy)
> +			usb_phy_shutdown(exynos_ehci->phy);
> +	}
> +
> +	return ret;
> +}
> +
> +static void exynos_ehci_phy_disable(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> +	int i;
> +
> +	if (exynos_ehci->phy)
> +		usb_phy_shutdown(exynos_ehci->phy);
> +
> +	for (i = 0; i < PHY_NUMBER; i++)
> +		if (exynos_ehci->phy_g[i])
> +			phy_power_off(exynos_ehci->phy_g[i]);
> +}
> +
>   static void exynos_setup_vbus_gpio(struct device *dev)
>   {
>   	int err;
> @@ -74,7 +180,6 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>   	struct usb_hcd *hcd;
>   	struct ehci_hcd *ehci;
>   	struct resource *res;
> -	struct usb_phy *phy;
>   	int irq;
>   	int err;
>
> @@ -101,15 +206,9 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>   					"samsung,exynos5440-ehci"))
>   		goto skip_phy;
>
> -	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
> -	if (IS_ERR(phy)) {
> -		usb_put_hcd(hcd);
> -		dev_warn(&pdev->dev, "no platform data or transceiver defined\n");
> -		return -EPROBE_DEFER;
> -	} else {
> -		exynos_ehci->phy = phy;
> -		exynos_ehci->otg = phy->otg;
> -	}
> +	err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
> +	if (err)
> +		goto fail_clk;
>
>   skip_phy:
>
> @@ -151,8 +250,11 @@ skip_phy:
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_init(exynos_ehci->phy);
> +	err = exynos_ehci_phy_enable(&pdev->dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to enable USB phy\n");
> +		goto fail_io;
> +	}
>
>   	ehci = hcd_to_ehci(hcd);
>   	ehci->caps = hcd->regs;
> @@ -172,8 +274,7 @@ skip_phy:
>   	return 0;
>
>   fail_add_hcd:
> -	if (exynos_ehci->phy)
> -		usb_phy_shutdown(exynos_ehci->phy);
> +	exynos_ehci_phy_disable(&pdev->dev);
>   fail_io:
>   	clk_disable_unprepare(exynos_ehci->clk);
>   fail_clk:
> @@ -191,8 +292,7 @@ static int exynos_ehci_remove(struct platform_device *pdev)
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_shutdown(exynos_ehci->phy);
> +	exynos_ehci_phy_disable(&pdev->dev);
>
>   	clk_disable_unprepare(exynos_ehci->clk);
>
> @@ -217,8 +317,7 @@ static int exynos_ehci_suspend(struct device *dev)
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_shutdown(exynos_ehci->phy);
> +	exynos_ehci_phy_disable(dev);
>
>   	clk_disable_unprepare(exynos_ehci->clk);
>
> @@ -229,14 +328,19 @@ static int exynos_ehci_resume(struct device *dev)
>   {
>   	struct usb_hcd *hcd = dev_get_drvdata(dev);
>   	struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
> +	int ret;
>
>   	clk_prepare_enable(exynos_ehci->clk);
>
>   	if (exynos_ehci->otg)
>   		exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self);
>
> -	if (exynos_ehci->phy)
> -		usb_phy_init(exynos_ehci->phy);
> +	ret = exynos_ehci_phy_enable(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable USB phy\n");
> +		clk_disable_unprepare(exynos_ehci->clk);
> +		return ret;
> +	}
>
>   	/* DMA burst Enable */
>   	writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
>

Re: [PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

Hi Tomasz,


On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Vivek,
>
> Please see my comments inline.

Thanks for the review, please find my answers inline.

>
>
> On 30.04.2014 07:19, Vivek Gautam wrote:
>>
>> Add support to consume phy provided by Generic phy framework.
>> Keeping the support for older usb-phy intact right now, in order
>> to prevent any functionality break in absence of relevant
>> device tree side change for ohci-exynos.
>> Once we move to new phy in the device nodes for ohci, we can
>> remove the support for older phys.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> ---
>>
>> Changes from v3:
>>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>   - Made exynos_ohci_phy_disable() return void, since its return value
>>     did not serve any purpose.
>>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>     exynos_ohci_phy_enable() is failed.
>>
>>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>>   drivers/usb/host/ohci-exynos.c                     |  128
>> +++++++++++++++++---
>>   2 files changed, 132 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> index d967ba1..a90c973 100644
>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> @@ -38,6 +38,15 @@ Required properties:
>>    - interrupts: interrupt number to the cpu.
>>    - clocks: from common clock binding: handle to usb clock.
>>    - clock-names: from common clock binding: Shall be "usbhost".
>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>> +   One phy per port. Each port should have following entries:
>> +       - reg: port number on OHCI controller, e.g
>> +              On Exynos5250, port 0 is USB2.0 otg phy
>> +                             port 1 is HSIC phy0
>> +                             port 2 is HSIC phy1
>> +       - phys: from the *Generic PHY* bindings, specifying phy used by
>> port.
>> +       - phy-names: from the *Generic PHY* bindings, specifying name of
>> phy
>> +                    used by the port.
>
>
> I think you don't need this property for this binding, as the PHYs are being
> requested by indices (in fact only index 0 is used).

True, that phy-names property is not used, since PHYs are being
requested using indices.
We can remove this.

>
>
>>
>>   Example:
>>         usb@12120000 {
>> @@ -47,6 +56,16 @@ Example:
>>
>>                 clocks = <&clock 285>;
>>                 clock-names = "usbhost";
>> +
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               port@0 {
>> +                   reg = <0>;
>> +                   phys = <&usb2phy 1>;
>> +                   phy-names = "host";
>
>
> Ditto.
will remove this.

>
>
>> +                   status = "disabled";
>> +               };
>> +
>>         };
>>
>>   DWC3
>> diff --git a/drivers/usb/host/ohci-exynos.c
>> b/drivers/usb/host/ohci-exynos.c
>> index 05f00e3..f90bf9a 100644
>> --- a/drivers/usb/host/ohci-exynos.c
>> +++ b/drivers/usb/host/ohci-exynos.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>>   #include <linux/usb/phy.h>
>>   #include <linux/usb/samsung_usb_phy.h>
>>   #include <linux/usb.h>
>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>> exynos_ohci_hc_driver;
>>
>>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>> *)(hcd_to_ohci(hcd)->priv)
>>
>> +#define PHY_NUMBER 3
>
>
> nit: A blank line would be nice here to separate from the struct below.

Ok, will add one.

>
> By the way, doesn't the number of PHY ports depend on platform? Of course
> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
> interface, so it might be changed in further patches.

Yes, the number of PHY ports will be platform dependent feature.
In subsequent patches we can add support to count the number of PHYs,
or rather in this patch
itself, when we do -
         for_each_available_child_of_node(dev->of_node, child) {
we can keep a count of how many child nodes we found, and then
configure those many.
As such the host controller drivers will have 'host' and 'hsic' PHYs.

>
>
>>   struct exynos_ohci_hcd {
>>         struct clk *clk;
>>         struct usb_phy *phy;
>>         struct usb_otg *otg;
>> +       struct phy *phy_g[PHY_NUMBER];
>>   };
>>
>> -static void exynos_ohci_phy_enable(struct device *dev)
>> +static int exynos_ohci_get_phy(struct device *dev,
>> +                               struct exynos_ohci_hcd *exynos_ohci)
>> +{
>> +       struct device_node *child;
>> +       struct phy *phy;
>> +       int phy_number;
>> +       int ret = 0;
>> +
>> +       exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> +       if (IS_ERR(exynos_ohci->phy)) {
>> +               ret = PTR_ERR(exynos_ohci->phy);
>> +               /* This is the case when PHY config is disabled */
>> +               if (ret == -ENXIO || ret == -ENODEV) {
>> +                       dev_dbg(dev, "Failed to get usb2 phy\n");
>> +                       exynos_ohci->phy = NULL;
>
>
> I think you should keep this an ERR_PTR() and just use IS_ERR() check
> further in the driver instead of checking for NULL.

Yea, that's also one way to check for phy, i can modify this.

>
>> +                       ret = 0;
>
>
> Do you need to set ret to 0 here? The code for getting generic PHYs will
> either leave it unchanged when there are no port nodes defined or overwrite
> it with value returned by of_property_read_u32(). In the first case, an
> error code should be returned, not zero, as the driver was unable to get any
> PHY.

The idea was to not fail exynos_ohci_get_phy() call when the PHY
configs are not even enabled.
Since this would mean, that the driver will never be able to get a PHY
in future, and there will be no point in
failing the driver probe.

In a case when the host controller dts is still on older PHY bindings,
this 'ret' will *not* be over-wriiten, since the
generic phy nodes will not be there.
And in case the host dts is moved to the new generic PHY based
bindings. then the part of this function exynos_ohci_get_phy()
related to older usb-phy, shall not execute.

This is reason why i did not really add a fall-back mechanism for getting PHY.
Since from DT either of the two bindings be supplied, and then things
here will be handles almost independently.

>
>
>> +               } else if (ret == -EPROBE_DEFER) {
>
>
> I think you could merge this case with the else clause below, as most driver
> do. Moreover, since the only thing done after the fail_phy label is
> returning the error code, you could just immediately return from here. So
> the whole block of code would end up like this:
>
>         if (IS_ERR(exynos_ohci->phy)) {
>                 ret = PTR_ERR(exynos_ohci->phy);
>                 if (ret != -ENXIO && ret != -ENODEV) {
>
>                         dev_err(dev, "no usb2 phy configured\n");
>                         return ret;
>
>                 }
>                 dev_dbg(dev, "Failed to get usb2 phy\n");
>                 exynos_ohci->phy = NULL;
>         } else {
>
>                 exynos_ohci->otg = exynos_ohci->phy->otg;
>         }

Ok, this seems a good restructuring. I shall change this.

>
>> +                       goto fail_phy;
>>
>> +               } else {
>> +                       dev_err(dev, "no usb2 phy configured\n");
>> +                       goto fail_phy;
>> +               }
>> +       } else {
>> +               exynos_ohci->otg = exynos_ohci->phy->otg;
>> +       }
>> +
>> +       /* Getting generic phy:
>
>
> CodingStyle: Multi-line comments should begin and end with a single empty
> line:
>
>         /*
>          * Getting generic phy:
>          * ...
>          */
>
> also nit: s/phy/PHY/
>

Ok, will correct this.

>
>> +        * We are keeping both types of phys as a part of transiting OHCI
>> +        * to generic phy framework, so that in absence of supporting dts
>> +        * changes the functionality doesn't break.
>> +        * Once we move the ohci dt nodes to use new generic phys,
>> +        * we can remove support for older PHY in this driver.
>
>
> Well, this is not entirely true. The problem here is not caused by existing
> DTS files, but rather a chance that there are existing devices using DTB
> files built from them. So to remove the support for old bindings, we need to
> make sure that such devices have their DTBs updated to ones built from new
> DTS.

Fair enough, thanks for explaining. :-)
I shall modify this comment.

>
>
>> +        */
>> +       for_each_available_child_of_node(dev->of_node, child) {
>> +               ret = of_property_read_u32(child, "reg", &phy_number);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to parse device tree\n");
>> +                       of_node_put(child);
>> +                       goto fail_phy;
>
>
> Why not just return ret here?
>
>> +               }
>
>
> nit: Blank line here would be nice.

ok

>
>
>> +               if (phy_number >= PHY_NUMBER) {
>> +                       dev_err(dev, "Invalid number of PHYs\n");
>> +                       of_node_put(child);
>> +                       ret = -EINVAL;
>> +                       goto fail_phy;
>
>
> What about just return -EINVAL;

Yea, just another way of doing things. ;-)
I felt 'goto' to be a bit clean than adding number of returns in
between statements.

>
>> +               }
>
>
> nit: Here too.
yea, will add a blank line.

>
>> +               phy = devm_of_phy_get(dev, child, 0);
>> +               of_node_put(child);
>> +               if (IS_ERR(phy)) {
>> +                       ret = PTR_ERR(phy);
>> +                       /* This is the case when PHY config is disabled */
>> +                       if (ret == -ENOSYS || ret == -ENODEV) {
>> +                               dev_dbg(dev, "Failed to get usb2 phy\n");
>> +                               phy = NULL;
>>
>> +                               ret = 0;
>> +                       } else if (ret == -EPROBE_DEFER) {
>> +                               goto fail_phy;
>> +                       } else {
>> +                               dev_err(dev, "no usb2 phy configured\n");
>> +                               goto fail_phy;
>> +                       }
>> +               }
>
>
> Similar comments to this block apply as for the block getting legacy USB
> PHY.

Ok, i will restructure this same as 'legacy PHY  block'

>
>
>> +               exynos_ohci->phy_g[phy_number] = phy;
>> +       }
>> +
>> +fail_phy:
>> +       return ret;
>> +}
>> +
>> +static int exynos_ohci_phy_enable(struct device *dev)
>>   {
>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> +       int i;
>> +       int ret = 0;
>>
>> -       if (exynos_ohci->phy)
>> -               usb_phy_init(exynos_ohci->phy);
>> +       if (exynos_ohci->phy) {
>
>
> !IS_ERR() should be used to check for validity.

Ok, this with the earlier change of setting exynos_ohci->phy as
ERR_PTR(), should be good then.

>
>
>> +               ret = usb_phy_init(exynos_ohci->phy);
>> +               if (ret)
>> +                       return ret;
>
>
> IMHO a simple return usb_phy_init(...) could be used here, if we are using
> the legacy PHY interface.

Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
since those devices will not have the generic
PHYs.

>
>
>> +       }
>> +
>> +       for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>> +               if (exynos_ohci->phy_g[i])
>
>
> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
> with some error code, (-ENODEV) probably

Ok.

>
>
>> +                       ret = phy_power_on(exynos_ohci->phy_g[i]);
>> +       if (ret) {
>> +               for (i--; i >= 0; i--)
>> +                       if (exynos_ohci->phy_g[i])
>
>
> Ditto.
>
>
>> +                               phy_power_off(exynos_ohci->phy_g[i]);
>> +               if (exynos_ohci->phy)
>> +                       usb_phy_shutdown(exynos_ohci->phy);
>
>
> I don't think handling of legacy PHY is needed here. I don't think we can
> have both legacy and generic PHY used at the same time.

Right, we will not have both the PHYs existing at the same time.
And if we are doing a simple return usb_phy_init(...); then ofcourse
we will not need to do this.

>
>
>> +       }
>> +
>> +       return ret;
>>   }
>>
>>   static void exynos_ohci_phy_disable(struct device *dev)
>>   {
>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> +       int i;
>>
>>         if (exynos_ohci->phy)
>>                 usb_phy_shutdown(exynos_ohci->phy);
>> +
>> +       for (i = 0; i < PHY_NUMBER; i++)
>> +               if (exynos_ohci->phy_g[i])
>
>
> !IS_ERR()

Yes.



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

Hi Tomasz,


On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Vivek,
>
> Please see my comments inline.

Thanks for the review, please find my answers inline.

>
>
> On 30.04.2014 07:19, Vivek Gautam wrote:
>>
>> Add support to consume phy provided by Generic phy framework.
>> Keeping the support for older usb-phy intact right now, in order
>> to prevent any functionality break in absence of relevant
>> device tree side change for ohci-exynos.
>> Once we move to new phy in the device nodes for ohci, we can
>> remove the support for older phys.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> ---
>>
>> Changes from v3:
>>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>   - Made exynos_ohci_phy_disable() return void, since its return value
>>     did not serve any purpose.
>>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>     exynos_ohci_phy_enable() is failed.
>>
>>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>>   drivers/usb/host/ohci-exynos.c                     |  128
>> +++++++++++++++++---
>>   2 files changed, 132 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> index d967ba1..a90c973 100644
>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> @@ -38,6 +38,15 @@ Required properties:
>>    - interrupts: interrupt number to the cpu.
>>    - clocks: from common clock binding: handle to usb clock.
>>    - clock-names: from common clock binding: Shall be "usbhost".
>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>> +   One phy per port. Each port should have following entries:
>> +       - reg: port number on OHCI controller, e.g
>> +              On Exynos5250, port 0 is USB2.0 otg phy
>> +                             port 1 is HSIC phy0
>> +                             port 2 is HSIC phy1
>> +       - phys: from the *Generic PHY* bindings, specifying phy used by
>> port.
>> +       - phy-names: from the *Generic PHY* bindings, specifying name of
>> phy
>> +                    used by the port.
>
>
> I think you don't need this property for this binding, as the PHYs are being
> requested by indices (in fact only index 0 is used).

True, that phy-names property is not used, since PHYs are being
requested using indices.
We can remove this.

>
>
>>
>>   Example:
>>         usb@12120000 {
>> @@ -47,6 +56,16 @@ Example:
>>
>>                 clocks = <&clock 285>;
>>                 clock-names = "usbhost";
>> +
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               port@0 {
>> +                   reg = <0>;
>> +                   phys = <&usb2phy 1>;
>> +                   phy-names = "host";
>
>
> Ditto.
will remove this.

>
>
>> +                   status = "disabled";
>> +               };
>> +
>>         };
>>
>>   DWC3
>> diff --git a/drivers/usb/host/ohci-exynos.c
>> b/drivers/usb/host/ohci-exynos.c
>> index 05f00e3..f90bf9a 100644
>> --- a/drivers/usb/host/ohci-exynos.c
>> +++ b/drivers/usb/host/ohci-exynos.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>>   #include <linux/usb/phy.h>
>>   #include <linux/usb/samsung_usb_phy.h>
>>   #include <linux/usb.h>
>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>> exynos_ohci_hc_driver;
>>
>>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>> *)(hcd_to_ohci(hcd)->priv)
>>
>> +#define PHY_NUMBER 3
>
>
> nit: A blank line would be nice here to separate from the struct below.

Ok, will add one.

>
> By the way, doesn't the number of PHY ports depend on platform? Of course
> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
> interface, so it might be changed in further patches.

Yes, the number of PHY ports will be platform dependent feature.
In subsequent patches we can add support to count the number of PHYs,
or rather in this patch
itself, when we do -
         for_each_available_child_of_node(dev->of_node, child) {
we can keep a count of how many child nodes we found, and then
configure those many.
As such the host controller drivers will have 'host' and 'hsic' PHYs.

>
>
>>   struct exynos_ohci_hcd {
>>         struct clk *clk;
>>         struct usb_phy *phy;
>>         struct usb_otg *otg;
>> +       struct phy *phy_g[PHY_NUMBER];
>>   };
>>
>> -static void exynos_ohci_phy_enable(struct device *dev)
>> +static int exynos_ohci_get_phy(struct device *dev,
>> +                               struct exynos_ohci_hcd *exynos_ohci)
>> +{
>> +       struct device_node *child;
>> +       struct phy *phy;
>> +       int phy_number;
>> +       int ret = 0;
>> +
>> +       exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> +       if (IS_ERR(exynos_ohci->phy)) {
>> +               ret = PTR_ERR(exynos_ohci->phy);
>> +               /* This is the case when PHY config is disabled */
>> +               if (ret == -ENXIO || ret == -ENODEV) {
>> +                       dev_dbg(dev, "Failed to get usb2 phy\n");
>> +                       exynos_ohci->phy = NULL;
>
>
> I think you should keep this an ERR_PTR() and just use IS_ERR() check
> further in the driver instead of checking for NULL.

Yea, that's also one way to check for phy, i can modify this.

>
>> +                       ret = 0;
>
>
> Do you need to set ret to 0 here? The code for getting generic PHYs will
> either leave it unchanged when there are no port nodes defined or overwrite
> it with value returned by of_property_read_u32(). In the first case, an
> error code should be returned, not zero, as the driver was unable to get any
> PHY.

The idea was to not fail exynos_ohci_get_phy() call when the PHY
configs are not even enabled.
Since this would mean, that the driver will never be able to get a PHY
in future, and there will be no point in
failing the driver probe.

In a case when the host controller dts is still on older PHY bindings,
this 'ret' will *not* be over-wriiten, since the
generic phy nodes will not be there.
And in case the host dts is moved to the new generic PHY based
bindings. then the part of this function exynos_ohci_get_phy()
related to older usb-phy, shall not execute.

This is reason why i did not really add a fall-back mechanism for getting PHY.
Since from DT either of the two bindings be supplied, and then things
here will be handles almost independently.

>
>
>> +               } else if (ret == -EPROBE_DEFER) {
>
>
> I think you could merge this case with the else clause below, as most driver
> do. Moreover, since the only thing done after the fail_phy label is
> returning the error code, you could just immediately return from here. So
> the whole block of code would end up like this:
>
>         if (IS_ERR(exynos_ohci->phy)) {
>                 ret = PTR_ERR(exynos_ohci->phy);
>                 if (ret != -ENXIO && ret != -ENODEV) {
>
>                         dev_err(dev, "no usb2 phy configured\n");
>                         return ret;
>
>                 }
>                 dev_dbg(dev, "Failed to get usb2 phy\n");
>                 exynos_ohci->phy = NULL;
>         } else {
>
>                 exynos_ohci->otg = exynos_ohci->phy->otg;
>         }

Ok, this seems a good restructuring. I shall change this.

>
>> +                       goto fail_phy;
>>
>> +               } else {
>> +                       dev_err(dev, "no usb2 phy configured\n");
>> +                       goto fail_phy;
>> +               }
>> +       } else {
>> +               exynos_ohci->otg = exynos_ohci->phy->otg;
>> +       }
>> +
>> +       /* Getting generic phy:
>
>
> CodingStyle: Multi-line comments should begin and end with a single empty
> line:
>
>         /*
>          * Getting generic phy:
>          * ...
>          */
>
> also nit: s/phy/PHY/
>

Ok, will correct this.

>
>> +        * We are keeping both types of phys as a part of transiting OHCI
>> +        * to generic phy framework, so that in absence of supporting dts
>> +        * changes the functionality doesn't break.
>> +        * Once we move the ohci dt nodes to use new generic phys,
>> +        * we can remove support for older PHY in this driver.
>
>
> Well, this is not entirely true. The problem here is not caused by existing
> DTS files, but rather a chance that there are existing devices using DTB
> files built from them. So to remove the support for old bindings, we need to
> make sure that such devices have their DTBs updated to ones built from new
> DTS.

Fair enough, thanks for explaining. :-)
I shall modify this comment.

>
>
>> +        */
>> +       for_each_available_child_of_node(dev->of_node, child) {
>> +               ret = of_property_read_u32(child, "reg", &phy_number);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to parse device tree\n");
>> +                       of_node_put(child);
>> +                       goto fail_phy;
>
>
> Why not just return ret here?
>
>> +               }
>
>
> nit: Blank line here would be nice.

ok

>
>
>> +               if (phy_number >= PHY_NUMBER) {
>> +                       dev_err(dev, "Invalid number of PHYs\n");
>> +                       of_node_put(child);
>> +                       ret = -EINVAL;
>> +                       goto fail_phy;
>
>
> What about just return -EINVAL;

Yea, just another way of doing things. ;-)
I felt 'goto' to be a bit clean than adding number of returns in
between statements.

>
>> +               }
>
>
> nit: Here too.
yea, will add a blank line.

>
>> +               phy = devm_of_phy_get(dev, child, 0);
>> +               of_node_put(child);
>> +               if (IS_ERR(phy)) {
>> +                       ret = PTR_ERR(phy);
>> +                       /* This is the case when PHY config is disabled */
>> +                       if (ret == -ENOSYS || ret == -ENODEV) {
>> +                               dev_dbg(dev, "Failed to get usb2 phy\n");
>> +                               phy = NULL;
>>
>> +                               ret = 0;
>> +                       } else if (ret == -EPROBE_DEFER) {
>> +                               goto fail_phy;
>> +                       } else {
>> +                               dev_err(dev, "no usb2 phy configured\n");
>> +                               goto fail_phy;
>> +                       }
>> +               }
>
>
> Similar comments to this block apply as for the block getting legacy USB
> PHY.

Ok, i will restructure this same as 'legacy PHY  block'

>
>
>> +               exynos_ohci->phy_g[phy_number] = phy;
>> +       }
>> +
>> +fail_phy:
>> +       return ret;
>> +}
>> +
>> +static int exynos_ohci_phy_enable(struct device *dev)
>>   {
>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> +       int i;
>> +       int ret = 0;
>>
>> -       if (exynos_ohci->phy)
>> -               usb_phy_init(exynos_ohci->phy);
>> +       if (exynos_ohci->phy) {
>
>
> !IS_ERR() should be used to check for validity.

Ok, this with the earlier change of setting exynos_ohci->phy as
ERR_PTR(), should be good then.

>
>
>> +               ret = usb_phy_init(exynos_ohci->phy);
>> +               if (ret)
>> +                       return ret;
>
>
> IMHO a simple return usb_phy_init(...) could be used here, if we are using
> the legacy PHY interface.

Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
since those devices will not have the generic
PHYs.

>
>
>> +       }
>> +
>> +       for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>> +               if (exynos_ohci->phy_g[i])
>
>
> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
> with some error code, (-ENODEV) probably

Ok.

>
>
>> +                       ret = phy_power_on(exynos_ohci->phy_g[i]);
>> +       if (ret) {
>> +               for (i--; i >= 0; i--)
>> +                       if (exynos_ohci->phy_g[i])
>
>
> Ditto.
>
>
>> +                               phy_power_off(exynos_ohci->phy_g[i]);
>> +               if (exynos_ohci->phy)
>> +                       usb_phy_shutdown(exynos_ohci->phy);
>
>
> I don't think handling of legacy PHY is needed here. I don't think we can
> have both legacy and generic PHY used at the same time.

Right, we will not have both the PHYs existing at the same time.
And if we are doing a simple return usb_phy_init(...); then ofcourse
we will not need to do this.

>
>
>> +       }
>> +
>> +       return ret;
>>   }
>>
>>   static void exynos_ohci_phy_disable(struct device *dev)
>>   {
>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> +       int i;
>>
>>         if (exynos_ohci->phy)
>>                 usb_phy_shutdown(exynos_ohci->phy);
>> +
>> +       for (i = 0; i < PHY_NUMBER; i++)
>> +               if (exynos_ohci->phy_g[i])
>
>
> !IS_ERR()

Yes.



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

[PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

Hi Tomasz,


On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Vivek,
>
> Please see my comments inline.

Thanks for the review, please find my answers inline.

>
>
> On 30.04.2014 07:19, Vivek Gautam wrote:
>>
>> Add support to consume phy provided by Generic phy framework.
>> Keeping the support for older usb-phy intact right now, in order
>> to prevent any functionality break in absence of relevant
>> device tree side change for ohci-exynos.
>> Once we move to new phy in the device nodes for ohci, we can
>> remove the support for older phys.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> ---
>>
>> Changes from v3:
>>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>   - Made exynos_ohci_phy_disable() return void, since its return value
>>     did not serve any purpose.
>>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>     exynos_ohci_phy_enable() is failed.
>>
>>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>>   drivers/usb/host/ohci-exynos.c                     |  128
>> +++++++++++++++++---
>>   2 files changed, 132 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> index d967ba1..a90c973 100644
>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>> @@ -38,6 +38,15 @@ Required properties:
>>    - interrupts: interrupt number to the cpu.
>>    - clocks: from common clock binding: handle to usb clock.
>>    - clock-names: from common clock binding: Shall be "usbhost".
>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>> +   One phy per port. Each port should have following entries:
>> +       - reg: port number on OHCI controller, e.g
>> +              On Exynos5250, port 0 is USB2.0 otg phy
>> +                             port 1 is HSIC phy0
>> +                             port 2 is HSIC phy1
>> +       - phys: from the *Generic PHY* bindings, specifying phy used by
>> port.
>> +       - phy-names: from the *Generic PHY* bindings, specifying name of
>> phy
>> +                    used by the port.
>
>
> I think you don't need this property for this binding, as the PHYs are being
> requested by indices (in fact only index 0 is used).

True, that phy-names property is not used, since PHYs are being
requested using indices.
We can remove this.

>
>
>>
>>   Example:
>>         usb at 12120000 {
>> @@ -47,6 +56,16 @@ Example:
>>
>>                 clocks = <&clock 285>;
>>                 clock-names = "usbhost";
>> +
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               port at 0 {
>> +                   reg = <0>;
>> +                   phys = <&usb2phy 1>;
>> +                   phy-names = "host";
>
>
> Ditto.
will remove this.

>
>
>> +                   status = "disabled";
>> +               };
>> +
>>         };
>>
>>   DWC3
>> diff --git a/drivers/usb/host/ohci-exynos.c
>> b/drivers/usb/host/ohci-exynos.c
>> index 05f00e3..f90bf9a 100644
>> --- a/drivers/usb/host/ohci-exynos.c
>> +++ b/drivers/usb/host/ohci-exynos.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>>   #include <linux/usb/phy.h>
>>   #include <linux/usb/samsung_usb_phy.h>
>>   #include <linux/usb.h>
>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>> exynos_ohci_hc_driver;
>>
>>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>> *)(hcd_to_ohci(hcd)->priv)
>>
>> +#define PHY_NUMBER 3
>
>
> nit: A blank line would be nice here to separate from the struct below.

Ok, will add one.

>
> By the way, doesn't the number of PHY ports depend on platform? Of course
> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
> interface, so it might be changed in further patches.

Yes, the number of PHY ports will be platform dependent feature.
In subsequent patches we can add support to count the number of PHYs,
or rather in this patch
itself, when we do -
         for_each_available_child_of_node(dev->of_node, child) {
we can keep a count of how many child nodes we found, and then
configure those many.
As such the host controller drivers will have 'host' and 'hsic' PHYs.

>
>
>>   struct exynos_ohci_hcd {
>>         struct clk *clk;
>>         struct usb_phy *phy;
>>         struct usb_otg *otg;
>> +       struct phy *phy_g[PHY_NUMBER];
>>   };
>>
>> -static void exynos_ohci_phy_enable(struct device *dev)
>> +static int exynos_ohci_get_phy(struct device *dev,
>> +                               struct exynos_ohci_hcd *exynos_ohci)
>> +{
>> +       struct device_node *child;
>> +       struct phy *phy;
>> +       int phy_number;
>> +       int ret = 0;
>> +
>> +       exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> +       if (IS_ERR(exynos_ohci->phy)) {
>> +               ret = PTR_ERR(exynos_ohci->phy);
>> +               /* This is the case when PHY config is disabled */
>> +               if (ret == -ENXIO || ret == -ENODEV) {
>> +                       dev_dbg(dev, "Failed to get usb2 phy\n");
>> +                       exynos_ohci->phy = NULL;
>
>
> I think you should keep this an ERR_PTR() and just use IS_ERR() check
> further in the driver instead of checking for NULL.

Yea, that's also one way to check for phy, i can modify this.

>
>> +                       ret = 0;
>
>
> Do you need to set ret to 0 here? The code for getting generic PHYs will
> either leave it unchanged when there are no port nodes defined or overwrite
> it with value returned by of_property_read_u32(). In the first case, an
> error code should be returned, not zero, as the driver was unable to get any
> PHY.

The idea was to not fail exynos_ohci_get_phy() call when the PHY
configs are not even enabled.
Since this would mean, that the driver will never be able to get a PHY
in future, and there will be no point in
failing the driver probe.

In a case when the host controller dts is still on older PHY bindings,
this 'ret' will *not* be over-wriiten, since the
generic phy nodes will not be there.
And in case the host dts is moved to the new generic PHY based
bindings. then the part of this function exynos_ohci_get_phy()
related to older usb-phy, shall not execute.

This is reason why i did not really add a fall-back mechanism for getting PHY.
Since from DT either of the two bindings be supplied, and then things
here will be handles almost independently.

>
>
>> +               } else if (ret == -EPROBE_DEFER) {
>
>
> I think you could merge this case with the else clause below, as most driver
> do. Moreover, since the only thing done after the fail_phy label is
> returning the error code, you could just immediately return from here. So
> the whole block of code would end up like this:
>
>         if (IS_ERR(exynos_ohci->phy)) {
>                 ret = PTR_ERR(exynos_ohci->phy);
>                 if (ret != -ENXIO && ret != -ENODEV) {
>
>                         dev_err(dev, "no usb2 phy configured\n");
>                         return ret;
>
>                 }
>                 dev_dbg(dev, "Failed to get usb2 phy\n");
>                 exynos_ohci->phy = NULL;
>         } else {
>
>                 exynos_ohci->otg = exynos_ohci->phy->otg;
>         }

Ok, this seems a good restructuring. I shall change this.

>
>> +                       goto fail_phy;
>>
>> +               } else {
>> +                       dev_err(dev, "no usb2 phy configured\n");
>> +                       goto fail_phy;
>> +               }
>> +       } else {
>> +               exynos_ohci->otg = exynos_ohci->phy->otg;
>> +       }
>> +
>> +       /* Getting generic phy:
>
>
> CodingStyle: Multi-line comments should begin and end with a single empty
> line:
>
>         /*
>          * Getting generic phy:
>          * ...
>          */
>
> also nit: s/phy/PHY/
>

Ok, will correct this.

>
>> +        * We are keeping both types of phys as a part of transiting OHCI
>> +        * to generic phy framework, so that in absence of supporting dts
>> +        * changes the functionality doesn't break.
>> +        * Once we move the ohci dt nodes to use new generic phys,
>> +        * we can remove support for older PHY in this driver.
>
>
> Well, this is not entirely true. The problem here is not caused by existing
> DTS files, but rather a chance that there are existing devices using DTB
> files built from them. So to remove the support for old bindings, we need to
> make sure that such devices have their DTBs updated to ones built from new
> DTS.

Fair enough, thanks for explaining. :-)
I shall modify this comment.

>
>
>> +        */
>> +       for_each_available_child_of_node(dev->of_node, child) {
>> +               ret = of_property_read_u32(child, "reg", &phy_number);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to parse device tree\n");
>> +                       of_node_put(child);
>> +                       goto fail_phy;
>
>
> Why not just return ret here?
>
>> +               }
>
>
> nit: Blank line here would be nice.

ok

>
>
>> +               if (phy_number >= PHY_NUMBER) {
>> +                       dev_err(dev, "Invalid number of PHYs\n");
>> +                       of_node_put(child);
>> +                       ret = -EINVAL;
>> +                       goto fail_phy;
>
>
> What about just return -EINVAL;

Yea, just another way of doing things. ;-)
I felt 'goto' to be a bit clean than adding number of returns in
between statements.

>
>> +               }
>
>
> nit: Here too.
yea, will add a blank line.

>
>> +               phy = devm_of_phy_get(dev, child, 0);
>> +               of_node_put(child);
>> +               if (IS_ERR(phy)) {
>> +                       ret = PTR_ERR(phy);
>> +                       /* This is the case when PHY config is disabled */
>> +                       if (ret == -ENOSYS || ret == -ENODEV) {
>> +                               dev_dbg(dev, "Failed to get usb2 phy\n");
>> +                               phy = NULL;
>>
>> +                               ret = 0;
>> +                       } else if (ret == -EPROBE_DEFER) {
>> +                               goto fail_phy;
>> +                       } else {
>> +                               dev_err(dev, "no usb2 phy configured\n");
>> +                               goto fail_phy;
>> +                       }
>> +               }
>
>
> Similar comments to this block apply as for the block getting legacy USB
> PHY.

Ok, i will restructure this same as 'legacy PHY  block'

>
>
>> +               exynos_ohci->phy_g[phy_number] = phy;
>> +       }
>> +
>> +fail_phy:
>> +       return ret;
>> +}
>> +
>> +static int exynos_ohci_phy_enable(struct device *dev)
>>   {
>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> +       int i;
>> +       int ret = 0;
>>
>> -       if (exynos_ohci->phy)
>> -               usb_phy_init(exynos_ohci->phy);
>> +       if (exynos_ohci->phy) {
>
>
> !IS_ERR() should be used to check for validity.

Ok, this with the earlier change of setting exynos_ohci->phy as
ERR_PTR(), should be good then.

>
>
>> +               ret = usb_phy_init(exynos_ohci->phy);
>> +               if (ret)
>> +                       return ret;
>
>
> IMHO a simple return usb_phy_init(...) could be used here, if we are using
> the legacy PHY interface.

Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
since those devices will not have the generic
PHYs.

>
>
>> +       }
>> +
>> +       for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>> +               if (exynos_ohci->phy_g[i])
>
>
> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
> with some error code, (-ENODEV) probably

Ok.

>
>
>> +                       ret = phy_power_on(exynos_ohci->phy_g[i]);
>> +       if (ret) {
>> +               for (i--; i >= 0; i--)
>> +                       if (exynos_ohci->phy_g[i])
>
>
> Ditto.
>
>
>> +                               phy_power_off(exynos_ohci->phy_g[i]);
>> +               if (exynos_ohci->phy)
>> +                       usb_phy_shutdown(exynos_ohci->phy);
>
>
> I don't think handling of legacy PHY is needed here. I don't think we can
> have both legacy and generic PHY used at the same time.

Right, we will not have both the PHYs existing at the same time.
And if we are doing a simple return usb_phy_init(...); then ofcourse
we will not need to do this.

>
>
>> +       }
>> +
>> +       return ret;
>>   }
>>
>>   static void exynos_ohci_phy_disable(struct device *dev)
>>   {
>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>> +       int i;
>>
>>         if (exynos_ohci->phy)
>>                 usb_phy_shutdown(exynos_ohci->phy);
>> +
>> +       for (i = 0; i < PHY_NUMBER; i++)
>> +               if (exynos_ohci->phy_g[i])
>
>
> !IS_ERR()

Yes.



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

On Fri, May 2, 2014 at 2:55 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> Hi Tomasz,
>
>
> On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Vivek,
>>
>> Please see my comments inline.
>
> Thanks for the review, please find my answers inline.
>
>>
>>
>> On 30.04.2014 07:19, Vivek Gautam wrote:
>>>
>>> Add support to consume phy provided by Generic phy framework.
>>> Keeping the support for older usb-phy intact right now, in order
>>> to prevent any functionality break in absence of relevant
>>> device tree side change for ohci-exynos.
>>> Once we move to new phy in the device nodes for ohci, we can
>>> remove the support for older phys.
>>>
>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>> Cc: Jingoo Han <jg1.han@samsung.com>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> ---
>>>
>>> Changes from v3:
>>>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>>   - Made exynos_ohci_phy_disable() return void, since its return value
>>>     did not serve any purpose.
>>>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>>     exynos_ohci_phy_enable() is failed.
>>>
>>>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>>>   drivers/usb/host/ohci-exynos.c                     |  128
>>> +++++++++++++++++---
>>>   2 files changed, 132 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> index d967ba1..a90c973 100644
>>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> @@ -38,6 +38,15 @@ Required properties:
>>>    - interrupts: interrupt number to the cpu.
>>>    - clocks: from common clock binding: handle to usb clock.
>>>    - clock-names: from common clock binding: Shall be "usbhost".
>>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>>> +   One phy per port. Each port should have following entries:
>>> +       - reg: port number on OHCI controller, e.g
>>> +              On Exynos5250, port 0 is USB2.0 otg phy
>>> +                             port 1 is HSIC phy0
>>> +                             port 2 is HSIC phy1
>>> +       - phys: from the *Generic PHY* bindings, specifying phy used by
>>> port.
>>> +       - phy-names: from the *Generic PHY* bindings, specifying name of
>>> phy
>>> +                    used by the port.
>>
>>
>> I think you don't need this property for this binding, as the PHYs are being
>> requested by indices (in fact only index 0 is used).
>
> True, that phy-names property is not used, since PHYs are being
> requested using indices.
> We can remove this.
>
>>
>>
>>>
>>>   Example:
>>>         usb@12120000 {
>>> @@ -47,6 +56,16 @@ Example:
>>>
>>>                 clocks = <&clock 285>;
>>>                 clock-names = "usbhost";
>>> +
>>> +               #address-cells = <1>;
>>> +               #size-cells = <0>;
>>> +               port@0 {
>>> +                   reg = <0>;
>>> +                   phys = <&usb2phy 1>;
>>> +                   phy-names = "host";
>>
>>
>> Ditto.
> will remove this.
>
>>
>>
>>> +                   status = "disabled";
>>> +               };
>>> +
>>>         };
>>>
>>>   DWC3
>>> diff --git a/drivers/usb/host/ohci-exynos.c
>>> b/drivers/usb/host/ohci-exynos.c
>>> index 05f00e3..f90bf9a 100644
>>> --- a/drivers/usb/host/ohci-exynos.c
>>> +++ b/drivers/usb/host/ohci-exynos.c
>>> @@ -18,6 +18,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/of.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/phy/phy.h>
>>>   #include <linux/usb/phy.h>
>>>   #include <linux/usb/samsung_usb_phy.h>
>>>   #include <linux/usb.h>
>>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>>> exynos_ohci_hc_driver;
>>>
>>>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>>> *)(hcd_to_ohci(hcd)->priv)
>>>
>>> +#define PHY_NUMBER 3
>>
>>
>> nit: A blank line would be nice here to separate from the struct below.
>
> Ok, will add one.
>
>>
>> By the way, doesn't the number of PHY ports depend on platform? Of course
>> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
>> interface, so it might be changed in further patches.
>
> Yes, the number of PHY ports will be platform dependent feature.
> In subsequent patches we can add support to count the number of PHYs,
> or rather in this patch
> itself, when we do -
>          for_each_available_child_of_node(dev->of_node, child) {
> we can keep a count of how many child nodes we found, and then
> configure those many.
> As such the host controller drivers will have 'host' and 'hsic' PHYs.
>
>>
>>
>>>   struct exynos_ohci_hcd {
>>>         struct clk *clk;
>>>         struct usb_phy *phy;
>>>         struct usb_otg *otg;
>>> +       struct phy *phy_g[PHY_NUMBER];
>>>   };
>>>
>>> -static void exynos_ohci_phy_enable(struct device *dev)
>>> +static int exynos_ohci_get_phy(struct device *dev,
>>> +                               struct exynos_ohci_hcd *exynos_ohci)
>>> +{
>>> +       struct device_node *child;
>>> +       struct phy *phy;
>>> +       int phy_number;
>>> +       int ret = 0;
>>> +
>>> +       exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>>> +       if (IS_ERR(exynos_ohci->phy)) {
>>> +               ret = PTR_ERR(exynos_ohci->phy);
>>> +               /* This is the case when PHY config is disabled */
>>> +               if (ret == -ENXIO || ret == -ENODEV) {
>>> +                       dev_dbg(dev, "Failed to get usb2 phy\n");
>>> +                       exynos_ohci->phy = NULL;
>>
>>
>> I think you should keep this an ERR_PTR() and just use IS_ERR() check
>> further in the driver instead of checking for NULL.
>
> Yea, that's also one way to check for phy, i can modify this.
>
>>
>>> +                       ret = 0;
>>
>>
>> Do you need to set ret to 0 here? The code for getting generic PHYs will
>> either leave it unchanged when there are no port nodes defined or overwrite
>> it with value returned by of_property_read_u32(). In the first case, an
>> error code should be returned, not zero, as the driver was unable to get any
>> PHY.
>
> The idea was to not fail exynos_ohci_get_phy() call when the PHY
> configs are not even enabled.
> Since this would mean, that the driver will never be able to get a PHY
> in future, and there will be no point in
> failing the driver probe.
>
> In a case when the host controller dts is still on older PHY bindings,
> this 'ret' will *not* be over-wriiten, since the
> generic phy nodes will not be there.
> And in case the host dts is moved to the new generic PHY based
> bindings. then the part of this function exynos_ohci_get_phy()
> related to older usb-phy, shall not execute.
>
> This is reason why i did not really add a fall-back mechanism for getting PHY.
> Since from DT either of the two bindings be supplied, and then things
> here will be handles almost independently.
>
>>
>>
>>> +               } else if (ret == -EPROBE_DEFER) {
>>
>>
>> I think you could merge this case with the else clause below, as most driver
>> do. Moreover, since the only thing done after the fail_phy label is
>> returning the error code, you could just immediately return from here. So
>> the whole block of code would end up like this:
>>
>>         if (IS_ERR(exynos_ohci->phy)) {
>>                 ret = PTR_ERR(exynos_ohci->phy);
>>                 if (ret != -ENXIO && ret != -ENODEV) {
>>
>>                         dev_err(dev, "no usb2 phy configured\n");
>>                         return ret;
>>
>>                 }
>>                 dev_dbg(dev, "Failed to get usb2 phy\n");
>>                 exynos_ohci->phy = NULL;
>>         } else {
>>
>>                 exynos_ohci->otg = exynos_ohci->phy->otg;
>>         }
>
> Ok, this seems a good restructuring. I shall change this.
>
>>
>>> +                       goto fail_phy;
>>>
>>> +               } else {
>>> +                       dev_err(dev, "no usb2 phy configured\n");
>>> +                       goto fail_phy;
>>> +               }
>>> +       } else {
>>> +               exynos_ohci->otg = exynos_ohci->phy->otg;
>>> +       }
>>> +
>>> +       /* Getting generic phy:
>>
>>
>> CodingStyle: Multi-line comments should begin and end with a single empty
>> line:
>>
>>         /*
>>          * Getting generic phy:
>>          * ...
>>          */
>>
>> also nit: s/phy/PHY/
>>
>
> Ok, will correct this.
>
>>
>>> +        * We are keeping both types of phys as a part of transiting OHCI
>>> +        * to generic phy framework, so that in absence of supporting dts
>>> +        * changes the functionality doesn't break.
>>> +        * Once we move the ohci dt nodes to use new generic phys,
>>> +        * we can remove support for older PHY in this driver.
>>
>>
>> Well, this is not entirely true. The problem here is not caused by existing
>> DTS files, but rather a chance that there are existing devices using DTB
>> files built from them. So to remove the support for old bindings, we need to
>> make sure that such devices have their DTBs updated to ones built from new
>> DTS.
>
> Fair enough, thanks for explaining. :-)
> I shall modify this comment.
>
>>
>>
>>> +        */
>>> +       for_each_available_child_of_node(dev->of_node, child) {
>>> +               ret = of_property_read_u32(child, "reg", &phy_number);
>>> +               if (ret) {
>>> +                       dev_err(dev, "Failed to parse device tree\n");
>>> +                       of_node_put(child);
>>> +                       goto fail_phy;
>>
>>
>> Why not just return ret here?
>>
>>> +               }
>>
>>
>> nit: Blank line here would be nice.
>
> ok
>
>>
>>
>>> +               if (phy_number >= PHY_NUMBER) {
>>> +                       dev_err(dev, "Invalid number of PHYs\n");
>>> +                       of_node_put(child);
>>> +                       ret = -EINVAL;
>>> +                       goto fail_phy;
>>
>>
>> What about just return -EINVAL;
>
> Yea, just another way of doing things. ;-)
> I felt 'goto' to be a bit clean than adding number of returns in
> between statements.
>
>>
>>> +               }
>>
>>
>> nit: Here too.
> yea, will add a blank line.
>
>>
>>> +               phy = devm_of_phy_get(dev, child, 0);
>>> +               of_node_put(child);
>>> +               if (IS_ERR(phy)) {
>>> +                       ret = PTR_ERR(phy);
>>> +                       /* This is the case when PHY config is disabled */
>>> +                       if (ret == -ENOSYS || ret == -ENODEV) {
>>> +                               dev_dbg(dev, "Failed to get usb2 phy\n");
>>> +                               phy = NULL;
>>>
>>> +                               ret = 0;
>>> +                       } else if (ret == -EPROBE_DEFER) {
>>> +                               goto fail_phy;
>>> +                       } else {
>>> +                               dev_err(dev, "no usb2 phy configured\n");
>>> +                               goto fail_phy;
>>> +                       }
>>> +               }
>>
>>
>> Similar comments to this block apply as for the block getting legacy USB
>> PHY.
>
> Ok, i will restructure this same as 'legacy PHY  block'
>
>>
>>
>>> +               exynos_ohci->phy_g[phy_number] = phy;
>>> +       }
>>> +
>>> +fail_phy:
>>> +       return ret;
>>> +}
>>> +
>>> +static int exynos_ohci_phy_enable(struct device *dev)
>>>   {
>>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>>> +       int i;
>>> +       int ret = 0;
>>>
>>> -       if (exynos_ohci->phy)
>>> -               usb_phy_init(exynos_ohci->phy);
>>> +       if (exynos_ohci->phy) {
>>
>>
>> !IS_ERR() should be used to check for validity.
>
> Ok, this with the earlier change of setting exynos_ohci->phy as
> ERR_PTR(), should be good then.
>
>>
>>
>>> +               ret = usb_phy_init(exynos_ohci->phy);
>>> +               if (ret)
>>> +                       return ret;
>>
>>
>> IMHO a simple return usb_phy_init(...) could be used here, if we are using
>> the legacy PHY interface.
>
> Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
> since those devices will not have the generic
> PHYs.
>
>>
>>
>>> +       }
>>> +
>>> +       for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>>> +               if (exynos_ohci->phy_g[i])
>>
>>
>> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
>> with some error code, (-ENODEV) probably
>
> Ok.

I think we don't need to initialize this array to any ERR_PTR(), since
devm_of_phy_get()
will anyways assign an ERR_PTR() to phy in unsuccessful case, and a
valid phy pointer
when it successfully gets one.
Isn't it ?

I think the same goes with legacy usb-phy function devm_usb_get_phy().


[snip]


-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

On Fri, May 2, 2014 at 2:55 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> Hi Tomasz,
>
>
> On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Vivek,
>>
>> Please see my comments inline.
>
> Thanks for the review, please find my answers inline.
>
>>
>>
>> On 30.04.2014 07:19, Vivek Gautam wrote:
>>>
>>> Add support to consume phy provided by Generic phy framework.
>>> Keeping the support for older usb-phy intact right now, in order
>>> to prevent any functionality break in absence of relevant
>>> device tree side change for ohci-exynos.
>>> Once we move to new phy in the device nodes for ohci, we can
>>> remove the support for older phys.
>>>
>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>> Cc: Jingoo Han <jg1.han@samsung.com>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> ---
>>>
>>> Changes from v3:
>>>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>>   - Made exynos_ohci_phy_disable() return void, since its return value
>>>     did not serve any purpose.
>>>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>>     exynos_ohci_phy_enable() is failed.
>>>
>>>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>>>   drivers/usb/host/ohci-exynos.c                     |  128
>>> +++++++++++++++++---
>>>   2 files changed, 132 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> index d967ba1..a90c973 100644
>>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> @@ -38,6 +38,15 @@ Required properties:
>>>    - interrupts: interrupt number to the cpu.
>>>    - clocks: from common clock binding: handle to usb clock.
>>>    - clock-names: from common clock binding: Shall be "usbhost".
>>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>>> +   One phy per port. Each port should have following entries:
>>> +       - reg: port number on OHCI controller, e.g
>>> +              On Exynos5250, port 0 is USB2.0 otg phy
>>> +                             port 1 is HSIC phy0
>>> +                             port 2 is HSIC phy1
>>> +       - phys: from the *Generic PHY* bindings, specifying phy used by
>>> port.
>>> +       - phy-names: from the *Generic PHY* bindings, specifying name of
>>> phy
>>> +                    used by the port.
>>
>>
>> I think you don't need this property for this binding, as the PHYs are being
>> requested by indices (in fact only index 0 is used).
>
> True, that phy-names property is not used, since PHYs are being
> requested using indices.
> We can remove this.
>
>>
>>
>>>
>>>   Example:
>>>         usb@12120000 {
>>> @@ -47,6 +56,16 @@ Example:
>>>
>>>                 clocks = <&clock 285>;
>>>                 clock-names = "usbhost";
>>> +
>>> +               #address-cells = <1>;
>>> +               #size-cells = <0>;
>>> +               port@0 {
>>> +                   reg = <0>;
>>> +                   phys = <&usb2phy 1>;
>>> +                   phy-names = "host";
>>
>>
>> Ditto.
> will remove this.
>
>>
>>
>>> +                   status = "disabled";
>>> +               };
>>> +
>>>         };
>>>
>>>   DWC3
>>> diff --git a/drivers/usb/host/ohci-exynos.c
>>> b/drivers/usb/host/ohci-exynos.c
>>> index 05f00e3..f90bf9a 100644
>>> --- a/drivers/usb/host/ohci-exynos.c
>>> +++ b/drivers/usb/host/ohci-exynos.c
>>> @@ -18,6 +18,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/of.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/phy/phy.h>
>>>   #include <linux/usb/phy.h>
>>>   #include <linux/usb/samsung_usb_phy.h>
>>>   #include <linux/usb.h>
>>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>>> exynos_ohci_hc_driver;
>>>
>>>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>>> *)(hcd_to_ohci(hcd)->priv)
>>>
>>> +#define PHY_NUMBER 3
>>
>>
>> nit: A blank line would be nice here to separate from the struct below.
>
> Ok, will add one.
>
>>
>> By the way, doesn't the number of PHY ports depend on platform? Of course
>> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
>> interface, so it might be changed in further patches.
>
> Yes, the number of PHY ports will be platform dependent feature.
> In subsequent patches we can add support to count the number of PHYs,
> or rather in this patch
> itself, when we do -
>          for_each_available_child_of_node(dev->of_node, child) {
> we can keep a count of how many child nodes we found, and then
> configure those many.
> As such the host controller drivers will have 'host' and 'hsic' PHYs.
>
>>
>>
>>>   struct exynos_ohci_hcd {
>>>         struct clk *clk;
>>>         struct usb_phy *phy;
>>>         struct usb_otg *otg;
>>> +       struct phy *phy_g[PHY_NUMBER];
>>>   };
>>>
>>> -static void exynos_ohci_phy_enable(struct device *dev)
>>> +static int exynos_ohci_get_phy(struct device *dev,
>>> +                               struct exynos_ohci_hcd *exynos_ohci)
>>> +{
>>> +       struct device_node *child;
>>> +       struct phy *phy;
>>> +       int phy_number;
>>> +       int ret = 0;
>>> +
>>> +       exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>>> +       if (IS_ERR(exynos_ohci->phy)) {
>>> +               ret = PTR_ERR(exynos_ohci->phy);
>>> +               /* This is the case when PHY config is disabled */
>>> +               if (ret == -ENXIO || ret == -ENODEV) {
>>> +                       dev_dbg(dev, "Failed to get usb2 phy\n");
>>> +                       exynos_ohci->phy = NULL;
>>
>>
>> I think you should keep this an ERR_PTR() and just use IS_ERR() check
>> further in the driver instead of checking for NULL.
>
> Yea, that's also one way to check for phy, i can modify this.
>
>>
>>> +                       ret = 0;
>>
>>
>> Do you need to set ret to 0 here? The code for getting generic PHYs will
>> either leave it unchanged when there are no port nodes defined or overwrite
>> it with value returned by of_property_read_u32(). In the first case, an
>> error code should be returned, not zero, as the driver was unable to get any
>> PHY.
>
> The idea was to not fail exynos_ohci_get_phy() call when the PHY
> configs are not even enabled.
> Since this would mean, that the driver will never be able to get a PHY
> in future, and there will be no point in
> failing the driver probe.
>
> In a case when the host controller dts is still on older PHY bindings,
> this 'ret' will *not* be over-wriiten, since the
> generic phy nodes will not be there.
> And in case the host dts is moved to the new generic PHY based
> bindings. then the part of this function exynos_ohci_get_phy()
> related to older usb-phy, shall not execute.
>
> This is reason why i did not really add a fall-back mechanism for getting PHY.
> Since from DT either of the two bindings be supplied, and then things
> here will be handles almost independently.
>
>>
>>
>>> +               } else if (ret == -EPROBE_DEFER) {
>>
>>
>> I think you could merge this case with the else clause below, as most driver
>> do. Moreover, since the only thing done after the fail_phy label is
>> returning the error code, you could just immediately return from here. So
>> the whole block of code would end up like this:
>>
>>         if (IS_ERR(exynos_ohci->phy)) {
>>                 ret = PTR_ERR(exynos_ohci->phy);
>>                 if (ret != -ENXIO && ret != -ENODEV) {
>>
>>                         dev_err(dev, "no usb2 phy configured\n");
>>                         return ret;
>>
>>                 }
>>                 dev_dbg(dev, "Failed to get usb2 phy\n");
>>                 exynos_ohci->phy = NULL;
>>         } else {
>>
>>                 exynos_ohci->otg = exynos_ohci->phy->otg;
>>         }
>
> Ok, this seems a good restructuring. I shall change this.
>
>>
>>> +                       goto fail_phy;
>>>
>>> +               } else {
>>> +                       dev_err(dev, "no usb2 phy configured\n");
>>> +                       goto fail_phy;
>>> +               }
>>> +       } else {
>>> +               exynos_ohci->otg = exynos_ohci->phy->otg;
>>> +       }
>>> +
>>> +       /* Getting generic phy:
>>
>>
>> CodingStyle: Multi-line comments should begin and end with a single empty
>> line:
>>
>>         /*
>>          * Getting generic phy:
>>          * ...
>>          */
>>
>> also nit: s/phy/PHY/
>>
>
> Ok, will correct this.
>
>>
>>> +        * We are keeping both types of phys as a part of transiting OHCI
>>> +        * to generic phy framework, so that in absence of supporting dts
>>> +        * changes the functionality doesn't break.
>>> +        * Once we move the ohci dt nodes to use new generic phys,
>>> +        * we can remove support for older PHY in this driver.
>>
>>
>> Well, this is not entirely true. The problem here is not caused by existing
>> DTS files, but rather a chance that there are existing devices using DTB
>> files built from them. So to remove the support for old bindings, we need to
>> make sure that such devices have their DTBs updated to ones built from new
>> DTS.
>
> Fair enough, thanks for explaining. :-)
> I shall modify this comment.
>
>>
>>
>>> +        */
>>> +       for_each_available_child_of_node(dev->of_node, child) {
>>> +               ret = of_property_read_u32(child, "reg", &phy_number);
>>> +               if (ret) {
>>> +                       dev_err(dev, "Failed to parse device tree\n");
>>> +                       of_node_put(child);
>>> +                       goto fail_phy;
>>
>>
>> Why not just return ret here?
>>
>>> +               }
>>
>>
>> nit: Blank line here would be nice.
>
> ok
>
>>
>>
>>> +               if (phy_number >= PHY_NUMBER) {
>>> +                       dev_err(dev, "Invalid number of PHYs\n");
>>> +                       of_node_put(child);
>>> +                       ret = -EINVAL;
>>> +                       goto fail_phy;
>>
>>
>> What about just return -EINVAL;
>
> Yea, just another way of doing things. ;-)
> I felt 'goto' to be a bit clean than adding number of returns in
> between statements.
>
>>
>>> +               }
>>
>>
>> nit: Here too.
> yea, will add a blank line.
>
>>
>>> +               phy = devm_of_phy_get(dev, child, 0);
>>> +               of_node_put(child);
>>> +               if (IS_ERR(phy)) {
>>> +                       ret = PTR_ERR(phy);
>>> +                       /* This is the case when PHY config is disabled */
>>> +                       if (ret == -ENOSYS || ret == -ENODEV) {
>>> +                               dev_dbg(dev, "Failed to get usb2 phy\n");
>>> +                               phy = NULL;
>>>
>>> +                               ret = 0;
>>> +                       } else if (ret == -EPROBE_DEFER) {
>>> +                               goto fail_phy;
>>> +                       } else {
>>> +                               dev_err(dev, "no usb2 phy configured\n");
>>> +                               goto fail_phy;
>>> +                       }
>>> +               }
>>
>>
>> Similar comments to this block apply as for the block getting legacy USB
>> PHY.
>
> Ok, i will restructure this same as 'legacy PHY  block'
>
>>
>>
>>> +               exynos_ohci->phy_g[phy_number] = phy;
>>> +       }
>>> +
>>> +fail_phy:
>>> +       return ret;
>>> +}
>>> +
>>> +static int exynos_ohci_phy_enable(struct device *dev)
>>>   {
>>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>>> +       int i;
>>> +       int ret = 0;
>>>
>>> -       if (exynos_ohci->phy)
>>> -               usb_phy_init(exynos_ohci->phy);
>>> +       if (exynos_ohci->phy) {
>>
>>
>> !IS_ERR() should be used to check for validity.
>
> Ok, this with the earlier change of setting exynos_ohci->phy as
> ERR_PTR(), should be good then.
>
>>
>>
>>> +               ret = usb_phy_init(exynos_ohci->phy);
>>> +               if (ret)
>>> +                       return ret;
>>
>>
>> IMHO a simple return usb_phy_init(...) could be used here, if we are using
>> the legacy PHY interface.
>
> Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
> since those devices will not have the generic
> PHYs.
>
>>
>>
>>> +       }
>>> +
>>> +       for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>>> +               if (exynos_ohci->phy_g[i])
>>
>>
>> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
>> with some error code, (-ENODEV) probably
>
> Ok.

I think we don't need to initialize this array to any ERR_PTR(), since
devm_of_phy_get()
will anyways assign an ERR_PTR() to phy in unsuccessful case, and a
valid phy pointer
when it successfully gets one.
Isn't it ?

I think the same goes with legacy usb-phy function devm_usb_get_phy().


[snip]


-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

[PATCH v4 3/4] usb: ohci-exynos: Add facility to use phy provided by the generic phy framework

[Vivek Gautam]

On Fri, May 2, 2014 at 2:55 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> Hi Tomasz,
>
>
> On Thu, May 1, 2014 at 10:46 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Vivek,
>>
>> Please see my comments inline.
>
> Thanks for the review, please find my answers inline.
>
>>
>>
>> On 30.04.2014 07:19, Vivek Gautam wrote:
>>>
>>> Add support to consume phy provided by Generic phy framework.
>>> Keeping the support for older usb-phy intact right now, in order
>>> to prevent any functionality break in absence of relevant
>>> device tree side change for ohci-exynos.
>>> Once we move to new phy in the device nodes for ohci, we can
>>> remove the support for older phys.
>>>
>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>> Cc: Jingoo Han <jg1.han@samsung.com>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> ---
>>>
>>> Changes from v3:
>>>   - Calling usb_phy_shutdown() when exynos_ohci_phy_enable() is failing.
>>>   - Made exynos_ohci_phy_disable() return void, since its return value
>>>     did not serve any purpose.
>>>   - Calling clk_disable_unprepare() in exynos_ohci_resume() when
>>>     exynos_ohci_phy_enable() is failed.
>>>
>>>   .../devicetree/bindings/usb/exynos-usb.txt         |   19 +++
>>>   drivers/usb/host/ohci-exynos.c                     |  128
>>> +++++++++++++++++---
>>>   2 files changed, 132 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> index d967ba1..a90c973 100644
>>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
>>> @@ -38,6 +38,15 @@ Required properties:
>>>    - interrupts: interrupt number to the cpu.
>>>    - clocks: from common clock binding: handle to usb clock.
>>>    - clock-names: from common clock binding: Shall be "usbhost".
>>> + - port: if in the SoC there are OHCI phys, they should be listed here.
>>> +   One phy per port. Each port should have following entries:
>>> +       - reg: port number on OHCI controller, e.g
>>> +              On Exynos5250, port 0 is USB2.0 otg phy
>>> +                             port 1 is HSIC phy0
>>> +                             port 2 is HSIC phy1
>>> +       - phys: from the *Generic PHY* bindings, specifying phy used by
>>> port.
>>> +       - phy-names: from the *Generic PHY* bindings, specifying name of
>>> phy
>>> +                    used by the port.
>>
>>
>> I think you don't need this property for this binding, as the PHYs are being
>> requested by indices (in fact only index 0 is used).
>
> True, that phy-names property is not used, since PHYs are being
> requested using indices.
> We can remove this.
>
>>
>>
>>>
>>>   Example:
>>>         usb at 12120000 {
>>> @@ -47,6 +56,16 @@ Example:
>>>
>>>                 clocks = <&clock 285>;
>>>                 clock-names = "usbhost";
>>> +
>>> +               #address-cells = <1>;
>>> +               #size-cells = <0>;
>>> +               port at 0 {
>>> +                   reg = <0>;
>>> +                   phys = <&usb2phy 1>;
>>> +                   phy-names = "host";
>>
>>
>> Ditto.
> will remove this.
>
>>
>>
>>> +                   status = "disabled";
>>> +               };
>>> +
>>>         };
>>>
>>>   DWC3
>>> diff --git a/drivers/usb/host/ohci-exynos.c
>>> b/drivers/usb/host/ohci-exynos.c
>>> index 05f00e3..f90bf9a 100644
>>> --- a/drivers/usb/host/ohci-exynos.c
>>> +++ b/drivers/usb/host/ohci-exynos.c
>>> @@ -18,6 +18,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/of.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/phy/phy.h>
>>>   #include <linux/usb/phy.h>
>>>   #include <linux/usb/samsung_usb_phy.h>
>>>   #include <linux/usb.h>
>>> @@ -33,28 +34,122 @@ static struct hc_driver __read_mostly
>>> exynos_ohci_hc_driver;
>>>
>>>   #define to_exynos_ohci(hcd) (struct exynos_ohci_hcd
>>> *)(hcd_to_ohci(hcd)->priv)
>>>
>>> +#define PHY_NUMBER 3
>>
>>
>> nit: A blank line would be nice here to separate from the struct below.
>
> Ok, will add one.
>
>>
>> By the way, doesn't the number of PHY ports depend on platform? Of course
>> right now the driver supports only Exynos >= 4210 SoCs with the generic PHY
>> interface, so it might be changed in further patches.
>
> Yes, the number of PHY ports will be platform dependent feature.
> In subsequent patches we can add support to count the number of PHYs,
> or rather in this patch
> itself, when we do -
>          for_each_available_child_of_node(dev->of_node, child) {
> we can keep a count of how many child nodes we found, and then
> configure those many.
> As such the host controller drivers will have 'host' and 'hsic' PHYs.
>
>>
>>
>>>   struct exynos_ohci_hcd {
>>>         struct clk *clk;
>>>         struct usb_phy *phy;
>>>         struct usb_otg *otg;
>>> +       struct phy *phy_g[PHY_NUMBER];
>>>   };
>>>
>>> -static void exynos_ohci_phy_enable(struct device *dev)
>>> +static int exynos_ohci_get_phy(struct device *dev,
>>> +                               struct exynos_ohci_hcd *exynos_ohci)
>>> +{
>>> +       struct device_node *child;
>>> +       struct phy *phy;
>>> +       int phy_number;
>>> +       int ret = 0;
>>> +
>>> +       exynos_ohci->phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>>> +       if (IS_ERR(exynos_ohci->phy)) {
>>> +               ret = PTR_ERR(exynos_ohci->phy);
>>> +               /* This is the case when PHY config is disabled */
>>> +               if (ret == -ENXIO || ret == -ENODEV) {
>>> +                       dev_dbg(dev, "Failed to get usb2 phy\n");
>>> +                       exynos_ohci->phy = NULL;
>>
>>
>> I think you should keep this an ERR_PTR() and just use IS_ERR() check
>> further in the driver instead of checking for NULL.
>
> Yea, that's also one way to check for phy, i can modify this.
>
>>
>>> +                       ret = 0;
>>
>>
>> Do you need to set ret to 0 here? The code for getting generic PHYs will
>> either leave it unchanged when there are no port nodes defined or overwrite
>> it with value returned by of_property_read_u32(). In the first case, an
>> error code should be returned, not zero, as the driver was unable to get any
>> PHY.
>
> The idea was to not fail exynos_ohci_get_phy() call when the PHY
> configs are not even enabled.
> Since this would mean, that the driver will never be able to get a PHY
> in future, and there will be no point in
> failing the driver probe.
>
> In a case when the host controller dts is still on older PHY bindings,
> this 'ret' will *not* be over-wriiten, since the
> generic phy nodes will not be there.
> And in case the host dts is moved to the new generic PHY based
> bindings. then the part of this function exynos_ohci_get_phy()
> related to older usb-phy, shall not execute.
>
> This is reason why i did not really add a fall-back mechanism for getting PHY.
> Since from DT either of the two bindings be supplied, and then things
> here will be handles almost independently.
>
>>
>>
>>> +               } else if (ret == -EPROBE_DEFER) {
>>
>>
>> I think you could merge this case with the else clause below, as most driver
>> do. Moreover, since the only thing done after the fail_phy label is
>> returning the error code, you could just immediately return from here. So
>> the whole block of code would end up like this:
>>
>>         if (IS_ERR(exynos_ohci->phy)) {
>>                 ret = PTR_ERR(exynos_ohci->phy);
>>                 if (ret != -ENXIO && ret != -ENODEV) {
>>
>>                         dev_err(dev, "no usb2 phy configured\n");
>>                         return ret;
>>
>>                 }
>>                 dev_dbg(dev, "Failed to get usb2 phy\n");
>>                 exynos_ohci->phy = NULL;
>>         } else {
>>
>>                 exynos_ohci->otg = exynos_ohci->phy->otg;
>>         }
>
> Ok, this seems a good restructuring. I shall change this.
>
>>
>>> +                       goto fail_phy;
>>>
>>> +               } else {
>>> +                       dev_err(dev, "no usb2 phy configured\n");
>>> +                       goto fail_phy;
>>> +               }
>>> +       } else {
>>> +               exynos_ohci->otg = exynos_ohci->phy->otg;
>>> +       }
>>> +
>>> +       /* Getting generic phy:
>>
>>
>> CodingStyle: Multi-line comments should begin and end with a single empty
>> line:
>>
>>         /*
>>          * Getting generic phy:
>>          * ...
>>          */
>>
>> also nit: s/phy/PHY/
>>
>
> Ok, will correct this.
>
>>
>>> +        * We are keeping both types of phys as a part of transiting OHCI
>>> +        * to generic phy framework, so that in absence of supporting dts
>>> +        * changes the functionality doesn't break.
>>> +        * Once we move the ohci dt nodes to use new generic phys,
>>> +        * we can remove support for older PHY in this driver.
>>
>>
>> Well, this is not entirely true. The problem here is not caused by existing
>> DTS files, but rather a chance that there are existing devices using DTB
>> files built from them. So to remove the support for old bindings, we need to
>> make sure that such devices have their DTBs updated to ones built from new
>> DTS.
>
> Fair enough, thanks for explaining. :-)
> I shall modify this comment.
>
>>
>>
>>> +        */
>>> +       for_each_available_child_of_node(dev->of_node, child) {
>>> +               ret = of_property_read_u32(child, "reg", &phy_number);
>>> +               if (ret) {
>>> +                       dev_err(dev, "Failed to parse device tree\n");
>>> +                       of_node_put(child);
>>> +                       goto fail_phy;
>>
>>
>> Why not just return ret here?
>>
>>> +               }
>>
>>
>> nit: Blank line here would be nice.
>
> ok
>
>>
>>
>>> +               if (phy_number >= PHY_NUMBER) {
>>> +                       dev_err(dev, "Invalid number of PHYs\n");
>>> +                       of_node_put(child);
>>> +                       ret = -EINVAL;
>>> +                       goto fail_phy;
>>
>>
>> What about just return -EINVAL;
>
> Yea, just another way of doing things. ;-)
> I felt 'goto' to be a bit clean than adding number of returns in
> between statements.
>
>>
>>> +               }
>>
>>
>> nit: Here too.
> yea, will add a blank line.
>
>>
>>> +               phy = devm_of_phy_get(dev, child, 0);
>>> +               of_node_put(child);
>>> +               if (IS_ERR(phy)) {
>>> +                       ret = PTR_ERR(phy);
>>> +                       /* This is the case when PHY config is disabled */
>>> +                       if (ret == -ENOSYS || ret == -ENODEV) {
>>> +                               dev_dbg(dev, "Failed to get usb2 phy\n");
>>> +                               phy = NULL;
>>>
>>> +                               ret = 0;
>>> +                       } else if (ret == -EPROBE_DEFER) {
>>> +                               goto fail_phy;
>>> +                       } else {
>>> +                               dev_err(dev, "no usb2 phy configured\n");
>>> +                               goto fail_phy;
>>> +                       }
>>> +               }
>>
>>
>> Similar comments to this block apply as for the block getting legacy USB
>> PHY.
>
> Ok, i will restructure this same as 'legacy PHY  block'
>
>>
>>
>>> +               exynos_ohci->phy_g[phy_number] = phy;
>>> +       }
>>> +
>>> +fail_phy:
>>> +       return ret;
>>> +}
>>> +
>>> +static int exynos_ohci_phy_enable(struct device *dev)
>>>   {
>>>         struct usb_hcd *hcd = dev_get_drvdata(dev);
>>>         struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>>> +       int i;
>>> +       int ret = 0;
>>>
>>> -       if (exynos_ohci->phy)
>>> -               usb_phy_init(exynos_ohci->phy);
>>> +       if (exynos_ohci->phy) {
>>
>>
>> !IS_ERR() should be used to check for validity.
>
> Ok, this with the earlier change of setting exynos_ohci->phy as
> ERR_PTR(), should be good then.
>
>>
>>
>>> +               ret = usb_phy_init(exynos_ohci->phy);
>>> +               if (ret)
>>> +                       return ret;
>>
>>
>> IMHO a simple return usb_phy_init(...) could be used here, if we are using
>> the legacy PHY interface.
>
> Right, with legacy PHY, just a 'return usb_phy_init(...)' should do;
> since those devices will not have the generic
> PHYs.
>
>>
>>
>>> +       }
>>> +
>>> +       for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
>>> +               if (exynos_ohci->phy_g[i])
>>
>>
>> !IS_ERR(). Just make sure that the array is initialized with an ERR_PTR()
>> with some error code, (-ENODEV) probably
>
> Ok.

I think we don't need to initialize this array to any ERR_PTR(), since
devm_of_phy_get()
will anyways assign an ERR_PTR() to phy in unsuccessful case, and a
valid phy pointer
when it successfully gets one.
Isn't it ?

I think the same goes with legacy usb-phy function devm_usb_get_phy().


[snip]


-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India