Re: [PATCH v4 1/2] arm64: dts: qcom: sdm845-google: Add dual front IMX355 cameras

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware Pin Configuration Error overriding `cci0_sleep` instead of `cci1_sleep`, causing power drainage.
- [High] Placing the `MCLK2` pinctrl configuration in the `&camss` node creates a severe probe ordering race condition.
- [Medium] Mismatch between the commit message asserting both sensors share CSIPHY1 and the Device Tree mapping them to separate PHYs.
- [Medium] The `vreg_l22a_3p3` regulator is defined with a voltage of 2.864V instead of 3.3V and causes a continuous power leak.
- [Low] Addition of undocumented and unsupported `vdda-csiX-supply` properties in the CAMSS node.
--

--- Patch [1]: [PATCH v4 1/2] arm64: dts: qcom: sdm845-google: Add dual front IMX355 cameras ---
commit 24802f71e3152de09fabf33fc17c82719cd2b67b
Author: David Heidelberg <david@ixit.cz>

arm64: dts: qcom: sdm845-google: Add dual front IMX355 cameras

This commit adds device tree nodes for the Pixel 3's dual front-facing Sony
IMX355 sensors. It describes the camera nodes, MCLK2 clock source, CCI I2C1
bus connections, and associated regulators.

> The Pixel 3 features two front-facing Sony IMX355 sensors with
> different focal lengths (standard and wide-angle).
> 
> Both sensors are connected via CSIPHY1 and controlled over CCI I2C1,

[Severity: Medium]
Is this commit message description accurate?

The device tree maps the endpoints to port@1 (CSIPHY1) and port@2 (CSIPHY2).
Because both sensors request 4 data lanes (data-lanes = <1 2 3 4>), is it
possible for them to concurrently share a single 4-lane CSIPHY without a
hardware multiplexer?

Could the commit message be updated to reflect the actual hardware
connectivity shown in the device tree?

> diff --git a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> --- a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi

[ ... ]

> @@ -288,6 +324,12 @@ vreg_l21a_2p95: ldo21 {
>  			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>  		};
>  
> +		vreg_l22a_3p3: ldo22 {
> +			regulator-min-microvolt = <2864000>;
> +			regulator-max-microvolt = <2864000>;
> +			regulator-boot-on;
> +		};
> +

[Severity: Medium]
Does this regulator configuration inadvertently cause a continuous power leak?

The label indicates 3.3V, but the voltage is constrained to 2.864V. Also,
this regulator does not appear to be used by any device node in the device
tree, yet it is marked with regulator-boot-on. Should this unused, boot-on
regulator be removed to prevent battery drain?

[ ... ]

> +&camss {
> +	vdda-phy-supply = <&vreg_l1a_0p875>;
> +	vdda-pll-supply = <&vreg_l26a_1p2>;
> +
> +	vdda-csi0-supply = <&vdda_mipi_csi0_0p9>;
> +	vdda-csi1-supply = <&vdda_mipi_csi1_0p9>;
> +	vdda-csi2-supply = <&vdda_mipi_csi2_0p9>;

[Severity: Low]
Will adding these properties cause dtbs_check validation errors?

The upstream qcom,sdm845-camss.yaml devicetree binding explicitly specifies
additionalProperties: false and only supports vdda-phy-supply and
vdda-pll-supply. Are these undocumented vdda-csiX-supply properties ignored
by the Linux camss driver?

> +
> +	/*
> +	 * MCLK2 (GPIO15) is shared between both front camera sensors.
> +	 * The clock is generated by CAMSS, therefore the pin is
> +	 * configured here rather than in individual sensor nodes.
> +	 */
> +	pinctrl-0 = <&cam_mclk2_default>;
> +	pinctrl-names = "default";
> +

[Severity: High]
Does placing the MCLK2 pinctrl configuration in the &camss node create a
probe ordering race condition?

The comment states the clock is generated by CAMSS, but the clock is actually
generated by camcc. Since the sensors rely on camcc and the I2C controller,
they may probe before the camss node is initialized.

If the sensors attempt an initial I2C read before camss probes, the MCLK2
pinmux will not be applied to GPIO15, causing the clock signal to be missing
and the probe to intermittently fail. Should this pinctrl be managed
elsewhere to guarantee availability during sensor probe?

[ ... ]

> +&cci0_sleep {
> +	/* bus has external pull-up, don't pull down */
> +	bias-disable;
> +};
> +

[Severity: High]
Is this bias-disable override applied to the correct I2C bus?

The front cameras are connected to the cci_i2c1 bus. However, this override
is applied to the &cci0_sleep pinctrl node (which controls cci_i2c0 pins),
instead of the required &cci1_sleep node (which controls cci_i2c1 pins).

Will this leave the cci_i2c1 bus with its default bias-pull-down in sleep
mode, drawing current through the external pull-up and draining the battery?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719-pixel3-camera-v4-0-48cab1236852@ixit.cz?part=1