[PATCH v3 0/2] i.MX8MQ EVK: Enable dual OV5640 cameras

[PATCH v3 0/2] i.MX8MQ EVK: Enable dual OV5640 cameras

[Robby Cai]

This series enables dual OV5640 camera support on the i.MX8MQ EVK.

The DT describes two sensors connected to different MIPI CSI-2
interfaces, while both sensors share a common reset GPIO.

To properly handle the shared reset line, the OV5640 driver is updated
to use the reset control framework instead of directly controlling the
GPIO.

Changes in v3:
- Add OV5640 driver changes to use reset control framework for shared reset
- Drop GPIO hog for reset in DTS

Link to v2: https://lore.kernel.org/imx/20260515111143.2980956-1-robby.cai@nxp.com/

Changes in v2:
- Address comments on MIPI clock configuration (Frank, Sebastian):
  drop the first patch and consolidate the correct clock configuration
  into the second patch
- Address comments from sashiko:
  * Use MEDIA_BUS_TYPE_CSI2_DPHY instead of a literal value
  * Fix a probe-order dependency related to reset handling. Switch to
    software reset, as the shared hardware reset line prevents
    independent reset when both cameras are enabled due to a board
    design limitation
  * Fix incorrect voltage value in the reg_2v8 node

Link to v1: https://lore.kernel.org/imx/20260417110200.753678-1-robby.cai@nxp.com/


Signed-off-by: Robby Cai <robby.cai@nxp.com>

Robby Cai (2):
  arm64: dts: imx8mq-evk: Enable MIPI CSI and dual OV5640 cameras
  media: i2c: ov5640: Use reset control framework to support shared
    reset

 arch/arm64/boot/dts/freescale/imx8mq-evk.dts | 150 +++++++++++++++++++
 drivers/media/i2c/ov5640.c                   |  18 +--
 2 files changed, 159 insertions(+), 9 deletions(-)

-- 
2.50.1

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

[Robby Cai]

Enable the MIPI CSI-2 host controllers and CSI bridges, and add two
OV5640 sensors on I2C1 and I2C2, forming two media pipelines:

  - OV5640 (I2C2) -> MIPI CSI1 -> CSI1 bridge
  - OV5640 (I2C1) -> MIPI CSI2 -> CSI2 bridge

On the i.MX8MQ EVK, both sensors share a single reset GPIO line,
while each sensor has an independent powerdown (PWDN) GPIO.

Both sensors also share the same MCLK source (CLKO2), configured
identically as required by the hardware design.

Signed-off-by: Robby Cai <robby.cai@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts | 150 +++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
index d48f901487d4..7ff1a763890a 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
@@ -6,6 +6,7 @@
 
 /dts-v1/;
 
+#include <dt-bindings/media/video-interfaces.h>
 #include "imx8mq.dtsi"
 
 / {
@@ -50,6 +51,20 @@ reg_usdhc2_vmmc: regulator-vsd-3v3 {
 		enable-active-high;
 	};
 
+	reg_1v5: regulator-1v5 {
+		compatible = "regulator-fixed";
+		regulator-name = "DVDD_1V5";
+		regulator-min-microvolt = <1500000>;
+		regulator-max-microvolt = <1500000>;
+	};
+
+	reg_2v8: regulator-2v8 {
+		compatible = "regulator-fixed";
+		regulator-name = "AVDD_2V8";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+	};
+
 	buck2_reg: regulator-buck2 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&pinctrl_buck2>;
@@ -172,6 +187,14 @@ &A53_3 {
 	cpu-supply = <&buck2_reg>;
 };
 
+&csi1 {
+	status = "okay";
+};
+
+&csi2 {
+	status = "okay";
+};
+
 &ddrc {
 	operating-points-v2 = <&ddrc_opp_table>;
 	status = "okay";
@@ -330,12 +353,103 @@ 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>;
+		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+		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>;
+		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+		DOVDD-supply = <&sw4_reg>;
+		AVDD-supply = <&reg_2v8>;
+		DVDD-supply = <&reg_1v5>;
+
+		port {
+			camera1_ep: endpoint {
+				remote-endpoint = <&mipi_csi1_in_ep>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
+			};
+		};
+	};
 };
 
 &lcdif {
 	status = "okay";
 };
 
