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

From: Axel Haslam
Date: Tue Nov 22 2016 - 15:47:19 EST

On Tue, Nov 22, 2016 at 9:37 PM, David Lechner <david@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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);
>

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