Re: [PATCH 3/3] arm64: dts: rockchip: Add Radxa E24C

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

Hi,

> +	avddl_1v1: avddh_3v3: avdd_rtl8367rb: regulator-avdd-rtl8367rb {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpios = <&gpio1 RK_PC3 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gpio_8367_en>;
> +		regulator-name = "avdd_rtl8367rb";

I don't see the avdd_rtl8367rb regulator in the schematics. It looks like
DVDDIO (RTL8367RB power) is connected to AVDDH_3V3 via a magnetic bead.

> +&gmac1 {
> +	clock_in_out = "output";
> +	phy-mode = "rgmii-id";
> +	phy-supply = <&avdd_rtl8367rb>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&rgmii_miim>, <&rgmii_tx_bus2>, <&rgmii_rx_bus2>,
> +		    <&rgmii_rgmii_clk>, <&rgmii_rgmii_bus>, <&gmac1_rstn_l>;

Should the pinctrl of gmac1_rstn_l be written together with the
reset-gpios of the rtl8367rb switch?

```
reset-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_LOW>;
pinctrl-0 = <&gmac1_rstn_l>;
```

> +&i2c0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c0m0_xfer>;
> +	status = "okay";
> +
> +	rk805: pmic@18 {
> +		compatible = "rockchip,rk805";
> +		reg = <0x18>;
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <RK_PB2 IRQ_TYPE_LEVEL_LOW>;
> +		#clock-cells = <1>;
> +		clock-output-names = "rk805-clkout1", "rk805-clkout2";

The clkout pin is not connected, but the dt-bindings require it.
Maybe clock-output-names could be made optional?

+&mdio1 {
+	reset-delay-us = <25000>;
+	reset-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_LOW>;
+	reset-post-delay-us = <100000>;
+};

I don't think this is correct, reset-gpios should be written on the
rtl8365mb switch node. The switch driver has defined the reset time.

```
&mdio1 {
	switch@29 {
		compatible = "realtek,rtl8365mb";
		reg = <29>;
		reset-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_LOW>;
```

Thanks,
Chukun

--
2.25.1