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

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

> 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 { ... }
[...]

> 
> > > > +
> > > > +	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

> 
> > 
> > > 
> > > > +	};
> > > > +
> > > > +	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