[PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a
couple of fixed GPIO-controlled regulators too.

The regulator configurations were mostly taken from the ChromeOS 3.2
kernel. Exceptions are:

* The schematic lists a fixed voltage for each rail, whereas the ChromeOS
  kernel lists a range for many rails. I used the values from the ChromeOS
  kernel in all cases, since I know the board file there is the most
  complete available for this hardware.

* The vdd_1v2 fixed regulator is present only in the schematic. So, I added
  this based on the schematic.

* A 3.3v fixed regulator using GPIO3 of the TPS6586x is present in the
  ChromeOS kernel, but not in the schematic. So, I dropped this based on
  the schematic.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-seaboard.dts |  164 ++++++++++++++++++++++++++++++++
 1 files changed, 164 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
index 85e621a..fe2bdd0 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -374,6 +374,141 @@
 		status = "okay";
 		clock-frequency = <400000>;
 
+		pmic: tps6586x@34 {
+			compatible = "ti,tps6586x";
+			reg = <0x34>;
+			interrupts = <0 86 0x4>;
+
+			#gpio-cells = <2>;
+			gpio-controller;
+
+			regulators {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				regulator@0 {
+					reg = <0>;
+					regulator-compatible = "sm0";
+					regulator-name = "vdd_sm0";
+					regulator-min-microvolt = < 950000>;
+					regulator-max-microvolt = <1300000>;
+					regulator-always-on;
+				};
+
+				regulator@1 {
+					reg = <1>;
+					regulator-compatible = "sm1";
+					regulator-name = "vdd_sm1";
+					regulator-min-microvolt = < 750000>;
+					regulator-max-microvolt = <1275000>;
+					regulator-always-on;
+				};
+
+				sm2_reg: regulator@2 {
+					reg = <2>;
+					regulator-compatible = "sm2";
+					regulator-name = "vdd_sm2";
+					regulator-min-microvolt = <3000000>;
+					regulator-max-microvolt = <4550000>;
+					regulator-always-on;
+				};
+
+				regulator@3 {
+					reg = <3>;
+					regulator-compatible = "ldo0";
+					regulator-name = "vdd_ldo0";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@4 {
+					reg = <4>;
+					regulator-compatible = "ldo1";
+					regulator-name = "vdd_ldo1";
+					regulator-min-microvolt = <1100000>;
+					regulator-max-microvolt = <1100000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@5 {
+					reg = <5>;
+					regulator-compatible = "ldo2";
+					regulator-name = "vdd_ldo2";
+					regulator-min-microvolt = < 900000>;
+					regulator-max-microvolt = <1300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@6 {
+					reg = <6>;
+					regulator-compatible = "ldo3";
+					regulator-name = "vdd_ldo3";
+					regulator-min-microvolt = <3300000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@7 {
+					reg = <7>;
+					regulator-compatible = "ldo4";
+					regulator-name = "vdd_ldo4";
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@8 {
+					reg = <8>;
+					regulator-compatible = "ldo5";
+					regulator-name = "vdd_ldo5";
+					regulator-min-microvolt = <2850000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+
+				regulator@9 {
+					reg = <9>;
+					regulator-compatible = "ldo6";
+					regulator-name = "vdd_ldo6";
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@10 {
+					reg = <10>;
+					regulator-compatible = "ldo7";
+					regulator-name = "vdd_ldo7";
+					regulator-min-microvolt = <3300000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@11 {
+					reg = <11>;
+					regulator-compatible = "ldo8";
+					regulator-name = "vdd_ldo8";
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@12 {
+					reg = <12>;
+					regulator-compatible = "ldo9";
+					regulator-name = "vdd_ldo9";
+					regulator-min-microvolt = <2850000>;
+					regulator-max-microvolt = <2850000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+			};
+		};
+
 		temperature-sensor@4c {
 			compatible = "nct1008";
 			reg = <0x4c>;
@@ -387,6 +522,10 @@
 		};
 	};
 
+	pmc {
+		nvidia,invert-interrupt;
+	};
+
 	memory-controller@0x7000f400 {
 		emc-table@190000 {
 			reg = <190000>;
@@ -473,6 +612,31 @@
 		};
 	};
 
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		regulator@0 {
+			compatible = "regulator-fixed";
+			reg = <0>;
+			regulator-name = "vdd_1v5";
+			regulator-min-microvolt = <1500000>;
+			regulator-max-microvolt = <1500000>;
+			gpio = <&pmic 0 0>;
+		};
+
+		regulator@1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "vdd_1v2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			gpio = <&pmic 1 0>;
+			enable-active-high;
+		};
+	};
+
 	sound {
 		compatible = "nvidia,tegra-audio-wm8903-seaboard",
 			     "nvidia,tegra-audio-wm8903";
-- 
1.7.0.4

[PATCH 2/3] ARM: dt: tegra: ventana: add regulators

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Ventana uses a TPS6586x regulator. Instantiate this, and hook up a
couple of fixed GPIO-controlled regulators too.

The regulator configurations were all taken from NVIDIA's downstream
android-tegra-nv-3.1 kernel.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-ventana.dts |  173 +++++++++++++++++++++++++++++++++
 1 files changed, 173 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..330c212 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -289,6 +289,144 @@
 	i2c@7000d000 {
 		status = "okay";
 		clock-frequency = <400000>;
+
+		pmic: tps6586x@34 {
+			compatible = "ti,tps6586x";
+			reg = <0x34>;
+			interrupts = <0 86 0x4>;
+
+			#gpio-cells = <2>;
+			gpio-controller;
+
+			regulators {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				regulator@0 {
+					reg = <0>;
+					regulator-compatible = "sm0";
+					regulator-name = "vdd_sm0";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+				};
+
+				regulator@1 {
+					reg = <1>;
+					regulator-compatible = "sm1";
+					regulator-name = "vdd_sm1";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+				};
+
+				sm2_reg: regulator@2 {
+					reg = <2>;
+					regulator-compatible = "sm2";
+					regulator-name = "vdd_sm2";
+					regulator-min-microvolt = <3000000>;
+					regulator-max-microvolt = <4550000>;
+					regulator-always-on;
+				};
+
+				regulator@3 {
+					reg = <3>;
+					regulator-compatible = "ldo0";
+					regulator-name = "vdd_ldo0";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@4 {
+					reg = <4>;
+					regulator-compatible = "ldo1";
+					regulator-name = "vdd_ldo1";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@5 {
+					reg = <5>;
+					regulator-compatible = "ldo2";
+					regulator-name = "vdd_ldo2";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1500000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@6 {
+					reg = <6>;
+					regulator-compatible = "ldo3";
+					regulator-name = "vdd_ldo3";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@7 {
+					reg = <7>;
+					regulator-compatible = "ldo4";
+					regulator-name = "vdd_ldo4";
+					regulator-min-microvolt = <1700000>;
+					regulator-max-microvolt = <2475000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@8 {
+					reg = <8>;
+					regulator-compatible = "ldo5";
+					regulator-name = "vdd_ldo5";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+				};
+
+				regulator@9 {
+					reg = <9>;
+					regulator-compatible = "ldo6";
+					regulator-name = "vdd_ldo6";
+					regulator-min-microvolt = <1800000>;
+					regulator-max-microvolt = <1800000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@10 {
+					reg = <10>;
+					regulator-compatible = "ldo7";
+					regulator-name = "vdd_ldo7";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@11 {
+					reg = <11>;
+					regulator-compatible = "ldo8";
+					regulator-name = "vdd_ldo8";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@12 {
+					reg = <12>;
+					regulator-compatible = "ldo9";
+					regulator-name = "vdd_ldo9";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+			};
+		};
+	};
+
+	pmc {
+		nvidia,invert-interrupt;
 	};
 
 	usb@c5000000 {
@@ -317,6 +455,41 @@
 		bus-width = <8>;
 	};
 
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		regulator@0 {
+			compatible = "regulator-fixed";
+			reg = <0>;
+			regulator-name = "vdd_1v5";
+			regulator-min-microvolt = <1500000>;
+			regulator-max-microvolt = <1500000>;
+			gpio = <&pmic 0 0>;
+		};
+
+		regulator@1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "vdd_1v2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			gpio = <&pmic 1 0>;
+			enable-active-high;
+		};
+
+		regulator@2 {
+			compatible = "regulator-fixed";
+			reg = <2>;
+			regulator-name = "pnl_pwr";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2800000>;
+			gpio = <&gpio 22 0>; /* gpio PC6 */
+			enable-active-high;
+		};
+	};
+
 	sound {
 		compatible = "nvidia,tegra-audio-wm8903-ventana",
 			     "nvidia,tegra-audio-wm8903";
-- 
1.7.0.4

[PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The Toshiba AC100/PAZ00 uses a TPS6586x regulator. Instantiate this.

The regulator configurations were all taken from the AC100 kernel used by
the Ubuntu port, specifically:

git://gitorious.org/~marvin24/ac100/marvin24s-kernel.git chromeos-ac100-3.0-exp

Cc: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-paz00.dts |  138 +++++++++++++++++++++++++++++++++++
 1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index 684a9e1..55ca34c 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -272,12 +272,150 @@
 		status = "okay";
 		clock-frequency = <400000>;
 
+		pmic: tps6586x@34 {
+			compatible = "ti,tps6586x";
+			reg = <0x34>;
+			interrupts = <0 86 0x4>;
+
+			#gpio-cells = <2>;
+			gpio-controller;
+
+			regulators {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				regulator@0 {
+					reg = <0>;
+					regulator-compatible = "sm0";
+					regulator-name = "+1.2vs_sm0";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1300000>;
+					regulator-always-on;
+				};
+
+				regulator@1 {
+					reg = <1>;
+					regulator-compatible = "sm1";
+					regulator-name = "+1.0vs_sm1";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1125000>;
+					regulator-always-on;
+				};
+
+				sm2_reg: regulator@2 {
+					reg = <2>;
+					regulator-compatible = "sm2";
+					regulator-name = "+3.7vs_sm2";
+					regulator-min-microvolt = <3000000>;
+					regulator-max-microvolt = <3700000>;
+					regulator-always-on;
+				};
+
+				regulator@3 {
+					reg = <3>;
+					regulator-compatible = "ldo0";
+					regulator-name = "+3.3vs_ldo0";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@4 {
+					reg = <4>;
+					regulator-compatible = "ldo1";
+					regulator-name = "+1.1vs_ldo1";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1100000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@5 {
+					reg = <5>;
+					regulator-compatible = "ldo2";
+					regulator-name = "+1.2vs_ldo2";
+					regulator-min-microvolt = < 725000>;
+					regulator-max-microvolt = <1275000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@6 {
+					reg = <6>;
+					regulator-compatible = "ldo3";
+					regulator-name = "+3.3vs_ldo3";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@7 {
+					reg = <7>;
+					regulator-compatible = "ldo4";
+					regulator-name = "+1.8vs_ldo4";
+					regulator-min-microvolt = <1700000>;
+					regulator-max-microvolt = <1800000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@8 {
+					reg = <8>;
+					regulator-compatible = "ldo5";
+					regulator-name = "+2.85vs_ldo5";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <2850000>;
+					regulator-always-on;
+				};
+
+				regulator@9 {
+					reg = <9>;
+					regulator-compatible = "ldo6";
+					regulator-name = "+2.85vs_ldo6";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <2850000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@10 {
+					reg = <10>;
+					regulator-compatible = "ldo7";
+					regulator-name = "+3.3vs_ldo7";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <3300000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@11 {
+					reg = <11>;
+					regulator-compatible = "ldo8";
+					regulator-name = "+1.8vs_ldo8";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <1800000>;
+					vin-supply = <&sm2_reg>;
+				};
+
+				regulator@12 {
+					reg = <12>;
+					regulator-compatible = "ldo9";
+					regulator-name = "+2.85vs_ldo9";
+					regulator-min-microvolt = <1250000>;
+					regulator-max-microvolt = <2850000>;
+					regulator-always-on;
+					vin-supply = <&sm2_reg>;
+				};
+			};
+		};
+
 		adt7461@4c {
 			compatible = "adi,adt7461";
 			reg = <0x4c>;
 		};
 	};
 
+	pmc {
+		nvidia,invert-interrupt;
+	};
+
 	usb@c5000000 {
 		status = "okay";
 	};
-- 
1.7.0.4

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Marc Dietrich]

Hi Stephen,

On Friday 22 June 2012 17:14:02 Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The Toshiba AC100/PAZ00 uses a TPS6586x regulator. Instantiate this.
> 
> The regulator configurations were all taken from the AC100 kernel used by
> the Ubuntu port, specifically:
> 
> git://gitorious.org/~marvin24/ac100/marvin24s-kernel.git
> chromeos-ac100-3.0-exp
> 
> Cc: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra20-paz00.dts |  138
> +++++++++++++++++++++++++++++++++++ 1 files changed, 138 insertions(+), 0
> deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
> b/arch/arm/boot/dts/tegra20-paz00.dts index 684a9e1..55ca34c 100644
> --- a/arch/arm/boot/dts/tegra20-paz00.dts
> +++ b/arch/arm/boot/dts/tegra20-paz00.dts
> @@ -272,12 +272,150 @@
>  		status = "okay";
>  		clock-frequency = <400000>;
> 
> +		pmic: tps6586x@34 {
> +			compatible = "ti,tps6586x";
> +			reg = <0x34>;
> +			interrupts = <0 86 0x4>;
> +
> +			#gpio-cells = <2>;
> +			gpio-controller;
> +
> +			regulators {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				regulator@0 {
> +					reg = <0>;
> +					regulator-compatible = "sm0";
> +					regulator-name = "+1.2vs_sm0";
> +					regulator-min-microvolt = < 725000>;
> +					regulator-max-microvolt = <1300000>;
> +					regulator-always-on;
> +				};
> +
> +				regulator@1 {
> +					reg = <1>;
> +					regulator-compatible = "sm1";
> +					regulator-name = "+1.0vs_sm1";
> +					regulator-min-microvolt = < 725000>;
> +					regulator-max-microvolt = <1125000>;
> +					regulator-always-on;
> +				};
> +
> +				sm2_reg: regulator@2 {
> +					reg = <2>;
> +					regulator-compatible = "sm2";
> +					regulator-name = "+3.7vs_sm2";
> +					regulator-min-microvolt = <3000000>;
> +					regulator-max-microvolt = <3700000>;
> +					regulator-always-on;
> +				};
> +
> +				regulator@3 {
> +					reg = <3>;
> +					regulator-compatible = "ldo0";
> +					regulator-name = "+3.3vs_ldo0";
> +					regulator-min-microvolt = <1250000>;

I think the common sense was that this should also be 3.3 V as it is the pex 
clock (which is not used at all on this board). I guess it doesn't matter 
much. So ...

Acked-By: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>


> +					regulator-max-microvolt = <3300000>;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@4 {
> +					reg = <4>;
> +					regulator-compatible = "ldo1";
> +					regulator-name = "+1.1vs_ldo1";
> +					regulator-min-microvolt = < 725000>;
> +					regulator-max-microvolt = <1100000>;
> +					regulator-always-on;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@5 {
> +					reg = <5>;
> +					regulator-compatible = "ldo2";
> +					regulator-name = "+1.2vs_ldo2";
> +					regulator-min-microvolt = < 725000>;
> +					regulator-max-microvolt = <1275000>;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@6 {
> +					reg = <6>;
> +					regulator-compatible = "ldo3";
> +					regulator-name = "+3.3vs_ldo3";
> +					regulator-min-microvolt = <1250000>;
> +					regulator-max-microvolt = <3300000>;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@7 {
> +					reg = <7>;
> +					regulator-compatible = "ldo4";
> +					regulator-name = "+1.8vs_ldo4";
> +					regulator-min-microvolt = <1700000>;
> +					regulator-max-microvolt = <1800000>;
> +					regulator-always-on;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@8 {
> +					reg = <8>;
> +					regulator-compatible = "ldo5";
> +					regulator-name = "+2.85vs_ldo5";
> +					regulator-min-microvolt = <1250000>;
> +					regulator-max-microvolt = <2850000>;
> +					regulator-always-on;
> +				};
> +
> +				regulator@9 {
> +					reg = <9>;
> +					regulator-compatible = "ldo6";
> +					regulator-name = "+2.85vs_ldo6";
> +					regulator-min-microvolt = <1250000>;
> +					regulator-max-microvolt = <2850000>;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@10 {
> +					reg = <10>;
> +					regulator-compatible = "ldo7";
> +					regulator-name = "+3.3vs_ldo7";
> +					regulator-min-microvolt = <1250000>;
> +					regulator-max-microvolt = <3300000>;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@11 {
> +					reg = <11>;
> +					regulator-compatible = "ldo8";
> +					regulator-name = "+1.8vs_ldo8";
> +					regulator-min-microvolt = <1250000>;
> +					regulator-max-microvolt = <1800000>;
> +					vin-supply = <&sm2_reg>;
> +				};
> +
> +				regulator@12 {
> +					reg = <12>;
> +					regulator-compatible = "ldo9";
> +					regulator-name = "+2.85vs_ldo9";
> +					regulator-min-microvolt = <1250000>;
> +					regulator-max-microvolt = <2850000>;
> +					regulator-always-on;
> +					vin-supply = <&sm2_reg>;
> +				};
> +			};
> +		};
> +
>  		adt7461@4c {
>  			compatible = "adi,adt7461";
>  			reg = <0x4c>;
>  		};
>  	};
> 
> +	pmc {
> +		nvidia,invert-interrupt;
> +	};
> +
>  	usb@c5000000 {
>  		status = "okay";
>  	};

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Mark Brown]

On Sat, Jun 23, 2012 at 06:35:01PM +0200, Marc Dietrich wrote:

> > The regulator configurations were all taken from the AC100 kernel used by
> > the Ubuntu port, specifically:

These generally all look pretty broken...

> > +				regulator@0 {
> > +					reg = <0>;
> > +					regulator-compatible = "sm0";
> > +					regulator-name = "+1.2vs_sm0";
> > +					regulator-min-microvolt = < 725000>;
> > +					regulator-max-microvolt = <1300000>;

Most of these ranges look suspiciously like the maximum possible
variation the regulator has, not what the board actually requires (which
is a depressingly common error, I've no idea why people seem to think
they're supposed to cut'n'paste the physical limits of the regualtor out
of the driver).  If something decides to take advantage of the variation
this could be problematic.

> > +				regulator@3 {
> > +					reg = <3>;
> > +					regulator-compatible = "ldo0";
> > +					regulator-name = "+3.3vs_ldo0";
> > +					regulator-min-microvolt = <1250000>;

> I think the common sense was that this should also be 3.3 V as it is the pex 
> clock (which is not used at all on this board). I guess it doesn't matter 
> much. So ...

> Acked-By: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>

> > +					regulator-max-microvolt = <3300000>;

This is one example, it looks like the rail needs to be fixed to 3.3V.

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Marc Dietrich]

On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
> On Sat, Jun 23, 2012 at 06:35:01PM +0200, Marc Dietrich wrote:
> > > The regulator configurations were all taken from the AC100 kernel used
> > > by
> 
> > > the Ubuntu port, specifically:
> These generally all look pretty broken...
> 
> > > +				regulator@0 {
> > > +					reg = <0>;
> > > +					regulator-compatible = "sm0";
> > > +					regulator-name = "+1.2vs_sm0";
> > > +					regulator-min-microvolt = < 725000>;
> > > +					regulator-max-microvolt = <1300000>;
> 
> Most of these ranges look suspiciously like the maximum possible
> variation the regulator has, not what the board actually requires (which
> is a depressingly common error, I've no idea why people seem to think
> they're supposed to cut'n'paste the physical limits of the regualtor out
> of the driver).  If something decides to take advantage of the variation
> this could be problematic.

AFAIR we saw some instabilities with 1.2 V here, but looking back, that could 
also be related to something else. Finding these "undervolt" bugs is really 
hard to do. I indeed just copied the values from the original source ( 
http://gitorious.org/ac100/kernel/blobs/2.6.32/arch/arm/mach-
tegra/odm_kit/adaptations/pmu/tps6586x/nvodm_pmu_tps6586x.c#line136 ) and 
bumped up sm0 by 100 mV for the said stabilty reasons. 

According to the datasheet, sm0 can go up to 1.5 V if I read it correctly, so 
1.3 V is still inside the spec and not the maximum the regulator can provide.

> 
> > > +				regulator@3 {
> > > +					reg = <3>;
> > > +					regulator-compatible = "ldo0";
> > > +					regulator-name = "+3.3vs_ldo0";
> > > +					regulator-min-microvolt = <1250000>;
> > 
> > I think the common sense was that this should also be 3.3 V as it is the
> > pex clock (which is not used at all on this board). I guess it doesn't
> > matter much. So ...
> > 
> > Acked-By: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
> > 
> > > +					regulator-max-microvolt = <3300000>;
> 
> This is one example, it looks like the rail needs to be fixed to 3.3V.

I think nowhere in the code a regulator (beside sm*) is programmed to some 
different value that the maximum given here (this is not the maximum the 
regulator can provide). I never understood why the kernel code always sets the 
regulator to the maximum value if no other value was specified. IMHO, there 
should be some initial value, e.g. regulator-default-microvolt, as the 
original driver (from 2.6.32 ages) did. This way the maximum value can be set 
to the hw limits, but maybe this is a bit dangerous.

Marc

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2315 bytes --]

On Sun, Jun 24, 2012 at 02:01:58PM +0200, Marc Dietrich wrote:
> On Sunday 24 June 2012 12:03:06 Mark Brown wrote:

> > > > +                                 	regulator-name = "+3.3vs_ldo0";
> > > > +					regulator-max-microvolt = <3300000>;

> > This is one example, it looks like the rail needs to be fixed to 3.3V.

> I think nowhere in the code a regulator (beside sm*) is programmed to some 
> different value that the maximum given here (this is not the maximum the 
> regulator can provide). I never understood why the kernel code always sets the 
> regulator to the maximum value if no other value was specified. IMHO, there 
> should be some initial value, e.g. regulator-default-microvolt, as the 
> original driver (from 2.6.32 ages) did. This way the maximum value can be set 

That's *never* been in mainline, and nobody even bothered trying to
submit it.

> to the hw limits, but maybe this is a bit dangerous.

One of two things should be happening.  Either a single voltage is
specified (in which case that voltage will be configured in the
hardware and consumer drivers can't change anything) or a voltage range
is specified (in which case the consumers are expected to manage the
voltage and the most the API should do is bring the voltage within the
limits given, though I don't think that's actually implemented yet).
Specifying an initial value within the range should at best be redundant
as the drivers that are actively managing their voltages will be
overriding it anyway.

We certainly shouldn't be specifying the limits of the regulator itself
as normally the board design will be much more constrained than the
regulator itself and like I said it's stupid to have to cut'n'paste the
numbers out of the driver into the machine constraints.  We should
instead be specifying the constraints the system is designed to operate
in.  Chances are that if nothing is able to actively manage the voltage
it's not in fact safe to change the voltage at all and therefore the
constraints should specify only one voltage.

In the above case the fact that the supply is named "+3.3vs_ldo0" seems
like a fairly clear sign that the board has been designed for this to
operate at 3.3V which makes the fact that the constraints go down to
1.25V seem at best odd.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Marc Dietrich]

On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
> On Sun, Jun 24, 2012 at 02:01:58PM +0200, Marc Dietrich wrote:
> > On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
> > > > > +                                 	regulator-name = "+3.3vs_ldo0";
> > > > > +					regulator-max-microvolt = <3300000>;
> > > 
> > > This is one example, it looks like the rail needs to be fixed to 3.3V.
> > 
> > I think nowhere in the code a regulator (beside sm*) is programmed to some
> > different value that the maximum given here (this is not the maximum the
> > regulator can provide). I never understood why the kernel code always sets
> > the regulator to the maximum value if no other value was specified. IMHO,
> > there should be some initial value, e.g. regulator-default-microvolt, as
> > the original driver (from 2.6.32 ages) did. This way the maximum value
> > can be set
> That's *never* been in mainline, and nobody even bothered trying to
> submit it.

which was the best thing to do ;-)

> > to the hw limits, but maybe this is a bit dangerous.
> 
> One of two things should be happening.  Either a single voltage is
> specified (in which case that voltage will be configured in the

I'm not an expert on this, but it seems to me that only sm0 and sm1 should be 
changeable (and some rail called vdd_aon, which seems to be ldo2 in case of 
paz00 connected to the rtc). So, all others can be constant voltage. Maybe 
Stephen can comment on the actual requirements (also for the other boards 
which may have similar layout).

Marc

> hardware and consumer drivers can't change anything) or a voltage range
> is specified (in which case the consumers are expected to manage the
> voltage and the most the API should do is bring the voltage within the
> limits given, though I don't think that's actually implemented yet).
> Specifying an initial value within the range should at best be redundant
> as the drivers that are actively managing their voltages will be
> overriding it anyway.
> 
> We certainly shouldn't be specifying the limits of the regulator itself
> as normally the board design will be much more constrained than the
> regulator itself and like I said it's stupid to have to cut'n'paste the
> numbers out of the driver into the machine constraints.  We should
> instead be specifying the constraints the system is designed to operate
> in.  Chances are that if nothing is able to actively manage the voltage
> it's not in fact safe to change the voltage at all and therefore the
> constraints should specify only one voltage.
> 
> In the above case the fact that the supply is named "+3.3vs_ldo0" seems
> like a fairly clear sign that the board has been designed for this to
> operate at 3.3V which makes the fact that the constraints go down to
> 1.25V seem at best odd.

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Sun, Jun 24, 2012 at 03:27:42PM +0200, Marc Dietrich wrote:
> On Sunday 24 June 2012 13:31:51 Mark Brown wrote:

> > > there should be some initial value, e.g. regulator-default-microvolt, as
> > > the original driver (from 2.6.32 ages) did. This way the maximum value
> > > can be set

> > That's *never* been in mainline, and nobody even bothered trying to
> > submit it.

> which was the best thing to do ;-)

Well, no - had there been some attempt to submit it it might've helped
stop people writing constaints like this.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Marc Dietrich]

Am Montag, 25. Juni 2012, 09:46:56 schrieb Mark Brown:
> On Sun, Jun 24, 2012 at 03:27:42PM +0200, Marc Dietrich wrote:
> > On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
> > > > there should be some initial value, e.g. regulator-default-microvolt,
> > > > as
> > > > the original driver (from 2.6.32 ages) did. This way the maximum value
> > > > can be set
> > > 
> > > That's *never* been in mainline, and nobody even bothered trying to
> > > submit it.
> > 
> > which was the best thing to do ;-)
> 
> Well, no - had there been some attempt to submit it it might've helped
> stop people writing constaints like this.

by saying "best thing to do" I meant that the driver isn't mainline compatible 
as it is based on some cross OS HAL. On the other hand, I think the current
tps6586x could be enhanced to include such a field. I don't know if the 
regulator api supports this, though.

Marc

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

On Sun, Jun 24, 2012 at 03:27:42PM +0200, Marc Dietrich wrote:
> On Sunday 24 June 2012 13:31:51 Mark Brown wrote:
> > On Sun, Jun 24, 2012 at 02:01:58PM +0200, Marc Dietrich wrote:
> > > On Sunday 24 June 2012 12:03:06 Mark Brown wrote:
> > > > > > +                                 	regulator-name = "+3.3vs_ldo0";
> > > > > > +					regulator-max-microvolt = <3300000>;
> > > > 
> > > > This is one example, it looks like the rail needs to be fixed to 3.3V.
> > > 
> > > I think nowhere in the code a regulator (beside sm*) is programmed to some
> > > different value that the maximum given here (this is not the maximum the
> > > regulator can provide). I never understood why the kernel code always sets
> > > the regulator to the maximum value if no other value was specified. IMHO,
> > > there should be some initial value, e.g. regulator-default-microvolt, as
> > > the original driver (from 2.6.32 ages) did. This way the maximum value
> > > can be set
> > That's *never* been in mainline, and nobody even bothered trying to
> > submit it.
> 
> which was the best thing to do ;-)
> 
> > > to the hw limits, but maybe this is a bit dangerous.
> > 
> > One of two things should be happening.  Either a single voltage is
> > specified (in which case that voltage will be configured in the
> 
> I'm not an expert on this, but it seems to me that only sm0 and sm1 should be 
> changeable (and some rail called vdd_aon, which seems to be ldo2 in case of 
> paz00 connected to the rtc). So, all others can be constant voltage. Maybe 
> Stephen can comment on the actual requirements (also for the other boards 
> which may have similar layout).

I can confirm that at least for ldo0 the value needs to be fixed. I did
in fact post a patch back in February that was needed to fix PCIe on
Harmony. I also sat down with one of our hardware engineers and worked
through the list and wrote down the requirements for Harmony. I need to
check where our notes have gone, though.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Stephen Warren]

On 06/24/2012 05:03 AM, Mark Brown wrote:
> On Sat, Jun 23, 2012 at 06:35:01PM +0200, Marc Dietrich wrote:
> 
>>> The regulator configurations were all taken from the AC100 kernel used by
>>> the Ubuntu port, specifically:
> 
> These generally all look pretty broken...
> 
>>> +				regulator@0 {
>>> +					reg = <0>;
>>> +					regulator-compatible = "sm0";
>>> +					regulator-name = "+1.2vs_sm0";
>>> +					regulator-min-microvolt = < 725000>;
>>> +					regulator-max-microvolt = <1300000>;
> 
> Most of these ranges look suspiciously like the maximum possible
> variation the regulator has, not what the board actually requires (which
> is a depressingly common error, I've no idea why people seem to think
> they're supposed to cut'n'paste the physical limits of the regualtor out
> of the driver).  If something decides to take advantage of the variation
> this could be problematic.

So I think this sm0 (and the sm1) entry might actually be correct; sm0
feeds vdd_core and sm1 feeds vdd_cpu. These rails have DVFS and hence
the voltage can vary. I guess I should change the regulator-name in
these cases to something more useful than the very first signal name on
the schematics too.

That said, we don't actually have DVFS in the mainline kernel yet, and
correlating the regulator limits in our downstream kernels against the
DVFS tables we use is ... challenging; I'm not sure they're consistent
anyway:-(

However, it probably doesn't make sense to vary sm2, since all that is
used for is to feed the TPS6586x's input pins for the the LDO regulators.

Likewise, all the LDO regulators are used for various peripherals in
general, and in the main it probably makes no sense for those rails to vary.

So, what I think I'll do for any regulators where the downstream board
files specify a voltage range, is boot the kernel and find out what
voltage is selected by default, and program both min-/max-microvolt to
that specific value. That way, there will be no behaviour change. We can
expand the ranges on the regulators later if/when we add DVFS etc. Does
that sound like a reasonable approach?

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On Tue, Jun 26, 2012 at 04:35:46PM -0600, Stephen Warren wrote:

> So I think this sm0 (and the sm1) entry might actually be correct; sm0
> feeds vdd_core and sm1 feeds vdd_cpu. These rails have DVFS and hence
> the voltage can vary. I guess I should change the regulator-name in
> these cases to something more useful than the very first signal name on
> the schematics too.

Even if they feed these supplies are they capable of varying on this
board if they're shared with other things?  This is one of the common
issues with constraints...  But generally if the supply covers more than
one thing the idiomatic thing is to list them all separated by / or
something.

> That said, we don't actually have DVFS in the mainline kernel yet, and
> correlating the regulator limits in our downstream kernels against the
> DVFS tables we use is ... challenging; I'm not sure they're consistent
> anyway:-(

Well, if you're using the regulator API the regulator API limits will
win if they're more constrained.  But this is half the problem with
these silly constraints that just copy the regulator limits, you've no
idea what the board is actually supposed to do.

> However, it probably doesn't make sense to vary sm2, since all that is
> used for is to feed the TPS6586x's input pins for the the LDO regulators.

Actually if we get clever then there's some fun to be had there.  If we
can have all the LDOs specify the headroom they need then we should be
able to arrange to vary the DCDC depending on what the needs of the
currently active LDOs are which would improve power efficiency as the
goal here is to drop the voltage as much as possible using the more
efficient DCDC then regulate on from there with the LDOs.

I keep considering doing this but don't have any real need for it
myself, it's just amusing.  Might fall out of (or be similar to) some
work I'm going to be doing soon for bypass modes though.

> Likewise, all the LDO regulators are used for various peripherals in
> general, and in the main it probably makes no sense for those rails to vary.

This is typically very unusual, the devices on the rails are normally
heavily constrained when active.

> So, what I think I'll do for any regulators where the downstream board
> files specify a voltage range, is boot the kernel and find out what
> voltage is selected by default, and program both min-/max-microvolt to
> that specific value. That way, there will be no behaviour change. We can
> expand the ranges on the regulators later if/when we add DVFS etc. Does
> that sound like a reasonable approach?

Yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Stephen Warren]

On 06/26/2012 05:02 PM, Mark Brown wrote:
> On Tue, Jun 26, 2012 at 04:35:46PM -0600, Stephen Warren wrote:
> 
>> So I think this sm0 (and the sm1) entry might actually be
>> correct; sm0 feeds vdd_core and sm1 feeds vdd_cpu. These rails
>> have DVFS and hence the voltage can vary. I guess I should change
>> the regulator-name in these cases to something more useful than
>> the very first signal name on the schematics too.
> 
> Even if they feed these supplies are they capable of varying on
> this board if they're shared with other things?  This is one of the
> common issues with constraints...  But generally if the supply
> covers more than one thing the idiomatic thing is to list them all
> separated by / or something.

These two supplies aren't shared, it's just that the signal on the
schematic ends up being named different things in different places; on
the regulator page it's named vdd_sm0, but on some other page it gets
renamed vdd_core.

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Stephen Warren]

On 06/26/2012 05:02 PM, Mark Brown wrote:
> On Tue, Jun 26, 2012 at 04:35:46PM -0600, Stephen Warren wrote:
> 
>> So I think this sm0 (and the sm1) entry might actually be
>> correct; sm0 feeds vdd_core and sm1 feeds vdd_cpu. These rails
>> have DVFS and hence the voltage can vary. I guess I should change
>> the regulator-name in these cases to something more useful than
>> the very first signal name on the schematics too.
> 
> Even if they feed these supplies are they capable of varying on
> this board if they're shared with other things?  This is one of the
> common issues with constraints...  But generally if the supply
> covers more than one thing the idiomatic thing is to list them all
> separated by / or something.

Just FYI, the particular choice of "/" as a separator doesn't work
well, because then /sys/kernel/debug/regulator/${regulator-name} can't
be created. I guess I'll use a comma. I assume it's not worth fixing
the regulator core to s@/@,@ when generating the debugfs filenames?

Re: [PATCH 3/3] ARM: dt: tegra: paz00: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

On Fri, Jun 29, 2012 at 11:32:30AM -0600, Stephen Warren wrote:

> Just FYI, the particular choice of "/" as a separator doesn't work
> well, because then /sys/kernel/debug/regulator/${regulator-name} can't
> be created. I guess I'll use a comma. I assume it's not worth fixing
> the regulator core to s@/@,@ when generating the debugfs filenames?

I don't think that's sensible, no.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Saturday 23 June 2012 04:44 AM, Stephen Warren wrote:
> From: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a
> couple of fixed GPIO-controlled regulators too.
>
> The regulator configurations were mostly taken from the ChromeOS 3.2
> kernel. Exceptions are:
>
> * The schematic lists a fixed voltage for each rail, whereas the ChromeOS
>    kernel lists a range for many rails. I used the values from the ChromeOS
>    kernel in all cases, since I know the board file there is the most
>    complete available for this hardware.
>
> * The vdd_1v2 fixed regulator is present only in the schematic. So, I added
>    this based on the schematic.
>
> * A 3.3v fixed regulator using GPIO3 of the TPS6586x is present in the
>    ChromeOS kernel, but not in the schematic. So, I dropped this based on
>    the schematic.
>
> Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---

Acked-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I think this series very much depends on the series
[PATCH V3 0/3] regulator: dt: add policy to match regulator with prop 
"regulator-compatible"


> +				regulator@3 {
> +					reg =<3>;
> +					regulator-compatible = "ldo0";
> +					regulator-name = "vdd_ldo0";
> +					regulator-min-microvolt =<1250000>;
> +					regulator-max-microvolt =<3300000>;
> +					vin-supply =<&sm2_reg>;

I think support for vin-supply is still not there for this regulator in 
driver.

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Stephen Warren]

On 06/25/2012 12:24 AM, Laxman Dewangan wrote:
> On Saturday 23 June 2012 04:44 AM, Stephen Warren wrote:
>> From: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a
>> couple of fixed GPIO-controlled regulators too.
>>
>> The regulator configurations were mostly taken from the ChromeOS 3.2
>> kernel. Exceptions are:
>>
>> * The schematic lists a fixed voltage for each rail, whereas the ChromeOS
>>    kernel lists a range for many rails. I used the values from the
>> ChromeOS
>>    kernel in all cases, since I know the board file there is the most
>>    complete available for this hardware.
>>
>> * The vdd_1v2 fixed regulator is present only in the schematic. So, I
>> added
>>    this based on the schematic.
>>
>> * A 3.3v fixed regulator using GPIO3 of the TPS6586x is present in the
>>    ChromeOS kernel, but not in the schematic. So, I dropped this based on
>>    the schematic.
>>
>> Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
> 
> Acked-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> I think this series very much depends on the series
> [PATCH V3 0/3] regulator: dt: add policy to match regulator with prop
> "regulator-compatible"

That's certainly true. However, it's a runtime dependency to enable a
new feature, and shouldn't cause any breakage without that patch, so we
don't need to be explicit about the dependency in git.

>> +                regulator@3 {
>> +                    reg =<3>;
>> +                    regulator-compatible = "ldo0";
>> +                    regulator-name = "vdd_ldo0";
>> +                    regulator-min-microvolt =<1250000>;
>> +                    regulator-max-microvolt =<3300000>;
>> +                    vin-supply =<&sm2_reg>;
> 
> I think support for vin-supply is still not there for this regulator in
> driver.

That's also true. I wonder if we shouldn't support this in the regulator
core bindings instead, since the existence of a parent regulator seems
likely to be common. Either way, I'd like to include the property to
document it for now.

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Monday 25 June 2012 08:42 PM, Stephen Warren wrote:
> On 06/25/2012 12:24 AM, Laxman Dewangan wrote:
>> On Saturday 23 June 2012 04:44 AM, Stephen Warren wrote:
>>> From: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a
>>> couple of fixed GPIO-controlled regulators too.
>>>
>>>
>>> Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>> Acked-by: Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> I think this series very much depends on the series
>> [PATCH V3 0/3] regulator: dt: add policy to match regulator with prop
>> "regulator-compatible"
> That's certainly true. However, it's a runtime dependency to enable a
> new feature, and shouldn't cause any breakage without that patch, so we
> don't need to be explicit about the dependency in git.
>

Here, wanted to say that although we enable it but it will not work. The 
functionality depends on the above patch.

>>> +                regulator@3 {
>>> +                    reg =<3>;
>>> +                    regulator-compatible = "ldo0";
>>> +                    regulator-name = "vdd_ldo0";
>>> +                    regulator-min-microvolt =<1250000>;
>>> +                    regulator-max-microvolt =<3300000>;
>>> +                    vin-supply =<&sm2_reg>;
>> I think support for vin-supply is still not there for this regulator in
>> driver.
> That's also true. I wonder if we shouldn't support this in the regulator
> core bindings instead, since the existence of a parent regulator seems
> likely to be common. Either way, I'd like to include the property to
> document it for now.

I had detailed discussion with Mark on  this support and as per him 
(based on my understanding), the input to different regulator is from 
the pin of the chips and so the name should be the <pin-name>-supply 
which should be part of chip-dt binding, not to the particular rail.

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Stephen Warren]

On 06/25/2012 09:24 AM, Laxman Dewangan wrote:
> On Monday 25 June 2012 08:42 PM, Stephen Warren wrote:
>> On 06/25/2012 12:24 AM, Laxman Dewangan wrote:
>>> On Saturday 23 June 2012 04:44 AM, Stephen Warren wrote:
>>>> From: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> Seaboard uses a TPS6586x regulator. Instantiate this, and hook up a
>>>> couple of fixed GPIO-controlled regulators too.
...
>>>> +                regulator@3 {
>>>> +                    reg =<3>;
>>>> +                    regulator-compatible = "ldo0";
>>>> +                    regulator-name = "vdd_ldo0";
>>>> +                    regulator-min-microvolt =<1250000>;
>>>> +                    regulator-max-microvolt =<3300000>;
>>>> +                    vin-supply =<&sm2_reg>;
>>> I think support for vin-supply is still not there for this regulator in
>>> driver.
>> That's also true. I wonder if we shouldn't support this in the regulator
>> core bindings instead, since the existence of a parent regulator seems
>> likely to be common. Either way, I'd like to include the property to
>> document it for now.
> 
> I had detailed discussion with Mark on  this support and as per him
> (based on my understanding), the input to different regulator is from
> the pin of the chips and so the name should be the <pin-name>-supply
> which should be part of chip-dt binding, not to the particular rail.

OK, that's fine. Can you please update the TPS6586x binding and driver
to allow this to be represented correctly then. Thanks.

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Mon, Jun 25, 2012 at 08:54:21PM +0530, Laxman Dewangan wrote:

> I had detailed discussion with Mark on  this support and as per him
> (based on my understanding), the input to different regulator is
> from the pin of the chips and so the name should be the
> <pin-name>-supply which should be part of chip-dt binding, not to
> the particular rail.

More specifically, all the supplies for a device (including those that
happen to be inputs for regulators) should be specified in exactly the
same fashion.  This makes the binding more regular and means that users
can just go through the schematic adding the mappings without worrying
about what what the supply happens to be.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Stephen Warren]

On 06/25/2012 04:26 PM, Mark Brown wrote:
> On Mon, Jun 25, 2012 at 08:54:21PM +0530, Laxman Dewangan wrote:
> 
>> I had detailed discussion with Mark on  this support and as per
>> him (based on my understanding), the input to different regulator
>> is from the pin of the chips and so the name should be the 
>> <pin-name>-supply which should be part of chip-dt binding, not
>> to the particular rail.
> 
> More specifically, all the supplies for a device (including those
> that happen to be inputs for regulators) should be specified in
> exactly the same fashion.  This makes the binding more regular and
> means that users can just go through the schematic adding the
> mappings without worrying about what what the supply happens to
> be.

Just making sure I parsed that right. I think what you're saying is
that the device itself should represent its input pins, e.g.:

tps6586x {
    vin-ldo01-supply = <&some_regulator>;
    vin-ldo23-supply = <...>;
    vin-ldo4-supply = <...>;
    vin-ldo678-supply = <...>;
    vin-ldo9-supply = <...>;

    regulators {
        regulator@0 {
            regulator-compatible = "ldo0";
            ...
        };
        regulator@1 {
            regulator-compatible = "ldo1";
            ...
        };
    };
};

(and then the driver internally uses the *-supply to set up the
parents of each of its own regulators)

... rather than each regulator specifying its parent, which might
result in some duplication, since in this case both ldo0/1 are
supplied from the same input pin:

tps6586x {
    regulators {
        regulator@0 {
            regulator-compatible = "ldo0";
            vin-supply = <&some_regulator>;
            ...
        };
        regulator@1 {
            regulator-compatible = "ldo1";
            vin-supply = <&some_regulator>;
            ...
        };
    };
};

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Tuesday 26 June 2012 04:39 AM, Stephen Warren wrote:
> On 06/25/2012 04:26 PM, Mark Brown wrote:
>> On Mon, Jun 25, 2012 at 08:54:21PM +0530, Laxman Dewangan wrote:
>>
>>> I had detailed discussion with Mark on  this support and as per
>>> him (based on my understanding), the input to different regulator
>>> is from the pin of the chips and so the name should be the
>>> <pin-name>-supply which should be part of chip-dt binding, not
>>> to the particular rail.
>> More specifically, all the supplies for a device (including those
>> that happen to be inputs for regulators) should be specified in
>> exactly the same fashion.  This makes the binding more regular and
>> means that users can just go through the schematic adding the
>> mappings without worrying about what what the supply happens to
>> be.
> Just making sure I parsed that right. I think what you're saying is
> that the device itself should represent its input pins, e.g.:
>
> tps6586x {
>      vin-ldo01-supply =<&some_regulator>;

I think it is fine. The pin name as per data sheet is VINLDO01 and so we 
can have vin-ldo01.


>      vin-ldo23-supply =<...>;
>      vin-ldo4-supply =<...>;
>      vin-ldo678-supply =<...>;
>      vin-ldo9-supply =<...>;
>
>      regulators {
>          regulator@0 {
>              regulator-compatible = "ldo0";
>              ...
>          };
>          regulator@1 {
>              regulator-compatible = "ldo1";
>              ...
>          };
>      };
> };
>
> (and then the driver internally uses the *-supply to set up the
> parents of each of its own regulators)
>
> ... rather than each regulator specifying its parent, which might
> result in some duplication, since in this case both ldo0/1 are
> supplied from the same input pin:
>
> tps6586x {
>      regulators {
>          regulator@0 {
>              regulator-compatible = "ldo0";
>              vin-supply =<&some_regulator>;
>              ...
>          };
>          regulator@1 {
>              regulator-compatible = "ldo1";
>              vin-supply =<&some_regulator>;
>              ...
>          };
>      };
> };

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Mon, Jun 25, 2012 at 05:09:56PM -0600, Stephen Warren wrote:

> Just making sure I parsed that right. I think what you're saying is
> that the device itself should represent its input pins, e.g.:
> 
> tps6586x {
>     vin-ldo01-supply = <&some_regulator>;

Yes, that's what I'd expect here.

> ... rather than each regulator specifying its parent, which might
> result in some duplication, since in this case both ldo0/1 are
> supplied from the same input pin:

Plus you then get back to the problem of users having to figure out
where to put the references which makes everything harder to use.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

Hi Mark,
I require  your input on supporting the vin-supply for tps6586x.

On Tuesday 26 June 2012 04:39 AM, Stephen Warren wrote:
> On 06/25/2012 04:26 PM, Mark Brown wrote:
>>
>> More specifically, all the supplies for a device (including those
>> that happen to be inputs for regulators) should be specified in
>> exactly the same fashion.  This makes the binding more regular and
>> means that users can just go through the schematic adding the
>> mappings without worrying about what what the supply happens to
>> be.
> Just making sure I parsed that right. I think what you're saying is
> that the device itself should represent its input pins, e.g.:
>
> tps6586x {
>      vin-ldo01-supply =<&some_regulator>;
>      vin-ldo23-supply =<...>;
>      vin-ldo4-supply =<...>;
>      vin-ldo678-supply =<...>;
>      vin-ldo9-supply =<...>;
> :::::::::::
> };

Looked tps6586x-regulator driver and it has the platform data which is 
regulator_init_data.
So for adding the vin-supply similar to what we have in fixed or 
tps6591x regulator to pass through the desc.supply_name, we are not 
having option here in platform data which can work for DT and non-DT case.

So if still want to have the DT and non-DT case similar, we can add one 
tps6586x_regulator_platform_data as

struct tps6586x_regulator_platform_data
{
     const char *input_supply;
     struct regulator_init_data *reg_init_data;
}

and then pass this when registering the regulator.

Or,
second option is to support the input supply name for DT case through 
desc.supply_name and for non-DT let it be there through regulator_init_data.

Please let me know your opinion.

Thanks,
Laxman

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

On Tue, Jul 10, 2012 at 05:29:06PM +0530, Laxman Dewangan wrote:

> Looked tps6586x-regulator driver and it has the platform data which
> is regulator_init_data.
> So for adding the vin-supply similar to what we have in fixed or
> tps6591x regulator to pass through the desc.supply_name, we are not
> having option here in platform data which can work for DT and non-DT
> case.

> So if still want to have the DT and non-DT case similar, we can add
> one tps6586x_regulator_platform_data as

The supply names are not supposed to be conditional (I realise they are
for your current DT patch, I let that in because I just want to see an
end to all this DT stuff).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Tuesday 10 July 2012 07:14 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 10, 2012 at 05:29:06PM +0530, Laxman Dewangan wrote:
>
>> Looked tps6586x-regulator driver and it has the platform data which
>> is regulator_init_data.
>> So for adding the vin-supply similar to what we have in fixed or
>> tps6591x regulator to pass through the desc.supply_name, we are not
>> having option here in platform data which can work for DT and non-DT
>> case.
>> So if still want to have the DT and non-DT case similar, we can add
>> one tps6586x_regulator_platform_data as
> The supply names are not supposed to be conditional (I realise they are
> for your current DT patch, I let that in because I just want to see an
> end to all this DT stuff).
>

Sorry, I did not get it fully. I had two patches, one for fixed one and 
other for tps65910. On which patch, you are seeing issue and which part 
of code?
I like to fix it if it does not conform the DT or your expectation or 
any future enhancement which you are planning.
Little bit more details will help so that I will not repeat the same and 
fix it in my future patches.

Thanks,
Laxman

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

On Tue, Jul 10, 2012 at 07:14:53PM +0530, Laxman Dewangan wrote:

> Sorry, I did not get it fully. I had two patches, one for fixed one
> and other for tps65910. On which patch, you are seeing issue and
> which part of code?

Mainly tps65910.  The fixed voltage regulator does have the same issue
but it is somewhat special here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Tuesday 10 July 2012 07:23 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 10, 2012 at 07:14:53PM +0530, Laxman Dewangan wrote:
>
>> Sorry, I did not get it fully. I had two patches, one for fixed one
>> and other for tps65910. On which patch, you are seeing issue and
>> which part of code?
> Mainly tps65910.  The fixed voltage regulator does have the same issue
> but it is somewhat special here.

Oops, I follow the same approach for tps65910 what it was with fixed 
regulator, was thinking that this is good approach.

The sample code for tps65910 is as follows. So where do you think that 
it is wrong and what should be correct approach.
I need to follow the same for tps6586x also.


@@ -1023,6 +1051,13 @@ static struct tps65910_board 
*tps65910_parse_dt_reg_data(
                  "ti,regulator-ext-sleep-control", &prop);
          if (!ret)
              pmic_plat_data->regulator_ext_sleep_control[idx] = prop;
+
+
+        if (info->vin_name) {
+            snprintf(in_supply, 32, "%s-supply", info->vin_name);
+            if (of_find_property(np, in_supply, 0))
+                pmic_plat_data->input_supply[idx] =
+                                info->vin_name;
+        }
      }

      return pmic_plat_data;
@@ -1126,6 +1161,7 @@ static __devinit int tps65910_probe(struct 
platform_device *pdev)
          pmic->info[i] = info;

          pmic->desc[i].name = info->name;
+        pmic->desc[i].supply_name = pmic_plat_data->input_supply[i];
          pmic->desc[i].id = i;

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Tue, Jul 10, 2012 at 08:34:24PM +0530, Laxman Dewangan wrote:

> The sample code for tps65910 is as follows. So where do you think
> that it is wrong and what should be correct approach.
> I need to follow the same for tps6586x also.

Like I said the use of a supply name should be unconditional, you should
just set the supply name in the descriptor.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Tuesday 10 July 2012 09:12 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 10, 2012 at 08:34:24PM +0530, Laxman Dewangan wrote:
>
>> The sample code for tps65910 is as follows. So where do you think
>> that it is wrong and what should be correct approach.
>> I need to follow the same for tps6586x also.
> Like I said the use of a supply name should be unconditional, you should
> just set the supply name in the descriptor.
>
if I set the desc.supply_name unconditional then if the entry of that 
supply name is not there in the DT entry (on device node) then that 
regulator will not get registered because the search for 
supply_regulator will fail during registration.

I am setting the desc.supply only when there is supply entry in DT or if 
it is from platform.

Do we need to make such supply pin entry as required inplace of optional?

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 160 bytes --]

On Tue, Jul 10, 2012 at 10:09:38PM +0530, Laxman Dewangan wrote:

> Do we need to make such supply pin entry as required inplace of optional?

That's the idea.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Tuesday 10 July 2012 10:22 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 10, 2012 at 10:09:38PM +0530, Laxman Dewangan wrote:
>
>> Do we need to make such supply pin entry as required inplace of optional?
> That's the idea.

Then I will have 2 question:
1. If that pin is connected to battery then what should be entry?
    vcc1-supply = <>>>;
Do we need to register battery supply as fixed (non-gpio based) and then 
refer here for phandle?

2. what about non-dt case? The regulator registration will also fail 
here because of there is no regulator saying vcc1 in this case. Do we 
need to register the vcc1 regualtor as fixed, battery supplied regulator 
in board files so that the ldo's registration will success?

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

On Tue, Jul 10, 2012 at 10:23:48PM +0530, Laxman Dewangan wrote:

> Then I will have 2 question:
> 1. If that pin is connected to battery then what should be entry?
>    vcc1-supply = <>>>;
> Do we need to register battery supply as fixed (non-gpio based) and
> then refer here for phandle?

Use a fixed voltage regulator to represent the battery.

> 2. what about non-dt case? The regulator registration will also fail
> here because of there is no regulator saying vcc1 in this case. Do
> we need to register the vcc1 regualtor as fixed, battery supplied
> regulator in board files so that the ldo's registration will
> success?

Same thing, use a fixed voltage regulator.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] ARM: dt: tegra: seaboard: add regulators

[Laxman Dewangan]

On Tuesday 10 July 2012 10:31 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 10, 2012 at 10:23:48PM +0530, Laxman Dewangan wrote:
>
>> Then I will have 2 question:
>> 1. If that pin is connected to battery then what should be entry?
>>     vcc1-supply =<>>>;
>> Do we need to register battery supply as fixed (non-gpio based) and
>> then refer here for phandle?
> Use a fixed voltage regulator to represent the battery.
>
>> 2. what about non-dt case? The regulator registration will also fail
>> here because of there is no regulator saying vcc1 in this case. Do
>> we need to register the vcc1 regualtor as fixed, battery supplied
>> regulator in board files so that the ldo's registration will
>> success?
> Same thing, use a fixed voltage regulator.


Understood completely and it is so simple to support vin-supply. Will 
implement the same. Thanks for clearing doubts.