+&mipi_csi1 {
+	assigned-clock-rates = <266000000>, <200000000>, <66000000>;
+	status = "okay";
+
+	ports {
+		port@0 {
+			reg = <0>;
+
+			mipi_csi1_in_ep: endpoint {
+				remote-endpoint = <&camera1_ep>;
+				data-lanes = <1 2>;
+				bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
+			};
+		};
+	};
+};
+
+&mipi_csi2 {
+	assigned-clock-rates = <266000000>, <200000000>, <66000000>;
+	status = "okay";
+
+	ports {
+		port@0 {
+			reg = <0>;
+
+			mipi_csi2_in_ep: endpoint {
+				remote-endpoint = <&camera2_ep>;
+				data-lanes = <1 2>;
+				bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
+			};
+		};
+	};
+};
+
 &mipi_dsi {
 	#address-cells = <1>;
 	#size-cells = <0>;
@@ -532,12 +646,34 @@ &wdog1 {
 };
 
 &iomuxc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_mclk>, <&pinctrl_camera_reset>;
+
 	pinctrl_buck2: vddarmgrp {
 		fsl,pins = <
 			MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13		0x19
 		>;
 	};
 
+	pinctrl_camera1_pwdn: camera1pwdngrp {
+		fsl,pins = <
+			MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3		0x19
+		>;
+	};
+
+	pinctrl_camera2_pwdn: camera2pwdngrp {
+		fsl,pins = <
+			MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5		0x19
+		>;
+	};
+
+	/* Shared reset line for cameras on CSI1 and CSI2. */
+	pinctrl_camera_reset: cameraresetgrp {
+		fsl,pins = <
+			MX8MQ_IOMUXC_GPIO1_IO06_GPIO1_IO6		0x19
+		>;
+	};
+
 	pinctrl_fec1: fec1grp {
 		fsl,pins = <
 			MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC			0x3
@@ -565,12 +701,26 @@ MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA			0x4000007f
 		>;
 	};
 
+	pinctrl_i2c2: i2c2grp {
+		fsl,pins = <
+			MX8MQ_IOMUXC_I2C2_SCL_I2C2_SCL			0x4000007f
+			MX8MQ_IOMUXC_I2C2_SDA_I2C2_SDA			0x4000007f
+		>;
+	};
+
 	pinctrl_ir: irgrp {
 		fsl,pins = <
 			MX8MQ_IOMUXC_GPIO1_IO12_GPIO1_IO12		0x4f
 		>;
 	};
 
+	/* Shared MCLK for cameras on CSI1 and CSI2. */
+	pinctrl_mclk: mclkgrp {
+		fsl,pins = <
+			MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2	0x59
+		>;
+	};
+
 	pinctrl_mipi_dsi: mipidsigrp {
 		fsl,pins = <
 			MX8MQ_IOMUXC_ECSPI1_SCLK_GPIO5_IO6		0x16
-- 
2.50.1

[PATCH v3 2/2] media: i2c: ov5640: Use reset control framework to support shared reset

[Robby Cai]

Replace the direct GPIO-based reset handling with the reset control
framework.

Use devm_reset_control_get_optional_shared() to acquire the reset line,
allowing the driver to support both dedicated and shared reset signals
described via reset-gpios in DT.

This simplifies the driver by delegating reset handling and sharing
semantics to the reset framework instead of managing GPIOs directly.

Signed-off-by: Robby Cai <robby.cai@nxp.com>
---
 drivers/media/i2c/ov5640.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 85ecc23b3587..754be6573b8d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <media/v4l2-async.h>
@@ -442,7 +443,7 @@ struct ov5640_dev {
 	u32 xclk_freq;
 
 	struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES];
-	struct gpio_desc *reset_gpio;
+	struct reset_control *reset;
 	struct gpio_desc *pwdn_gpio;
 	bool   upside_down;
 
@@ -2448,12 +2449,12 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable)
  *
  * In such cases, this gpio should be mapped to pwdn_gpio in the driver, and we
  * should still toggle the pwdn_gpio below with the appropriate delays, while
- * the calls to reset_gpio will be ignored.
+ * the calls to reset will be ignored.
  */
 static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
 {
 	if (sensor->pwdn_gpio) {
-		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+		reset_control_assert(sensor->reset);
 
 		/* camera power cycle */
 		ov5640_power(sensor, false);
@@ -2461,7 +2462,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
 		ov5640_power(sensor, true);
 		usleep_range(1000, 2000);	/* t3 */
 
-		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
+		reset_control_deassert(sensor->reset);
 	} else {
 		/* software reset */
 		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
@@ -3914,11 +3915,10 @@ static int ov5640_probe(struct i2c_client *client)
 	if (IS_ERR(sensor->pwdn_gpio))
 		return PTR_ERR(sensor->pwdn_gpio);
 
-	/* request optional reset pin */
-	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
-						     GPIOD_OUT_HIGH);
-	if (IS_ERR(sensor->reset_gpio))
-		return PTR_ERR(sensor->reset_gpio);
+	sensor->reset = devm_reset_control_get_optional_shared(dev, NULL);
+	if (IS_ERR(sensor->reset))
+		return dev_err_probe(dev, PTR_ERR(sensor->reset),
+				     "Failed to get reset\n");
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov5640_subdev_ops);
 	sensor->sd.internal_ops = &ov5640_internal_ops;
-- 
2.50.1

Re: [PATCH v3 2/2] media: i2c: ov5640: Use reset control framework to support shared reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver misuses the shared reset control by attempting to pulse it, which triggers kernel warnings, leaks references, and disrupts other shared devices.
- [High] Switching to the reset control framework breaks backward compatibility for existing DTs using the 'reset-gpios' property unless CONFIG_RESET_GPIO is enabled.
- [Medium] The driver ignores the return values of reset control API calls, which can fail and cause silent hardware initialization errors.
--

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

media: i2c: ov5640: Use reset control framework to support shared reset

This commit replaces the direct GPIO-based reset handling in the ov5640
driver with the reset control framework. It uses
devm_reset_control_get_optional_shared() to acquire the reset line, aiming
to support both dedicated and shared reset signals described via reset-gpios
in DT.

> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 92d2d6cd4ba48..427feedede75c 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c

[ ... ]

> @@ -2448,12 +2449,12 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable)
>   *
>   * In such cases, this gpio should be mapped to pwdn_gpio in the driver, and we
>   * should still toggle the pwdn_gpio below with the appropriate delays, while
> - * the calls to reset_gpio will be ignored.
> + * the calls to reset will be ignored.
>   */
>  static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  {
>  	if (sensor->pwdn_gpio) {
> -		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +		reset_control_assert(sensor->reset);

[Severity: High]
Does this code incorrectly pulse a shared reset line?

For shared reset controls, the initial deassert_count is 0. Calling
reset_control_assert() when deassert_count == 0 triggers a WARN_ON in the
reset core and fails with -EINVAL.

Additionally, if multiple devices share the line and another device is
currently active (so deassert_count > 0), calling reset_control_assert()
here will decrement the count.

If the count reaches 0, it physically resets the line while the other device
is actively using it, which could cause that device to crash.

[Severity: Medium]
Should the return value of reset_control_assert() be checked?

Unlike simple memory-mapped GPIOs, reset controllers may reside on slow
buses (like I2C/SPI) and their operations can fail.

If this fails, the driver will silently proceed, which might result in
unexplained I2C transfer errors later.

>  
>  		/* camera power cycle */
>  		ov5640_power(sensor, false);
> @@ -2461,7 +2462,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  		ov5640_power(sensor, true);
>  		usleep_range(1000, 2000);	/* t3 */
>  
> -		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +		reset_control_deassert(sensor->reset);

[Severity: High]
Does this leak the reset control reference?

Because the reset control is never asserted during a power-down sequence,
the deassert_count incremented by this reset_control_deassert() call will
never be decremented, leaking the reference.

[Severity: Medium]
Should the return value of reset_control_deassert() be checked?

If a reset deassertion fails (e.g., due to an I2C bus timeout), the hardware
remains physically in reset. The driver will silently proceed to communicate
with the sensor, resulting in unexplained errors.

>  	} else {
>  		/* software reset */
>  		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,

[ ... ]

> @@ -3914,11 +3915,10 @@ static int ov5640_probe(struct i2c_client *client)
>  	if (IS_ERR(sensor->pwdn_gpio))
>  		return PTR_ERR(sensor->pwdn_gpio);
>  
> -	/* request optional reset pin */
> -	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> -						     GPIOD_OUT_HIGH);
> -	if (IS_ERR(sensor->reset_gpio))
> -		return PTR_ERR(sensor->reset_gpio);
> +	sensor->reset = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(sensor->reset))
> +		return dev_err_probe(dev, PTR_ERR(sensor->reset),
> +				     "Failed to get reset\n");

[Severity: High]
Does switching to the reset control framework break backward compatibility
for existing device trees using the reset-gpios property?

When the resets property is absent, the reset framework has a fallback to
parse reset-gpios. However, this fallback explicitly checks
IS_ENABLED(CONFIG_RESET_GPIO). If this config is disabled, it silently
returns NULL for optional resets.

Because VIDEO_OV5640 does not select RESET_GPIO in Kconfig, any existing
platform using reset-gpios without CONFIG_RESET_GPIO enabled will silently
skip the reset sequence entirely, leading to initialization failures.

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