Re: [PATCH 2/2] arm64: dts: rockchip: Add Orange Pi 5 Max board

["Heiko Stübner" <heiko@sntech.de>]

Hi Jimmy,

Am Donnerstag, 31. Oktober 2024, 04:14:51 CET schrieb Jimmy Hon:
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-max.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-max.dts
> new file mode 100644
> index 000000000000..d31b13f99fdc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-max.dts
> @@ -0,0 +1,887 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +#include "rk3588.dtsi"

can you check how similar the rk3588-orangepi--5-max is to the
already existing orangepi-5-plus please?

For rk3588s-orangepi-5 and orangepi-5b we already have this nicely
shared dtsi for commit things, and somehow I assume this might be
similar for 5-plus vs 5-max.

> +
> +/ {
> +	model = "Xunlong Orange Pi 5 Max";
> +	compatible = "xunlong,orangepi-5-max", "rockchip,rk3588";
> +

[...]

> +	/* PMIC_EXT_EN */
> +	vcc_1v1_nldo_s3: vcc-1v1-ndlo-s3-regulator {

vcc_1v1_nldo_s3: regulator-vcc-1v1-ndlo-s3 {

preferred pattern for fixed-regulator nodenames is regulator-......

Same for the othe fixed regulators below.

> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_1v1_nldo_s3";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1100000>;
> +		regulator-max-microvolt = <1100000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
> +	/* pcie eth. not a real regulator. 33VAUX */
> +	vcc_3v3_aux: vcc-3v3-aux-regulator {
> +		compatible = "regulator-fixed";
> +		/* Ethernet_power_en */
> +		gpios = <&gpio0 RK_PD3 GPIO_ACTIVE_LOW>;
> +		regulator-name = "33vaux";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		startup-delay-us = <50000>;
> +		vin-supply = <&vcc_3v3_s3>;
> +	};
> +
> +	vcc3v3_pcie30: vcc3v3-pcie30-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		/* PCIE_PWREN_H */
> +		gpios = <&gpio2 RK_PB6 GPIO_ACTIVE_HIGH>;
> +		regulator-name = "vcc3v3_pcie30";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		startup-delay-us = <5000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
> +	/* Regulator is enabled whenever vcc_1v8_s0 is on */
> +	vcc_3v3_s0: vcc-3v3-s0-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_3v3_s0";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc_3v3_s3>;
> +
> +		regulator-state-mem {
> +			regulator-off-in-suspend;
> +		};
> +	};
> +
> +	vcc5v0_sys: vcc5v0-sys-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +
> +	/* Represents the vcc5v0_usb20 in the schematic */
> +	/* Also represents the vcc5v0_usb30 in the schematic,
> +	   both regulators share the same enable gpio */

please make that one comment block and use appropriate formatting

/*
 * Represents the vcc5v0_usb20 and vcc5v0_usb30 in the schematic,
 * both regulators share the same enable gpio
 */


> +	vcc5v0_host: vcc5v0-host-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		/* USB_HOST_PWREN */
> +		gpios = <&gpio3 RK_PD5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_host_pwren>;
> +		regulator-name = "vcc5v0_host";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
> +	vcc5v0_usb30_otg: vcc5v0-usb30-otg-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		/* USB_OTG_PWREN */
> +		gpios = <&gpio4 RK_PB3 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_otg_pwren>;
> +		regulator-name = "vcc5v0_usb30_otg";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +};


> +	hym8563: rtc@51 {
> +		compatible = "haoyu,hym8563";
> +		reg = <0x51>;
> +		/* RTC_INT_L */

what does this comment try to explain? If that is the name of the pin,
just name the pinctrl below accordingly (instead of hym8563_int
if applicable) .

> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PC4 IRQ_TYPE_LEVEL_LOW>;
> +		#clock-cells = <0>;
> +		clock-output-names = "hym8563";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&hym8563_int>;
> +		wakeup-source;
> +	};
> +};
> +
> +&pwm9 {
> +	pinctrl-names = "active";

this needs to be "default" to actually work

> +	pinctrl-0 = <&pwm9m2_pins>;
> +	status = "okay";
> +};


Thanks
Heiko