Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756655AbcKVUhU (ORCPT ); Tue, 22 Nov 2016 15:37:20 -0500 Received: from vern.gendns.com ([206.190.152.46]:39545 "EHLO vern.gendns.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752188AbcKVUhS (ORCPT ); Tue, 22 Nov 2016 15:37:18 -0500 Subject: Re: [PATCH v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS To: Axel Haslam , gregkh@linuxfoundation.org, nsekhar@ti.com, khilman@baylibre.com References: <20161121163014.28008-1-ahaslam@baylibre.com> <20161121163014.28008-4-ahaslam@baylibre.com> Cc: linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org From: David Lechner Message-ID: Date: Tue, 22 Nov 2016 14:37:17 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161121163014.28008-4-ahaslam@baylibre.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - vern.gendns.com X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - lechnology.com X-Get-Message-Sender-Via: vern.gendns.com: authenticated_id: davidmain+lechnology.com/only user confirmed/virtual account not confirmed X-Authenticated-Sender: vern.gendns.com: davidmain@lechnology.com X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7600 Lines: 255 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 > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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); 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. 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)) { >