Skip to contentSkip to conversation

riscv: dts: spacemit: Add PMIC regulators usb pcie

PATCH Superseded Han Gao 6 participants 19 messages Jan 31 – Mar 14

Subsystems (3)

v1 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Han Gao

author
#0 lore raw

cover riscv: dts: spacemit: Add PMIC regulators usb pcie

This patch series enables the PMIC, voltage regulators, usb and pcie for the OrangePi rv2 board.

Han Gao (7):
riscv: dts: spacemit: Enable i2c8 adapter for OrangePi RV2
riscv: dts: spacemit: Define fixed regulators for OrangePi RV2
riscv: dts: spacemit: Define the P1 PMIC regulators for OrangePi RV2
riscv: dts: spacemit: Enable USB3.0 on OrangePi RV2
riscv: dts: spacemit: Update PMIC supply properties for OrangePi RV2
riscv: dts: spacemit: Add a PCIe regulator for OrangePi RV2
riscv: dts: spacemit: PCIe and PHY-related updates for OrangePi RV2

 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 235 ++++++++++++++++++
 1 file changed, 235 insertions(+)

base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
prerequisite-patch-id: 32d6acfcb4801407e38510b391407943e9a8ec82
prerequisite-patch-id: ae644485e1a8ead3fa7a087c8db9062c7acc14ff
prerequisite-patch-id: 605d537aa0f8387b3523786a875c76b9edfbcbb4
prerequisite-patch-id: faab9c820ac67de42c40f2c4578bb5b8753fcb71
prerequisite-patch-id: f9c4643cc99e5b9f1acc9daf1e679b95f81cee73
prerequisite-patch-id: 50138c8918218ea3b8650999bd558ed6350ed2f2
prerequisite-patch-id: f8f91e32361cc99c531170c39ed54cd1b3c5997d
prerequisite-patch-id: 475d1968c0c41895578809410c9bd8fe24997de4
prerequisite-patch-id: c40f1562f712f04578ef58472840d257a19919fe
prerequisite-patch-id: e9ef3af6c80187e6f05444704d13212d3beaf40a
prerequisite-patch-id: 7076909bb4103588b4761d175eca40bce470230f
prerequisite-patch-id: 0c859b4d131b3360875c795c6148c6176b55fb91
prerequisite-patch-id: 2ed98dc1ab0f5ed923cc252415c345dc8caf6f17
prerequisite-patch-id: 1be1a031763fac029076a768f012af31e455be66
prerequisite-patch-id: 21bb8387c946e050910440e7a7622305d46d946d
prerequisite-patch-id: f3bdc2c74b230663710086bd770a755d56cb8b9c
prerequisite-patch-id: ba6a7df5c7a2fa9ce0b8a72a9177e42a4dcdab01
prerequisite-patch-id: 33fd23112b55b5e6b1e276594826ee454afc1c7e
prerequisite-patch-id: 009a5aaff0768d90d148eb9d5d999c5098fd6d93
prerequisite-patch-id: 7e41df07cbce163a689f5dca246b98a0122d1610

Signature · 1 line
2.47.3
author
#1 lore raw

PATCH 1/7 riscv: dts: spacemit: Enable i2c8 adapter for OrangePi RV2

The adapter is used to access the SpacemiT P1 PMIC present in this board.

---
 arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts | 6 ++++++
 1 file changed, 6 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +6 -0
index 7b7331cb3c72..93880ba7bdfe 100644
@@ -87,6 +87,12 @@ &pdma {
 	status = "okay";
 };
 
