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

[Axel Haslam]

Hi David,

Thanks for the review,


On Sun, Nov 20, 2016 at 4:31 AM, David Lechner <david@xxxxxxxxxxxxxx> wrote:
> On 11/14/2016 08:41 AM, ahaslam@xxxxxxxxxxxx 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@xxxxxxxxxxxx>
>> ---
>>  drivers/usb/host/ohci-da8xx.c | 95
>> ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index 83e3c98..42eaeb9 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 is_powered;
>
>
> Since is_powered is only used to indicate if we have called
> regulator_enable(), I think it would make more sense to name it reg_enabled
> instead.

ok.

>
>>  };
>>
>>  #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->is_powered) {
>> +               ret = regulator_enable(da8xx_ohci->vbus_reg);
>> +               if (ret) {
>> +                       dev_err(dev, "Fail to enable regulator: %d\n",
>> ret);
>
>
> s/Fail/Failed/

will fix in both places.

>
>> +                       return ret;
>> +               }
>> +               da8xx_ohci->is_powered = 1;
>> +
>> +       } else if (!on && da8xx_ohci->is_powered) {
>> +               ret = regulator_disable(da8xx_ohci->vbus_reg);
>> +               if (ret) {
>> +                       dev_err(dev, "Fail to disable regulator: %d\n",
>> ret);
>
>
> s/Fail/Failed/
>
>> +                       return ret;
>> +               }
>> +               da8xx_ohci->is_powered = 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)
>
>
> Is this supposed to be...

yes!

>
>         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");
>
>
> Won't this result in duplicate overcurrent warnings in the kernel log? It
> seems like in previous version of this patch series, we would get an
> overcurrent error from the core usb driver.

you mean in the regulator driver? i did not make changes to core usb.
but, no,  i did not add a print in the fixed regulator driver itself. Since
the regulator is  a separate driver, and could be implemented with or without
a trace, i think its better to leave this print. It shows that the usb driver
has well received the notification.

>
>> +               ocic_mask |= 1;
>
>
> I thought that a previous patch got rid of all globals. Why is ocic_mask
> still a global variable?
>

its is used in the platform callbacks which will be removed,
and i will remove it in future series. but you already saw this ;)

>> +               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)
>> -               return hub->ocic_notify(ohci_da8xx_ocic_handler);
>> +               ret = hub->ocic_notify(ohci_da8xx_ocic_handler);
>
>
> nit: add { } around the if body too since there is { } around the else if
> body.

will do.

>
>> +       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);
>> +       }
>>
>> -       return 0;
>> +       if (ret)
>> +               dev_err(dev, "Failed to register notifier: %d\n", ret);
>> +
>> +       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 {
>> +                       dev_err(&pdev->dev,
>> +                               "Failed to get regulator: %d\n", error);
>
>
> I'm pretty sure that the probe error number is displayed elsewhere in the
> kernel log, so probably no need for %d here.

Ok i will remove the %d.

>
>> +                       goto err;
>> +               }
>> +       }
>> +
>>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>         hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
>>         if (IS_ERR(hcd->regs)) {
>>
>