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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 9/30/25 6:32 AM, Hongyang Zhao wrote:
> Add DTS for Thundercomm qcs6490-rubikpi3 board which uses
> QCS6490 SoC.
> 
> Works:
> - Wi-Fi (AP6256)
> - Bluetooth (AP6256)
> - Ethernet (AX88179B connected to UPD720201)
> - USB Type-C
> - USB Type-A 2.0 port
> - Two USB Type-A 3.0 ports (UPD720201 connected to PCIe0)
> - M.2 M-Key 2280 PCIe 3.0
> - RTC
> - 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 V1<loic.minier@oss.qualcomm.com>

Is the "V1" part of Loic's name?> ---

[...]

> +	fan0: pwm-fan {
> +		compatible = "pwm-fan";
> +		pwms = <&pm8350c_pwm 3 1000000>;
> +		pinctrl-0 = <&fan_pwm_out_default>;
> +		pinctrl-names = "default";

You most definitely want some cooling-levels in here, so that the
fan can be then referred to as a cooling device from some thermal-zone

[...]

> +	thermal-zones {
> +		sdm-skin-thermal {
> +			thermal-sensors = <&pmk8350_adc_tm 3>;
> +
> +			trips {
> +				active-config0 {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "passive";
> +				};
> +			};
> +		};
> +
> +		quiet-thermal {

'q' < 's', but perhaps sorting by the thermal provider index (i.e.
pmk8350_adc_tm 0/1/3) could make this more legible

[...]

> +	vreg_lt9611_3p3: vreg_lt9611_3p3 {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "vreg_lt9611_3p3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&tlmm 83 0>;

"0" is supposed to be "GPIO_ACTIVE_HIGH" here, similarly elsewhere

[...]

> +&pcie0_phy {
> +	status = "okay";

Please put 'status' uniformly as the last property, with a \n preceeding it

[...]

> +&pm8350c_pwm {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	status = "okay";
> +
> +	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";
> +	};
> +
> +	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";
> +	};

Are these three separate LEDs (i.e. are none of them combined)?

[...]

> +&thermal_zones {
> +	cpu0-thermal {
> +		trips {
> +			cpu_tepid: cpu-tepid {
> +				temperature = <65000>;
> +				hysteresis = <5000>;
> +				type = "active";
> +			};

these are not useful unless you wire up the fan here

> +
> +			cpu_warm: cpu-warm {
> +				temperature = <80000>;
> +				hysteresis = <5000>;
> +				type = "active";
> +			};
> +
> +			cpu_hot: cpu-hot {
> +				temperature = <90000>;
> +				hysteresis = <5000>;
> +				type = "active";
> +			};
> +		};
> +	};
> +
> +	gpuss0-thermal {
> +		trips {
> +			gpuss0_alert0: trip-point0 {
> +				temperature = <95000>;
> +				hysteresis = <5000>;
> +				type = "passive";
> +			};
> +		};
> +	};
> +
> +	gpuss1-thermal {
> +		trips {
> +			gpuss1_alert0: trip-point0 {
> +				temperature = <95000>;
> +				hysteresis = <5000>;
> +				type = "passive";
> +			};
> +		};
> +	};
> +
> +	nspss0-thermal {
> +		trips {
> +			nspss0_alert0: trip-point0 {
> +				temperature = <95000>;
> +				hysteresis = <5000>;
> +				type = "hot";
> +			};
> +		};
> +	};
> +
> +	nspss1-thermal {
> +		trips {
> +			nspss1_alert0: trip-point0 {
> +				temperature = <95000>;
> +				hysteresis = <5000>;
> +				type = "hot";
> +			};
> +		};
> +	};

neither are any of these ^

[...]

> +&tlmm {
> +	bt_device_wake: bt-device-wake-state {
> +		pins = "gpio39";

It'd be nice if entries here were sorted by the GPIO index, as per

https://docs.kernel.org/devicetree/bindings/dts-coding-style.html

[...]

> +/*
> + * Update the lpassaudio node to support the new compatible as the
> + * lpassaudio needs to support the reset functionality on the
> + * QCS6490 RUBIK Pi 3 board and the rest of the Audio functionality would be
> + * provided from the LPASS firmware.
> + */

I'm not sure this comment is useful, given this is a "common" situation,
but this override should most definitely not hang out at the tail end of
the file

Konrad

> +&lpass_audiocc {
> +	compatible = "qcom,qcm6490-lpassaudiocc";
> +	/delete-property/ power-domains;
> +};