Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756991AbcKVU6q (ORCPT ); Tue, 22 Nov 2016 15:58:46 -0500 Received: from vern.gendns.com ([206.190.152.46]:40893 "EHLO vern.gendns.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756968AbcKVU6n (ORCPT ); Tue, 22 Nov 2016 15:58:43 -0500 Subject: Re: [PATCH v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS To: Axel Haslam References: <20161121163014.28008-1-ahaslam@baylibre.com> <20161121163014.28008-4-ahaslam@baylibre.com> Cc: Greg KH , Sekhar Nori , Kevin Hilman , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org From: David Lechner Message-ID: <73a66052-eeec-bb9a-de7c-3de8a441d1e8@lechnology.com> Date: Tue, 22 Nov 2016 14:58:37 -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: Content-Type: text/plain; charset=utf-8; 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: 9631 Lines: 283 On 11/22/2016 02:46 PM, Axel Haslam wrote: > On Tue, Nov 22, 2016 at 9:37 PM, David Lechner 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 >>> --- >>> 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); 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)) { >>> >>