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

[rock-chips(daniel.meng)]

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 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?

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
>>
>