Re: [PATCH] arm64: dts: rockchip: add support for Rongpin king3399 board

[Heiko Stübner]

Hi,

all of what Krzysztof said in his mail + the stuff below please:

Am Freitag, 30. Juni 2023, 19:27:52 CEST schrieb ocfox:
> king3399 is a bord from Rongpin, you can find detail about
> it here:
> (http://www.rpdzkj.com/copy_2_1703937_175104_2536441.html)
> 
> This patch add basic node for the board and make it able to bring
> up.
> 
> Works:
> - USB, type-C port
> - Ethernet
> - WiFi/BT
> - HDMI
> - eMMC
> - PCIe
> - sd-card
> 
> Not test for other peripheral:
> - AUDIO
> - MIC
> - SIM
> - MIPI
> 
> Signed-off-by: ocfox <i@xxxxxxxx>

I think we request a full name here and as patch author.

[...]

> +	vcc3v3_sys: vcc3v3-sys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
> +	// 4G Module

please use a consistent comment style ... in the "/* foobar */" style

> +	vcc3v3_gsm: vcc3v3-gsm {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio4 RK_PC6 GPIO_ACTIVE_HIGH>; // bsp
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc3v3_gsm_en>;
> +		regulator-name = "vcc3v3_gsm";
> +		// regulator-always-on;
> +		vin-supply = <&dc_12v>;
> +	};
> +
> +	// vdd 5v: USB 2&3, USB Hub, Type-C, HDMI, MIPI, IR

so this regulator controls all of those?

In that case maybe do it similarly to vcc1v8-s3 and use multiple labels
for the node - i.e. the line name leading to the consumer from the
schematics.

That way comparing schematics against the dts gets easier.

See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts#n40
for an example


> +	vcc5v0_host: vcc5v0-host-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		regulator-boot-on;
> +		gpio = <&gpio4 RK_PD6 GPIO_ACTIVE_HIGH>; // bsp
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc5v0_host_en>;
> +		regulator-name = "vcc5v0_host";
> +		// regulator-always-on;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
> +	vcc5v0_typec0: vbus-typec-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>; // bsp
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc5v0_typec_en>;
> +		regulator-name = "vcc5v0_typec0";
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
> +	vcc_sys: vcc5v0_sys: vcc5v0-sys { // bsp
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&dc_12v>;
> +	};
> +
> +

double empty line

Thanks
Heiko