On 2017/6/18 13:11, Marek Vasut wrote:
On 06/18/2017 12:10 AM, Simon Glass wrote:
Hi Marek,

On 17 June 2017 at 13:33, Marek Vasut <ma...@denx.de> wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,

On 17 June 2017 at 00:22, Marek Vasut <ma...@denx.de> wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,

On 15 June 2017 at 10:53, Marek Vasut <ma...@denx.de> wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:

On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off
vbus when usb stop.

Signed-off-by: Meng Dongyang <daniel.m...@rock-chips.com>
---

Changes in v5:
- Propagate return value and print error message when failed

Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set

Changes in v3: None
Changes in v2:
- Use fixed regulator to control vbus instead of gpio

   drivers/usb/host/xhci-rockchip.c | 55
++++++++++++++++++++++++++++++----------
   1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-rockchip.c
b/drivers/usb/host/xhci-rockchip.c
index f559830..15df6ef 100644
--- a/drivers/usb/host/xhci-rockchip.c
+++ b/drivers/usb/host/xhci-rockchip.c
@@ -11,10 +11,10 @@
   #include <malloc.h>
   #include <usb.h>
   #include <watchdog.h>
-#include <asm/gpio.h>
   #include <linux/errno.h>
   #include <linux/compat.h>
   #include <linux/usb/dwc3.h>
+#include <power/regulator.h>
     #include "xhci.h"
   @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;
   struct rockchip_xhci_platdata {
       fdt_addr_t hcd_base;
       fdt_addr_t phy_base;
-    struct gpio_desc vbus_gpio;
+    struct udevice *vbus_supply;
   };
     /*
@@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct
udevice *dev)
        */
       plat->hcd_base = dev_get_addr(dev);
       if (plat->hcd_base == FDT_ADDR_T_NONE) {
-        debug("Can't get the XHCI register base address\n");
+        error("Can't get the XHCI register base address\n");
           return -ENXIO;
       }
   @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct
udevice *dev)
       }
         if (plat->phy_base == FDT_ADDR_T_NONE) {
-        debug("Can't get the usbphy register address\n");
+        error("Can't get the usbphy register address\n");
           return -ENXIO;
       }
   -    /* Vbus gpio */
-    ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
-                   &plat->vbus_gpio, GPIOD_IS_OUT);
+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or
should) already anyway.
Yes, I will remove it in v6.
+    /* Vbus regulator */
+    ret = device_get_supply_regulator(dev, "vbus-supply",
+                      &plat->vbus_supply);
So I was curious, does this expand to empty function or is this not
defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation
of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some
rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what
those bad boards are, actually.
Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators.
Presumably this #ifdef is because of a board that does not.
This is IMO unrelated to rockchip.

Anyway, what if you just disable regulator support (because you don't
need it for whatever reason), but want to keep USB ? Wouldn't it make
more sense to have empty stub functions instead of swamping the drivers
with ifdefs ?
USB would not work without VBUS, which is handled with regulators
The VBUS can be directly tied to 5V rail without power switching.

, so
there is something I don't understand here. Anyway I don't see any
point in adding stub functions here.
Not here, into the regulator framework, so you don't have to pollute
drivers which use the regulators with ifdefs if the regulator framework
is not enabled in the config.

Meng can you please explain why the #ifdef is needed?

Because the function "device_get_supply_regulator" is undefined if undefined
CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the regulator
framework can be optimized and make it compiled successful whether define
CONFIG_DM_REGULATOR.
So is this #ifdef still needed here?



Regards,
Simon




_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to