Re: [PATCH v3 5/7] arm64: dts: qcom: Add LeEco Le2

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The TUSB320 Type-C controller's interrupt is incorrectly configured with a rising edge trigger.
- [Medium] The GPU regulator `VDD_GFX` is marked `regulator-always-on`, which defeats dynamic power scaling.
- [Medium] WLED is claimed as supported but the pmi8950_wled node is left disabled.
- [Medium] The 'vdd_l9_l10_l13_l14_l15_l18-supply' property is missing from the pm8950 rpm_requests regulator node.
--

--- Patch [5]: [PATCH v3 5/7] arm64: dts: qcom: Add LeEco Le2 ---
commit cfadc21619b39e4cbfad0d3c04b195d16b58fa22
Author: Adam Skladowski <[email protected]>

arm64: dts: qcom: Add LeEco Le2

Adds a devicetree for the LeEco Le2 smartphone based on the
MSM8976 SoC. Supported features include I2C peripherals, WLED, GPU,
and regulators.

> diff --git a/arch/arm64/boot/dts/qcom/msm8976-leeco-s2.dts b/arch/arm64/boot/dts/qcom/msm8976-leeco-s2.dts
> new file mode 100644
> index 0000000000000..086fab5ae6e7c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8976-leeco-s2.dts

[ ... ]

> +	tiusb320: typec@47 {
> +		compatible = "ti,tusb320";
> +		reg = <0x47>;
> +		interrupts-extended = <&tlmm 21 IRQ_TYPE_EDGE_RISING>;

[Severity: High]
The TUSB320 INT_N interrupt signal is typically an active-low open-drain
output.

Will configuring it to trigger on a rising edge cause the driver to miss
Type-C events, since it would only process the interrupt when the hardware
deasserts the line?

> +
> +		pinctrl-0 = <&tusb320_int_active>;
> +		pinctrl-names = "default";
> +	};

[ ... ]

> +	pm8004_s5: s5 {
> +		regulator-name = "VDD_GFX";
> +		regulator-min-microvolt = <950000>;
> +		regulator-max-microvolt = <1165000>;
> +		regulator-enable-ramp-delay = <500>;
> +		regulator-always-on;

[Severity: Medium]
Does setting regulator-always-on for the VDD_GFX regulator defeat the Adreno
GPU driver's dynamic power scaling?

This appears to keep the GPU rail powered even when idle or suspended.

> +	};
> +};

[ ... ]

> +&pm8950_resin {
> +	linux,code = <KEY_VOLUMEDOWN>;
> +
> +	status = "okay";
> +};
> +
> +&pmi8950_wled {
> +	qcom,current-limit-microamp = <20000>;
> +	qcom,enabled-strings = <0 1>;
> +	qcom,ovp-millivolt = <29500>;

[Severity: Medium]
The commit message lists WLED as supported, but this node lacks a
status = "okay" property.

Since pmi8950.dtsi defaults the WLED node to disabled, will the driver fail
to probe?

> +};
> +
> +&rpm_requests {
> +	regulators {
> +		compatible = "qcom,rpm-pm8950-regulators";
> +
> +		vdd_s1-supply = <&vph_pwr>;
> +		vdd_s2-supply = <&vph_pwr>;
> +		vdd_s3-supply = <&vph_pwr>;
> +		vdd_s4-supply = <&vph_pwr>;
> +		vdd_s6-supply = <&vph_pwr>;
> +		vdd_l1_l19-supply = <&pm8950_s3>;
> +		vdd_l2_l23-supply = <&pm8950_s3>;
> +		vdd_l3-supply = <&pm8950_s3>;
> +		vdd_l5_l6_l7_l16-supply = <&pm8950_s4>;
> +		vdd_l8_l11_l12_l17_l22-supply = <&vph_pwr>;

[Severity: Medium]
Are we missing the vdd_l9_l10_l13_l14_l15_l18-supply property here?

Without it, the regulator core might not be able to track the parent supply
for LDOs 9, 10, 13, 14, 15, and 18.

> +
> +		pm8950_s3: s3 {
> +			regulator-min-microvolt = <1325000>;
> +			regulator-max-microvolt = <1325000>;
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5