[PATCH v1 2/2] arm64: dts: rockchip: add DTs for Firefly ROC-RK3588S-PC

[Chukun Pan]

Hi,

> <snip>
> +	leds: leds {

No aliases needed.

> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&leds_gpio>;
> +
> +		power_led: power {
> +			label = ":power";

Please use standard LED binding.
e.g.
```
color = <LED_COLOR_ID_BLUE>;
function = LED_FUNCTION_POWER;
```

> +			linux,default-trigger = "heartbeat";
> +			default-state = "on";

default-state is not needed.

> +			gpios = <&gpio1 RK_PD5 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		user_led: user {
> +			label = ":user";
> +			linux,default-trigger = "ir-user-click";

This LED should be user-defined?

> +			default-state = "off";
> +			gpios = <&gpio3 RK_PB2 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		user1_led: user1 {
> +			label = ":user1";
> +			default-state = "off";
> +			gpios = <&gpio3 RK_PC0 GPIO_ACTIVE_HIGH>;
> +		};
> +	};

> <snip>
> +	vcc3v3_pcie20: regulator-vcc3v3-pcie20 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3_pcie20";
> +		regulator-always-on;

Why does this regulator require always-on?

> <snip>
> +	vcc5v0_host: regulator-vcc5v0-host {
> +		regulator-name = "vcc5v0_host";
> +		regulator-boot-on;
> +		regulator-always-on;

Why does this regulator require always-on and boot-on?

> +		vin-supply = <&vcc5v0_sys>;

The vendor dts says it's from vcc5v0_usb?

> +	vbus5v0_typec_pwr_en: vbus5v0-typec-pwr-en-regulator {
> +		regulator-name = "vbus5v0_typec_pwr_en";

Please use the name from schematics.

> +		regulator-boot-on;
> +		regulator-always-on;

Why does this regulator require always-on and boot-on?

> +		vin-supply = <&vcc5v0_sys>;

The vendor dts says it's from vcc5v0_usb?

> <snip>
> +&cpu_l0 {
> +	cpu-supply = <&vdd_cpu_lit_s0>;
> +};
> +
> +&cpu_b0 {
> +	cpu-supply = <&vdd_cpu_big0_s0>;
> +};
> +
> +&cpu_b2 {
> +	cpu-supply = <&vdd_cpu_big1_s0>;
> +};

Please add other cpu nodes: cpu_l1-l3, cpu_b1-b3

+&gmac1 {
+	clock_in_out = "output";
+	phy-handle = <&rgmii_phy1>;
+	phy-mode = "rgmii-rxid";
+	tx_delay = <0x43>;

Please try using rgmii-id and remove the delay.

> <snip>
> +&i2c2 {
> +	status = "okay";
> +	pinctrl-0 = <&i2c2m0_xfer>;

status should be placed behind.

> +	hym8563: rtc at 51 {
> +		compatible = "haoyu,hym8563";
> +		reg = <0x51>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PB0 IRQ_TYPE_LEVEL_LOW>;
> +		#clock-cells = <0>;
> +		clock-output-names = "hym8563";

clock should be placed before interrupt.

> +	usbc0: usb-typec at 22 {

This node should be placed before vdd_npu_s0.

> <snip>
> +&mdio1 {
> +	rgmii_phy1: ethernet-phy at 1 {
> +		compatible = "ethernet-phy-id001c.c916";

The phy used in this board is RTL8211FVD, not RTL8211F.
Please use "ethernet-phy-ieee802.3-c22" instead of hardcoding.

> <snip>
> +&sdhci {
> +	bus-width = <8>;
> +	max-frequency = <200000000>;

max-frequency is already defined in rk3588-base.dtsi

> <snip>
> +&sdmmc {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	no-sdio;
> +	no-mmc;

The sdmmc controller supports sdio devices.
If no-mmc is defined, cap-mmc-highspeed is useless.

> <snip>
> +&spi2 {
> +	status = "okay";

status should be placed behind.

> +		pinctrl-0 = <&pmic_pins>, <&rk806_dvs1_null>,
> +				<&rk806_dvs2_null>, <&rk806_dvs3_null>;

Align Indent.

> <snip>
> +&uart7 {
> +	pinctrl-0 = <&uart7m2_xfer>;
> +	status = "okay";
> +};

It seems that uart2 (default serial port) is missing?

--
2.25.1