Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521714 > unrolled thread

[PATCH v5 0/5] USB: ohci-da8xx: Add device tree support

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/5] USB: ohci-da8xx: Add device tree support Axel Haslam <ahaslam@baylibre.com> - 2016-11-14 15:50 +0100
    [PATCH v5 2/5] USB: ohci: da8xx: Add wrappers for platform callbacks Axel Haslam <ahaslam@baylibre.com> - 2016-11-14 15:50 +0100
    [PATCH v5 1/5] USB: ohci: da8xx: use ohci priv data instead of globals Axel Haslam <ahaslam@baylibre.com> - 2016-11-14 15:50 +0100
      Re: [v5,1/5] USB: ohci: da8xx: use ohci priv data instead of globals David Lechner <david@lechnology.com> - 2016-11-20 04:00 +0100
        Re: [v5,1/5] USB: ohci: da8xx: use ohci priv data instead of globals Axel Haslam <ahaslam@baylibre.com> - 2016-11-21 10:10 +0100
    [PATCH v5 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS Axel Haslam <ahaslam@baylibre.com> - 2016-11-14 15:50 +0100
      Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS David Lechner <david@lechnology.com> - 2016-11-20 04:40 +0100
      Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS David Lechner <david@lechnology.com> - 2016-11-20 04:40 +0100
        Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS Axel Haslam <ahaslam@baylibre.com> - 2016-11-21 11:30 +0100
          Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS David Lechner <david@lechnology.com> - 2016-11-21 18:00 +0100
            Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS Axel Haslam <ahaslam@baylibre.com> - 2016-11-22 15:30 +0100

#1521714 — [PATCH v5 0/5] USB: ohci-da8xx: Add device tree support

When booting using device tree, we can not make use of
platform callbacks to handle vbus and over current gpios.

This series allows the ohci-da8xx driver to use a regulator
instead of the platform callbacks to control vbus and adds
the device tree bindings to be able to probe using DT.

Once all users of the platform callbacks will be converted to
use a regulator, we will be able to remove platform data completely.

Changes from v4->v5
* Append the Device tree patches to v4.
* Submit only the first part of the series (no dependencies).
this can be applied and merged through the usb tree.

Changes from v3->v4
* separate the series into smaller series for driver and arch/arm code,
  to ease review and merging to different trees.

Changes form v2->v3
* drop patches that have been integrated to arch/arm
* drop regulator patches which will be integrated through regulator tree
* use of the accepted regulator API to get over current status
* better patch separation with the use of wrappers

Changes from v1->v2
* Rebased and added patch to make ohci a separate driver
* Use a regulator instead of handling Gpios (David Lechner)
* Add an over current mode to regulator framework
* Fixed regulator is able to register for and over current irq
* Added patch by Alexandre to remove build warnings
* Moved global variables into private hcd structure.
Axel Haslam (5):
  USB: ohci: da8xx: use ohci priv data instead of globals
  USB: ohci: da8xx: Add wrappers for platform callbacks
  USB: ohci: da8xx: Allow a regulator to handle VBUS
  USB: ohci: da8xx: Add devicetree bindings
  USB: ohci: da8xx: Allow probing from DT

 .../devicetree/bindings/usb/ohci-da8xx.txt         |  23 ++
 drivers/usb/host/ohci-da8xx.c                      | 291 +++++++++++++++++----
 2 files changed, 264 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ohci-da8xx.txt

-- 
2.10.1

[toc] | [next] | [standalone]

#1521716 — [PATCH v5 2/5] USB: ohci: da8xx: Add wrappers for platform callbacks

In preparation to use a regulator instead of platform callbacks,
move the platform callbacks into separate functions.

This provides a well defined place to for the regulator API to coexist
with the callbacks until all users are converted, and the callbacks
can be removed.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/usb/host/ohci-da8xx.c | 125 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 102 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 438970b..83e3c98 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -81,6 +81,72 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
 	clk_disable_unprepare(da8xx_ohci->usb11_clk);
 }
 
+static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->set_power)
+		return hub->set_power(1, on);
+
+	return 0;
+}
+
+static int ohci_da8xx_get_power(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->get_power)
+		return hub->get_power(1);
+
+	return 1;
+}
+
+static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->get_oci)
+		return hub->get_oci(1);
+
+	return 0;
+}
+
+static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->set_power)
+		return 1;
+
+	return 0;
+}
+
+static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->get_oci)
+		return 1;
+
+	return 0;
+}
+
+static int ohci_da8xx_has_potpgt(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->potpgt)
+		return 1;
+
+	return 0;
+}
+
 /*
  * Handle the port over-current indicator change.
  */
@@ -94,6 +160,26 @@ static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub,
 		hub->set_power(port, 0);
 }
 
