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

[Chukun Pan <amadeus@jmu.edu.cn>]

Hi,

> <snip>
> +		led-0 {
> +			default-state = "on";
> +			function = LED_FUNCTION_POWER;
> +			gpios = <&gpio1 RK_PD5 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		led-1 {
> +			function = LED_FUNCTION_HEARTBEAT;
> +			gpios = <&gpio3 RK_PB2 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};

From PATCH v1, led-1 is a user-led, so it would be better
to make it off by default.

Please also add `color = xxx` to these leds.

> +	vcc5v0_usbdcin: regulator-vcc5v0-usbdcin {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_usbdcin";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc12v_dcin>;
> +	};

The DC of this board is 12V, why is there 5V?

> <snip>
> +	vcc3v3_pcie20: regulator-vcc3v3-pcie20 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3_pcie20";
> +		enable-active-high;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		startup-delay-us = <5000>;
> +		gpio = <&gpio1 RK_PD7 GPIO_ACTIVE_HIGH>;

Please put enable/gpio before regulator.

> <snip>
> +	vcc5v0_host: regulator-vcc5v0-host {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc5v0_host_en>;
> +		regulator-name = "vcc5v0_host";
> +		regulator-boot-on;
> +		regulator-always-on;

Why does this regulator require always-on and boot-on?
It will be enabled through the corresponding phy-supply.

> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc5v0_sys>;

The vendor dts says it's from vcc5v0_usb?

> +	vbus5v0_typec_pwr_en: vbus5v0-typec-pwr-en-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio1 RK_PB1 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&typec5v_pwren>;
> +		regulator-name = "vbus5v0_typec_pwr_en";

Please use the regulator name from schematics.
xxx_pwr_en is usually the name of the pinctrl.

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

Why does this regulator require always-on and boot-on?
It will be enabled through the corresponding phy-supply.

> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc5v0_sys>;

The vendor dts says it's from vcc5v0_usb?

> +&gmac1 {
> +	clock_in_out = "output";
> +	phy-handle = <&rgmii_phy1>;
> +	phy-mode = "rgmii-id";
> +	pinctrl-0 = ...
> +	pinctrl-names = "default";

pinctrl-names should be placed before pinctrl-0

> <snip>
> +		usb_con: connector {
> +			compatible = "usb-c-connector";
> +			label = "USB-C";
> +			data-role = "dual";
> +			op-sink-microwatt = <1000000>;
> +			power-role = "dual";
> +			sink-pdos =
> +				<PDO_FIXED(5000, 1000, PDO_FIXED_USB_COMM)>;
> +			source-pdos =
> +				<PDO_FIXED(5000, 3000, PDO_FIXED_USB_COMM)>;
> +			try-power-role = "sink";

The TYPE-C of this board does not seem to support pd power supply?

> <snip>
> +	};
> +};
> +

Extra blank lines.

> +
> +&i2c3 {
> +	status = "okay";

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

Align Indent.

> <snip>
> +	};
> +};
> +

Extra blank lines.

> +
> +&tsadc {
> +	status = "okay";
> +};

> <snip>
> +&u2phy0_otg {
> +	status = "okay";
> +};

> +&u2phy3_host {
> +	status = "okay";
> +};

Missing phy-supply?

--
2.25.1