Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut marex@denx.de wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@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.meng@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.
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, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Meng can you please explain why the #ifdef is needed?
Regards, Simon