Re: [PATCH v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

[David Lechner <david@lechnology.com>]

On 11/22/2016 02:46 PM, Axel Haslam wrote:
> On Tue, Nov 22, 2016 at 9:37 PM, David Lechner <david@lechnology.com> wrote:
>> On 11/21/2016 10:30 AM, Axel Haslam wrote:
>>>
>>> Using a regulator to handle VBUS will eliminate the need for
>>> platform data and callbacks, and make the driver more generic
>>> allowing different types of regulators to handle VBUS.
>>>
>>> The regulator equivalents to the platform callbacks are:
>>>     set_power   ->  regulator_enable/regulator_disable
>>>     get_power   ->  regulator_is_enabled
>>>     get_oci     ->  regulator_get_error_flags
>>>     ocic_notify ->  regulator event notification
>>>
>>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>>> ---
>>>  drivers/usb/host/ohci-da8xx.c | 97
>>> +++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 94 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>>> index 90763ad..d0eb754 100644
>>> --- a/drivers/usb/host/ohci-da8xx.c
>>> +++ b/drivers/usb/host/ohci-da8xx.c
>>> @@ -20,6 +20,7 @@
>>>  #include <linux/platform_device.h>
>>>  #include <linux/phy/phy.h>
>>>  #include <linux/platform_data/usb-davinci.h>
>>> +#include <linux/regulator/consumer.h>
>>>  #include <linux/usb.h>
>>>  #include <linux/usb/hcd.h>
>>>  #include <asm/unaligned.h>
>>> @@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd
>>> *hcd, u16 typeReq,
>>>  static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>>>
>>>  struct da8xx_ohci_hcd {
>>> +       struct usb_hcd *hcd;
>>>         struct clk *usb11_clk;
>>>         struct phy *usb11_phy;
>>> +       struct regulator *vbus_reg;
>>> +       struct notifier_block nb;
>>> +       unsigned int reg_enabled;
>>>  };
>>>
>>>  #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd
>>> *)(hcd_to_ohci(hcd)->priv)
>>> @@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
>>>
>>>  static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
>>>  {
>>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>>         struct device *dev              = hcd->self.controller;
>>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>> +       int ret;
>>>
>>>         if (hub && hub->set_power)
>>>                 return hub->set_power(1, on);
>>>
>>> +       if (!da8xx_ohci->vbus_reg)
>>> +               return 0;
>>> +
>>> +       if (on && !da8xx_ohci->reg_enabled) {
>>> +               ret = regulator_enable(da8xx_ohci->vbus_reg);
>>> +               if (ret) {
>>> +                       dev_err(dev, "Failed to enable regulator: %d\n",
>>> ret);
>>> +                       return ret;
>>> +               }
>>> +               da8xx_ohci->reg_enabled = 1;
>>> +
>>> +       } else if (!on && da8xx_ohci->reg_enabled) {
>>> +               ret = regulator_disable(da8xx_ohci->vbus_reg);
>>> +               if (ret) {
>>> +                       dev_err(dev, "Failed  to disable regulator: %d\n",
>>> ret);
>>> +                       return ret;
>>> +               }
>>> +               da8xx_ohci->reg_enabled = 0;
>>> +       }
>>> +
>>>         return 0;
>>>  }
>>>
>>>  static int ohci_da8xx_get_power(struct usb_hcd *hcd)
>>>  {
>>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>>         struct device *dev              = hcd->self.controller;
>>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>>
>>>         if (hub && hub->get_power)
>>>                 return hub->get_power(1);
>>>
>>> +       if (da8xx_ohci->vbus_reg)
>>> +               return regulator_is_enabled(da8xx_ohci->vbus_reg);
>>> +
>>>         return 1;
>>>  }
>>>
>>>  static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
>>>  {
>>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>>         struct device *dev              = hcd->self.controller;
>>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>> +       unsigned int flags;
>>> +       int ret;
>>>
>>>         if (hub && hub->get_oci)
>>>                 return hub->get_oci(1);
>>>
>>> +       if (!da8xx_ohci->vbus_reg)
>>> +               return 0;
>>> +
>>> +       ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, &flags);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       if (flags & REGULATOR_ERROR_OVER_CURRENT)
>>> +               return 1;
>>> +
>>>         return 0;
>>>  }
>>>
>>>  static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
>>>  {
>>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>>         struct device *dev              = hcd->self.controller;
>>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>>
>>>         if (hub && hub->set_power)
>>>                 return 1;
>>>
>>> +       if (da8xx_ohci->vbus_reg)
>>> +               return 1;
>>> +
>>>         return 0;
>>>  }
>>>
>>>  static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
>>>  {
>>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>>         struct device *dev              = hcd->self.controller;
>>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>>
>>>         if (hub && hub->get_oci)
>>>                 return 1;
>>>
>>> +       if (da8xx_ohci->vbus_reg)
>>> +               return 1;
>>> +
>>>         return 0;
>>>  }
>>>
>>> @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
>>> da8xx_ohci_root_hub *hub,
>>>                 hub->set_power(port, 0);
>>>  }
>>>
>>> +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
>>> +                               unsigned long event, void *data)
>>> +{
>>> +       struct da8xx_ohci_hcd *da8xx_ohci =
>>> +                               container_of(nb, struct da8xx_ohci_hcd,
>>> nb);
>>> +       struct device *dev = da8xx_ohci->hcd->self.controller;
>>> +
>>> +       if (event & REGULATOR_EVENT_OVER_CURRENT) {
>>> +               dev_warn(dev, "over current event\n");
>>> +               ocic_mask |= 1;
>>
>>
>> Following up from a v5 thread that is still applicable here, Axel said:
>>
>>> I did a couple of tests, and i don't get those prints do you get them?.
>>
>> The problem here is that ocic_mask |= 1; is wrong. It needs to be...
>>
>>         ocic_mask |= (1 << 1);

Actually parentheses are not needed

	ocic_mask |= 1 << 1;

>>
>
> i see, i will fix it thanks!
>
>> If you look at the other uses of ocic_mask, you will see why this it needs
>> to be this way. Once you make this change, then you will see this in the
>> kernel log:
>>
>>   usb 1-1: USB disconnect, device number 2
>>   usb 1-1: may be reset is needed?..
>>   ohci ohci: over current event
>>   usb usb1-port1: over-current condition
>>
>> So, we don't need the dev_warn() here.
>
> agree!
>
>>
>>
>> More from Axel:
>>
>>> What i understand is that they happen when we get a hub event that is
>>> reporting the over current. Which is what the root hub of the davinci
>>> system
>>> does not have, and why we use gpios instead).
>>
>> Even though the hardware is not capable of detecting the overcurrent by
>> itself, we are poking the registers in ohci_da8xx_hub_control(), so the core
>> hub driver sees it just the same as if the hardware itself changed the
>> register.
>>
>>
>>> +               ohci_da8xx_set_power(da8xx_ohci->hcd, 0);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  static int ohci_da8xx_register_notify(struct usb_hcd *hcd)
>>>  {
>>> +       struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>>>         struct device *dev              = hcd->self.controller;
>>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>> +       int ret = 0;
>>> +
>>> +       if (hub && hub->ocic_notify) {
>>> +               ret = hub->ocic_notify(ohci_da8xx_ocic_handler);
>>> +       } else if (da8xx_ohci->vbus_reg) {
>>> +               da8xx_ohci->nb.notifier_call = ohci_da8xx_regulator_event;
>>> +               ret =
>>> devm_regulator_register_notifier(da8xx_ohci->vbus_reg,
>>> +                                               &da8xx_ohci->nb);
>>> +       }
>>>
>>> -       if (hub && hub->ocic_notify)
>>> -               return hub->ocic_notify(ohci_da8xx_ocic_handler);
>>> +       if (ret)
>>> +               dev_err(dev, "Failed to register notifier: %d\n", ret);
>>>
>>> -       return 0;
>>> +       return ret;
>>>  }
>>>
>>>  static void ohci_da8xx_unregister_notify(struct usb_hcd *hcd)
>>> @@ -331,6 +409,7 @@ static int ohci_da8xx_probe(struct platform_device
>>> *pdev)
>>>                 return -ENOMEM;
>>>
>>>         da8xx_ohci = to_da8xx_ohci(hcd);
>>> +       da8xx_ohci->hcd = hcd;
>>>
>>>         da8xx_ohci->usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>>>         if (IS_ERR(da8xx_ohci->usb11_clk)) {
>>> @@ -348,6 +427,18 @@ static int ohci_da8xx_probe(struct platform_device
>>> *pdev)
>>>                 goto err;
>>>         }
>>>
>>> +       da8xx_ohci->vbus_reg = devm_regulator_get_optional(&pdev->dev,
>>> "vbus");
>>> +       if (IS_ERR(da8xx_ohci->vbus_reg)) {
>>> +               error = PTR_ERR(da8xx_ohci->vbus_reg);
>>> +               if (error == -ENODEV) {
>>> +                       da8xx_ohci->vbus_reg = NULL;
>>> +               } else {
>>
>>
>> It might be good to skip the dev_err() if we get -EPROBE_DEFER
>>
>>> +                       dev_err(&pdev->dev,
>>> +                               "Failed to get regulator\n");
>>> +                       goto err;
>>> +               }
>>> +       }
>>> +
>>>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>         hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
>>>         if (IS_ERR(hcd->regs)) {
>>>
>>