[PATCH v4 0/2] imx8mq-evk and ov5640: Add overlay-based camera support with shared reset handling

[PATCH v4 0/2] imx8mq-evk and ov5640: Add overlay-based camera support with shared reset handling

[robby.cai]

From: Robby Cai <robby.cai@nxp.com>

This series enables OV5640 camera support on the i.MX8MQ EVK using
device tree overlays, including single- and dual-camera configurations.

The DT overlays describe two OV5640 sensors connected to different
MIPI CSI-2 interfaces:

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

On this platform, both sensors share a common reset GPIO line, while
each has an independent powerdown (PWDN) GPIO. Due to this hardware
constraint, proper handling of the shared reset line is required when
both cameras are present.

To address this, the OV5640 driver is updated to use the reset controller
framework, allowing it to correctly support shared reset lines. Legacy
reset-gpios support is retained as a fallback when no reset controller
is defined, ensuring compatibility with existing device tree
descriptions without requiring changes.

Note:
1) With commit 8f040b5c5e3a ("leds: class: Use firmware nodes for device lookup"),
the OV5640 driver reports the following errors:

  [   11.373844] ov5640 0-003c: error -EINVAL: getting privacy LED
  [   11.376442] ov5640 0-003c: probe with driver ov5640 failed with error -22
  [   11.906977] ov5640 1-003c: error -EINVAL: getting privacy LED
  [   11.909793] ov5640 1-003c: probe with driver ov5640 failed with error -22

This issue has been reported and discussed in [1] and related threads.
As a temporary workaround for testing OV5640, the patch can be reverted.

Link [1]: https://lore.kernel.org/all/aignTNlK5kCLmQ2A@tom-desktop/

2) The patch at:
     https://lore.kernel.org/imx/20260619073115.3778313-1-robby.cai@oss.nxp.com/
   is also required for OV5640 to function properly on the i.MX8MQ EVK.




Changes in v4:
- Switch EVK camera support to DT overlays for CSI1/CSI2/dual configurations (Kieran Bingham)
- Convert OV5640 driver to use reset controller framework with GPIO fallback (sashiko)
- Ensure correct handling of reset line (sashiko)

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

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: Add OV5640 camera support via overlays
  media: i2c: ov5640: Add reset controller support with GPIO fallback

 arch/arm64/boot/dts/freescale/Makefile        |  7 ++
 .../dts/freescale/imx8mq-evk-ov5640-csi1.dtso | 69 ++++++++++++++++
 .../dts/freescale/imx8mq-evk-ov5640-csi2.dtso | 65 +++++++++++++++
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts  | 50 ++++++++++++
 drivers/media/i2c/ov5640.c                    | 80 ++++++++++++++++---
 5 files changed, 261 insertions(+), 10 deletions(-)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso

-- 
2.50.1

[PATCH v4 1/2] arm64: dts: imx8mq-evk: Add OV5640 camera support via overlays

[robby.cai]

From: Robby Cai <robby.cai@nxp.com>

Add overlays for single and dual camera setups on CSI1 and CSI2, enabling
the following media pipelines:

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

On the i.MX8MQ EVK, both sensors share a common reset GPIO, 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/Makefile        |  7 ++
 .../dts/freescale/imx8mq-evk-ov5640-csi1.dtso | 69 +++++++++++++++++++
 .../dts/freescale/imx8mq-evk-ov5640-csi2.dtso | 65 +++++++++++++++++
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts  | 50 ++++++++++++++
 4 files changed, 191 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 8ddaab127ab9..8507cbdb5556 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -501,6 +501,13 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
 imx8mq-evk-pcie1-ep-dtbs += imx8mq-evk.dtb imx-pcie1-ep.dtbo
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk-pcie1-ep.dtb
 
