Re: [PATCH v6 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] Assigning the MCLK2 `pinctrl` to the `camss` node breaks the camera sensor probe due to decoupled runtime PM lifecycles.
- [Medium] Incorrect CCI I2C sleep state modification overrides `&cci0_sleep` instead of `&cci1_sleep`.
--

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

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

This commit adds device tree nodes for the two front-facing Sony IMX355
sensors on the Pixel 3. The sensors are connected via CSIPHY ports and
controlled over CCI I2C1, sharing MCLK2.

> 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
[ ... ]
> +&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 putting the MCLK2 pinctrl states in the camss node break the sensor probe?

Because camss operates with decoupled asynchronous runtime PM, it will likely
be suspended during the imx355 sensor probe. The PM core will put the pin into
the sleep state, which configures bias-pull-down. 

When the imx355 driver calls clk_prepare_enable(mclk) during I2C probe to read
its chip ID, the clock controller generates the clock, but the sleep state on
camss will prevent the clock signal from reaching the sensors, causing the
probe to abort.

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

[Severity: Medium]
Since the front cameras are added under the cci_i2c1 bus, should this be
modifying &cci1_sleep instead of &cci0_sleep?

This leaves the cci_i2c1 sleep state with its default bias-pull-down setting,
which will fight the external pull-up and could drain the battery during
sleep. It also incorrectly modifies cci_i2c0 which might affect other devices
on that bus.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-pixel3-camera-v6-0-afe419bef476@ixit.cz?part=1