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

[Robby Cai <robby.cai@nxp.com>]

On Fri, May 15, 2026 at 11:33:17AM +0000, sashiko-bot@kernel.org wrote:
> 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?
> 

Yes, that's correct.

When `powerdown-gpios` is present, the OV5640 driver follows the
PWDN-controlled power cycle path and skips the explicit software reset.

This is intentional for this board, where both sensors share a single
reset line and cannot be reset independently. To avoid interference,
the reset line is kept deasserted (via a GPIO hog), and initialization
relies on the powerdown GPIO and power cycling instead.

This matches setups described in the driver where a single control
signal effectively drives both reset and power sequencing (e.g. PWUP-
style designs), making a separate software reset unnecessary. [1]

Additionally, using the powerdown path preserves the intended hardware
integration: PWDN is the only per-device control signal, while reset
is shared. This allows independent control of each sensor and avoids
introducing artificial dependencies on the shared reset line.

For these reasons, the design intentionally relies on powerdown-based
sequencing rather than the software reset path.

I will update the commit message to make this behavior explicit.


[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov5640.c#L2449-L2464

> > 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?

Not exactly.

The software reset is skipped, but the sensor is still reset
via the power-cycle sequence using the PWDN GPIO.

The hardware reset line is intentionally not used since it is
shared between both sensors.

> 
> > +		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?
> 

No, it should not.

Each sensor has its own powerdown GPIO, so initialization is handled
independently via the PWDN-controlled power cycle.

The shared reset line is not used, so there is no cross-impact between
the two cameras during initialization.

> > +		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