Re: [PATCH] arm64: dts: qcom: sm8650-valve-deckard: Add initial support of camera sensors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dt: pinctrl states grouped incorrectly under pinctrl-0
- [High] dt: out-of-bounds reset-gpios index for og01a1b sensor
- [Medium] dt: invalid data-lanes enum value for og01a1b sensor
--

commit 80cdd7bb4552bb56ef59eb14b62e12c079117298
Author: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

arm64: dts: qcom: sm8650-valve-deckard: Add initial support of camera sensors

This commit adds initial device tree support for camera sensors on the Valve
Steam Frame device. It enables CAMSS and adds descriptions for pairs of
OmniVision OV6211 and OG01A1B monochrome sensors.

> diff --git a/arch/arm64/boot/dts/qcom/sm8650-valve-deckard.dts b/arch/arm64/boot/dts/qcom/sm8650-valve-deckard.dts
> index d25a09e162dd3..3b2503c3c5413 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650-valve-deckard.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8650-valve-deckard.dts

[ ... ]

> +&cci0_i2c1 {
> +	sensor@60 {
> +		compatible = "ovti,ov6211";
> +		reg = <0x60>;
> +		clocks = <&camcc CAM_CC_MCLK4_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK4_CLK>;
> +		assigned-clock-rates = <24000000>;
> +		pinctrl-0 = <&cam4_default>, <&cam4_sleep>;
> +		pinctrl-names = "default", "sleep";

[Severity: High]
Does this correctly map the sleep state for the camera pins?

Assigning both the default and sleep state phandles to pinctrl-0 while
defining pinctrl-names as "default", "sleep" breaks pin control behavior.
The pinctrl subsystem expects index 1 to map to pinctrl-1.

Due to this misconfiguration, the kernel may mistakenly apply both default and
sleep states sequentially when setting the default state, leaving the pins in
the sleep configuration. Furthermore, it will fail to find the required
pinctrl-1 property when attempting to transition to sleep later.

[ ... ]

> +&cci2_i2c0 {
> +	sensor@36 {
> +		compatible = "ovti,og01a1b";
> +		reg = <0x36>;
> +		clocks = <&camcc CAM_CC_MCLK0_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK0_CLK>;
> +		assigned-clock-rates = <19200000>;
> +		reset-gpios = <&pm8550vs_e_gpios 4 GPIO_ACTIVE_LOW>;
> +		pinctrl-0 = <&cam0_default>, <&cam0_sleep>;
> +		pinctrl-names = "default", "sleep";

[Severity: High]
Will this experience the same pinctrl mapping issue described above for
the ovti,ov6211 sensor?

> +		avdd-supply = <&vreg_l3m_2p8>;
> +		dovdd-supply = <&vreg_cam_front_right_1p8>;
> +		dvdd-supply = <&vreg_cam_front_right_1p2>;
> +
> +		port {
> +			cam_front_right: endpoint {
> +				data-lanes = <0 1>;

[Severity: Medium]
Is the value <0 1> for data-lanes intentional?

The device tree schema for the ovti,og01a1b sensor only permits the enum
values of 1 and 2. This configuration will lead to a dtbs_check validation
error, and could cause driver configuration issues if the PHY driver strictly
expects 1-based physical lane indexing.

> +				link-frequencies = /bits/ 64 <500000000>;
> +				remote-endpoint = <&csiphy1_ep>;
> +			};
> +		};
> +	};
> +
> +	sensor@60 {
> +		compatible = "ovti,og01a1b";
> +		reg = <0x60>;
> +		clocks = <&camcc CAM_CC_MCLK1_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK1_CLK>;
> +		assigned-clock-rates = <19200000>;
> +		reset-gpios = <&pm8550vs_e_gpios 6 GPIO_ACTIVE_LOW>;

[Severity: High]
Could this out-of-bounds index cause a probe failure?

The pm8550vs PMIC only supports 6 GPIOs (indices 0 through 5). Referencing
index 6 guarantees an out-of-bounds error during the GPIO lookup, which will
cause the sensor driver to fail to request its reset pin during probe.

> +		pinctrl-0 = <&cam1_default>, <&cam1_sleep>;
> +		pinctrl-names = "default", "sleep";

[Severity: High]
As noted above, shouldn't the sleep state be mapped to pinctrl-1?

> +		avdd-supply = <&vreg_l3n_2p8>;
> +		dovdd-supply = <&vreg_cam_front_left_1p8>;
> +		dvdd-supply = <&vreg_cam_front_left_1p2>;
> +
> +		port {
> +			cam_front_left: endpoint {
> +				data-lanes = <0 1>;

[Severity: Medium]
Does this data-lanes configuration also need to be updated to use 1-based
physical lane indexing to satisfy the ovti,og01a1b schema?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001145206.2442665-1-vladimir.zapolskiy@linaro.org?part=1