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

[Marek Vasut]

On 06/09/2017 09:49 AM, rock-chips(daniel.meng) wrote:
> 
> 
> On 2017/6/8 21:17, Marek Vasut wrote:
>> On 06/08/2017 09:31 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 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 | 41
>>> +++++++++++++++++++++++++++++++---------
>>>   1 file changed, 32 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-rockchip.c
>>> b/drivers/usb/host/xhci-rockchip.c
>>> index f559830..dc9cd56 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;
>>>   };
>>>     /*
>>> @@ -66,15 +66,37 @@ static int xhci_usb_ofdata_to_platdata(struct
>>> udevice *dev)
>>>           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)
>>> +    /* Vbus regulator */
>>> +    ret = device_get_supply_regulator(dev, "vbus-supply",
>>> +                      &plat->vbus_supply);
>>>       if (ret)
>>> -        debug("rockchip,vbus-gpio node missing!");
>>> +        debug("Can't get vbus supply\n");
>> VBUS in caps
>>
>>> +#endif
>>>         return 0;
>>>   }
>>>   +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
>>> +static int rockchip_xhci_set_vbus(struct rockchip_xhci_platdata *plat,
>>> +                  bool value)
>>> +{
>>> +    int ret = 0;
>> You don't need to init ret, it's always set right below.
>>
>>> +
>>> +    ret = regulator_set_enable(plat->vbus_supply, value);
>>> +    if (ret)
>>> +        debug("XHCI: Failed to set vbus supply\n");
>> That shouldn't be debug, that's a printf() because it's actually a
>> failure. Or error() I guess ...
> Considering the case vbus is always on, it is right with no vbus regulator.
> So maybe it's not an error actually.

Regulator failed to turn on, that's an error, right ?
Why would VBUS be always on ? You can just add a GPIO-regulator into the
DT and then the VBUS is no longer always on.

>>> +    return ret;
>>> +}
>>> +#else
>>> +static int rockchip_xhci_set_vbus(struct rockchip_xhci_platdata *plat,
>>> +                  bool value)
>>> +{
>>> +    return 0;
>>> +}
>>> +#endif
>>> +
>>>   /*
>>>    * rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3
>>> Core
>>>    * @dwc: Pointer to our controller context structure
>>> @@ -153,9 +175,7 @@ static int xhci_usb_probe(struct udevice *dev)
>>>       hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd +
>>>               HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase)));
>>>   -    /* setup the Vbus gpio here */
>>> -    if (dm_gpio_is_valid(&plat->vbus_gpio))
>>> -        dm_gpio_set_value(&plat->vbus_gpio, 1);
>>> +    rockchip_xhci_set_vbus(plat, true);
>> What about handling the return value ?
> The return value can be ignored when vbus is always on. So is it enough
> just print message in
> the rockchip_xhci_set_vbus function?

See above, why is VBUS always on

>>
>>>       ret = rockchip_xhci_core_init(ctx, dev);
>>>       if (ret) {
>>> @@ -168,6 +188,7 @@ static int xhci_usb_probe(struct udevice *dev)
>>>     static int xhci_usb_remove(struct udevice *dev)
>>>   {
>>> +    struct rockchip_xhci_platdata *plat = dev_get_platdata(dev);
>>>       struct rockchip_xhci *ctx = dev_get_priv(dev);
>>>       int ret;
>>>   @@ -178,6 +199,8 @@ static int xhci_usb_remove(struct udevice *dev)
>>>       if (ret)
>>>           return ret;
>>>   +    rockchip_xhci_set_vbus(plat, false);
>> Handle return value
>>
>>>       return 0;
>>>   }
>>>  
>>
> 
> 


-- 
Best regards,
Marek Vasut