+imx8mq-evk-ov5640-csi1-dtbs := imx8mq-evk.dtb imx8mq-evk-ov5640-csi1.dtbo
+dtb-${CONFIG_ARCH_MXC} += imx8mq-evk-ov5640-csi1.dtb
+imx8mq-evk-ov5640-csi2-dtbs := imx8mq-evk.dtb imx8mq-evk-ov5640-csi2.dtbo
+dtb-${CONFIG_ARCH_MXC} += imx8mq-evk-ov5640-csi2.dtb
+imx8mq-evk-ov5640-dual-dtbs := imx8mq-evk.dtb imx8mq-evk-ov5640-csi1.dtbo imx8mq-evk-ov5640-csi2.dtbo
+dtb-${CONFIG_ARCH_MXC} += imx8mq-evk-ov5640-dual.dtb
+
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-hummingboard-pulse.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-kontron-pitx-imx8m.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso
new file mode 100644
index 000000000000..1e9931802cdc
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2026 NXP
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/clock/imx8mq-clock.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/media/video-interfaces.h>
+
+&csi1 {
+	status = "okay";
+};
+
+&i2c2 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	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>;
+			};
+		};
+	};
+};
+
+&mipi_csi1 {
+	assigned-clock-rates = <266000000>, <200000000>, <66000000>;
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			mipi_csi1_in_ep: endpoint {
+				remote-endpoint = <&camera1_ep>;
+				data-lanes = <1 2>;
+				bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso
new file mode 100644
index 000000000000..fd247b3b5982
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2026 NXP
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/clock/imx8mq-clock.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/media/video-interfaces.h>
+
+&csi2 {
+	status = "okay";
+};
+
+&i2c1 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	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>;
+			};
+		};
+	};
+};
+
+&mipi_csi2 {
+	assigned-clock-rates = <266000000>, <200000000>, <66000000>;
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			mipi_csi2_in_ep: endpoint {
+				remote-endpoint = <&camera2_ep>;
+				data-lanes = <1 2>;
+				bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
index e7d87ea81b69..d8c139c9128d 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
@@ -50,6 +50,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>;
@@ -542,12 +556,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
@@ -575,12 +611,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 v4 2/2] media: i2c: ov5640: Add reset controller support with GPIO fallback

[robby.cai]

From: Robby Cai <robby.cai@nxp.com>

Add support for the reset controller framework by acquiring the reset
line using devm_reset_control_get_optional_shared_deasserted(). This
allows the driver to handle reset lines provided by a reset controller,
including shared ones, while avoiding unbalanced deassert counts.

Retain support for legacy reset-gpios as a fallback when no reset
controller is defined. In that case, request the GPIO and keep it in the
deasserted state as the initial configuration.

This enables the driver to support both reset-controller-backed reset
lines and older GPIO-based descriptions while preserving the existing
power-up sequencing behavior.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 85ecc23b3587..5e6db8aacb11 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,6 +443,7 @@ struct ov5640_dev {
 	u32 xclk_freq;
 
 	struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES];
+	struct reset_control *reset;
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *pwdn_gpio;
 	bool   upside_down;
@@ -2431,6 +2433,48 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 	return ov5640_set_framefmt(sensor, &sensor->fmt);
 }
 
+static int ov5640_get_reset(struct device *dev, struct ov5640_dev *sensor)
+{
+	/* use deasserted version to avoid unbalanced deassert counts */
+	sensor->reset =
+	    devm_reset_control_get_optional_shared_deasserted(dev, NULL);
+	if (IS_ERR(sensor->reset))
+		return dev_err_probe(dev, PTR_ERR(sensor->reset),
+				     "Failed to get reset\n");
+	else if (sensor->reset)
+		return 0;
+
+	/*
+	 * fallback to legacy reset-gpios
+	 * GPIOD_OUT_HIGH ensures deasserted state for ACTIVE_LOW reset
+	 */
+	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(sensor->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(sensor->reset_gpio),
+				     "Failed to get reset gpio");
+
+	return 0;
+}
+
+static int ov5640_reset_assert(struct ov5640_dev *sensor)
+{
+	if (sensor->reset)
+		return reset_control_assert(sensor->reset);
+
+	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+	return 0;
+}
+
+static int ov5640_reset_deassert(struct ov5640_dev *sensor)
+{
+	if (sensor->reset)
+		return reset_control_deassert(sensor->reset);
+
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
+	return 0;
+}
+
 static void ov5640_power(struct ov5640_dev *sensor, bool enable)
 {
 	gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
@@ -2448,12 +2492,19 @@ 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.
+ * reset handling (via reset controller or GPIO) will be ignored.
  */
-static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
+static int ov5640_powerup_sequence(struct ov5640_dev *sensor)
 {
+	int ret;
+
 	if (sensor->pwdn_gpio) {
-		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+		ret = ov5640_reset_assert(sensor);
+		if (ret) {
+			dev_err(&sensor->i2c_client->dev,
+				"Failed to assert reset: %d\n", ret);
+			return ret;
+		}
 
 		/* camera power cycle */
 		ov5640_power(sensor, false);
@@ -2461,7 +2512,13 @@ 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);
+		ret = ov5640_reset_deassert(sensor);
+		if (ret) {
+			dev_err(&sensor->i2c_client->dev,
+				"Failed to deassert reset: %d\n", ret);
+			ov5640_power(sensor, false);
+			return ret;
+		}
 	} else {
 		/* software reset */
 		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
@@ -2475,6 +2532,8 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
 	 */
 	ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
 			 OV5640_REG_SYS_CTRL0_SW_PWDN);
+
+	return 0;
 }
 
 static int ov5640_set_power_on(struct ov5640_dev *sensor)
