| Message ID | 1372240781-1017-2-git-send-email-mperttunen@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 06/26/2013 03:59 AM, Mikko Perttunen wrote: > After this patch, usb vbus regulators for tegra usb phy devices can be specified > with the device tree attribute vbus-supply = <&x> where x is a regulator defined > in the device tree. > diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c > @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy *phy) > return PTR_ERR(phy->pad_clk); > } > > + phy->vbus = devm_regulator_get(phy->dev, "vbus"); > + /* On some boards, the VBUS regulator doesn't need to be controlled */ > + if (IS_ERR(phy->vbus)) { > + if (PTR_ERR(phy->vbus) == -ENODEV) { > + dev_notice(phy->dev, "no vbus regulator"); > + phy->vbus = NULL; > + } else { > + return PTR_ERR(phy->vbus); > + } > + } I think this code should be added to some more core initialization function; IIRC, there are separate utmip_pad_open() and some other function for ULPI mode, and in the future there may be more for HSIC, etc. For the error-handling, I think it'd be better to do: * If property doesn't exist, set phy->vbus to some error value, e.g. ERR_PTR(-ENODEV). * If property does exist, call devm_regulator_get(). ** If devm_regulator_get() returned any error, return it. Or, does devm_regulator_get() return -ENODEV if-and-only-if the vbus-supply DT property does not exist? and ... > @@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct tegra_usb_phy *phy) > spin_unlock_irqrestore(&utmip_pad_lock, flags); > > clk_disable_unprepare(phy->pad_clk); > + > + if (phy->vbus) { Here, check if (IS_ERR(phy->vbus) instead. The reason is if devm_regulator_get() returns either a valid value or an error-pointer, then NULL could in theory be a valid value (it's up the the regulator API to determine that), and hence this code shouldn't assume that it can use NULL to represent "no regulator". -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/26/2013 03:59 AM, Mikko Perttunen wrote: >> After this patch, usb vbus regulators for tegra usb phy devices can be >> specified >> with the device tree attribute vbus-supply = <&x> where x is a >> regulator defined >> in the device tree. > >> diff --git a/drivers/usb/phy/phy-tegra-usb.c >> b/drivers/usb/phy/phy-tegra-usb.c > >> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy >> *phy) >> return PTR_ERR(phy->pad_clk); >> } >> >> + phy->vbus = devm_regulator_get(phy->dev, "vbus"); >> + /* On some boards, the VBUS regulator doesn't need to be controlled */ >> + if (IS_ERR(phy->vbus)) { >> + if (PTR_ERR(phy->vbus) == -ENODEV) { >> + dev_notice(phy->dev, "no vbus regulator"); >> + phy->vbus = NULL; >> + } else { >> + return PTR_ERR(phy->vbus); >> + } >> + } > > I think this code should be added to some more core initialization > function; IIRC, there are separate utmip_pad_open() and some other > function for ULPI mode, and in the future there may be more for HSIC, > etc. I don't think ULPI and VBUS have a VBUS pin, though. The pinmux doesn't even list a pin for USB2 which is a ULPI/HSIC only controller. > > For the error-handling, I think it'd be better to do: > > * If property doesn't exist, set phy->vbus to some error value, e.g. > ERR_PTR(-ENODEV). > * If property does exist, call devm_regulator_get(). > ** If devm_regulator_get() returned any error, return it. > > Or, does devm_regulator_get() return -ENODEV if-and-only-if the > vbus-supply DT property does not exist? Yes, devm_regulator_get uses regulator_dev_lookup which returns -ENODEV if of_reg_regulator returns NULL, and there is no other place a -ENODEV could be returned from. Comment in regulator_dev_lookup: /* * If we couldn't even get the node then it's * not just that the device didn't register * yet, there's no node and we'll never * succeed. */ *ret = -ENODEV; > > and ... > >> @@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct >> tegra_usb_phy *phy) >> spin_unlock_irqrestore(&utmip_pad_lock, flags); >> >> clk_disable_unprepare(phy->pad_clk); >> + >> + if (phy->vbus) { > > Here, check if (IS_ERR(phy->vbus) instead. The reason is if > devm_regulator_get() returns either a valid value or an error-pointer, > then NULL could in theory be a valid value (it's up the the regulator > API to determine that), and hence this code shouldn't assume that it can > use NULL to represent "no regulator". I'll fix this along with the problems with the other patches. - Mikko -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/27/2013 01:20 AM, Mikko Perttunen wrote: > On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren > <swarren@wwwdotorg.org> wrote: > >> On 06/26/2013 03:59 AM, Mikko Perttunen wrote: >>> After this patch, usb vbus regulators for tegra usb phy devices can >>> be specified >>> with the device tree attribute vbus-supply = <&x> where x is a >>> regulator defined >>> in the device tree. >> >>> diff --git a/drivers/usb/phy/phy-tegra-usb.c >>> b/drivers/usb/phy/phy-tegra-usb.c >> >>> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy >>> *phy) >>> return PTR_ERR(phy->pad_clk); >>> } >>> >>> + phy->vbus = devm_regulator_get(phy->dev, "vbus"); >>> + /* On some boards, the VBUS regulator doesn't need to be >>> controlled */ >>> + if (IS_ERR(phy->vbus)) { >>> + if (PTR_ERR(phy->vbus) == -ENODEV) { >>> + dev_notice(phy->dev, "no vbus regulator"); >>> + phy->vbus = NULL; >>> + } else { >>> + return PTR_ERR(phy->vbus); >>> + } >>> + } >> >> I think this code should be added to some more core initialization >> function; IIRC, there are separate utmip_pad_open() and some other >> function for ULPI mode, and in the future there may be more for HSIC, >> etc. > > I don't think ULPI and VBUS have a VBUS pin, though. The pinmux doesn't > even list a pin for USB2 which is a ULPI/HSIC only controller. IIUC, ULPI, UTMI, and HSIC are just different physical layers for the USB data signals between Tegra and whatever is connected. Any of those could be translated to regular USB ports on the board. Certainly, UTMI is the common one for external ports, and we have Tegra boards with ULPI PHYs that translate to regular external ports. I don't know about HSIC; it's less common, but I see no reason one couldn't have a PHY that translates it to an external USB socket just like the rest. On Tegra, most of the USB signals aren't pinmux'd at all. There's also aren't dedicated VBUS control pins, so you'd never find them in the pinmux. Instead, a GPIO is used to gate a power signal, or perhaps in some cases we might program a PMIC directly. As such, I'm pretty sure we should just make VBUS handling entirely independent from PHY type. >> For the error-handling, I think it'd be better to do: >> >> * If property doesn't exist, set phy->vbus to some error value, e.g. >> ERR_PTR(-ENODEV). >> * If property does exist, call devm_regulator_get(). >> ** If devm_regulator_get() returned any error, return it. >> >> Or, does devm_regulator_get() return -ENODEV if-and-only-if the >> vbus-supply DT property does not exist? > > Yes, devm_regulator_get uses regulator_dev_lookup which returns -ENODEV > if of_reg_regulator returns NULL, and there is no other place a -ENODEV > could be returned from. Comment in regulator_dev_lookup: > > /* > * If we couldn't even get the node then it's > * not just that the device didn't register > * yet, there's no node and we'll never > * succeed. > */ > *ret = -ENODEV; I believe that of_parse_phandle() can fail for a lot more reasons than just "property doesn't exist", (yes, I just checked: if the property has too little data, it will also error out), so while "property doesn't exist" will trigger a NULL then -ENODEV return, there are also other error cases that return that value, so we should take care to differentiate them, with an explicit check whether the property exists. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 27, 2013 at 06:15:02PM +0200, Stephen Warren wrote: > On 06/27/2013 01:20 AM, Mikko Perttunen wrote: > > On Wed, 26 Jun 2013 20:14:35 +0300, Stephen Warren > > <swarren@wwwdotorg.org> wrote: > > > >> On 06/26/2013 03:59 AM, Mikko Perttunen wrote: > >>> After this patch, usb vbus regulators for tegra usb phy devices can > >>> be specified > >>> with the device tree attribute vbus-supply = <&x> where x is a > >>> regulator defined > >>> in the device tree. > >> > >>> diff --git a/drivers/usb/phy/phy-tegra-usb.c > >>> b/drivers/usb/phy/phy-tegra-usb.c > >> > >>> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy > >>> *phy) > >>> return PTR_ERR(phy->pad_clk); > >>> } > >>> > >>> + phy->vbus = devm_regulator_get(phy->dev, "vbus"); > >>> + /* On some boards, the VBUS regulator doesn't need to be > >>> controlled */ > >>> + if (IS_ERR(phy->vbus)) { > >>> + if (PTR_ERR(phy->vbus) == -ENODEV) { > >>> + dev_notice(phy->dev, "no vbus regulator"); > >>> + phy->vbus = NULL; > >>> + } else { > >>> + return PTR_ERR(phy->vbus); > >>> + } > >>> + } > >> > >> I think this code should be added to some more core initialization > >> function; IIRC, there are separate utmip_pad_open() and some other > >> function for ULPI mode, and in the future there may be more for HSIC, > >> etc. > > > > I don't think ULPI and VBUS have a VBUS pin, though. The pinmux doesn't > > even list a pin for USB2 which is a ULPI/HSIC only controller. > > IIUC, ULPI, UTMI, and HSIC are just different physical layers for the > USB data signals between Tegra and whatever is connected. Any of those > could be translated to regular USB ports on the board. Certainly, UTMI > is the common one for external ports, and we have Tegra boards with ULPI > PHYs that translate to regular external ports. I don't know about HSIC; > it's less common, but I see no reason one couldn't have a PHY that > translates it to an external USB socket just like the rest. > AFAIK HSIC is meant to attach eg modems or other onboard peripherals to USB without having the (power) overhead of a PHY on both host and device side. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c index d7d6bd7..6122590 100644 --- a/drivers/usb/phy/phy-tegra-usb.c +++ b/drivers/usb/phy/phy-tegra-usb.c @@ -34,6 +34,7 @@ #include <asm/mach-types.h> #include <linux/usb/ehci_def.h> #include <linux/usb/tegra_usb_phy.h> +#include <linux/regulator/consumer.h> #define ULPI_VIEWPORT 0x170 @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy *phy) return PTR_ERR(phy->pad_clk); } + phy->vbus = devm_regulator_get(phy->dev, "vbus"); + /* On some boards, the VBUS regulator doesn't need to be controlled */ + if (IS_ERR(phy->vbus)) { + if (PTR_ERR(phy->vbus) == -ENODEV) { + dev_notice(phy->dev, "no vbus regulator"); + phy->vbus = NULL; + } else { + return PTR_ERR(phy->vbus); + } + } + return 0; } static void utmip_pad_power_on(struct tegra_usb_phy *phy) { unsigned long val, flags; + int err; void __iomem *base = phy->pad_regs; clk_prepare_enable(phy->pad_clk); @@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct tegra_usb_phy *phy) spin_unlock_irqrestore(&utmip_pad_lock, flags); clk_disable_unprepare(phy->pad_clk); + + if (phy->vbus) { + err = regulator_enable(phy->vbus); + if (err) + dev_err(phy->dev, + "failed to enable usb vbus regulator: %d\n", + err); + } } static int utmip_pad_power_off(struct tegra_usb_phy *phy) @@ -306,6 +327,9 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy) clk_disable_unprepare(phy->pad_clk); + if (phy->vbus) + regulator_disable(phy->vbus); + return 0; } diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h index d2ca919..2b5fa94 100644 --- a/include/linux/usb/tegra_usb_phy.h +++ b/include/linux/usb/tegra_usb_phy.h @@ -55,6 +55,7 @@ struct tegra_usb_phy { struct clk *clk; struct clk *pll_u; struct clk *pad_clk; + struct regulator *vbus; enum tegra_usb_phy_mode mode; void *config; struct usb_phy *ulpi;
After this patch, usb vbus regulators for tegra usb phy devices can be specified with the device tree attribute vbus-supply = <&x> where x is a regulator defined in the device tree. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/usb/phy/phy-tegra-usb.c | 24 ++++++++++++++++++++++++ include/linux/usb/tegra_usb_phy.h | 1 + 2 files changed, 25 insertions(+)