Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753966AbcKUKXU (ORCPT ); Mon, 21 Nov 2016 05:23:20 -0500 Received: from mail-qk0-f181.google.com ([209.85.220.181]:35357 "EHLO mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753869AbcKUKXS (ORCPT ); Mon, 21 Nov 2016 05:23:18 -0500 MIME-Version: 1.0 In-Reply-To: <26d7b4c1-a7ef-7250-eb6a-270182d279ea@lechnology.com> References: <20161114144103.12120-4-ahaslam@baylibre.com> <26d7b4c1-a7ef-7250-eb6a-270182d279ea@lechnology.com> From: Axel Haslam Date: Mon, 21 Nov 2016 11:22:36 +0100 Message-ID: Subject: Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS To: David Lechner Cc: Greg KH , Alan Stern , Kevin Hilman , kishon@ti.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9396 Lines: 304 Hi David, Thanks for the review, On Sun, Nov 20, 2016 at 4:31 AM, David Lechner wrote: > On 11/14/2016 08:41 AM, ahaslam@baylibre.com 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 | 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 >> #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 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)) { >> >