+&i2c8 {
+	pinctrl-0 = <&i2c8_cfg>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_2_cfg>;
Signature · 1 line
2.47.3
author
#2 lore raw

PATCH 2/7 riscv: dts: spacemit: Define fixed regulators for OrangePi RV2

In reply to Han Gao

Define the DC power input and the 4v power as fixed regulator supplies.

---
 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +19 -0
index 93880ba7bdfe..bd1e45e95e38 100644
@@ -23,6 +23,25 @@ chosen {
 		stdout-path = "serial0";
 	};
 
+	reg_dc_in: dc-in-12v {
+		compatible = "regulator-fixed";
+		regulator-name = "dc_in_12v";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
+	reg_vcc_4v: vcc-4v {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_4v";
+		regulator-min-microvolt = <4000000>;
+		regulator-max-microvolt = <4000000>;
+		regulator-boot-on;
+		regulator-always-on;
+		vin-supply = <&reg_dc_in>;
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
Signature · 1 line
2.47.3
#3 lore raw

Re: [PATCH 2/7] riscv: dts: spacemit: Define fixed regulators for OrangePi RV2

Hi Han,

Quoted 12 lines · Han Gao

Define the DC power input and the 4v power as fixed regulator supplies.

---
 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +0 -0
index 93880ba7bdfe..bd1e45e95e38 100644

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts chosen { · line 26

Quoted 4 lines
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +1 -0
@@ -23,6 +23,25 @@ chosen {
 		stdout-path = "serial0";
 	};
 
+	reg_dc_in: dc-in-12v {

this is copy & paste.. per discussion with Chukun, I'd suggest to add 'regulator-' prefix, see

https://lore.kernel.org/r/[email protected]

Quoted 24 lines
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +18 -0
+		compatible = "regulator-fixed";
+		regulator-name = "dc_in_12v";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
+	reg_vcc_4v: vcc-4v {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_4v";
+		regulator-min-microvolt = <4000000>;
+		regulator-max-microvolt = <4000000>;
+		regulator-boot-on;
+		regulator-always-on;
+		vin-supply = <&reg_dc_in>;
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
Signature · 1 line
2.47.3
Signature · 1 line
Yixun Lan (dlan)
author
#4 lore raw

PATCH 3/7 riscv: dts: spacemit: Define the P1 PMIC regulators for OrangePi RV2

In reply to Han Gao

Define the SpacemiT P1 PMIC voltage regulators and their constraints.

The power management hardware design on the OrangePi RV2 is identical to the Banana Pi BPI-F3, so the DT Nodes were taken from k1-bananapi-f3.dts.

---
 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +110 -0
index bd1e45e95e38..f87c6ab417c4 100644
@@ -110,6 +110,116 @@ &i2c8 {
 	pinctrl-0 = <&i2c8_cfg>;
 	pinctrl-names = "default";
 	status = "okay";
+
+	pmic@41 {
+		compatible = "spacemit,p1";
+		reg = <0x41>;
+		interrupts = <64>;
+		vin-supply = <&reg_vcc_4v>;
+
+		regulators {
+			buck1 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck2 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck3_1v8: buck3 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck4 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck5 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck6 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			aldo1 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+			};
+
+			aldo2 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			aldo3 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			aldo4 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo1 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+			};
+
+			dldo2 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo3 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo4 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-always-on;
+			};
+
+			dldo5 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo6 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-always-on;
+			};
+
+			dldo7 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+		};
+	};
 };
 
 &uart0 {
Signature · 1 line
2.47.3
#5 lore raw

Re: [PATCH 3/7] riscv: dts: spacemit: Define the P1 PMIC regulators for OrangePi RV2

Quoted 6 lines · Han Gao
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +0 -0
index bd1e45e95e38..f87c6ab417c4 100644

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts &i2c8 { · line 119

Quoted 10 lines
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +7 -0
@@ -110,6 +110,116 @@ &i2c8 {
 	pinctrl-0 = <&i2c8_cfg>;
 	pinctrl-names = "default";
 	status = "okay";
+
+	pmic@41 {
+		compatible = "spacemit,p1";
+		reg = <0x41>;
+		interrupts = <64>;
+		vin-supply = <&reg_vcc_4v>;
+

This vin-supply is replaced by per-rail supplies (vin1-supply through vin6-supply, aldoin-supply, dldoin1-supply, dldoin2-supply) in patch 5/7. Consider using the final supply properties here directly to avoid an intermediate state that is immediately superseded.

Signature · 1 line
Konstantin Ryabitsev <[email protected]>
author
#6 lore raw

PATCH 4/7 riscv: dts: spacemit: Enable USB3.0 on OrangePi RV2

In reply to Han Gao

Enable the DWC3 USB 3.0 controller and its associated usbphy2 on the OrangePi RV2 board.

The board utilizes a Genesys Logic GL3523 hub, which requires two separate power supplies: one VBUS and one for hub itself. Add two GPIO-controlled fixed-regulators to manage this.

---
 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +46 -0
index f87c6ab417c4..d8d0baaec731 100644
@@ -42,6 +42,25 @@ reg_vcc_4v: vcc-4v {
 		vin-supply = <&reg_dc_in>;
 	};
 
+	usb3-vbus-5v {
+		compatible = "regulator-fixed";
+		regulator-name = "USB30_VBUS";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		gpio = <&gpio K1_GPIO(97) GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
+	usb3_hub_5v: usb3-hub-5v {
+		compatible = "regulator-fixed";
+		regulator-name = "USB30_HUB";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&gpio K1_GPIO(123) GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
@@ -227,3 +246,30 @@ &uart0 {
 	pinctrl-0 = <&uart0_2_cfg>;
 	status = "okay";
 };
+
+&usbphy2 {
+	status = "okay";
+};
+
+&usb_dwc3 {
+	dr_mode = "host";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	hub_2_0: hub@1 {
+		compatible = "usb5e3,610";
+		reg = <0x1>;
+		vdd-supply = <&usb3_hub_5v>;
+		peer-hub = <&hub_3_0>;
+		reset-gpios = <&gpio K1_GPIO(124) GPIO_ACTIVE_LOW>;
+	};
+
+	hub_3_0: hub@2 {
+		compatible = "usb5e3,620";
+		reg = <0x2>;
+		vdd-supply = <&usb3_hub_5v>;
+		peer-hub = <&hub_2_0>;
+		reset-gpios = <&gpio K1_GPIO(124) GPIO_ACTIVE_LOW>;
+	};
+};
Signature · 1 line
2.47.3
author
#7 lore raw

PATCH 5/7 riscv: dts: spacemit: Update PMIC supply properties for OrangePi RV2

In reply to Han Gao

Update individual supply properties in pmic "spacemit,p1" node to specify the board's power tree topology for OrangePi RV2.

---
 arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +10 -2
index d8d0baaec731..c0a0c96f2ac4 100644
@@ -134,7 +134,15 @@ pmic@41 {
 		compatible = "spacemit,p1";
 		reg = <0x41>;
 		interrupts = <64>;
-		vin-supply = <&reg_vcc_4v>;
+		vin1-supply = <&reg_vcc_4v>;
+		vin2-supply = <&reg_vcc_4v>;
+		vin3-supply = <&reg_vcc_4v>;
+		vin4-supply = <&reg_vcc_4v>;
+		vin5-supply = <&reg_vcc_4v>;
+		vin6-supply = <&reg_vcc_4v>;
+		aldoin-supply = <&reg_vcc_4v>;
+		dldoin1-supply = <&buck5>;
+		dldoin2-supply = <&buck5>;
 
 		regulators {
 			buck1 {
@@ -165,7 +173,7 @@ buck4 {
 				regulator-always-on;
 			};
 
-			buck5 {
+			buck5: buck5 {
 				regulator-min-microvolt = <500000>;
 				regulator-max-microvolt = <3450000>;
 				regulator-ramp-delay = <5000>;
Signature · 1 line
2.47.3
#8 lore raw

Re: [PATCH 5/7] riscv: dts: spacemit: Update PMIC supply properties for OrangePi RV2

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts pmic@41 { · line 138

Quoted 12 lines · Han Gao
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +2 -1
index d8d0baaec731..c0a0c96f2ac4 100644
@@ -134,7 +134,15 @@ pmic@41 {
 		compatible = "spacemit,p1";
 		reg = <0x41>;
 		interrupts = <64>;
-		vin-supply = <&reg_vcc_4v>;
+		vin1-supply = <&reg_vcc_4v>;
+		vin2-supply = <&reg_vcc_4v>;

These per-rail supply properties (vin1-supply through dldoin2-supply) are not declared in the spacemit,p1.yaml binding, which only has vin-supply. With unevaluatedProperties: false on the PMIC node, these will cause dtbs_check failures. The binding needs to be updated to match, or these should remain as the single vin-supply.

Signature · 1 line
Konstantin Ryabitsev <[email protected]>
author
#9 lore raw

PATCH 6/7 riscv: dts: spacemit: Add a PCIe regulator for OrangePi RV2

In reply to Han Gao

Define a 3.3v fixed voltage regulator to be used by PCIe on the OrangePi RV2. On this platform, this regulator is always on.

---
 arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +10 -0
index c0a0c96f2ac4..a370dd35e9bc 100644
@@ -23,6 +23,16 @@ chosen {
 		stdout-path = "serial0";
 	};
 
+	pcie_vcc_3v3: pcie-vcc3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "PCIE_VCC3V3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-always-on;
+		gpio = <&gpio K1_GPIO(116) GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	reg_dc_in: dc-in-12v {
 		compatible = "regulator-fixed";
 		regulator-name = "dc_in_12v";
Signature · 1 line
2.47.3
#10 lore raw

Re: [PATCH 6/7] riscv: dts: spacemit: Add a PCIe regulator for OrangePi RV2

Hi,

Quoted 2 lines

Define a 3.3v fixed voltage regulator to be used by PCIe on the OrangePi RV2. On this platform, this regulator is always on.

(...)

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts chosen { · line 32

Quoted 2 lines
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +2 -0
+		regulator-always-on;
+		gpio = <&gpio K1_GPIO(116) GPIO_ACTIVE_HIGH>;

Why is this regulator always on?
It is clearly controlled by gpio.

Thanks,
Chukun

author
#11 lore raw

PATCH 7/7 riscv: dts: spacemit: PCIe and PHY-related updates for OrangePi RV2

In reply to Han Gao

Define PCIe and PHY-related Device Tree nodes for the OrangePi RV2.

---
 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +36 -0
index a370dd35e9bc..ffcbe3e2b4e2 100644
@@ -83,6 +83,12 @@ led1 {
 	};
 };
 
+&combo_phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie0_3_cfg>;
+	status = "okay";
+};
+
 &eth0 {
 	phy-handle = <&rgmii0>;
 	phy-mode = "rgmii-id";
@@ -259,6 +265,36 @@ dldo7 {
 	};
 };
 
+&pcie1_phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie1_3_cfg>;
+	status = "okay";
+};
+
+&pcie1_port {
+	phys = <&pcie1_phy>;
+};
+
+&pcie1 {
+	vpcie3v3-supply = <&pcie_vcc_3v3>;
+	status = "okay";
+};
+
+&pcie2_phy {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie2_4_cfg>;
+	status = "okay";
+};
+
+&pcie2_port {
+	phys = <&pcie2_phy>;
+};
+
+&pcie2 {
+	vpcie3v3-supply = <&pcie_vcc_3v3>;
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_2_cfg>;
Signature · 1 line
2.47.3
#12 lore raw

Re: [PATCH 7/7] riscv: dts: spacemit: PCIe and PHY-related updates for OrangePi RV2

Hi,

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts led1 { · line 90

Quoted 4 lines

+&combo_phy {

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +3 -0
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie0_3_cfg>;
+};

This pinctrl is clearly incorrect (including BPI-F3). Checking the schematic reveals that these pins are for CAM_I2C. By the way, combo_phy connects to USB3, so it should be enabled in the USB3 patch.

Thanks,
Chukun

maintainer
#13 lore raw

Re: [PATCH 7/7] riscv: dts: spacemit: PCIe and PHY-related updates for OrangePi RV2

Hi Chukun,

Quoted 9 lines · Chukun Pan

Hi,

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts led1 { · line 90

Quoted 4 lines

+&combo_phy {

arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts +3 -0
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie0_3_cfg>;
+};

This pinctrl is clearly incorrect (including BPI-F3). Checking the schematic reveals that these pins are for CAM_I2C.

I agree, if the phy is used for USB, the pinctrl info can be dropped Do you want to submit a patch for this?

Quoted 3 lines

By the way, combo_phy connects to USB3, so it should be enabled in the USB3 patch.

I think this is already done, check usb_dwc3 node in k1.dtsi?
https://github.com/torvalds/linux/blob/v7.0-rc3/arch/riscv/boot/dts/spacemit/k1.dtsi#L1180

Signature · 1 line
Yixun Lan (dlan)
#14 lore raw

Re: [PATCH 7/7] riscv: dts: spacemit: PCIe and PHY-related updates for OrangePi RV2

Hi,

Quoted 2 lines

I agree, if the phy is used for USB, the pinctrl info can be dropped Do you want to submit a patch for this?

I will submit a patch to drop the pinctrl.

Thanks,
Chukun

#15 lore raw
In reply to Han Gao
Quoted 2 lines · Han Gao

This patch series enables the PMIC, voltage regulators, usb and pcie for the OrangePi rv2 board.

Hi:

Thank you for sending this in. I am working on "b4 review" functionality and I am using arbitrary series to perform agent-assisted reviews to find bugs in the process.

This review was done by Claude Opus 4.5. I do not know enough about the subsystem to make any judgment about the quality of these reviews, but they look valid enough.

Hopefully, these were useful. If not, I am sorry for wasting your time.

Signature · 1 line
Konstantin Ryabitsev <[email protected]>
#16 lore raw
Quoted 5 lines · Konstantin Ryabitsev
Quoted 2 lines · Han Gao

This patch series enables the PMIC, voltage regulators, usb and pcie for the OrangePi rv2 board.

Hi:

Quoted 7 lines

Thank you for sending this in. I am working on "b4 review" functionality and I am using arbitrary series to perform agent-assisted reviews to find bugs in the process.

This review was done by Claude Opus 4.5. I do not know enough about the subsystem to make any judgment about the quality of these reviews, but they look valid enough.

I'm not sure if you see this, but your bot gave two contradictory recommendations.

On patch 3 it says:

Quoted 4 lines

This vin-supply is replaced by per-rail supplies (vin1-supply through vin6-supply, aldoin-supply, dldoin1-supply, dldoin2-supply) in patch 5/7. Consider using the final supply properties here directly to avoid an intermediate state that is immediately superseded.

Whereas on patch 5 it says

Quoted 5 lines

These per-rail supply properties (vin1-supply through dldoin2-supply) are not declared in the spacemit,p1.yaml binding, which only has vin-supply. With unevaluatedProperties: false on the PMIC node, these will cause dtbs_check failures. The binding needs to be updated to match, or these should remain as the single vin-supply.

"Use vin1-supply through dldoin2-supply, not vin-supply" and "use vin-supply only, not vin1-supply etc" can't both be correct, so obviously something weird is going on.

So what on earth is going on? Granted, Han could have described it better in his cover letter, but there's a proposed update to the spacemit,p1 bindings [1] that would have deprecated vin-supply and allowed vin1-supply etc. So there's a dependency in this series of something like:

    patch 1,2 -> patch 3 -> bindings update [1] -> patch 5

If/when the bindings update is accepted, patch 5 can be accepted into the DTS tree. Until then, patch 5 should be withheld.

This isn't obvious from the patch itself, but still, the two recommendations are contradictory on their own, and make dtbs_check (mentioned in the patch 5 comment!) would have caught the problem with the patch 3 comment.

Quoted 1 line

Hopefully, these were useful. If not, I am sorry for wasting your time.

I acknowledge that this is a sample size of 1, but this is not making me feel inclined to trust the future of whatever "agent-assisted review" is going to be happening. What good is an "agent" if it knows about make dtbs_check but doesn't even run it to see what would happen to its suggested fixes?

I don't know what you have in mind for "b4 review", but I think at the very least, "b4 review" should not encourage "I do not know much about the subsystem but the LLM comments look valid" replies.

Regards,
Vivian "dramforever" Wang

[1]: https://lore.kernel.org/spacemit/[email protected]/

#17 lore raw
Quoted 6 lines · Vivian Wang
Quoted 3 lines

This review was done by Claude Opus 4.5. I do not know enough about the subsystem to make any judgment about the quality of these reviews, but they look valid enough.

I'm not sure if you see this, but your bot gave two contradictory recommendations.

Criticism accepted. For the record, it's not a bot -- I'm writing a review tool that *may*, should the reviewer wish it, incorporate an LLM agent as a manual step.

My goal was to test end-to-end review functionality, but from now on I will focus on subsystems where I can actually discern actual useful reviews from "looks reasonable" reviews.

Thanks and my apologies,
-K

#18 lore raw
In reply to Han Gao

Hi Han,

Quoted 3 lines · Han Gao

This patch series enables the PMIC, voltage regulators, usb and pcie for the OrangePi rv2 board.

It's obvious too late for v6.20, besides this series depend on P1 PMIC patch which haven't settled down.. Guodong is working on v3, see

https://lore.kernel.org/all/CAH1PCMYmK=cenJAvhGdZuh68fwNu-c_JJFpyztZUXvcOTv0YSg@mail.gmail.com/

Quoted 37 lines

Han Gao (7):
riscv: dts: spacemit: Enable i2c8 adapter for OrangePi RV2
riscv: dts: spacemit: Define fixed regulators for OrangePi RV2
riscv: dts: spacemit: Define the P1 PMIC regulators for OrangePi RV2
riscv: dts: spacemit: Enable USB3.0 on OrangePi RV2
riscv: dts: spacemit: Update PMIC supply properties for OrangePi RV2
riscv: dts: spacemit: Add a PCIe regulator for OrangePi RV2
riscv: dts: spacemit: PCIe and PHY-related updates for OrangePi RV2

 .../boot/dts/spacemit/k1-orangepi-rv2.dts     | 235 ++++++++++++++++++
 1 file changed, 235 insertions(+)

base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
prerequisite-patch-id: 32d6acfcb4801407e38510b391407943e9a8ec82
prerequisite-patch-id: ae644485e1a8ead3fa7a087c8db9062c7acc14ff
prerequisite-patch-id: 605d537aa0f8387b3523786a875c76b9edfbcbb4
prerequisite-patch-id: faab9c820ac67de42c40f2c4578bb5b8753fcb71
prerequisite-patch-id: f9c4643cc99e5b9f1acc9daf1e679b95f81cee73
prerequisite-patch-id: 50138c8918218ea3b8650999bd558ed6350ed2f2
prerequisite-patch-id: f8f91e32361cc99c531170c39ed54cd1b3c5997d
prerequisite-patch-id: 475d1968c0c41895578809410c9bd8fe24997de4
prerequisite-patch-id: c40f1562f712f04578ef58472840d257a19919fe
prerequisite-patch-id: e9ef3af6c80187e6f05444704d13212d3beaf40a
prerequisite-patch-id: 7076909bb4103588b4761d175eca40bce470230f
prerequisite-patch-id: 0c859b4d131b3360875c795c6148c6176b55fb91
prerequisite-patch-id: 2ed98dc1ab0f5ed923cc252415c345dc8caf6f17
prerequisite-patch-id: 1be1a031763fac029076a768f012af31e455be66
prerequisite-patch-id: 21bb8387c946e050910440e7a7622305d46d946d
prerequisite-patch-id: f3bdc2c74b230663710086bd770a755d56cb8b9c
prerequisite-patch-id: ba6a7df5c7a2fa9ce0b8a72a9177e42a4dcdab01
prerequisite-patch-id: 33fd23112b55b5e6b1e276594826ee454afc1c7e
prerequisite-patch-id: 009a5aaff0768d90d148eb9d5d999c5098fd6d93
prerequisite-patch-id: 7e41df07cbce163a689f5dca246b98a0122d1610

Signature · 1 line
2.47.3
Signature · 1 line
Yixun Lan (dlan)