
On 06/20/2017 05:22 AM, Simon Glass wrote:
On 19 June 2017 at 04:15, Marek Vasut <marex@denx.de mailto:marex@denx.de> wrote:
On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote:
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 <marex@denx.de
mailto: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
mailto: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
mailto: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
mailto: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.
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?
Thus my discussion with Simon. Linux does keep the ifdefs in framework header files , so I think we should do the same.
OK, but arguably that is a separate issue from this patch.
It is, and I believe it should be discussed and possibly fixed.
Also I hope we can always enable DM_REGULATOR for rockchip and drop the #ifdefs.
If the driver explicitly depends on DM_REGULATOR in Kconfig , then yes.