Re: [PATCH v5 1/2] arm64: dts: qcom: rubikpi3: Add qcs6490-rubikpi3 board dts

[Hongyang Zhao <hongyang.zhao@thundersoft.com>]

> On Sun, Nov 02, 2025 at 11:09:00AM +0800, Hongyang Zhao wrote:
> > > On Fri, Oct 31, 2025 at 05:27:27PM +0800, Hongyang Zhao wrote:
> > > > > On Sat, Oct 25, 2025 at 08:27:22PM +0800, Hongyang Zhao wrote:
> > > > > > Add DTS for Thundercomm qcs6490-rubikpi3 board which uses
> > > > > > QCS6490 SoC.
> > > > > > 
> > > > > > Works:
> > > > > > - Bluetooth (AP6256)
> > > > > > - Wi-Fi (AP6256)
> > > > > > - Ethernet (AX88179B connected to UPD720201)
> > > > > > - FAN
> > > > > > - Two USB Type-A 3.0 ports (UPD720201 connected to PCIe0)
> > > > > > - M.2 M-Key 2280 PCIe 3.0
> > > > > > - RTC
> > > > > > - USB Type-C
> > > > > > - USB Type-A 2.0 port
> > > > > > - 40PIN: I2C x1, UART x1
> > > > > > 
> > > > > > Signed-off-by: Hongyang Zhao <hongyang.zhao@thundersoft.com>
> > > > > > Reviewed-by: Roger Shimizu <rosh@debian.org>
> > > > > > Cc: Casey Connolly <casey.connolly@linaro.org>
> > > > > > Cc: Christopher Obbard <christopher.obbard@linaro.org>
> > > > > > Cc: Loic Minier <loic.minier@oss.qualcomm.com>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/qcom/Makefile             |    1 +
> > > > > >  .../dts/qcom/qcs6490-thundercomm-rubikpi3.dts | 1390 +++++++++++++++++
> > > > > >  2 files changed, 1391 insertions(+)
> > > > > >  create mode 100644 arch/arm64/boot/dts/qcom/qcs6490-thundercomm-rubikpi3.dts
> > > > > > 
> > > > > > +	vreg_lt9611_3p3: vreg_lt9611_3p3 {
> > > > > 
> > > > > regulator-foo-bar-baz, please. This way VPH PWR doesn't stand out.
> > > > 
> > > > Understood, I will check the entire device tree and change:
> > > > vreg_lt9611_3v3: regulator-lt9611-3v3
> > > > vreg_m2_1v8: regulator-m2-1v8
> > > > vreg_sdio_wifi_1v8: regulator-wifi-1v8
> > > > ...
> > > > 
> > > > > > +
> > > > > > +	vph_pwr: vph-pwr-regulator {
> > > > > 
> > > > > Otherwise you currently stuffed it in the middle of other regulators,
> > > > > although it doesn't belong here.
> > > > 
> > > > Understood, I will move vph-pwr-regulator after usb1-sbu-mux.
> > > 
> > > I hope, you are talking about regulator-vph-pwr now.
> > 
> > Got it,
> > I will change it to regulator-vph-pwr and put it before all regulators:
> > 
> > reserved-memory { ... }
> > vph_pwr: regulator-vph-pwr { ... }
> > vreg_lt9611_3v3: regulator-lt9611-3v3 { ... }
> 
> Why?? What is the sort order?
> 

Sorry, the sorting was incorrect.
I will correct it according to the alphabetical order of the node names:

fan0: pwm-fan { ... }
vreg_eth_1v8: regulator-eth-1v8 { ... }
vreg_lt9611_3v3: regulator-lt9611-3v3 { ... }
vreg_m2_1v8: regulator-m2-1v8 { ... }
vreg_usbhub_pwr_1v8: regulator-usbhub-pwr-1v8 { ... }
vreg_usbhub_rest_1v8: regulator-usbhub-rest-1v8 { ... }
vph_pwr: regulator-vph-pwr { ... }
vreg_wifi_1v8: regulator-wifi-1v8 { ... }
reserved-memory { ... }

> > [...]
> > 
> > > 
> > > > > > +
> > > > > > +	led@1 {
> > > > > > +		reg = <1>;
> > > > > > +		color = <LED_COLOR_ID_GREEN>;
> > > > > > +		function = LED_FUNCTION_INDICATOR;
> > > > > > +		function-enumerator = <3>;
> > > > > > +		linux,default-trigger = "none";
> > > > > > +		default-state = "off";
> > > > > > +		panic-indicator;
> > > > > > +		label = "red";
> > > > > 
> > > > > So, is it "red" or LED_COLOR_ID_GREEN?
> > > > 
> > > > This should be changed to:
> > > > color = <LED_COLOR_ID_RED>;
> > > 
> > > So, what is the actual LED colour? Also, is it a single multi-colour LED
> > > or several separate LEDs?
> > 
> > The actual color is the same as the label attribute.
> > 
> > It's actually an LED package that combines three LEDs side-by-side.
> > Our hardware colleagues thought it was three separate LEDs.
> > I've uploaded a photo of the LEDs to Github:
> > https://hongyang-rp.github.io
> 
> Then please use the the multicolour or rgb led to describe it.
> 

Understood, I will change it to:

&pm8350c_pwm {
	status = "okay";

	multi-led {
		color = <LED_COLOR_ID_RGB>;
		function = LED_FUNCTION_INDICATOR;

		#address-cells = <1>;
		#size-cells = <0>;

		led@1 {
			reg = <1>;
			color = <LED_COLOR_ID_RED>;
		};

		led@2 {
			reg = <2>;
			color = <LED_COLOR_ID_GREEN>;
		};

		led@3 {
			reg = <3>;
			color = <LED_COLOR_ID_BLUE>;
		};
	};
};

> > 
> > > 
> > > > 
> > > > > 
> > > > > > +	};
> > > > > > +
> > > > > > +	led@2 {
> > > > > > +		reg = <2>;
> > > > > > +		color = <LED_COLOR_ID_GREEN>;
> > > > > > +		function = LED_FUNCTION_INDICATOR;
> > > > > > +		function-enumerator = <2>;
> > > > > > +		linux,default-trigger = "none";
> > > > > > +		default-state = "off";
> > > > > > +		label = "green";
> > > > > > +	};
> > > > > > +
> > > > > > +	led@3 {
> > > > > > +		reg = <3>;
> > > > > > +		color = <LED_COLOR_ID_GREEN>;
> > > > > > +		function = LED_FUNCTION_INDICATOR;
> > > > > > +		function-enumerator = <1>;
> > > > > > +		linux,default-trigger = "none";
> > > > > > +		default-state = "off";
> > > > > > +		label = "blue";
> > > > > 
> > > > > Likewise, why is this blue?
> > > > 
> > > > This should be changed to:
> > > > color = <LED_COLOR_ID_BLUE>;
> > > > 
> > > > > 
> > > > > > +	};
> > > > > > +};
> > > > > > +
> > 

---
Thank you for the review!
Hongyang