@@ -2497,7 +2556,9 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
 		goto xclk_off;
 	}
 
-	ov5640_powerup_sequence(sensor);
+	ret = ov5640_powerup_sequence(sensor);
+	if (ret)
+		goto regulator_off;
 
 	ret = ov5640_init_slave_id(sensor);
 	if (ret)
@@ -2507,6 +2568,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
 
 power_off:
 	ov5640_power(sensor, false);
+regulator_off:
 	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
 xclk_off:
 	clk_disable_unprepare(sensor->xclk);
@@ -3914,11 +3976,9 @@ 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);
+	ret = ov5640_get_reset(dev, sensor);
+	if (ret)
+		return ret;
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov5640_subdev_ops);
 	sensor->sd.internal_ops = &ov5640_internal_ops;
-- 
2.50.1

Re: [PATCH v4 1/2] arm64: dts: imx8mq-evk: Add OV5640 camera support via overlays

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dual camera setup fails due to shared reset GPIO exclusivity causing `-EBUSY` and unexpected hardware resets.
- [Medium] Overlay-specific pinmux configurations and unconditional hogs pollute the base device tree.
--

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

arm64: dts: imx8mq-evk: Add OV5640 camera support via overlays

This patch adds device tree overlays for single and dual OV5640 camera
setups on the i.MX8MQ EVK board. It configures media pipelines for CSI1
and CSI2, noting that both sensors share a common reset GPIO and MCLK
source.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso
> new file mode 100644
> index 0000000000000..1e9931802cdc4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso

[ ... ]

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

[Severity: High]
Is it safe to assign this reset GPIO (&gpio1 6) when it is also assigned
identically in the CSI2 overlay?

Since the GPIO framework enforces exclusive access, won't the second camera
to probe fail with -EBUSY?

Additionally, when ov5640_powerup_sequence() toggles this shared reset line
during power-on, couldn't this silently hard-reset the other active camera
and break its stream?

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

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso
> new file mode 100644
> index 0000000000000..fd247b3b59825
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso

[ ... ]

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

[Severity: High]
This assigns the exact same reset GPIO (&gpio1 6) as the CSI1 overlay,
which can cause probe contention and hardware state corruption as described
above.

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

[ ... ]

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

[ ... ]

> @@ -532,12 +546,34 @@ &wdog1 {
>  };
>  
>  &iomuxc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_mclk>, <&pinctrl_camera_reset>;

[Severity: Medium]
Does unconditionally hogging these camera-specific pins in the base device
tree unnecessarily consume GPIOs when the optional camera overlays are not
applied?

>  	pinctrl_buck2: vddarmgrp {
>  		fsl,pins = <
>  			MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13		0x19
>  		>;
>  	};
>  
> +	pinctrl_camera1_pwdn: camera1pwdngrp {

[Severity: Medium]
Could defining these overlay-specific pin groups in the base device tree,
rather than isolating them within their respective overlays, break the
modularity of the device tree overlays?

> +		fsl,pins = <
> +			MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3		0x19
> +		>;
> +	};
> +
> +	pinctrl_camera2_pwdn: camera2pwdngrp {
> +		fsl,pins = <
> +			MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5		0x19
> +		>;
> +	};

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

Re: [PATCH v4 2/2] media: i2c: ov5640: Add reset controller support with GPIO fallback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Misuse of the shared reset controller API violates hardware power-up sequencing, risking sensor damage via latch-up, and causes a power management resource leak.
- [Medium] Unbalanced shared reset control deassert_count triggers a kernel WARN_ON on the `ov5640_reset_deassert()` error path.
--

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

media: i2c: ov5640: Add reset controller support with GPIO fallback

Add support for the reset controller framework by acquiring the reset
line using devm_reset_control_get_optional_shared_deasserted(). This
allows the driver to handle reset lines provided by a reset controller,
including shared ones, while avoiding unbalanced deassert counts.

> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 85ecc23b35877..5e6db8aacb118 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c

[ ... ]

