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

[Marek Vasut]

On 06/18/2017 12:10 AM, Simon Glass wrote:
> 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.

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?
> 
> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut