Re: [PATCH v8 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 3 potential issue(s) to consider:
- [High] Moving the shared MCLK pinctrl to the CAMSS bridge node breaks sensor probing due to a PM lifecycle mismatch.
- [Medium] Modifying cci0_sleep instead of cci1_sleep leaves the active bus with a conflicting pull-down during suspend.
- [Low] The patch includes a non-existent header file and uses an undefined macro, causing a fatal build error.
--

commit 92554691cd8ea8c05d2770873409ffec3a11dddf
Author: David Heidelberg <david@ixit.cz>

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

This commit adds support for the dual front camera configuration on the
Pixel 3. It connects two Sony IMX355 sensors via CSIPHY ports 1 and 2,
controlling them over CCI I2C1 and using MCLK2 as the clock source.

> --- a/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-google-common.dtsi
> @@ -6,6 +6,8 @@
>  #include <dt-bindings/dma/qcom-gpi.h>
>  #include <dt-bindings/input/linux-event-codes.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/media/video-interface-devices.h>

[Severity: Low]
Does this header file exist in the source tree?

It looks like it might be missing, which would cause a device tree
compilation error.

> +#include <dt-bindings/media/video-interfaces.h>
>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>  
>  #include "sdm845.dtsi"
[ ... ]
> @@ -351,6 +390,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]
Will assigning the shared MCLK pinctrl to the camss bridge node break the
sensor probing due to a PM lifecycle mismatch?

When the imx355 sensor asynchronously probes, it enables the CCF clock
and attempts to read its I2C registers. Because camss is an independent
device, it remains runtime-suspended during the sensor's probe.

Since the pinctrl subsystem keeps suspended devices in their sleep state,
the MCLK pin would remain in sleep mode, blocking the clock signal and
causing an I2C timeout when the sensor reads its chip ID.

While fw_devlink typically creates a DL_FLAG_PM_RUNTIME device link, the
cross-referencing remote-endpoint properties create a dependency cycle that
the driver core relaxes by dropping the PM runtime flag.

Can the MCLK pad state be coupled to the sensor's PM lifecycle instead to
ensure the clock is active during probe?

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

[Severity: Medium]
Since the new sensors are added to the &cci_i2c1 bus, should this
modification apply to cci1_sleep instead of cci0_sleep?

Modifying cci0_sleep disables the internal pull-down for the unused I2C0
bus, while leaving the active I2C1 bus with its default pull-down behavior.

Since I2C1 has an external pull-up, leaving the internal pull-down active
could cause an unnecessary power leak during device sleep.

> +
>  &cci {
>  	status = "okay";
>  };
> @@ -358,7 +444,63 @@ &cci {
>  &cci_i2c1 {
>  	/* actuator @0c */
>  
> -	/* front camera, imx355 @1a */
> +	front_cam: camera@10 {
> +		compatible = "sony,imx355";
> +		reg = <0x10>;
> +
> +		clocks = <&clock_camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clocks = <&clock_camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clock-rates = <24000000>;
> +
> +		reset-gpios = <&tlmm 21 GPIO_ACTIVE_LOW>;
> +
> +		avdd-supply = <&camera_front_avdd>;
> +		dvdd-supply = <&vreg_s3a_1p35>;
> +		dovdd-supply = <&vreg_lvs1_1p8>;
> +
> +		pinctrl-0 = <&cam_front_reset_default>;
> +		pinctrl-names = "default";
> +
> +		rotation = <270>;
> +		orientation = <MEDIA_ORIENTATION_FRONT>;

[Severity: Low]
Is MEDIA_ORIENTATION_FRONT defined?

Since the corresponding header is missing, this looks like it will result
in a DTC compilation error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-pixel3-camera-v8-0-99f159883348@ixit.cz?part=1