| Message ID | 1461675460-2295-8-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Jisheng Zhang <jszhang@marvell.com> writes: > The Marvell BG4CT STB board has board level vbus control through gpio. > This patch adds the vbus regulator control to support this board. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > drivers/usb/host/xhci-plat.c | 40 +++++++++++++++++++++++++++++++++++++++- > drivers/usb/host/xhci.h | 2 ++ > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index d7f4f3c..0310c13 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -18,6 +18,7 @@ > #include <linux/of.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/usb/phy.h> > #include <linux/usb/xhci_pdriver.h> > @@ -178,6 +179,7 @@ static int xhci_plat_probe(struct platform_device *pdev) > struct clk *clk; > struct usb_phy *usb_phy; > struct phy *phy; > + struct regulator *vbus; > int ret; > int irq; > > @@ -249,13 +251,30 @@ static int xhci_plat_probe(struct platform_device *pdev) > > device_wakeup_enable(hcd->self.controller); > > + vbus = devm_regulator_get(&pdev->dev, "vbus"); devm_regulator_get_optional() ?? > + if (PTR_ERR(vbus) == -ENODEV) { > + vbus = NULL; > + } else if (IS_ERR(vbus)) { > + ret = PTR_ERR(vbus); > + goto disable_clk; > + } else if (vbus) { > + ret = regulator_enable(vbus); > + if (ret) { > + dev_err(&pdev->dev, > + "failed to enable usb vbus regulator: %d\n", > + ret); > + goto disable_clk; > + } > + } > + > xhci->clk = clk; > + xhci->vbus = vbus; > xhci->main_hcd = hcd; > xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev, > dev_name(&pdev->dev), hcd); > if (!xhci->shared_hcd) { > ret = -ENOMEM; > - goto disable_clk; > + goto disable_vbus; > } > > if ((node && of_property_read_bool(node, "usb3-lpm-capable")) || > @@ -323,6 +342,10 @@ disable_usb2_phy: > put_usb3_hcd: > usb_put_hcd(xhci->shared_hcd); > > +disable_vbus: > + if (vbus) > + regulator_disable(vbus); > + > disable_clk: > clk_disable_unprepare(clk); > > @@ -337,6 +360,7 @@ static int xhci_plat_remove(struct platform_device *dev) > struct usb_hcd *hcd = platform_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > struct clk *clk = xhci->clk; > + struct regulator *vbus = xhci->vbus; > > usb_remove_hcd(xhci->shared_hcd); > xhci_plat_phy_exit(xhci->shared_hcd); > @@ -347,6 +371,9 @@ static int xhci_plat_remove(struct platform_device *dev) > clk_disable_unprepare(clk); > usb_put_hcd(hcd); > > + if (vbus) > + regulator_disable(vbus); > + > return 0; > } > > @@ -356,6 +383,7 @@ static int xhci_plat_suspend(struct device *dev) > int ret; > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + struct regulator *vbus = xhci->vbus; > > /* > * xhci_suspend() needs `do_wakeup` to know whether host is allowed > @@ -373,6 +401,9 @@ static int xhci_plat_suspend(struct device *dev) > xhci_plat_phy_exit(hcd); > clk_disable_unprepare(xhci->clk); > > + if (vbus) > + ret = regulator_disable(vbus); > + > return ret; > } > > @@ -381,11 +412,18 @@ static int xhci_plat_resume(struct device *dev) > int ret; > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + struct regulator *vbus = xhci->vbus; > > ret = clk_prepare_enable(xhci->clk); > if (ret) > return ret; > > + if (vbus) { > + ret = regulator_enable(vbus); > + if (ret) > + return ret; > + } > + > ret = xhci_plat_phy_init(hcd); > if (ret) > return ret; > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 6c629c9..5fa8662 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1550,6 +1550,8 @@ struct xhci_hcd { > struct msix_entry *msix_entries; > /* optional clock */ > struct clk *clk; > + /* optional regulator */ > + struct regulator *vbus; > /* data structures */ > struct xhci_device_context_array *dcbaa; > struct xhci_ring *cmd_ring; > -- > 2.8.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote: > Jisheng Zhang <jszhang@marvell.com> writes: > > + vbus = devm_regulator_get(&pdev->dev, "vbus"); > devm_regulator_get_optional() ?? Does USB work without a VBUS? Unless the answer is yes then I'd expect this to be just a normal regulator_get(). > > > + if (PTR_ERR(vbus) == -ENODEV) { > > + vbus = NULL; > > + } else if (IS_ERR(vbus)) { > > + ret = PTR_ERR(vbus); > > + goto disable_clk; > > + } else if (vbus) { > > + ret = regulator_enable(vbus); > > + if (ret) { > > + dev_err(&pdev->dev, > > + "failed to enable usb vbus regulator: %d\n", > > + ret); > > + goto disable_clk; > > + } > > + } This is all completely broken unless the supply is optional.
Dear Mark, On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote: > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote: > > Jisheng Zhang <jszhang@marvell.com> writes: > > > > + vbus = devm_regulator_get(&pdev->dev, "vbus"); > > > devm_regulator_get_optional() ?? > > Does USB work without a VBUS? Unless the answer is yes then I'd expect > this to be just a normal regulator_get(). Per spec no. But the vbus may be transparent to SW on some platforms, so I think devm_regulator_get_optional() is better. > > > > > > + if (PTR_ERR(vbus) == -ENODEV) { > > > + vbus = NULL; > > > + } else if (IS_ERR(vbus)) { > > > + ret = PTR_ERR(vbus); > > > + goto disable_clk; > > > + } else if (vbus) { > > > + ret = regulator_enable(vbus); > > > + if (ret) { > > > + dev_err(&pdev->dev, > > > + "failed to enable usb vbus regulator: %d\n", > > > + ret); > > > + goto disable_clk; > > > + } > > > + } > > This is all completely broken unless the supply is optional. The supply is optional. Thanks for your review, Jisheng
Hi, Mark Brown <broonie@kernel.org> writes: > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote: >> Jisheng Zhang <jszhang@marvell.com> writes: > >> > + vbus = devm_regulator_get(&pdev->dev, "vbus"); > >> devm_regulator_get_optional() ?? > > Does USB work without a VBUS? Unless the answer is yes then I'd expect <joke> it can, just source VBUS from a lab power supply </joke> > this to be just a normal regulator_get(). jokes aside, this regulator is optional because not all platforms require a SW controlled regulator, no ? Will normal regulator_get() give us a dummy regulator in case it's not listed in DT/ACPI ?
On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote: > Mark Brown <broonie@kernel.org> writes: > > this to be just a normal regulator_get(). > jokes aside, this regulator is optional because not all platforms > require a SW controlled regulator, no ? Will normal regulator_get() give > us a dummy regulator in case it's not listed in DT/ACPI ? Yes we do that, but even regulators that are not software controlled should really be described anyway since it's a much simpler rule for people to understand, it ensures that we can just scale up on systems where there does happen to be software control and it makes all the resulting code much simpler and hence less error prone if we're not randomly ignoring some errors.
Hi, Mark Brown <broonie@kernel.org> writes: > On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote: >> Mark Brown <broonie@kernel.org> writes: > >> > this to be just a normal regulator_get(). > >> jokes aside, this regulator is optional because not all platforms >> require a SW controlled regulator, no ? Will normal regulator_get() give >> us a dummy regulator in case it's not listed in DT/ACPI ? > > Yes we do that, but even regulators that are not software controlled okay, good. > should really be described anyway since it's a much simpler rule for okay, we'll wait until all vendors update their ACPI tables ;-) > people to understand, it ensures that we can just scale up on systems > where there does happen to be software control and it makes all the > resulting code much simpler and hence less error prone if we're not > randomly ignoring some errors.
On Wed, Apr 27, 2016 at 06:25:16PM +0800, Jisheng Zhang wrote: > On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote: > > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote: > > > > + vbus = devm_regulator_get(&pdev->dev, "vbus"); > > > devm_regulator_get_optional() ?? > > Does USB work without a VBUS? Unless the answer is yes then I'd expect > > this to be just a normal regulator_get(). > Per spec no. But the vbus may be transparent to SW on some platforms, so I > think devm_regulator_get_optional() is better. Really, no. If it's physically there the software needs to be written as such. Please see the documentation and list archives for extensive discussion on this topic. > > This is all completely broken unless the supply is optional. > The supply is optional. To repeat a supply is only optional if it might be physically absent.
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index d7f4f3c..0310c13 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -18,6 +18,7 @@ #include <linux/of.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/usb/phy.h> #include <linux/usb/xhci_pdriver.h> @@ -178,6 +179,7 @@ static int xhci_plat_probe(struct platform_device *pdev) struct clk *clk; struct usb_phy *usb_phy; struct phy *phy; + struct regulator *vbus; int ret; int irq; @@ -249,13 +251,30 @@ static int xhci_plat_probe(struct platform_device *pdev) device_wakeup_enable(hcd->self.controller); + vbus = devm_regulator_get(&pdev->dev, "vbus"); + if (PTR_ERR(vbus) == -ENODEV) { + vbus = NULL; + } else if (IS_ERR(vbus)) { + ret = PTR_ERR(vbus); + goto disable_clk; + } else if (vbus) { + ret = regulator_enable(vbus); + if (ret) { + dev_err(&pdev->dev, + "failed to enable usb vbus regulator: %d\n", + ret); + goto disable_clk; + } + } + xhci->clk = clk; + xhci->vbus = vbus; xhci->main_hcd = hcd; xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev, dev_name(&pdev->dev), hcd); if (!xhci->shared_hcd) { ret = -ENOMEM; - goto disable_clk; + goto disable_vbus; } if ((node && of_property_read_bool(node, "usb3-lpm-capable")) || @@ -323,6 +342,10 @@ disable_usb2_phy: put_usb3_hcd: usb_put_hcd(xhci->shared_hcd); +disable_vbus: + if (vbus) + regulator_disable(vbus); + disable_clk: clk_disable_unprepare(clk); @@ -337,6 +360,7 @@ static int xhci_plat_remove(struct platform_device *dev) struct usb_hcd *hcd = platform_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct clk *clk = xhci->clk; + struct regulator *vbus = xhci->vbus; usb_remove_hcd(xhci->shared_hcd); xhci_plat_phy_exit(xhci->shared_hcd); @@ -347,6 +371,9 @@ static int xhci_plat_remove(struct platform_device *dev) clk_disable_unprepare(clk); usb_put_hcd(hcd); + if (vbus) + regulator_disable(vbus); + return 0; } @@ -356,6 +383,7 @@ static int xhci_plat_suspend(struct device *dev) int ret; struct usb_hcd *hcd = dev_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); + struct regulator *vbus = xhci->vbus; /* * xhci_suspend() needs `do_wakeup` to know whether host is allowed @@ -373,6 +401,9 @@ static int xhci_plat_suspend(struct device *dev) xhci_plat_phy_exit(hcd); clk_disable_unprepare(xhci->clk); + if (vbus) + ret = regulator_disable(vbus); + return ret; } @@ -381,11 +412,18 @@ static int xhci_plat_resume(struct device *dev) int ret; struct usb_hcd *hcd = dev_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); + struct regulator *vbus = xhci->vbus; ret = clk_prepare_enable(xhci->clk); if (ret) return ret; + if (vbus) { + ret = regulator_enable(vbus); + if (ret) + return ret; + } + ret = xhci_plat_phy_init(hcd); if (ret) return ret; diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 6c629c9..5fa8662 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1550,6 +1550,8 @@ struct xhci_hcd { struct msix_entry *msix_entries; /* optional clock */ struct clk *clk; + /* optional regulator */ + struct regulator *vbus; /* data structures */ struct xhci_device_context_array *dcbaa; struct xhci_ring *cmd_ring;
The Marvell BG4CT STB board has board level vbus control through gpio. This patch adds the vbus regulator control to support this board. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- drivers/usb/host/xhci-plat.c | 40 +++++++++++++++++++++++++++++++++++++++- drivers/usb/host/xhci.h | 2 ++ 2 files changed, 41 insertions(+), 1 deletion(-)