> @@ -2431,6 +2433,48 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
>  	return ov5640_set_framefmt(sensor, &sensor->fmt);
>  }
>  
> +static int ov5640_get_reset(struct device *dev, struct ov5640_dev *sensor)
> +{
> +	/* use deasserted version to avoid unbalanced deassert counts */
> +	sensor->reset =
> +	    devm_reset_control_get_optional_shared_deasserted(dev, NULL);

[Severity: High]
Does acquiring the reset control in the deasserted state risk hardware
damage and cause a power management leak?

Because this API call deasserts the reset line during probe, it pulls the
reset IO line HIGH before VDD regulators are enabled in
ov5640_set_power_on(). Applying voltage to the reset pin while VDD is off
can cause a destructive latch-up condition.

Additionally, because devm_reset_control_get_optional_shared_deasserted()
acquires a base deassert vote, and the driver never explicitly asserts the
reset line during ov5640_set_power_off() on runtime PM suspend, does this
permanently prevent the reset controller and parent power domains from
suspending?

Also, the inline comment below claims GPIOD_OUT_HIGH ensures a deasserted
state for an ACTIVE_LOW reset, but GPIOD_OUT_HIGH sets the logical value
to 1, which drives an ACTIVE_LOW line physically LOW (asserted).

> +	if (IS_ERR(sensor->reset))
> +		return dev_err_probe(dev, PTR_ERR(sensor->reset),
> +				     "Failed to get reset\n");
> +	else if (sensor->reset)
> +		return 0;
> +
> +	/*
> +	 * fallback to legacy reset-gpios
> +	 * GPIOD_OUT_HIGH ensures deasserted state for ACTIVE_LOW reset
> +	 */
> +	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						     GPIOD_OUT_HIGH);

[ ... ]

