[U-Boot] [PATCH v5 04/11] usb: host: xhci-rockchip: use fixed regulator to control vbus

[Simon Glass]

Hi Marek,

On 17 June 2017 at 13:33, Marek Vasut <marex at 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 at 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 at 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 at 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