+static int ohci_da8xx_register_notify(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->ocic_notify)
+		return hub->ocic_notify(ohci_da8xx_ocic_handler);
+
+	return 0;
+}
+
+static void ohci_da8xx_unregister_notify(struct usb_hcd *hcd)
+{
+	struct device *dev		= hcd->self.controller;
+	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+
+	if (hub && hub->ocic_notify)
+		hub->ocic_notify(NULL);
+}
+
 static int ohci_da8xx_reset(struct usb_hcd *hcd)
 {
 	struct device *dev		= hcd->self.controller;
@@ -127,16 +213,18 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 	 * the correct hub descriptor...
 	 */
 	rh_a = ohci_readl(ohci, &ohci->regs->roothub.a);
-	if (hub->set_power) {
+	if (ohci_da8xx_has_set_power(hcd)) {
 		rh_a &= ~RH_A_NPS;
 		rh_a |=  RH_A_PSM;
 	}
-	if (hub->get_oci) {
+	if (ohci_da8xx_has_oci(hcd)) {
 		rh_a &= ~RH_A_NOCP;
 		rh_a |=  RH_A_OCPM;
 	}
-	rh_a &= ~RH_A_POTPGT;
-	rh_a |= hub->potpgt << 24;
+	if (ohci_da8xx_has_potpgt(hcd)) {
+		rh_a &= ~RH_A_POTPGT;
+		rh_a |= hub->potpgt << 24;
+	}
 	ohci_writel(ohci, rh_a, &ohci->regs->roothub.a);
 
 	return result;
@@ -169,7 +257,6 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				  u16 wIndex, char *buf, u16 wLength)
 {
 	struct device *dev		= hcd->self.controller;
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
 	int temp;
 
 	switch (typeReq) {
@@ -183,11 +270,11 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		temp = roothub_portstatus(hcd_to_ohci(hcd), wIndex - 1);
 
 		/* The port power status (PPS) bit defaults to 1 */
-		if (hub->get_power && hub->get_power(wIndex) == 0)
+		if (!ohci_da8xx_get_power(hcd))
 			temp &= ~RH_PS_PPS;
 
 		/* The port over-current indicator (POCI) bit is always 0 */
-		if (hub->get_oci && hub->get_oci(wIndex) > 0)
+		if (ohci_da8xx_get_oci(hcd) > 0)
 			temp |=  RH_PS_POCI;
 
 		/* The over-current indicator change (OCIC) bit is 0 too */
@@ -212,10 +299,7 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex, "POWER");
 
-			if (!hub->set_power)
-				return -EPIPE;
-
-			return hub->set_power(wIndex, temp) ? -EPIPE : 0;
+			return ohci_da8xx_set_power(hcd, temp) ? -EPIPE : 0;
 		case USB_PORT_FEAT_C_OVER_CURRENT:
 			dev_dbg(dev, "%sPortFeature(%u): %s\n",
 				temp ? "Set" : "Clear", wIndex,
@@ -237,15 +321,10 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 static int ohci_da8xx_probe(struct platform_device *pdev)
 {
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
 	struct da8xx_ohci_hcd *da8xx_ohci;
 	struct usb_hcd	*hcd;
 	struct resource *mem;
 	int error, irq;
-
-	if (hub == NULL)
-		return -ENODEV;
-
 	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
 				dev_name(&pdev->dev));
 	if (!hcd)
@@ -290,12 +369,13 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 
 	device_wakeup_enable(hcd->self.controller);
 
-	if (hub->ocic_notify) {
-		error = hub->ocic_notify(ohci_da8xx_ocic_handler);
-		if (!error)
-			return 0;
-	}
+	error = ohci_da8xx_register_notify(hcd);
+	if (error)
+		goto err_remove_hcd;
+
+	return 0;
 
+err_remove_hcd:
 	usb_remove_hcd(hcd);
 err:
 	usb_put_hcd(hcd);
@@ -305,9 +385,8 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 static int ohci_da8xx_remove(struct platform_device *pdev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
-	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
 
-	hub->ocic_notify(NULL);
+	ohci_da8xx_unregister_notify(hcd);
 	usb_remove_hcd(hcd);
 	usb_put_hcd(hcd);
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1521717 — [PATCH v5 1/5] USB: ohci: da8xx: use ohci priv data instead of globals

Instead of global variables, use the extra_priv_size of
the ohci driver.

We cannot yet move the ocic mask because this is used on
the interrupt handler which is registerded through platform
data and does not have an hcd pointer. This will be moved
on a later patch.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/usb/host/ohci-da8xx.c | 73 +++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index b3de8bc..438970b 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -35,43 +35,50 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
 			u16 wValue, u16 wIndex, char *buf, u16 wLength);
 static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
 
-static struct clk *usb11_clk;
-static struct phy *usb11_phy;
+struct da8xx_ohci_hcd {
+	struct clk *usb11_clk;
+	struct phy *usb11_phy;
+};
+
+#define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
 
 /* Over-current indicator change bitmask */
 static volatile u16 ocic_mask;
 
-static int ohci_da8xx_enable(void)
+static int ohci_da8xx_enable(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	int ret;
 
-	ret = clk_prepare_enable(usb11_clk);
+	ret = clk_prepare_enable(da8xx_ohci->usb11_clk);
 	if (ret)
 		return ret;
 
-	ret = phy_init(usb11_phy);
+	ret = phy_init(da8xx_ohci->usb11_phy);
 	if (ret)
 		goto err_phy_init;
 
-	ret = phy_power_on(usb11_phy);
+	ret = phy_power_on(da8xx_ohci->usb11_phy);
 	if (ret)
 		goto err_phy_power_on;
 
 	return 0;
 
 err_phy_power_on:
-	phy_exit(usb11_phy);
+	phy_exit(da8xx_ohci->usb11_phy);
 err_phy_init:
-	clk_disable_unprepare(usb11_clk);
+	clk_disable_unprepare(da8xx_ohci->usb11_clk);
 
 	return ret;
 }
 
-static void ohci_da8xx_disable(void)
+static void ohci_da8xx_disable(struct usb_hcd *hcd)
 {
-	phy_power_off(usb11_phy);
-	phy_exit(usb11_phy);
-	clk_disable_unprepare(usb11_clk);
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
+
+	phy_power_off(da8xx_ohci->usb11_phy);
+	phy_exit(da8xx_ohci->usb11_phy);
+	clk_disable_unprepare(da8xx_ohci->usb11_clk);
 }
 
 /*
@@ -97,7 +104,7 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 
 	dev_dbg(dev, "starting USB controller\n");
 
-	result = ohci_da8xx_enable();
+	result = ohci_da8xx_enable(hcd);
 	if (result < 0)
 		return result;
 
@@ -109,7 +116,7 @@ static int ohci_da8xx_reset(struct usb_hcd *hcd)
 
 	result = ohci_setup(hcd);
 	if (result < 0) {
-		ohci_da8xx_disable();
+		ohci_da8xx_disable(hcd);
 		return result;
 	}
 
@@ -231,6 +238,7 @@ static int ohci_da8xx_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 static int ohci_da8xx_probe(struct platform_device *pdev)
 {
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(&pdev->dev);
+	struct da8xx_ohci_hcd *da8xx_ohci;
 	struct usb_hcd	*hcd;
 	struct resource *mem;
 	int error, irq;
@@ -238,25 +246,29 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 	if (hub == NULL)
 		return -ENODEV;
 
-	usb11_clk = devm_clk_get(&pdev->dev, "usb11");
-	if (IS_ERR(usb11_clk)) {
-		if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
+	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
+				dev_name(&pdev->dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	da8xx_ohci = to_da8xx_ohci(hcd);
+
+	da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
+	if (IS_ERR(da8xx_ohci->usb11_clk)) {
+		if (PTR_ERR(da8xx_ohci->usb11_clk) != -EPROBE_DEFER)
 			dev_err(&pdev->dev, "Failed to get clock.\n");
-		return PTR_ERR(usb11_clk);
+		error = PTR_ERR(da8xx_ohci->usb11_clk);
+		goto err;
 	}
 
-	usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
-	if (IS_ERR(usb11_phy)) {
-		if (PTR_ERR(usb11_phy) != -EPROBE_DEFER)
+	da8xx_ohci->usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
+	if (IS_ERR(da8xx_ohci->usb11_phy)) {
+		if (PTR_ERR(da8xx_ohci->usb11_phy) != -EPROBE_DEFER)
 			dev_err(&pdev->dev, "Failed to get phy.\n");
-		return PTR_ERR(usb11_phy);
+		error = PTR_ERR(da8xx_ohci->usb11_phy);
+		goto err;
 	}
 
-	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
-				dev_name(&pdev->dev));
-	if (!hcd)
-		return -ENOMEM;
-
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(hcd->regs)) {
@@ -320,7 +332,7 @@ static int ohci_da8xx_suspend(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	ohci_da8xx_disable();
+	ohci_da8xx_disable(hcd);
 	hcd->state = HC_STATE_SUSPENDED;
 
 	return ret;
@@ -336,7 +348,7 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 		msleep(5);
 	ohci->next_statechange = jiffies;
 
-	ret = ohci_da8xx_enable();
+	ret = ohci_da8xx_enable(hcd);
 	if (ret)
 		return ret;
 
@@ -348,7 +360,8 @@ static int ohci_da8xx_resume(struct platform_device *dev)
 #endif
 
 static const struct ohci_driver_overrides da8xx_overrides __initconst = {
-	.reset		= ohci_da8xx_reset,
+	.reset		 = ohci_da8xx_reset,
+	.extra_priv_size = sizeof(struct da8xx_ohci_hcd),
 };
 
 /*
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1526165 — Re: [v5,1/5] USB: ohci: da8xx: use ohci priv data instead of globals

On 11/14/2016 08:40 AM, ahaslam@baylibre.com wrote:
> Instead of global variables, use the extra_priv_size of
> the ohci driver.
>
> We cannot yet move the ocic mask because this is used on
> the interrupt handler which is registerded through platform
> data and does not have an hcd pointer. This will be moved
> on a later patch.
>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  drivers/usb/host/ohci-da8xx.c | 73 +++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
> index b3de8bc..438970b 100644
> --- a/drivers/usb/host/ohci-da8xx.c
> +++ b/drivers/usb/host/ohci-da8xx.c
...
> @@ -238,25 +246,29 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
>  	if (hub == NULL)
>  		return -ENODEV;
>
> -	usb11_clk = devm_clk_get(&pdev->dev, "usb11");
> -	if (IS_ERR(usb11_clk)) {
> -		if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
> +	hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
> +				dev_name(&pdev->dev));
> +	if (!hcd)
> +		return -ENOMEM;
> +
> +	da8xx_ohci = to_da8xx_ohci(hcd);
> +
> +	da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
> +	if (IS_ERR(da8xx_ohci->usb11_clk)) {
> +		if (PTR_ERR(da8xx_ohci->usb11_clk) != -EPROBE_DEFER)
>  			dev_err(&pdev->dev, "Failed to get clock.\n");
> -		return PTR_ERR(usb11_clk);
> +		error = PTR_ERR(da8xx_ohci->usb11_clk);
> +		goto err;
>  	}

Small thing, but this could be written slightly cleaner as:

	da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
	if (IS_ERR(da8xx_ohci->usb11_clk)) {
		error = PTR_ERR(da8xx_ohci->usb11_clk);
		if (error != -EPROBE_DEFER)
			dev_err(&pdev->dev, "Failed to get clock.\n");
		goto err;
	}

>
> -	usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
> -	if (IS_ERR(usb11_phy)) {
> -		if (PTR_ERR(usb11_phy) != -EPROBE_DEFER)
> +	da8xx_ohci->usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
> +	if (IS_ERR(da8xx_ohci->usb11_phy)) {
> +		if (PTR_ERR(da8xx_ohci->usb11_phy) != -EPROBE_DEFER)
>  			dev_err(&pdev->dev, "Failed to get phy.\n");
> -		return PTR_ERR(usb11_phy);
> +		error = PTR_ERR(da8xx_ohci->usb11_phy);
> +		goto err;
>  	}

same here

...


Tested-by: David Lechner <david@lechnology.com>

[toc] | [prev] | [next] | [standalone]

#1526467 — Re: [v5,1/5] USB: ohci: da8xx: use ohci priv data instead of globals

On Sun, Nov 20, 2016 at 3:58 AM, David Lechner <david@lechnology.com> wrote:
> On 11/14/2016 08:40 AM, ahaslam@baylibre.com wrote:
>>
>> Instead of global variables, use the extra_priv_size of
>> the ohci driver.
>>
>> We cannot yet move the ocic mask because this is used on
>> the interrupt handler which is registerded through platform
>> data and does not have an hcd pointer. This will be moved
>> on a later patch.
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>>  drivers/usb/host/ohci-da8xx.c | 73
>> +++++++++++++++++++++++++------------------
>>  1 file changed, 43 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index b3de8bc..438970b 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>
> ...
>>
>> @@ -238,25 +246,29 @@ static int ohci_da8xx_probe(struct platform_device
>> *pdev)
>>         if (hub == NULL)
>>                 return -ENODEV;
>>
>> -       usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>> -       if (IS_ERR(usb11_clk)) {
>> -               if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
>> +       hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
>> +                               dev_name(&pdev->dev));
>> +       if (!hcd)
>> +               return -ENOMEM;
>> +
>> +       da8xx_ohci = to_da8xx_ohci(hcd);
>> +
>> +       da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>> +       if (IS_ERR(da8xx_ohci->usb11_clk)) {
>> +               if (PTR_ERR(da8xx_ohci->usb11_clk) != -EPROBE_DEFER)
>>                         dev_err(&pdev->dev, "Failed to get clock.\n");
>> -               return PTR_ERR(usb11_clk);
>> +               error = PTR_ERR(da8xx_ohci->usb11_clk);
>> +               goto err;
>>         }
>
>
> Small thing, but this could be written slightly cleaner as:
>
>         da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>         if (IS_ERR(da8xx_ohci->usb11_clk)) {
>                 error = PTR_ERR(da8xx_ohci->usb11_clk);
>                 if (error != -EPROBE_DEFER)
>                         dev_err(&pdev->dev, "Failed to get clock.\n");
>                 goto err;
>         }
>

Yes, right, i will change it,
Thanks.

>>
>> -       usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
>> -       if (IS_ERR(usb11_phy)) {
>> -               if (PTR_ERR(usb11_phy) != -EPROBE_DEFER)
>> +       da8xx_ohci->usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
>> +       if (IS_ERR(da8xx_ohci->usb11_phy)) {
>> +               if (PTR_ERR(da8xx_ohci->usb11_phy) != -EPROBE_DEFER)
>>                         dev_err(&pdev->dev, "Failed to get phy.\n");
>> -               return PTR_ERR(usb11_phy);
>> +               error = PTR_ERR(da8xx_ohci->usb11_phy);
>> +               goto err;
>>         }
>
>
> same here
>
> ...
>
>
> Tested-by: David Lechner <david@lechnology.com>
>

[toc] | [prev] | [next] | [standalone]

#1521718 — [PATCH v5 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

Using a regulator to handle VBUS will eliminate the need for
platform data and callbacks, and make the driver more generic
allowing different types of regulators to handle VBUS.

The regulator equivalents to the platform callbacks are:
    set_power   ->  regulator_enable/regulator_disable
    get_power   ->  regulator_is_enabled
    get_oci     ->  regulator_get_error_flags
    ocic_notify ->  regulator event notification

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/usb/host/ohci-da8xx.c | 95 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 83e3c98..42eaeb9 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_data/usb-davinci.h>
+#include <linux/regulator/consumer.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <asm/unaligned.h>
@@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
 static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
 
 struct da8xx_ohci_hcd {
+	struct usb_hcd *hcd;
 	struct clk *usb11_clk;
 	struct phy *usb11_phy;
+	struct regulator *vbus_reg;
+	struct notifier_block nb;
+	unsigned int is_powered;
 };
 
 #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
@@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
 
 static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+	int ret;
 
 	if (hub && hub->set_power)
 		return hub->set_power(1, on);
 
+	if (!da8xx_ohci->vbus_reg)
+		return 0;
+
+	if (on && !da8xx_ohci->is_powered) {
+		ret = regulator_enable(da8xx_ohci->vbus_reg);
+		if (ret) {
+			dev_err(dev, "Fail to enable regulator: %d\n", ret);
+			return ret;
+		}
+		da8xx_ohci->is_powered = 1;
+
+	} else if (!on && da8xx_ohci->is_powered) {
+		ret = regulator_disable(da8xx_ohci->vbus_reg);
+		if (ret) {
+			dev_err(dev, "Fail to disable regulator: %d\n", ret);
+			return ret;
+		}
+		da8xx_ohci->is_powered = 0;
+	}
+
 	return 0;
 }
 
 static int ohci_da8xx_get_power(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
 
 	if (hub && hub->get_power)
 		return hub->get_power(1);
 
+	if (da8xx_ohci->vbus_reg)
+		return regulator_is_enabled(da8xx_ohci->vbus_reg);
+
 	return 1;
 }
 
 static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+	unsigned int flags;
+	int ret;
 
 	if (hub && hub->get_oci)
 		return hub->get_oci(1);
 
+	if (!da8xx_ohci->vbus_reg)
+		return 0;
+
+	ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, &flags);
+	if (ret)
+		return ret;
+
+	if (flags && REGULATOR_ERROR_OVER_CURRENT)
+		return 1;
+
 	return 0;
 }
 
 static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
 
 	if (hub && hub->set_power)
 		return 1;
 
+	if (da8xx_ohci->vbus_reg)
+		return 1;
+
 	return 0;
 }
 
 static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
 
 	if (hub && hub->get_oci)
 		return 1;
 
+	if (da8xx_ohci->vbus_reg)
+		return 1;
+
 	return 0;
 }
 
@@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub,
 		hub->set_power(port, 0);
 }
 
+static int ohci_da8xx_regulator_event(struct notifier_block *nb,
+				unsigned long event, void *data)
+{
+	struct da8xx_ohci_hcd *da8xx_ohci =
+				container_of(nb, struct da8xx_ohci_hcd, nb);
+	struct device *dev = da8xx_ohci->hcd->self.controller;
+
+	if (event & REGULATOR_EVENT_OVER_CURRENT) {
+		dev_warn(dev, "over current event\n");
+		ocic_mask |= 1;
+		ohci_da8xx_set_power(da8xx_ohci->hcd, 0);
+	}
+
+	return 0;
+}
+
 static int ohci_da8xx_register_notify(struct usb_hcd *hcd)
 {
+	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 	struct device *dev		= hcd->self.controller;
 	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
+	int ret = 0;
 
 	if (hub && hub->ocic_notify)
-		return hub->ocic_notify(ohci_da8xx_ocic_handler);
+		ret = hub->ocic_notify(ohci_da8xx_ocic_handler);
+	else if (da8xx_ohci->vbus_reg) {
+		da8xx_ohci->nb.notifier_call = ohci_da8xx_regulator_event;
+		ret = devm_regulator_register_notifier(da8xx_ohci->vbus_reg,
+						&da8xx_ohci->nb);
+	}
 
-	return 0;
+	if (ret)
+		dev_err(dev, "Failed to register notifier: %d\n", ret);
+
+	return ret;
 }
 
 static void ohci_da8xx_unregister_notify(struct usb_hcd *hcd)
@@ -331,6 +409,7 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	da8xx_ohci = to_da8xx_ohci(hcd);
+	da8xx_ohci->hcd = hcd;
 
 	da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
 	if (IS_ERR(da8xx_ohci->usb11_clk)) {
@@ -348,6 +427,18 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	da8xx_ohci->vbus_reg = devm_regulator_get_optional(&pdev->dev, "vbus");
+	if (IS_ERR(da8xx_ohci->vbus_reg)) {
+		error = PTR_ERR(da8xx_ohci->vbus_reg);
+		if (error == -ENODEV) {
+			da8xx_ohci->vbus_reg = NULL;
+		} else {
+			dev_err(&pdev->dev,
+				"Failed to get regulator: %d\n", error);
+			goto err;
+		}
+	}
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(hcd->regs)) {
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1526166 — Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

On 11/19/2016 09:31 PM, David Lechner wrote:
> On 11/14/2016 08:41 AM, ahaslam@baylibre.com wrote:
>> +        ocic_mask |= 1;
>
> I thought that a previous patch got rid of all globals. Why is ocic_mask
> still a global variable?

I suppose if I read the commit message, I will know the answer ;-)

[toc] | [prev] | [next] | [standalone]

#1526167 — Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

On 11/14/2016 08:41 AM, ahaslam@baylibre.com wrote:
> Using a regulator to handle VBUS will eliminate the need for
> platform data and callbacks, and make the driver more generic
> allowing different types of regulators to handle VBUS.
>
> The regulator equivalents to the platform callbacks are:
>     set_power   ->  regulator_enable/regulator_disable
>     get_power   ->  regulator_is_enabled
>     get_oci     ->  regulator_get_error_flags
>     ocic_notify ->  regulator event notification
>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  drivers/usb/host/ohci-da8xx.c | 95 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
> index 83e3c98..42eaeb9 100644
> --- a/drivers/usb/host/ohci-da8xx.c
> +++ b/drivers/usb/host/ohci-da8xx.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_data/usb-davinci.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
>  #include <asm/unaligned.h>
> @@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
>  static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>
>  struct da8xx_ohci_hcd {
> +	struct usb_hcd *hcd;
>  	struct clk *usb11_clk;
>  	struct phy *usb11_phy;
> +	struct regulator *vbus_reg;
> +	struct notifier_block nb;
> +	unsigned int is_powered;

Since is_powered is only used to indicate if we have called 
regulator_enable(), I think it would make more sense to name it 
reg_enabled instead.

>  };
>
>  #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
> @@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
>
>  static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
>  {
> +	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
> +	int ret;
>
>  	if (hub && hub->set_power)
>  		return hub->set_power(1, on);
>
> +	if (!da8xx_ohci->vbus_reg)
> +		return 0;
> +
> +	if (on && !da8xx_ohci->is_powered) {
> +		ret = regulator_enable(da8xx_ohci->vbus_reg);
> +		if (ret) {
> +			dev_err(dev, "Fail to enable regulator: %d\n", ret);

s/Fail/Failed/

> +			return ret;
> +		}
> +		da8xx_ohci->is_powered = 1;
> +
> +	} else if (!on && da8xx_ohci->is_powered) {
> +		ret = regulator_disable(da8xx_ohci->vbus_reg);
> +		if (ret) {
> +			dev_err(dev, "Fail to disable regulator: %d\n", ret);

s/Fail/Failed/

> +			return ret;
> +		}
> +		da8xx_ohci->is_powered = 0;
> +	}
> +
>  	return 0;
>  }
>
>  static int ohci_da8xx_get_power(struct usb_hcd *hcd)
>  {
> +	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
>
>  	if (hub && hub->get_power)
>  		return hub->get_power(1);
>
> +	if (da8xx_ohci->vbus_reg)
> +		return regulator_is_enabled(da8xx_ohci->vbus_reg);
> +
>  	return 1;
>  }
>
>  static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
>  {
> +	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
> +	unsigned int flags;
> +	int ret;
>
>  	if (hub && hub->get_oci)
>  		return hub->get_oci(1);
>
> +	if (!da8xx_ohci->vbus_reg)
> +		return 0;
> +
> +	ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, &flags);
> +	if (ret)
> +		return ret;
> +
> +	if (flags && REGULATOR_ERROR_OVER_CURRENT)

Is this supposed to be...

	if (flags & REGULATOR_ERROR_OVER_CURRENT)


> +		return 1;
> +
>  	return 0;
>  }
>
>  static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
>  {
> +	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
>
>  	if (hub && hub->set_power)
>  		return 1;
>
> +	if (da8xx_ohci->vbus_reg)
> +		return 1;
> +
>  	return 0;
>  }
>
>  static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
>  {
> +	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
>
>  	if (hub && hub->get_oci)
>  		return 1;
>
> +	if (da8xx_ohci->vbus_reg)
> +		return 1;
> +
>  	return 0;
>  }
>
> @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct da8xx_ohci_root_hub *hub,
>  		hub->set_power(port, 0);
>  }
>
> +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
> +				unsigned long event, void *data)
> +{
> +	struct da8xx_ohci_hcd *da8xx_ohci =
> +				container_of(nb, struct da8xx_ohci_hcd, nb);
> +	struct device *dev = da8xx_ohci->hcd->self.controller;
> +
> +	if (event & REGULATOR_EVENT_OVER_CURRENT) {
> +		dev_warn(dev, "over current event\n");

Won't this result in duplicate overcurrent warnings in the kernel log? 
It seems like in previous version of this patch series, we would get an 
overcurrent error from the core usb driver.

> +		ocic_mask |= 1;

I thought that a previous patch got rid of all globals. Why is ocic_mask 
still a global variable?

> +		ohci_da8xx_set_power(da8xx_ohci->hcd, 0);
> +	}
> +
> +	return 0;
> +}
> +
>  static int ohci_da8xx_register_notify(struct usb_hcd *hcd)
>  {
> +	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>  	struct device *dev		= hcd->self.controller;
>  	struct da8xx_ohci_root_hub *hub	= dev_get_platdata(dev);
> +	int ret = 0;
>
>  	if (hub && hub->ocic_notify)
> -		return hub->ocic_notify(ohci_da8xx_ocic_handler);
> +		ret = hub->ocic_notify(ohci_da8xx_ocic_handler);

nit: add { } around the if body too since there is { } around the else 
if body.

> +	else if (da8xx_ohci->vbus_reg) {
> +		da8xx_ohci->nb.notifier_call = ohci_da8xx_regulator_event;
> +		ret = devm_regulator_register_notifier(da8xx_ohci->vbus_reg,
> +						&da8xx_ohci->nb);
> +	}
>
> -	return 0;
> +	if (ret)
> +		dev_err(dev, "Failed to register notifier: %d\n", ret);
> +
> +	return ret;
>  }
>
>  static void ohci_da8xx_unregister_notify(struct usb_hcd *hcd)
> @@ -331,6 +409,7 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>
>  	da8xx_ohci = to_da8xx_ohci(hcd);
> +	da8xx_ohci->hcd = hcd;
>
>  	da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>  	if (IS_ERR(da8xx_ohci->usb11_clk)) {
> @@ -348,6 +427,18 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>
> +	da8xx_ohci->vbus_reg = devm_regulator_get_optional(&pdev->dev, "vbus");
> +	if (IS_ERR(da8xx_ohci->vbus_reg)) {
> +		error = PTR_ERR(da8xx_ohci->vbus_reg);
> +		if (error == -ENODEV) {
> +			da8xx_ohci->vbus_reg = NULL;
> +		} else {
> +			dev_err(&pdev->dev,
> +				"Failed to get regulator: %d\n", error);

I'm pretty sure that the probe error number is displayed elsewhere in 
the kernel log, so probably no need for %d here.

> +			goto err;
> +		}
> +	}
> +
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
>  	if (IS_ERR(hcd->regs)) {
>

[toc] | [prev] | [next] | [standalone]

#1526536 — Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

Hi David,

Thanks for the review,


On Sun, Nov 20, 2016 at 4:31 AM, David Lechner <david@lechnology.com> wrote:
> On 11/14/2016 08:41 AM, ahaslam@baylibre.com wrote:
>>
>> Using a regulator to handle VBUS will eliminate the need for
>> platform data and callbacks, and make the driver more generic
>> allowing different types of regulators to handle VBUS.
>>
>> The regulator equivalents to the platform callbacks are:
>>     set_power   ->  regulator_enable/regulator_disable
>>     get_power   ->  regulator_is_enabled
>>     get_oci     ->  regulator_get_error_flags
>>     ocic_notify ->  regulator event notification
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>>  drivers/usb/host/ohci-da8xx.c | 95
>> ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index 83e3c98..42eaeb9 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_data/usb-davinci.h>
>> +#include <linux/regulator/consumer.h>
>>  #include <linux/usb.h>
>>  #include <linux/usb/hcd.h>
>>  #include <asm/unaligned.h>
>> @@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd
>> *hcd, u16 typeReq,
>>  static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>>
>>  struct da8xx_ohci_hcd {
>> +       struct usb_hcd *hcd;
>>         struct clk *usb11_clk;
>>         struct phy *usb11_phy;
>> +       struct regulator *vbus_reg;
>> +       struct notifier_block nb;
>> +       unsigned int is_powered;
>
>
> Since is_powered is only used to indicate if we have called
> regulator_enable(), I think it would make more sense to name it reg_enabled
> instead.

ok.

>
>>  };
>>
>>  #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd
>> *)(hcd_to_ohci(hcd)->priv)
>> @@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
>>
>>  static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
>>  {
>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +       int ret;
>>
>>         if (hub && hub->set_power)
>>                 return hub->set_power(1, on);
>>
>> +       if (!da8xx_ohci->vbus_reg)
>> +               return 0;
>> +
>> +       if (on && !da8xx_ohci->is_powered) {
>> +               ret = regulator_enable(da8xx_ohci->vbus_reg);
>> +               if (ret) {
>> +                       dev_err(dev, "Fail to enable regulator: %d\n",
>> ret);
>
>
> s/Fail/Failed/

will fix in both places.

>
>> +                       return ret;
>> +               }
>> +               da8xx_ohci->is_powered = 1;
>> +
>> +       } else if (!on && da8xx_ohci->is_powered) {
>> +               ret = regulator_disable(da8xx_ohci->vbus_reg);
>> +               if (ret) {
>> +                       dev_err(dev, "Fail to disable regulator: %d\n",
>> ret);
>
>
> s/Fail/Failed/
>
>> +                       return ret;
>> +               }
>> +               da8xx_ohci->is_powered = 0;
>> +       }
>> +
>>         return 0;
>>  }
>>
>>  static int ohci_da8xx_get_power(struct usb_hcd *hcd)
>>  {
>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>>         if (hub && hub->get_power)
>>                 return hub->get_power(1);
>>
>> +       if (da8xx_ohci->vbus_reg)
>> +               return regulator_is_enabled(da8xx_ohci->vbus_reg);
>> +
>>         return 1;
>>  }
>>
>>  static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
>>  {
>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +       unsigned int flags;
>> +       int ret;
>>
>>         if (hub && hub->get_oci)
>>                 return hub->get_oci(1);
>>
>> +       if (!da8xx_ohci->vbus_reg)
>> +               return 0;
>> +
>> +       ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, &flags);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (flags && REGULATOR_ERROR_OVER_CURRENT)
>
>
> Is this supposed to be...

yes!

>
>         if (flags & REGULATOR_ERROR_OVER_CURRENT)
>
>
>> +               return 1;
>> +
>>         return 0;
>>  }
>>
>>  static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
>>  {
>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>>         if (hub && hub->set_power)
>>                 return 1;
>>
>> +       if (da8xx_ohci->vbus_reg)
>> +               return 1;
>> +
>>         return 0;
>>  }
>>
>>  static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
>>  {
>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>>         if (hub && hub->get_oci)
>>                 return 1;
>>
>> +       if (da8xx_ohci->vbus_reg)
>> +               return 1;
>> +
>>         return 0;
>>  }
>>
>> @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
>> da8xx_ohci_root_hub *hub,
>>                 hub->set_power(port, 0);
>>  }
>>
>> +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
>> +                               unsigned long event, void *data)
>> +{
>> +       struct da8xx_ohci_hcd *da8xx_ohci =
>> +                               container_of(nb, struct da8xx_ohci_hcd,
>> nb);
>> +       struct device *dev = da8xx_ohci->hcd->self.controller;
>> +
>> +       if (event & REGULATOR_EVENT_OVER_CURRENT) {
>> +               dev_warn(dev, "over current event\n");
>
>
> Won't this result in duplicate overcurrent warnings in the kernel log? It
> seems like in previous version of this patch series, we would get an
> overcurrent error from the core usb driver.

you mean in the regulator driver? i did not make changes to core usb.
but, no,  i did not add a print in the fixed regulator driver itself. Since
the regulator is  a separate driver, and could be implemented with or without
a trace, i think its better to leave this print. It shows that the usb driver
has well received the notification.

>
>> +               ocic_mask |= 1;
>
>
> I thought that a previous patch got rid of all globals. Why is ocic_mask
> still a global variable?
>

its is used in the platform callbacks which will be removed,
and i will remove it in future series. but you already saw this ;)

>> +               ohci_da8xx_set_power(da8xx_ohci->hcd, 0);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static int ohci_da8xx_register_notify(struct usb_hcd *hcd)
>>  {
>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +       int ret = 0;
>>
>>         if (hub && hub->ocic_notify)
>> -               return hub->ocic_notify(ohci_da8xx_ocic_handler);
>> +               ret = hub->ocic_notify(ohci_da8xx_ocic_handler);
>
>
> nit: add { } around the if body too since there is { } around the else if
> body.

will do.

>
>> +       else if (da8xx_ohci->vbus_reg) {
>> +               da8xx_ohci->nb.notifier_call = ohci_da8xx_regulator_event;
>> +               ret =
>> devm_regulator_register_notifier(da8xx_ohci->vbus_reg,
>> +                                               &da8xx_ohci->nb);
>> +       }
>>
>> -       return 0;
>> +       if (ret)
>> +               dev_err(dev, "Failed to register notifier: %d\n", ret);
>> +
>> +       return ret;
>>  }
>>
>>  static void ohci_da8xx_unregister_notify(struct usb_hcd *hcd)
>> @@ -331,6 +409,7 @@ static int ohci_da8xx_probe(struct platform_device
>> *pdev)
>>                 return -ENOMEM;
>>
>>         da8xx_ohci = to_da8xx_ohci(hcd);
>> +       da8xx_ohci->hcd = hcd;
>>
>>         da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>>         if (IS_ERR(da8xx_ohci->usb11_clk)) {
>> @@ -348,6 +427,18 @@ static int ohci_da8xx_probe(struct platform_device
>> *pdev)
>>                 goto err;
>>         }
>>
>> +       da8xx_ohci->vbus_reg = devm_regulator_get_optional(&pdev->dev,
>> "vbus");
>> +       if (IS_ERR(da8xx_ohci->vbus_reg)) {
>> +               error = PTR_ERR(da8xx_ohci->vbus_reg);
>> +               if (error == -ENODEV) {
>> +                       da8xx_ohci->vbus_reg = NULL;
>> +               } else {
>> +                       dev_err(&pdev->dev,
>> +                               "Failed to get regulator: %d\n", error);
>
>
> I'm pretty sure that the probe error number is displayed elsewhere in the
> kernel log, so probably no need for %d here.

Ok i will remove the %d.

>
>> +                       goto err;
>> +               }
>> +       }
>> +
>>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>         hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
>>         if (IS_ERR(hcd->regs)) {
>>
>

[toc] | [prev] | [next] | [standalone]

#1526914 — Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

On 11/21/2016 04:22 AM, Axel Haslam wrote:
> Hi David,
>
> Thanks for the review,
>

You're welcome.

>>>
>>> @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
>>> da8xx_ohci_root_hub *hub,
>>>                 hub->set_power(port, 0);
>>>  }
>>>
>>> +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
>>> +                               unsigned long event, void *data)
>>> +{
>>> +       struct da8xx_ohci_hcd *da8xx_ohci =
>>> +                               container_of(nb, struct da8xx_ohci_hcd,
>>> nb);
>>> +       struct device *dev = da8xx_ohci->hcd->self.controller;
>>> +
>>> +       if (event & REGULATOR_EVENT_OVER_CURRENT) {
>>> +               dev_warn(dev, "over current event\n");
>>
>>
>> Won't this result in duplicate overcurrent warnings in the kernel log? It
>> seems like in previous version of this patch series, we would get an
>> overcurrent error from the core usb driver.
>
> you mean in the regulator driver? i did not make changes to core usb.
> but, no,  i did not add a print in the fixed regulator driver itself. Since
> the regulator is  a separate driver, and could be implemented with or without
> a trace, i think its better to leave this print. It shows that the usb driver
> has well received the notification.
>

No, I mean in drivers/usb/core/hub.c. There is

	if (status & USB_PORT_STAT_OVERCURRENT)
		dev_err(&port_dev->dev, "over-current condition\n");

and

	if (status & HUB_STATUS_OVERCURRENT)
		dev_err(hub_dev, "over-current condition\n");

In ohci_da8xx_hub_control(), we are setting RH_PS_POCI and RH_PS_OCIC, 
so these messages will be printed via the core hub driver. We don't need 
to print another message from the same event.

[toc] | [prev] | [next] | [standalone]

#1527563 — Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

On Mon, Nov 21, 2016 at 5:29 PM, David Lechner <david@lechnology.com> wrote:
> On 11/21/2016 04:22 AM, Axel Haslam wrote:
>>
>> Hi David,
>>
>> Thanks for the review,
>>
>
> You're welcome.
>
>>>>
>>>> @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
>>>> da8xx_ohci_root_hub *hub,
>>>>                 hub->set_power(port, 0);
>>>>  }
>>>>
>>>> +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
>>>> +                               unsigned long event, void *data)
>>>> +{
>>>> +       struct da8xx_ohci_hcd *da8xx_ohci =
>>>> +                               container_of(nb, struct da8xx_ohci_hcd,
>>>> nb);
>>>> +       struct device *dev = da8xx_ohci->hcd->self.controller;
>>>> +
>>>> +       if (event & REGULATOR_EVENT_OVER_CURRENT) {
>>>> +               dev_warn(dev, "over current event\n");
>>>
>>>
>>>
>>> Won't this result in duplicate overcurrent warnings in the kernel log? It
>>> seems like in previous version of this patch series, we would get an
>>> overcurrent error from the core usb driver.
>>
>>
>> you mean in the regulator driver? i did not make changes to core usb.
>> but, no,  i did not add a print in the fixed regulator driver itself.
>> Since
>> the regulator is  a separate driver, and could be implemented with or
>> without
>> a trace, i think its better to leave this print. It shows that the usb
>> driver
>> has well received the notification.
>>
>
> No, I mean in drivers/usb/core/hub.c. There is
>
>         if (status & USB_PORT_STAT_OVERCURRENT)
>                 dev_err(&port_dev->dev, "over-current condition\n");
>
> and
>
>         if (status & HUB_STATUS_OVERCURRENT)
>                 dev_err(hub_dev, "over-current condition\n");
>
> In ohci_da8xx_hub_control(), we are setting RH_PS_POCI and RH_PS_OCIC, so
> these messages will be printed via the core hub driver. We don't need to
> print another message from the same event.
>
>

Hi David

I did a couple of tests, and i don't get those prints do you get them?.
What i understand is that they happen when we get a hub event that is
reporting the over current. Which is what the root hub of the davinci system
does not have, and why we use gpios instead).

Regards,
Axel.

>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web