Re: [PATCH v2 1/1] arm64: dts: imx8mq-evk: Enable MIPI CSI and dual OV5640 cameras

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The proposed device tree configuration intends to use software reset by omitting `reset-gpios`, but the upstream OV5640 driver skips the software reset sequence if `powerdown-gpios` is present.
--

commit d18d0c4c95f98f3312c777b828bf09c413169ce3
Author: Robby Cai <robby.cai@nxp.com>

arm64: dts: imx8mq-evk: Enable MIPI CSI and dual OV5640 cameras

Enable the MIPI CSI bridges and corresponding CSI-2 host interfaces
on the i.MX8MQ EVK, and add two OV5640 camera sensors.

> Both OV5640 sensors share a single reset GPIO on this board,
> which prevents independent hardware reset when both cameras
> are enabled. As a result, the reset line is kept deasserted
> via a GPIO hog, and sensor reset is performed via software.

Will the software reset actually occur if powerdown-gpios is provided?

Looking at the ov5640 driver's power up sequence, the software reset is
skipped if the powerdown gpio is present:

drivers/media/i2c/ov5640.c:ov5640_powerup_sequence() {
	if (sensor->pwdn_gpio) {
		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
		...
		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
	} else {
		/* software reset */
		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
				 OV5640_REG_SYS_CTRL0_SW_RST);
		...
	}
}

Since the device tree nodes define powerdown-gpios, does this mean the
driver will skip the software reset branch entirely?

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> index d48f901487d49..2311a05b88c50 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts

[ ... ]

> @@ -330,12 +367,101 @@ vgen6_reg: vgen6 {
>  			};
>  		};
>  	};
> +
> +	camera@3c {
> +		compatible = "ovti,ov5640";
> +		reg = <0x3c>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_camera2_pwdn>;
> +		clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		clock-names = "xclk";
> +		assigned-clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		assigned-clock-parents = <&clk IMX8MQ_SYS2_PLL_200M>;
> +		assigned-clock-rates = <20000000>;
> +		powerdown-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>;

Since the reset gpio is omitted here and is kept deasserted by a hog
elsewhere, does providing powerdown-gpios result in both hardware
and software resets being bypassed?

> +		DOVDD-supply = <&sw4_reg>;
> +		AVDD-supply = <&reg_2v8>;
> +		DVDD-supply = <&reg_1v5>;
> +
> +		port {
> +			camera2_ep: endpoint {
> +				remote-endpoint = <&mipi_csi2_in_ep>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2>;
> +			};
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c2>;
> +	status = "okay";
> +
> +	camera@3c {
> +		compatible = "ovti,ov5640";
> +		reg = <0x3c>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_camera1_pwdn>;
> +		clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		clock-names = "xclk";
> +		assigned-clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		assigned-clock-parents = <&clk IMX8MQ_SYS2_PLL_200M>;
> +		assigned-clock-rates = <20000000>;
> +		powerdown-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;

Could this regression also prevent initialization on the second camera
for the same reason?

> +		DOVDD-supply = <&sw4_reg>;
> +		AVDD-supply = <&reg_2v8>;
> +		DVDD-supply = <&reg_1v5>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515111143.2980956-1-robby.cai@nxp.com?part=1