Re: [PATCH v9 2/2] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

And thanks again Lee!

On Thu, Jul 05, 2018 at 10:24:44AM +0100, Lee Jones wrote:

On Thu, 05 Jul 2018, Matti Vaittinen wrote:

quoted

Document devicetree bindings for ROHM BD71837 PMIC MFD.

Signed-off-by: Matti Vaittinen <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 67 ++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
new file mode 100644
index 000000000000..67f2616288d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt

@@ -0,0 +1,67 @@
+* ROHM BD71837 Power Management Integrated Circuit bindings
+
+BD71837MWV is a programmable Power Management IC for powering single-core,
+dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for

Nit: s/SOC's/SOCs/

I'll change this.

quoted

+low BOM cost and compact solution footprint. It integrates 8 Buck
+egulators and 7 LDO’s to provide all the power rails required by the SoC and

Nit: s/LDO's/LDOs/

I'll change this.

quoted

+the commonly used peripherals.
+
+Datasheet for PMIC is available at:
+https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
+
+Required properties:
+ - compatible		: Should be "rohm,bd71837".
+ - reg			: I2C slave address.
+ - interrupt-parent	: Phandle to the parent interrupt controller.
+ - interrupts		: The interrupt line the device is connected to.
+ - clocks		: The parent clock connected to PMIC. If this is missng

s/missng/missing/

I'll change this.

quoted

+			  32768 KHz clock is assumed.
+ - #clock-cells		: Should be 0
+ - regulators:		: List of child nodes that specify the regulators

By your convention, the bottom two entries are missing '.'s.

I'll change this.

quoted

+			  Please see ../regulator/rohm,bd71837-regulator.txt
+
+Optional properties:
+- clock-output-names	: Should contain name for output clock.
+
+Example:
+
+	/* external oscillator node */

Uppercase character to start a sentence.

I'll change this.

quoted

+	osc: oscillator {
+		compatible = "fixed-clock";
+		#clock-cells = <1>;
+		clock-frequency  = <32768>;
+		clock-output-names = "osc";
+	};
+
+	/* PMIC node */

Is this comment really necessary?  The node is called "pmic".

I'll remove this.

quoted

+	pmic: pmic@4b {
+		compatible = "rohm,bd71837";
+		reg = <0x4b>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <29 GPIO_ACTIVE_LOW>;
+		interrupt-names = "irq";
+		#clock-cells = <0>;
+		clocks = <&osc 0>;
+		clock-output-names = "bd71837-32k-out";
+
+		regulators {
+			buck1: BUCK1 {
+				regulator-name = "buck1";
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-boot-on;
+				regulator-ramp-delay = <1250>;
+			};
+			/* ... */
+		};
+	};
+
+	/* Clock consumer node */
+
+	foo@0 {
+		compatible = "bar,foo";
+		/* ... */

No need for this line.  Additional properties are always expected.

I'll remove this.

quoted

+		clock-names = "my-clock";
+		clocks = <&pmic>;
+	};

Do you have a real example to give?

For clock consumer? Sorry, no I don't.

Br,
	Matti Vaittinen

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog