Facilitate getting required 3.3V and 1.0V VDD supply for
EHCI controller on Exynos.
For example, patches for regulators' nodes:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
enable only minimum number of regulators on smdk5250.
So ensuring now that the controller driver requests the necessary
VDD regulators (if available, unless there are direct VDD rails),
and enable them so as to make them working on exynos systems.
Signed-off-by: Vivek Gautam <[email protected]>
Cc: Jingoo Han <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Alan Stern <[email protected]>
---
These patches had long been posted, and i lost track of them.
My apologies for that.
Thanks to Krzysztof for catching this.
Kindly review.
Changes since v3:
- added a if (!IS_ERR()) check for regulators before disabling
them in error path.
Changes since v2:
- replaced devm_regulator_get() with devm_regulator_get_optional().
- Added Documentation for the vdd supplies for the controller.
- Re-did the commit message.
.../devicetree/bindings/usb/exynos-usb.txt | 2 +
drivers/usb/host/ehci-exynos.c | 57 +++++++++++++++++++++-
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 9b4dbe3..776fa03 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -23,6 +23,8 @@ Required properties:
Optional properties:
- samsung,vbus-gpio: if present, specifies the GPIO that
needs to be pulled up for the bus to be powered.
+ - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
+ - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
Example:
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index df538fd..160ad66 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -21,6 +21,7 @@
#include <linux/of_gpio.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
@@ -45,6 +46,8 @@ static struct hc_driver __read_mostly exynos_ehci_hc_driver;
struct exynos_ehci_hcd {
struct clk *clk;
struct phy *phy[PHY_NUMBER];
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};
#define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv)
@@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device *pdev)
err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
if (err)
- goto fail_clk;
+ goto fail_regulator1;
+
+ exynos_ehci->vdd33 = devm_regulator_get_optional(&pdev->dev, "vdd33");
+ if (!IS_ERR(exynos_ehci->vdd33)) {
+ err = regulator_enable(exynos_ehci->vdd33);
+ if (err) {
+ dev_err(&pdev->dev,
+ "Failed to enable 3.3V Vdd supply\n");
+ goto fail_regulator1;
+ }
+ }
+
+ exynos_ehci->vdd10 = devm_regulator_get_optional(&pdev->dev, "vdd10");
+ if (!IS_ERR(exynos_ehci->vdd10)) {
+ err = regulator_enable(exynos_ehci->vdd10);
+ if (err) {
+ dev_err(&pdev->dev,
+ "Failed to enable 1.0V Vdd supply\n");
+ goto fail_regulator2;
+ }
+ }
skip_phy:
@@ -231,6 +254,12 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ehci->clk);
fail_clk:
+ if (!IS_ERR(exynos_ehci->vdd10))
+ regulator_disable(exynos_ehci->vdd10);
+fail_regulator2:
+ if (!IS_ERR(exynos_ehci->vdd33))
+ regulator_disable(exynos_ehci->vdd33);
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -246,6 +275,11 @@ static int exynos_ehci_remove(struct platform_device *pdev)
clk_disable_unprepare(exynos_ehci->clk);
+ if (!IS_ERR(exynos_ehci->vdd33))
+ regulator_disable(exynos_ehci->vdd33);
+ if (!IS_ERR(exynos_ehci->vdd10))
+ regulator_disable(exynos_ehci->vdd10);
+
usb_put_hcd(hcd);
return 0;
@@ -268,6 +302,11 @@ static int exynos_ehci_suspend(struct device *dev)
clk_disable_unprepare(exynos_ehci->clk);
+ if (!IS_ERR(exynos_ehci->vdd33))
+ regulator_disable(exynos_ehci->vdd33);
+ if (!IS_ERR(exynos_ehci->vdd10))
+ regulator_disable(exynos_ehci->vdd10);
+
return rc;
}
@@ -277,6 +316,22 @@ static int exynos_ehci_resume(struct device *dev)
struct exynos_ehci_hcd *exynos_ehci = to_exynos_ehci(hcd);
int ret;
+ if (!IS_ERR(exynos_ehci->vdd33)) {
+ ret = regulator_enable(exynos_ehci->vdd33);
+ if (ret) {
+ dev_err(dev, "Failed to enable 3.3V Vdd supply\n");
+ return ret;
+ }
+ }
+
+ if (!IS_ERR(exynos_ehci->vdd10)) {
+ ret = regulator_enable(exynos_ehci->vdd10);
+ if (ret) {
+ dev_err(dev, "Failed to enable 1.0V Vdd supply\n");
+ return ret;
+ }
+ }
+
clk_prepare_enable(exynos_ehci->clk);
ret = exynos_ehci_phy_enable(dev);
--
2.2.0
Facilitate getting required 3.3V and 1.0V VDD supply for
OHCI controller on Exynos.
For example, patches for regulators' nodes:
c8c253f ARM: dts: Add regulator entries to smdk5420
275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
enable only minimum number of regulators on smdk5250.
So ensuring now that the controller driver requests the necessary
VDD regulators (if available, unless there are direct VDD rails),
and enable them so as to make them working on exynos systems.
Signed-off-by: Vivek Gautam <[email protected]>
Cc: Jingoo Han <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Alan Stern <[email protected]>
---
Changes since v3:
- added a if (!IS_ERR()) check for regulators before disabling
them in error path.
Changes since v2:
- replaced devm_regulator_get() with devm_regulator_get_optional().
- Added Documentation for the vdd supplies for the controller.
- Re-did the commit message.
.../devicetree/bindings/usb/exynos-usb.txt | 2 +
.../devicetree/bindings/usb/exynos-usb.txt | 4 ++
drivers/usb/host/ohci-exynos.c | 57 +++++++++++++++++++++-
2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 776fa03..3883baa 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -63,6 +63,10 @@ Required properties:
port 2 is HSIC phy1
- phys: from the *Generic PHY* bindings, specifying phy used by port.
+Optional properties:
+ - vdd33-supply: handle to 3.3V Vdd supply regulator for the controller.
+ - vdd10-supply: handle to 1.0V Vdd supply regulator for the controller.
+
Example:
usb@12120000 {
compatible = "samsung,exynos4210-ohci";
diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 2cd105b..4d6dea4 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -19,6 +19,7 @@
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/phy/phy.h>
+#include <linux/regulator/consumer.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
@@ -36,6 +37,8 @@ static struct hc_driver __read_mostly exynos_ohci_hc_driver;
struct exynos_ohci_hcd {
struct clk *clk;
struct phy *phy[PHY_NUMBER];
+ struct regulator *vdd33;
+ struct regulator *vdd10;
};
static int exynos_ohci_get_phy(struct device *dev,
@@ -139,7 +142,27 @@ static int exynos_ohci_probe(struct platform_device *pdev)
err = exynos_ohci_get_phy(&pdev->dev, exynos_ohci);
if (err)
- goto fail_clk;
+ goto fail_regulator1;
+
+ exynos_ohci->vdd33 = devm_regulator_get_optional(&pdev->dev, "vdd33");
+ if (!IS_ERR(exynos_ohci->vdd33)) {
+ err = regulator_enable(exynos_ohci->vdd33);
+ if (err) {
+ dev_err(&pdev->dev,
+ "Failed to enable 3.3V Vdd supply\n");
+ goto fail_regulator1;
+ }
+ }
+
+ exynos_ohci->vdd10 = devm_regulator_get_optional(&pdev->dev, "vdd10");
+ if (!IS_ERR(exynos_ohci->vdd10)) {
+ err = regulator_enable(exynos_ohci->vdd10);
+ if (err) {
+ dev_err(&pdev->dev,
+ "Failed to enable 1.0V Vdd supply\n");
+ goto fail_regulator2;
+ }
+ }
skip_phy:
exynos_ohci->clk = devm_clk_get(&pdev->dev, "usbhost");
@@ -191,6 +214,12 @@ fail_add_hcd:
fail_io:
clk_disable_unprepare(exynos_ohci->clk);
fail_clk:
+ if (!IS_ERR(exynos_ohci->vdd10))
+ regulator_disable(exynos_ohci->vdd10);
+fail_regulator2:
+ if (!IS_ERR(exynos_ohci->vdd33))
+ regulator_disable(exynos_ohci->vdd33);
+fail_regulator1:
usb_put_hcd(hcd);
return err;
}
@@ -206,6 +235,11 @@ static int exynos_ohci_remove(struct platform_device *pdev)
clk_disable_unprepare(exynos_ohci->clk);
+ if (!IS_ERR(exynos_ohci->vdd33))
+ regulator_disable(exynos_ohci->vdd33);
+ if (!IS_ERR(exynos_ohci->vdd10))
+ regulator_disable(exynos_ohci->vdd10);
+
usb_put_hcd(hcd);
return 0;
@@ -234,6 +268,11 @@ static int exynos_ohci_suspend(struct device *dev)
clk_disable_unprepare(exynos_ohci->clk);
+ if (!IS_ERR(exynos_ohci->vdd33))
+ regulator_disable(exynos_ohci->vdd33);
+ if (!IS_ERR(exynos_ohci->vdd10))
+ regulator_disable(exynos_ohci->vdd10);
+
return 0;
}
@@ -243,6 +282,22 @@ static int exynos_ohci_resume(struct device *dev)
struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
int ret;
+ if (!IS_ERR(exynos_ohci->vdd33)) {
+ ret = regulator_enable(exynos_ohci->vdd33);
+ if (ret) {
+ dev_err(dev, "Failed to enable 3.3V Vdd supply\n");
+ return ret;
+ }
+ }
+
+ if (!IS_ERR(exynos_ohci->vdd10)) {
+ ret = regulator_enable(exynos_ohci->vdd10);
+ if (ret) {
+ dev_err(dev, "Failed to enable 1.0V Vdd supply\n");
+ return ret;
+ }
+ }
+
clk_prepare_enable(exynos_ohci->clk);
ret = exynos_ohci_phy_enable(dev);
--
2.2.0
On Mon, 8 Jun 2015, Vivek Gautam wrote:
> Facilitate getting required 3.3V and 1.0V VDD supply for
> EHCI controller on Exynos.
>
> For example, patches for regulators' nodes:
> c8c253f ARM: dts: Add regulator entries to smdk5420
> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
> enable only minimum number of regulators on smdk5250.
>
> So ensuring now that the controller driver requests the necessary
> VDD regulators (if available, unless there are direct VDD rails),
> and enable them so as to make them working on exynos systems.
>
> Signed-off-by: Vivek Gautam <[email protected]>
Something about this looks a little fishy...
> @@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>
> err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
> if (err)
> - goto fail_clk;
> + goto fail_regulator1;
> +
> + exynos_ehci->vdd33 = devm_regulator_get_optional(&pdev->dev, "vdd33");
Just before this region of code, there is:
if (of_device_is_compatible(pdev->dev.of_node,
"samsung,exynos5440-ehci"))
goto skip_phy;
If that "goto" is taken, exynos_ehci->vdd33 and ->vdd10 will be NULL,
not an ERR_PTR code.
> + if (!IS_ERR(exynos_ehci->vdd33)) {
> + err = regulator_enable(exynos_ehci->vdd33);
> + if (err) {
> + dev_err(&pdev->dev,
> + "Failed to enable 3.3V Vdd supply\n");
> + goto fail_regulator1;
> + }
> + }
> +
> + exynos_ehci->vdd10 = devm_regulator_get_optional(&pdev->dev, "vdd10");
> + if (!IS_ERR(exynos_ehci->vdd10)) {
> + err = regulator_enable(exynos_ehci->vdd10);
> + if (err) {
> + dev_err(&pdev->dev,
> + "Failed to enable 1.0V Vdd supply\n");
> + goto fail_regulator2;
> + }
> + }
>
> skip_phy:
>
> @@ -231,6 +254,12 @@ fail_add_hcd:
> fail_io:
> clk_disable_unprepare(exynos_ehci->clk);
> fail_clk:
> + if (!IS_ERR(exynos_ehci->vdd10))
> + regulator_disable(exynos_ehci->vdd10);
> +fail_regulator2:
> + if (!IS_ERR(exynos_ehci->vdd33))
> + regulator_disable(exynos_ehci->vdd33);
Which means these regulator_disable() calls will crash when they
dereference a NULL pointer.
I think it would be simpler in the end to let a NULL pointer mean the
regulator isn't present. If devm_regulator_get_optional() returns an
error, convert it to NULL (or don't do the assignment to
exynos_ehci->vdd?? in the first place).
The same criticism applies to the other patch in this series.
Alan Stern
Hi,
On Mon, Jun 8, 2015 at 8:47 PM, Alan Stern <[email protected]> wrote:
> On Mon, 8 Jun 2015, Vivek Gautam wrote:
>
>> Facilitate getting required 3.3V and 1.0V VDD supply for
>> EHCI controller on Exynos.
>>
>> For example, patches for regulators' nodes:
>> c8c253f ARM: dts: Add regulator entries to smdk5420
>> 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>> enable only minimum number of regulators on smdk5250.
>>
>> So ensuring now that the controller driver requests the necessary
>> VDD regulators (if available, unless there are direct VDD rails),
>> and enable them so as to make them working on exynos systems.
>>
>> Signed-off-by: Vivek Gautam <[email protected]>
>
> Something about this looks a little fishy...
Sorry, i didn't get you.
This patch was initially posted by me sometime back.
>
>> @@ -170,7 +173,27 @@ static int exynos_ehci_probe(struct platform_device *pdev)
>>
>> err = exynos_ehci_get_phy(&pdev->dev, exynos_ehci);
>> if (err)
>> - goto fail_clk;
>> + goto fail_regulator1;
>> +
>> + exynos_ehci->vdd33 = devm_regulator_get_optional(&pdev->dev, "vdd33");
>
> Just before this region of code, there is:
>
> if (of_device_is_compatible(pdev->dev.of_node,
> "samsung,exynos5440-ehci"))
> goto skip_phy;
>
> If that "goto" is taken, exynos_ehci->vdd33 and ->vdd10 will be NULL,
> not an ERR_PTR code.
Right. This will hit NULL pointer dereferencing later in the code.
>
>> + if (!IS_ERR(exynos_ehci->vdd33)) {
>> + err = regulator_enable(exynos_ehci->vdd33);
>> + if (err) {
>> + dev_err(&pdev->dev,
>> + "Failed to enable 3.3V Vdd supply\n");
>> + goto fail_regulator1;
>> + }
>> + }
May be we can have something like this:
if (IS_ERR(exynos_ehci->vdd33)) {
exynos_ehci->vdd33 = NULL;
} else {
err = regulator_enable(exynos_ehci->vdd33);
if (err) {
dev_err(&pdev->dev,
"Failed to enable 3.3V Vdd supply\n");
goto fail_regulator1;
}
}
and later in the code check for NULL pointer before enabling the regulator.
>> +
>> + exynos_ehci->vdd10 = devm_regulator_get_optional(&pdev->dev, "vdd10");
>> + if (!IS_ERR(exynos_ehci->vdd10)) {
>> + err = regulator_enable(exynos_ehci->vdd10);
>> + if (err) {
>> + dev_err(&pdev->dev,
>> + "Failed to enable 1.0V Vdd supply\n");
>> + goto fail_regulator2;
>> + }
>> + }
>>
>> skip_phy:
>>
>> @@ -231,6 +254,12 @@ fail_add_hcd:
>> fail_io:
>> clk_disable_unprepare(exynos_ehci->clk);
>> fail_clk:
>> + if (!IS_ERR(exynos_ehci->vdd10))
>> + regulator_disable(exynos_ehci->vdd10);
>> +fail_regulator2:
>> + if (!IS_ERR(exynos_ehci->vdd33))
>> + regulator_disable(exynos_ehci->vdd33);
>
> Which means these regulator_disable() calls will crash when they
> dereference a NULL pointer.
>
> I think it would be simpler in the end to let a NULL pointer mean the
> regulator isn't present. If devm_regulator_get_optional() returns an
> error, convert it to NULL (or don't do the assignment to
> exynos_ehci->vdd?? in the first place).
>
> The same criticism applies to the other patch in this series.
Sure, i will amend both the patches.
Thanks !
--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India