> -static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
> +static int ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  {
> +	int ret;
> +
>  	if (sensor->pwdn_gpio) {
> -		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +		ret = ov5640_reset_assert(sensor);

[Severity: High]
If the reset line is shared with an active device, will this actually
assert the reset line? 

For shared reset controls, reset_control_assert() merely drops the local
vote. If another device is keeping the line deasserted, the physical pin
will remain HIGH.

Since the driver proceeds to power-cycle the OV5640's VDD regulators below:

> +		if (ret) {
> +			dev_err(&sensor->i2c_client->dev,
> +				"Failed to assert reset: %d\n", ret);
> +			return ret;
> +		}
>  
>  		/* camera power cycle */
>  		ov5640_power(sensor, false);

Doesn't power-cycling the sensor while the shared reset pin is driven HIGH
violate hardware sequencing and risk latch-up?

>  		usleep_range(5000, 10000);	/* t2 */
>  		ov5640_power(sensor, true);
>  		usleep_range(1000, 2000);	/* t3 */
>  
> -		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +		ret = ov5640_reset_deassert(sensor);
> +		if (ret) {
> +			dev_err(&sensor->i2c_client->dev,
> +				"Failed to deassert reset: %d\n", ret);
> +			ov5640_power(sensor, false);
> +			return ret;
> +		}

[Severity: Medium]
If ov5640_reset_deassert() fails here, does this cause a WARN_ON in the
reset core during cleanup?

The devm_reset_control_get_optional_shared_deasserted() call during probe
initializes the deassert_count to 1. The ov5640_reset_assert() call above
drops it to 0.

If this function returns an error without reclaiming the vote, the count
remains at 0. When probe subsequently fails, the devm framework will call
reset_control_assert() to balance the initial acquisition. Since the count
is already 0, this will underflow the counter and trigger a kernel warning
in drivers/reset/core.c.

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

Re: [PATCH v4 2/2] media: i2c: ov5640: Add reset controller support with GPIO fallback

[Frank Li]

On Fri, Jun 19, 2026 at 06:05:32PM +0800, robby.cai@oss.nxp.com wrote:
> [You don't often get email from robby.cai@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> From: Robby Cai <robby.cai@nxp.com>
>
> Add support for the reset controller framework by acquiring the reset
> line using devm_reset_control_get_optional_shared_deasserted(). This
> allows the driver to handle reset lines provided by a reset controller,
> including shared ones, while avoiding unbalanced deassert counts.
>
> Retain support for legacy reset-gpios as a fallback when no reset
> controller is defined. In that case, request the GPIO and keep it in the
> deasserted state as the initial configuration.
>
> This enables the driver to support both reset-controller-backed reset
> lines and older GPIO-based descriptions while preserving the existing
> power-up sequencing behavior.
>
> Signed-off-by: Robby Cai <robby.cai@nxp.com>
> ---
>  drivers/media/i2c/ov5640.c | 80 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 85ecc23b3587..5e6db8aacb11 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,6 +443,7 @@ struct ov5640_dev {
>         u32 xclk_freq;
>
>         struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES];
> +       struct reset_control *reset;
>         struct gpio_desc *reset_gpio;
>         struct gpio_desc *pwdn_gpio;
>         bool   upside_down;
> @@ -2431,6 +2433,48 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
>         return ov5640_set_framefmt(sensor, &sensor->fmt);
>  }
>
> +static int ov5640_get_reset(struct device *dev, struct ov5640_dev *sensor)
> +{
> +       /* use deasserted version to avoid unbalanced deassert counts */
> +       sensor->reset =
> +           devm_reset_control_get_optional_shared_deasserted(dev, NULL);
> +       if (IS_ERR(sensor->reset))
> +               return dev_err_probe(dev, PTR_ERR(sensor->reset),
> +                                    "Failed to get reset\n");
> +       else if (sensor->reset)
> +               return 0;
> +
> +       /*
> +        * fallback to legacy reset-gpios
> +        * GPIOD_OUT_HIGH ensures deasserted state for ACTIVE_LOW reset
> +        */
> +       sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +                                                    GPIOD_OUT_HIGH);
> +       if (IS_ERR(sensor->reset_gpio))
> +               return dev_err_probe(dev, PTR_ERR(sensor->reset_gpio),
> +                                    "Failed to get reset gpio");

I think needn't fallback here, NO ABI change, just change to use reset-gpio
driver.

> +
> +       return 0;
> +}
> +
> +static int ov5640_reset_assert(struct ov5640_dev *sensor)
> +{
> +       if (sensor->reset)
> +               return reset_control_assert(sensor->reset);

needn't check sensor->reset, reset_control_assert() is no ops if NULL.

> +
> +       gpiod_set_value_cansleep(sensor->reset_gpio, 1);

Needn't fallback, directly replace.

Frank

> +       return 0;
> +}
> +
> +static int ov5640_reset_deassert(struct ov5640_dev *sensor)
> +{
> +       if (sensor->reset)
> +               return reset_control_deassert(sensor->reset);
> +
> +       gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +       return 0;
> +}
> +
>  static void ov5640_power(struct ov5640_dev *sensor, bool enable)
>  {
>         gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
> @@ -2448,12 +2492,19 @@ 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.
> + * reset handling (via reset controller or GPIO) will be ignored.
>   */
> -static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
> +static int ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  {
> +       int ret;
> +
>         if (sensor->pwdn_gpio) {
> -               gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +               ret = ov5640_reset_assert(sensor);
> +               if (ret) {
> +                       dev_err(&sensor->i2c_client->dev,
> +                               "Failed to assert reset: %d\n", ret);
> +                       return ret;
> +               }
>
>                 /* camera power cycle */
>                 ov5640_power(sensor, false);
> @@ -2461,7 +2512,13 @@ 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);
> +               ret = ov5640_reset_deassert(sensor);
> +               if (ret) {
> +                       dev_err(&sensor->i2c_client->dev,
> +                               "Failed to deassert reset: %d\n", ret);
> +                       ov5640_power(sensor, false);
> +                       return ret;
> +               }
>         } else {
>                 /* software reset */
>                 ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> @@ -2475,6 +2532,8 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>          */
>         ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
>                          OV5640_REG_SYS_CTRL0_SW_PWDN);
> +
> +       return 0;
>  }
>
>  static int ov5640_set_power_on(struct ov5640_dev *sensor)
> @@ -2497,7 +2556,9 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
>                 goto xclk_off;
>         }
>
> -       ov5640_powerup_sequence(sensor);
> +       ret = ov5640_powerup_sequence(sensor);
> +       if (ret)
> +               goto regulator_off;
>
>         ret = ov5640_init_slave_id(sensor);
>         if (ret)
> @@ -2507,6 +2568,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
>
>  power_off:
>         ov5640_power(sensor, false);
> +regulator_off:
>         regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
>  xclk_off:
>         clk_disable_unprepare(sensor->xclk);
> @@ -3914,11 +3976,9 @@ 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);
> +       ret = ov5640_get_reset(dev, sensor);
> +       if (ret)
> +               return ret;
>
>         v4l2_i2c_subdev_init(&sensor->sd, client, &ov5640_subdev_ops);
>         sensor->sd.internal_ops = &ov5640_internal_ops;
> --
> 2.50.1
>
>