Re: [PATCH 2/2] arm64: dts: rockchip: add MNT Reform 2

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Patrick Wildt <patrick@blueri.se>, linux-rockchip@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	Kever Yang <kever.yang@rock-chips.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: add MNT Reform 2
Date: Thu, 30 Jan 2025 13:05:45 +0100	[thread overview]
Message-ID: <cd6edfed-08a8-44de-9bfb-5199fd2813fd@kernel.org> (raw)
In-Reply-To: <Z5tobRnjXmr4dl7j@windev.fritz.box>

On 30/01/2025 12:54, Patrick Wildt wrote:
> +
> +&hdptxphy_hdmi0 {
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c0m2_xfer>;
> +	status = "okay";
> +
> +	vdd_cpu_big0_s0: vdd_cpu_big0_mem_s0: rk8602@42 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "rockchip,rk8602";
> +		reg = <0x42>;
> +
> +		vin-supply = <&vcc5v0_sys>;
> +		fcs,suspend-voltage-selector = <1>;
> +		rockchip,suspend-voltage-selector = <1>;
> +
> +		regulator-compatible = "rk860x-reg";
> +		regulator-name = "vdd_cpu_big0_s0";
> +		regulator-min-microvolt = <0x86470>;
> +		regulator-max-microvolt = <0x100590>;
> +		regulator-ramp-delay = <0x8fc>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +
> +		regulator-state-mem {
> +			regulator-off-in-suspend;
> +		};
> +	};
> +
> +	vdd_cpu_big1_s0: vdd_cpu_big1_mem_s0: rk8603@43 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "rockchip,rk8602", "rockchip,rk8603";
> +		reg = <0x43>;
> +

...

> +&spi2 {
> +	status = "okay";
> +	assigned-clocks = <&cru CLK_SPI2>;
> +	assigned-clock-rates = <200000000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi2m2_cs0 &spi2m2_pins>;
> +	num-cs = <1>;
> +
> +	rk806single: pmic@0 {
> +		compatible = "rockchip,rk806";
> +		spi-max-frequency = <1000000>;
> +		reg = <0x0>;

reg is always theh second property.

> +
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +
> +		pinctrl-names = "default", "pmic-power-off";
> +		pinctrl-0 = <&pmic_pins>, <&rk806_dvs1_null>,
> +			    <&rk806_dvs2_null>, <&rk806_dvs3_null>;
> +		pinctrl-1 = <&rk806_dvs1_pwrdn>;
> +
> +		pmic-reset-func = <1>;
> +
> +		vcc1-supply = <&vcc5v0_sys>;
> +		vcc2-supply = <&vcc5v0_sys>;
> +		vcc3-supply = <&vcc5v0_sys>;
> +		vcc4-supply = <&vcc5v0_sys>;
> +		vcc5-supply = <&vcc5v0_sys>;
> +		vcc6-supply = <&vcc5v0_sys>;
> +		vcc7-supply = <&vcc5v0_sys>;
> +		vcc8-supply = <&vcc5v0_sys>;
> +		vcc9-supply = <&vcc5v0_sys>;
> +		vcc10-supply = <&vcc5v0_sys>;
> +		vcc11-supply = <&vcc_2v0_pldo_s3>;
> +		vcc12-supply = <&vcc5v0_sys>;
> +		vcc13-supply = <&vcc_1v1_nldo_s3>;
> +		vcc14-supply = <&vcc_1v1_nldo_s3>;
> +		vcca-supply = <&vcc5v0_sys>;
> +
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +
> +		rk806_dvs1_null: dvs1-null-pins {
> +			pins = "gpio_pwrctrl2";
> +			function = "pin_fun0";
> +		};
> +
> +		rk806_dvs1_slp: rk806_dvs1_slp {

No underscores in node names. Missing suffix also suggests this fails
tests, so standard disclaimer:

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

> +			pins = "gpio_pwrctrl1";
> +			function = "pin_fun1";
> +		};

Best regards,
Krzysztof