diff mbox

[1/2] Regulator: mc34708: Add DT binding documentation

Message ID 20150428141738.16243.18377.stgit@localhost
State Needs Review / ACK, archived
Headers show

Checks

Context Check Description
robh/checkpatch warning total: 1 errors, 0 warnings, 0 lines checked
robh/patch-applied success

Commit Message

Martin Fuzzey April 28, 2015, 2:17 p.m. UTC 
Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 .../bindings/regulator/mc34708-regulator.txt       |  198 ++++++++++++++++++++
 1 file changed, 198 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mc34708-regulator.txt


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark Brown April 30, 2015, 6:56 p.m. UTC | #1 
On Tue, Apr 28, 2015 at 04:17:38PM +0200, Martin Fuzzey wrote:

> +			pmic_sw3_reg: sw3 {
> +				regulator-name = "SW3";
> +				regulator-min-microvolt = <650000>;
> +				regulator-max-microvolt = <1425000>;
> +			};

These examples look awfully like you're just listiong the maximum range
allowed by the PMIC for all the supplies which is almost never something
someone would want to do on a real board.  Similarly all the names are
just the name of the regulator rather than something that looks like a
board specific name.
Stefan Wahren May 1, 2015, 10:34 a.m. UTC | #2 
Hi Martin,

> Martin Fuzzey <mfuzzey@parkeon.com> hat am 28. April 2015 um 16:17
> geschrieben:
>
>
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>

a commit message would be nice and the subject should specify dt-bindings
instead of regulator subsystem.

Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 1, 2015, 11:01 a.m. UTC | #3 
On Fri, May 01, 2015 at 12:34:21PM +0200, Stefan Wahren wrote:

> a commit message would be nice and the subject should specify dt-bindings
> instead of regulator subsystem.

No, it shouldn't.  DT bindings for a subsystem go through the subsystem
and the subsystem maintainer needs to see them to read them.
Stefan Wahren May 1, 2015, 12:33 p.m. UTC | #4 
Hi Mark,

> Mark Brown <broonie@kernel.org> hat am 1. Mai 2015 um 13:01 geschrieben:
>
>
> On Fri, May 01, 2015 at 12:34:21PM +0200, Stefan Wahren wrote:
>
> > a commit message would be nice and the subject should specify dt-bindings
> > instead of regulator subsystem.
>
> No, it shouldn't. DT bindings for a subsystem go through the subsystem
> and the subsystem maintainer needs to see them to read them.

sorry for my bad expressing. 

I tought the subject line should be something like:

DT: add binding documentation for mc34708 regulator

Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 1, 2015, 2:03 p.m. UTC | #5 
On Fri, May 01, 2015 at 02:33:44PM +0200, Stefan Wahren wrote:
> > Mark Brown <broonie@kernel.org> hat am 1. Mai 2015 um 13:01 geschrieben:

> > On Fri, May 01, 2015 at 12:34:21PM +0200, Stefan Wahren wrote:

> > > a commit message would be nice and the subject should specify dt-bindings
> > > instead of regulator subsystem.

> > No, it shouldn't. DT bindings for a subsystem go through the subsystem
> > and the subsystem maintainer needs to see them to read them.

> sorry for my bad expressing. 

> I tought the subject line should be something like:

> DT: add binding documentation for mc34708 regulator

No, it shouldn't.  The subject line should start "regulator: " like all
other regulator commits.
Javier Martinez Canillas May 11, 2015, 9:36 a.m. UTC | #6 
Hello Martin,

I haven't looked at your regulator driver in detail so this is not a
full review but I just noticed something so I wanted mention.

> +
> +
> +Example:
> +&i2c3 {
> +       pmic: mc34708@08 {
> +               compatible = "fsl,mc34708";
> +               reg = <0x08>;
> +               regulators {
> +#define PMIC_REGMODE_SW_PFM    1
> +#define PMIC_REGMODE_SW_AUTO   2
> +#define PMIC_REGMODE_SW_PWM    3
> +#define PMIC_REGMODE_LDO_LP    1
> +#define PMIC_REGMODE_LDO_NORMAL        2
> +

I see that you are defining these constants in your driver too (albeit
with slightly different names). These kind of constants that are
shared between the driver implementation and the Device Tree source
files should be added to include/dt-bindings/regulator/ and included
in both the driver and the DTS.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/regulator/mc34708-regulator.txt b/Documentation/devicetree/bindings/regulator/mc34708-regulator.txt
new file mode 100644
index 0000000..35efae0
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mc34708-regulator.txt
@@ -0,0 +1,198 @@ 
+Regulators included in the Freescale MC34708 PMIC
+
+Required properties:
+- compatible: "fsl,mc34708"
+- reg: I2C slave address
+
+
+Regulators subnode:
+-------------------
+This node contains children following the standard regulator binding format
+described in Documentation/devicetree/bindings/regulator/regulator.txt
+
+The allowed node names are:
+	Switchers:
+		sw1, sw2, sw3, sw4a, sw4b, sw5, swbst
+	LDOs:
+		vpll, vrefddr, vusb, vusb2, vdac, vgen1, vgen2
+
+The mode values are:
+	Switchers:
+		1 : Pulse Frequency Modulation (PFM) [for low loads]
+		2 : Auto
+		3 : Pulse Width Modulation (PWM) [for high loads]
+	LDOs:
+		1 : Low power
+		2 : Normal
+
+Optional properties:
+The input supply of regulators are the optional properties on the
+regulator node.
+
+- vinsw1-supply : phandle to input supply for sw1 regulator
+- vinsw2-supply : phandle to input supply for sw2 regulator
+- vinsw3-supply : phandle to input supply for sw3 regulator
+- vinsw4a-supply : phandle to input supply for sw4a regulator
+- vinsw4b-supply : phandle to input supply for sw4b regulator
+- vinsw5-supply : phandle to input supply for sw5 regulator
+- vinswbst-supply : phandle to input supply for swbst regulator
+- vinrefddr-supply : phandle to input supply for vrefddr regulator (/2)
+
+
+Example:
+&i2c3 {
+	pmic: mc34708@08 {
+		compatible = "fsl,mc34708";
+		reg = <0x08>;
+		regulators {
+#define PMIC_REGMODE_SW_PFM	1
+#define PMIC_REGMODE_SW_AUTO	2
+#define PMIC_REGMODE_SW_PWM	3
+#define PMIC_REGMODE_LDO_LP	1
+#define PMIC_REGMODE_LDO_NORMAL	2
+
+			vinrefddr-supply = <&pmic_sw4a_reg>;
+			pmic_sw1_reg: sw1 {
+				/* CPU Core */
+				regulator-name = "SW1";
+				regulator-min-microvolt = <650000>;
+				regulator-max-microvolt = <1437500>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_SW_PWM>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <850000>;
+					regulator-mode = <PMIC_REGMODE_SW_PFM>;
+				};
+			};
+
+			pmic_sw2_reg: sw2 {
+				/* SOC Periperals */
+				regulator-name = "SW2";
+				regulator-min-microvolt = <650000>;
+				regulator-max-microvolt = <1437500>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_SW_PWM>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <950000>;
+					regulator-mode = <PMIC_REGMODE_SW_PFM>;
+				};
+			};
+
+			pmic_sw3_reg: sw3 {
+				regulator-name = "SW3";
+				regulator-min-microvolt = <650000>;
+				regulator-max-microvolt = <1425000>;
+			};
+
+			pmic_sw4a_reg: sw4a {
+				/* DDR Ram */
+				regulator-name = "SW4A";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_SW_PWM>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-mode = <PMIC_REGMODE_SW_PFM>;
+				};
+			};
+
+			pmic_sw5_reg: sw5 {
+				/* 1v8 power */
+				regulator-name = "SW5";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1975000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_SW_PWM>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-mode = <PMIC_REGMODE_SW_PFM>;
+				};
+			};
+
+			pmic_swbst_reg: swbst {
+				regulator-name = "SWBST";
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_SW_AUTO>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			pmic_vpll_reg: vpll {
+				regulator-name = "VPLL";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			pmic_vrefddr_reg: vrefddr {
+				regulator-name = "VREFDDR";
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			pmic_vusb_reg: vusb {
+				regulator-name = "VUSB";
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			pmic_vusb2_reg: vusb2 {
+				regulator-name = "VUSB2";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			pmic_vdac_reg: vdac {
+				regulator-name = "VDAC";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <2775000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_LDO_NORMAL>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-mode = <PMIC_REGMODE_LDO_LP>;
+				};
+			};
+
+			pmic_vgen1_reg: vgen1 {
+				regulator-name = "VGEN1";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1550000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			pmic_vgen2_reg: vgen2 {
+				regulator-name = "VGEN2";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-initial-mode = <PMIC_REGMODE_LDO_NORMAL>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-mode = <PMIC_REGMODE_LDO_LP>;
+				};
+			};
+		};
+	};
+};