Re: [PATCH v10 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 2 potential issue(s) to consider:
- [High] arm64: dts: qcom: imx355 probe failure due to MCLK2 pinmux tied to camss runtime PM
- [Medium] arm64: dts: qcom: cci1 bus power leak due to typo in sleep pinctrl override
--

commit 2ddcaa50bae24562b4521343bdbdd9cdac1d78ec
Author: David Heidelberg <david@ixit.cz>

arm64: dts: qcom: sdm845-google: Add dual front IMX355 cameras
    
The Pixel 3 features two front-facing Sony IMX355 sensors with
different focal lengths (standard and wide-angle).
    
Sensors are connected via CSIPHY port 1 and 2 and controlled over CCI
I2C1, using MCLK2 as the clock source.

> diff --git a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> index f1379857e2742..8b430776e8a54 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
[ ... ]
> @@ -351,6 +389,53 @@ vreg_s3c_0p6: smps3 {
>  	};
>  };
>  
> +&camss {
> +	vdda-phy-supply = <&vreg_l1a_0p875>;
> +	vdda-pll-supply = <&vreg_l26a_1p2>;
> +
> +	/*
> +	 * MCLK2 (GPIO15) is shared between both front camera sensors.
> +	 * The clock is generated by CAMCC, therefore the pin is
> +	 * configured here rather than in individual sensor nodes.
> +	 */
> +	pinctrl-0 = <&cam_mclk2_default>;
> +	pinctrl-1 = <&cam_mclk2_sleep>;
> +	pinctrl-names = "default", "sleep";

[Severity: High]
Does assigning the MCLK2 pinmux to the &camss node cause probe failures?

Because camss uses runtime PM, it remains suspended while the sensors probe
asynchronously over I2C. During their probe routine, the sensors request the
clock and attempt an I2C read to identify the module.

Since camss is still suspended, MCLK2 will be left in the sleep state. This
prevents the clock signal from routing to the sensors, causing the I2C chip ID
read to fail.

Should this pinctrl be assigned to a node whose lifecycle matches the clock
or sensor, such as the clock controller (camcc)?

> +
> +	status = "okay";
[ ... ]
> +};
> +
> +&cci0_sleep {
> +	/* bus has external pull-up */
> +	/delete-property/ bias-pull-down;
> +	bias-disable;
> +};
> +

[Severity: Medium]
Does this override configure the wrong bus?

The fix meant to address power leakage caused by an external pull-up by
removing the internal bias-pull-down and applying bias-disable during sleep. 
However, it targets &cci0_sleep instead of &cci1_sleep.

>  &cci {
>  	status = "okay";
>  };
> @@ -358,7 +443,63 @@ &cci {
>  &cci_i2c1 {
>  	/* actuator @0c */
>  
> -	/* front camera, imx355 @1a */
> +	front_cam: camera@10 {

Since the newly added front cameras are connected to cci_i2c1, its sleep state
(cci1_sleep) should be updated to avoid conflicting with the external pull-up.

Because cci0 is unused for cameras on this board, updating &cci0_sleep leaves
the cci_i2c1 bus with internal pull-downs enabled during sleep, which will fight
the external pull-up and leak power.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-pixel3-camera-v10-0-df631279b39d@ixit.cz?part=1