Re: [PATCH v4 06/11] arm64: dts: qcom: x1e80100-t14s: Add pm8010 camera PMIC with voltage levels for IR and RGB camera

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `vdd-l5-supply` property for `vreg_l5m_2p8` (`ldo5`) in the PM8010 PMIC node.
- [Medium] Undocumented addition of `vreg_l7b_2p8` (`ldo7`) to the PM8550 PMIC block.
- [Low] Regulator names in the commit message do not match the implemented names in the device tree.
--

--- Patch [6]: [PATCH v4 06/11] arm64: dts: qcom: x1e80100-t14s: Add pm8010 camera PMIC with voltage levels for IR and RGB camera ---
commit 40ce9f41fc6767cc89c087d96f0381849341a04a
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

arm64: dts: qcom: x1e80100-t14s: Add pm8010 camera PMIC with voltage levels for IR and RGB camera

This commit adds the PM8010 PMIC node and its associated regulator voltage
rails for the IR and RGB sensors on the ThinkPad T14s.

> arm64: dts: qcom: x1e80100-t14s: Add pm8010 camera PMIC with voltage levels for IR and RGB camera
> 
> Add the PM8010 PMIC providing the following voltage rails:
> 
> vreg_l1m_r @ 1v2 IR sensor
> vreg_l2m_r @ 1v2 RGB sensor
> vreg_l3m_r @ 1v8 IR sensor
> vreg_l4m_r @ 1v8 RGB sensor
> vreg_l5m_r @ 2v8 IR sensor
> vreg_l7m_r @ 2v8 RGB sensor

[Severity: Low]
This isn't a bug, but the regulator names listed in the commit message use a
_r suffix (e.g., vreg_l1m_r), whereas the actual implemented names in the
device tree use voltage-based suffixes (e.g., vreg_l1m_1p2).

Should the commit message be updated to match the device tree names?

> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> @@ -562,6 +562,13 @@ vreg_l6b_1p8: ldo6 {
>  			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>  		};
>  
> +		vreg_l7b_2p8: ldo7 {
> +			regulator-name = "vreg_l7b_2p8";
> +			regulator-min-microvolt = <2800000>;
> +			regulator-max-microvolt = <2800000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +

[Severity: Medium]
Is this addition intentional for this patch?

The commit message only mentions adding the PM8010 camera PMIC voltage rails,
but this block adds a regulator to the entirely different PM8550 PMIC block.
Could the commit message be updated to document this, or does this belong in
a separate patch?

[ ... ]

> +	regulators-8 {
> +		compatible = "qcom,pm8010-rpmh-regulators";
> +		qcom,pmic-id = "m";
> +
> +		vdd-l1-l2-supply = <&vreg_s5j_1p2>;
> +		vdd-l3-l4-supply = <&vreg_s4c_1p8>;
> +		vdd-l7-supply = <&vreg_bob1>;

[Severity: High]
Is the vdd-l5-supply property missing from this list?

Looking at vreg_l5m_2p8 (ldo5) introduced below, it lacks a parent supply
linkage. If the parent supply is turned off by other consumers, will
vreg_l5m_2p8 silently lose power and cause the connected IR sensor to fail?

> +
> +		vreg_l1m_1p2: ldo1 {
> +			regulator-name = "vreg_l1m_1p2";
> +			regulator-min-microvolt = <1200000>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-x1e-camss-csi2-phy-dtsi-v4-0-572348ad1b2a@linaro.org?part=6