Messages in this thread Patch in this message | | | Subject | Re: [PATCH v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS | From | David Lechner <> | Date | Tue, 22 Nov 2016 14:58:37 -0600 |
| |
On 11/22/2016 02:46 PM, Axel Haslam wrote: > On Tue, Nov 22, 2016 at 9:37 PM, David Lechner <david@lechnology.com> 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@baylibre.com> >>> --- >>> 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);
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)) { >>> >>
| |