[PATCH v2 0/4] media: i2c: ov5640 feature enhancement and fixes

[PATCH v2 0/4] media: i2c: ov5640 feature enhancement and fixes

[Lad Prabhakar]

Hi All,

This patch series fixes DVP support and enables BT656 mode in
the driver.

@Jacopo Mondi - patch 1/4 will collide with your patch series [1],
feel free to merge it as part of your v2.

[1] https://www.spinics.net/lists/linux-renesas-soc/msg51236.html

Cheers,
Prabhakar

Changes for v2:
* Added support to fallback in parallel mode
* Documented bus-type property
* Added descriptive commit message for patch 2/4 as pointed
  by Sakari
* Fixed review comments pointed by Laurent to have separate functions
  for mipi and dvp setup
* Made sure the sensor is in power down mode during startup too for
  DVP mode

Lad Prabhakar (4):
  dt-bindings: media: i2c: ov5640: Document bus-type property
  media: i2c: ov5640: Enable data pins on poweron for DVP mode
  media: i2c: ov5640: Add support for BT656 mode
  media: i2c: ov5640: Fallback to parallel mode

 .../devicetree/bindings/media/i2c/ov5640.txt  |   9 +-
 drivers/media/i2c/ov5640.c                    | 333 ++++++++++--------
 2 files changed, 198 insertions(+), 144 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] dt-bindings: media: i2c: ov5640: Document bus-type property

[Lad Prabhakar]

Document the possible bus-type's supported by the OV5640 sensor driver.

Also add the bus-type in example node.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
index c97c2f2da12d..00131dbb147e 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
@@ -36,9 +36,15 @@ Endpoint node required properties for parallel connection are:
 - data-shift: shall be set to <2> for 8 bits parallel bus
 	      (lines 9:2 are used) or <0> for 10 bits parallel bus
 - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
+		(Required for bus-type 5)
 - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
+		(Required for bus-type 5)
 - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
-	       signal.
+	       signal. (Required for bus-type 5)
+- bus-type: data bus type. Possible values are:
+	    4 - MIPI CSI-2 D-PHY
+	    5 - Parallel
+	    6 - Bt.656
 
 Examples:
 
@@ -86,6 +92,7 @@ Examples:
 				hsync-active = <0>;
 				vsync-active = <0>;
 				pclk-sample = <1>;
+				bus-type = <5>;
 			};
 		};
 	};
-- 
2.17.1

[PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Lad Prabhakar]

During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
mode with rcar-vin bridge noticed the capture worked fine for the first run
(with yavta), but for subsequent runs the bridge driver waited for the
frame to be captured. Debugging further noticed the data lines were
enabled/disabled in stream on/off callback and dumping the register
contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
values, but yet frame capturing failed.

To get around this issue the following actions are performed for
parallel mode (DVP):
1: Keeps the sensor in software power down mode and is woken up only in
   ov5640_set_stream_dvp() callback.
2: Enables data lines in s_power callback
3: Configures HVP lines in s_power callback instead of configuring
   everytime in ov5640_set_stream_dvp().
4: Disables MIPI interface.

Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
 1 file changed, 172 insertions(+), 149 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2fe4a7ac0592..ec444bee2ce9 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -94,6 +94,9 @@
 #define OV5640_REG_SDE_CTRL5		0x5585
 #define OV5640_REG_AVG_READOUT		0x56a1
 
+#define OV5640_SOFTWARE_PWDN		0x42
+#define OV5640_SOFTWARE_WAKEUP		0x02
+
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
 	OV5640_MODE_QVGA_320_240,
@@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 /* YUV422 UYVY VGA@30fps */
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
-	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
+	{0x3103, 0x03, 0, 0},
 	{0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
@@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
 		val = regs->val;
 		mask = regs->mask;
 
+		/* remain in power down mode for DVP */
+		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
+		    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
+			continue;
+
 		if (mask)
 			ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
 		else
@@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
 
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
-	int ret;
-	unsigned int flags = sensor->ep.bus.parallel.flags;
-	u8 pclk_pol = 0;
-	u8 hsync_pol = 0;
-	u8 vsync_pol = 0;
-
-	/*
-	 * Note about parallel port configuration.
-	 *
-	 * When configured in parallel mode, the OV5640 will
-	 * output 10 bits data on DVP data lines [9:0].
-	 * If only 8 bits data are wanted, the 8 bits data lines
-	 * of the camera interface must be physically connected
-	 * on the DVP data lines [9:2].
-	 *
-	 * Control lines polarity can be configured through
-	 * devicetree endpoint control lines properties.
-	 * If no endpoint control lines properties are set,
-	 * polarity will be as below:
-	 * - VSYNC:	active high
-	 * - HREF:	active low
-	 * - PCLK:	active low
-	 */
-
-	if (on) {
-		/*
-		 * configure parallel port control lines polarity
-		 *
-		 * POLARITY CTRL0
-		 * - [5]:	PCLK polarity (0: active low, 1: active high)
-		 * - [1]:	HREF polarity (0: active low, 1: active high)
-		 * - [0]:	VSYNC polarity (mismatch here between
-		 *		datasheet and hardware, 0 is active high
-		 *		and 1 is active low...)
-		 */
-		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-			pclk_pol = 1;
-		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-			hsync_pol = 1;
-		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-			vsync_pol = 1;
-
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_POLARITY_CTRL00,
-				       (pclk_pol << 5) |
-				       (hsync_pol << 1) |
-				       vsync_pol);
-
-		if (ret)
-			return ret;
-	}
-
-	/*
-	 * powerdown MIPI TX/RX PHY & disable MIPI
-	 *
-	 * MIPI CONTROL 00
-	 * 4:	 PWDN PHY TX
-	 * 3:	 PWDN PHY RX
-	 * 2:	 MIPI enable
-	 */
-	ret = ov5640_write_reg(sensor,
-			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * enable VSYNC/HREF/PCLK DVP control lines
-	 * & D[9:6] DVP data lines
-	 *
-	 * PAD OUTPUT ENABLE 01
-	 * - 6:		VSYNC output enable
-	 * - 5:		HREF output enable
-	 * - 4:		PCLK output enable
-	 * - [3:0]:	D[9:6] output enable
-	 */
-	ret = ov5640_write_reg(sensor,
-			       OV5640_REG_PAD_OUTPUT_ENABLE01,
-			       on ? 0x7f : 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * enable D[5:0] DVP data lines
-	 *
-	 * PAD OUTPUT ENABLE 02
-	 * - [7:2]:	D[5:0] output enable
-	 */
-	return ov5640_write_reg(sensor,
-				OV5640_REG_PAD_OUTPUT_ENABLE02,
-				on ? 0xfc : 0);
+	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
+				OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
 }
 
 static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
@@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
 	clk_disable_unprepare(sensor->xclk);
 }
 
+static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)
+{
+	int ret = 0;
+
+	if (!on) {
+		/* Reset MIPI bus settings to their default values. */
+		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
+		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
+		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
+
+		return ret;
+	}
+
+	/*
+	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+	 *
+	 * 0x300e = 0x40
+	 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
+	 *		  "ov5640_set_stream_mipi()")
+	 * [4] = 0	: Power up MIPI HS Tx
+	 * [3] = 0	: Power up MIPI LS Rx
+	 * [2] = 0	: MIPI interface disabled
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
+	if (ret)
+		return ret;
+
+	/*
+	 * Gate clock and set LP11 in 'no packets mode' (idle)
+	 *
+	 * 0x4800 = 0x24
+	 * [5] = 1	: Gate clock when 'no packets'
+	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
+	if (ret)
+		return ret;
+
+	/*
+	 * Set data lanes and clock in LP11 when 'sleeping'
+	 *
+	 * 0x3019 = 0x70
+	 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
+	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
+	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
+	if (ret)
+		return ret;
+
+	/* Give lanes some time to coax into LP11 state. */
+	usleep_range(500, 1000);
+
+	return 0;
+}
+
+static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
+{
+	unsigned int flags = sensor->ep.bus.parallel.flags;
+	u8 pclk_pol = 0;
+	u8 hsync_pol = 0;
+	u8 vsync_pol = 0;
+	int ret = 0;
+
+	if (!on) {
+		/* Reset settings to their default values. */
+		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
+		ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
+		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
+		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
+
+		return ret;
+	}
+
+	/*
+	 * Note about parallel port configuration.
+	 *
+	 * When configured in parallel mode, the OV5640 will
+	 * output 10 bits data on DVP data lines [9:0].
+	 * If only 8 bits data are wanted, the 8 bits data lines
+	 * of the camera interface must be physically connected
+	 * on the DVP data lines [9:2].
+	 *
+	 * Control lines polarity can be configured through
+	 * devicetree endpoint control lines properties.
+	 * If no endpoint control lines properties are set,
+	 * polarity will be as below:
+	 * - VSYNC:	active high
+	 * - HREF:	active low
+	 * - PCLK:	active low
+	 */
+	/*
+	 * configure parallel port control lines polarity
+	 *
+	 * POLARITY CTRL0
+	 * - [5]:	PCLK polarity (0: active low, 1: active high)
+	 * - [1]:	HREF polarity (0: active low, 1: active high)
+	 * - [0]:	VSYNC polarity (mismatch here between
+	 *		datasheet and hardware, 0 is active high
+	 *		and 1 is active low...)
+	 */
+	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+		pclk_pol = 1;
+	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+		hsync_pol = 1;
+	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		vsync_pol = 1;
+
+	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
+			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * powerdown MIPI TX/RX PHY & disable MIPI
+	 *
+	 * MIPI CONTROL 00
+	 * 4:	 PWDN PHY TX
+	 * 3:	 PWDN PHY RX
+	 * 2:	 MIPI enable
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
+	if (ret)
+		return ret;
+
+	/*
+	 * enable VSYNC/HREF/PCLK DVP control lines
+	 * & D[9:6] DVP data lines
+	 *
+	 * PAD OUTPUT ENABLE 01
+	 * - 6:		VSYNC output enable
+	 * - 5:		HREF output enable
+	 * - 4:		PCLK output enable
+	 * - [3:0]:	D[9:6] output enable
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+	if (ret)
+		return ret;
+
+	/*
+	 * enable D[5:0] DVP data lines
+	 *
+	 * PAD OUTPUT ENABLE 02
+	 * - [7:2]:	D[5:0] output enable
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 {
 	int ret = 0;
@@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		ret = ov5640_restore_mode(sensor);
 		if (ret)
 			goto power_off;
+	}
 
-		/* We're done here for DVP bus, while CSI-2 needs setup. */
-		if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
-			return 0;
-
-		/*
-		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
-		 *
-		 * 0x300e = 0x40
-		 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
-		 *		  "ov5640_set_stream_mipi()")
-		 * [4] = 0	: Power up MIPI HS Tx
-		 * [3] = 0	: Power up MIPI LS Rx
-		 * [2] = 0	: MIPI interface disabled
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
-		if (ret)
-			goto power_off;
-
-		/*
-		 * Gate clock and set LP11 in 'no packets mode' (idle)
-		 *
-		 * 0x4800 = 0x24
-		 * [5] = 1	: Gate clock when 'no packets'
-		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_MIPI_CTRL00, 0x24);
-		if (ret)
-			goto power_off;
-
-		/*
-		 * Set data lanes and clock in LP11 when 'sleeping'
-		 *
-		 * 0x3019 = 0x70
-		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
-		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
-		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_PAD_OUTPUT00, 0x70);
-		if (ret)
-			goto power_off;
-
-		/* Give lanes some time to coax into LP11 state. */
-		usleep_range(500, 1000);
-
-	} else {
-		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
-			/* Reset MIPI bus settings to their default values. */
-			ov5640_write_reg(sensor,
-					 OV5640_REG_IO_MIPI_CTRL00, 0x58);
-			ov5640_write_reg(sensor,
-					 OV5640_REG_MIPI_CTRL00, 0x04);
-			ov5640_write_reg(sensor,
-					 OV5640_REG_PAD_OUTPUT00, 0x00);
-		}
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
+		ret = ov5640_set_mipi(sensor, on);
+	else
+		ret = ov5640_set_dvp(sensor, on);
+	if (ret)
+		goto power_off;
 
+	if (!on)
 		ov5640_set_power_off(sensor);
-	}
 
 	return 0;
 
-- 
2.17.1

[PATCH v2 3/4] media: i2c: ov5640: Add support for BT656 mode

[Lad Prabhakar]

Enable support for BT656 mode.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/i2c/ov5640.c | 40 +++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index ec444bee2ce9..08c67250042f 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -82,6 +82,7 @@
 #define OV5640_REG_VFIFO_HSIZE		0x4602
 #define OV5640_REG_VFIFO_VSIZE		0x4604
 #define OV5640_REG_JPG_MODE_SELECT	0x4713
+#define OV5640_REG_CCIR656_CTRL00	0x4730
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
@@ -1216,6 +1217,18 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
 			      BIT(1), on ? 0 : BIT(1));
 }
 
+static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
+{
+	int ret;
+
+	ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00, on ? 0x1 : 0x00);
+	if (ret)
+		return ret;
+
+	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
+				OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
+}
+
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
 	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
@@ -2022,18 +2035,20 @@ static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
 	 *		datasheet and hardware, 0 is active high
 	 *		and 1 is active low...)
 	 */
-	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-		pclk_pol = 1;
-	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-		hsync_pol = 1;
-	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-		vsync_pol = 1;
+	if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
+		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+			pclk_pol = 1;
+		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+			hsync_pol = 1;
+		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+			vsync_pol = 1;
 
-	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
-			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
+		ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
+				       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
 
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * powerdown MIPI TX/RX PHY & disable MIPI
@@ -2057,7 +2072,8 @@ static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
 	 * - 4:		PCLK output enable
 	 * - [3:0]:	D[9:6] output enable
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
+			       sensor->ep.bus_type == V4L2_MBUS_PARALLEL ? 0x7f : 0x1f);
 	if (ret)
 		return ret;
 
@@ -2911,6 +2927,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
 			ret = ov5640_set_stream_mipi(sensor, enable);
+		else if (sensor->ep.bus_type == V4L2_MBUS_BT656)
+			ret = ov5640_set_stream_bt656(sensor, enable);
 		else
 			ret = ov5640_set_stream_dvp(sensor, enable);
 
-- 
2.17.1

[PATCH v2 4/4] media: i2c: ov5640: Fallback to parallel mode

[Lad Prabhakar]

Fallback to parallel mode if bus_type doesn't match the supported
interfaces by the driver.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5640.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 08c67250042f..4e88b0540740 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -3074,6 +3074,12 @@ static int ov5640_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	/* fallback to parallel mode */
+	if (sensor->ep.bus_type != V4L2_MBUS_PARALLEL &&
+	    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY &&
+	    sensor->ep.bus_type != V4L2_MBUS_BT656)
+		sensor->ep.bus_type = V4L2_MBUS_PARALLEL;
+
 	/* get system clock (xclk) */
 	sensor->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(sensor->xclk)) {
-- 
2.17.1

Re: [PATCH v2 1/4] dt-bindings: media: i2c: ov5640: Document bus-type property

[Sakari Ailus]

Hi Prabhakar,

Thanks for the update.

On Mon, Aug 03, 2020 at 03:31:44PM +0100, Lad Prabhakar wrote:
> Document the possible bus-type's supported by the OV5640 sensor driver.
> 
> Also add the bus-type in example node.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.txt | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> index c97c2f2da12d..00131dbb147e 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> @@ -36,9 +36,15 @@ Endpoint node required properties for parallel connection are:
>  - data-shift: shall be set to <2> for 8 bits parallel bus
>  	      (lines 9:2 are used) or <0> for 10 bits parallel bus
>  - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> +		(Required for bus-type 5)
>  - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> +		(Required for bus-type 5)
>  - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> -	       signal.
> +	       signal. (Required for bus-type 5)
> +- bus-type: data bus type. Possible values are:
> +	    4 - MIPI CSI-2 D-PHY
> +	    5 - Parallel
> +	    6 - Bt.656

This is under required parallel properties. You could document value 4
under CSI-2 related properties.

>  
>  Examples:
>  
> @@ -86,6 +92,7 @@ Examples:
>  				hsync-active = <0>;
>  				vsync-active = <0>;
>  				pclk-sample = <1>;
> +				bus-type = <5>;
>  			};
>  		};
>  	};

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/4] media: i2c: ov5640: Fallback to parallel mode

[Sakari Ailus]

Hi Prabhakar,

On Mon, Aug 03, 2020 at 03:31:47PM +0100, Lad Prabhakar wrote:
> Fallback to parallel mode if bus_type doesn't match the supported
> interfaces by the driver.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5640.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 08c67250042f..4e88b0540740 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3074,6 +3074,12 @@ static int ov5640_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	/* fallback to parallel mode */
> +	if (sensor->ep.bus_type != V4L2_MBUS_PARALLEL &&
> +	    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY &&
> +	    sensor->ep.bus_type != V4L2_MBUS_BT656)
> +		sensor->ep.bus_type = V4L2_MBUS_PARALLEL;

You basically need the type from the v4l2_fwnode_endpoint_parse(), and if
you don't have any of the above bus types, probe should fail. The old
bindings were documented in a way that either parallel or CSI-2 bus will be
used, and there were no defaults. So all should be well.

-- 
Sakari Ailus

Re: [PATCH v2 1/4] dt-bindings: media: i2c: ov5640: Document bus-type property

[Lad, Prabhakar]

Hi Sakari,

Thank you for the review.

On Tue, Aug 4, 2020 at 9:15 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the update.
>
> On Mon, Aug 03, 2020 at 03:31:44PM +0100, Lad Prabhakar wrote:
> > Document the possible bus-type's supported by the OV5640 sensor driver.
> >
> > Also add the bus-type in example node.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ov5640.txt | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> > index c97c2f2da12d..00131dbb147e 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> > @@ -36,9 +36,15 @@ Endpoint node required properties for parallel connection are:
> >  - data-shift: shall be set to <2> for 8 bits parallel bus
> >             (lines 9:2 are used) or <0> for 10 bits parallel bus
> >  - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> > +             (Required for bus-type 5)
> >  - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> > +             (Required for bus-type 5)
> >  - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> > -            signal.
> > +            signal. (Required for bus-type 5)
> > +- bus-type: data bus type. Possible values are:
> > +         4 - MIPI CSI-2 D-PHY
> > +         5 - Parallel
> > +         6 - Bt.656
>
> This is under required parallel properties. You could document value 4
> under CSI-2 related properties.
>
Agreed will do that.

Cheers,
Prabhakar

> >
> >  Examples:
> >
> > @@ -86,6 +92,7 @@ Examples:
> >                               hsync-active = <0>;
> >                               vsync-active = <0>;
> >                               pclk-sample = <1>;
> > +                             bus-type = <5>;
> >                       };
> >               };
> >       };
>
> --
> Regards,
>
> Sakari Ailus

Re: [PATCH v2 4/4] media: i2c: ov5640: Fallback to parallel mode

[Lad, Prabhakar]

Hi Sakari,

Thank you for the review.

On Tue, Aug 4, 2020 at 9:18 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Mon, Aug 03, 2020 at 03:31:47PM +0100, Lad Prabhakar wrote:
> > Fallback to parallel mode if bus_type doesn't match the supported
> > interfaces by the driver.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 08c67250042f..4e88b0540740 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -3074,6 +3074,12 @@ static int ov5640_probe(struct i2c_client *client)
> >               return ret;
> >       }
> >
> > +     /* fallback to parallel mode */
> > +     if (sensor->ep.bus_type != V4L2_MBUS_PARALLEL &&
> > +         sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY &&
> > +         sensor->ep.bus_type != V4L2_MBUS_BT656)
> > +             sensor->ep.bus_type = V4L2_MBUS_PARALLEL;
>
> You basically need the type from the v4l2_fwnode_endpoint_parse(), and if
> you don't have any of the above bus types, probe should fail. The old
> bindings were documented in a way that either parallel or CSI-2 bus will be
> used, and there were no defaults. So all should be well.
>
The bus_type is coming from v4l2_fwnode_endpoint_parse(), I'll add the
check to fail if type doesn't match the supported interfaces and drop
the above check.

Cheers,
Prabhakar

> --
> Sakari Ailus

Re: [PATCH v2 0/4] media: i2c: ov5640 feature enhancement and fixes

[Jacopo Mondi]

Hello,

On Mon, Aug 03, 2020 at 03:31:43PM +0100, Lad Prabhakar wrote:
> Hi All,
>
> This patch series fixes DVP support and enables BT656 mode in
> the driver.
>
> @Jacopo Mondi - patch 1/4 will collide with your patch series [1],
> feel free to merge it as part of your v2.

This would actually make my life simpler, as one of the issues I had
was trying to make bus-type required to be able to differentiate
between different properties.

>
> [1] https://www.spinics.net/lists/linux-renesas-soc/msg51236.html
>
> Cheers,
> Prabhakar
>
> Changes for v2:
> * Added support to fallback in parallel mode
> * Documented bus-type property
> * Added descriptive commit message for patch 2/4 as pointed
>   by Sakari
> * Fixed review comments pointed by Laurent to have separate functions
>   for mipi and dvp setup
> * Made sure the sensor is in power down mode during startup too for
>   DVP mode
>
> Lad Prabhakar (4):
>   dt-bindings: media: i2c: ov5640: Document bus-type property
>   media: i2c: ov5640: Enable data pins on poweron for DVP mode
>   media: i2c: ov5640: Add support for BT656 mode
>   media: i2c: ov5640: Fallback to parallel mode
>
>  .../devicetree/bindings/media/i2c/ov5640.txt  |   9 +-
>  drivers/media/i2c/ov5640.c                    | 333 ++++++++++--------
>  2 files changed, 198 insertions(+), 144 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Jacopo Mondi]

Hi Prabhakar,

On Mon, Aug 03, 2020 at 03:31:45PM +0100, Lad Prabhakar wrote:
> During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> mode with rcar-vin bridge noticed the capture worked fine for the first run
> (with yavta), but for subsequent runs the bridge driver waited for the
> frame to be captured. Debugging further noticed the data lines were
> enabled/disabled in stream on/off callback and dumping the register
> contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> values, but yet frame capturing failed.

That's pretty weird, I wonder if that's not an issue in the bridge, as
I expect someone tryed to capture more than 1 image in DVP mode with
this driver already.

I didn't get from your commit message if you have been able to
identify where the issue is. You said register values are correct, but
did you try to plug a scope and see if data are actually put on the
bus ? Does this happen with full parallel too or BT.656 only ?

>
> To get around this issue the following actions are performed for
> parallel mode (DVP):
> 1: Keeps the sensor in software power down mode and is woken up only in
>    ov5640_set_stream_dvp() callback.
> 2: Enables data lines in s_power callback
> 3: Configures HVP lines in s_power callback instead of configuring
>    everytime in ov5640_set_stream_dvp().
> 4: Disables MIPI interface.
>
> Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
>  1 file changed, 172 insertions(+), 149 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2fe4a7ac0592..ec444bee2ce9 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -94,6 +94,9 @@
>  #define OV5640_REG_SDE_CTRL5		0x5585
>  #define OV5640_REG_AVG_READOUT		0x56a1
>
> +#define OV5640_SOFTWARE_PWDN		0x42
> +#define OV5640_SOFTWARE_WAKEUP		0x02
> +
>  enum ov5640_mode_id {
>  	OV5640_MODE_QCIF_176_144 = 0,
>  	OV5640_MODE_QVGA_320_240,
> @@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>  /* YUV422 UYVY VGA@30fps */
>  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
>  	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> -	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> +	{0x3103, 0x03, 0, 0},
>  	{0x3630, 0x36, 0, 0},
>  	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
>  	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> @@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>  		val = regs->val;
>  		mask = regs->mask;
>
> +		/* remain in power down mode for DVP */
> +		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
> +		    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> +			continue;
> +
>  		if (mask)
>  			ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>  		else
> @@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>
>  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
> -	int ret;
> -	unsigned int flags = sensor->ep.bus.parallel.flags;
> -	u8 pclk_pol = 0;
> -	u8 hsync_pol = 0;
> -	u8 vsync_pol = 0;
> -
> -	/*
> -	 * Note about parallel port configuration.
> -	 *
> -	 * When configured in parallel mode, the OV5640 will
> -	 * output 10 bits data on DVP data lines [9:0].
> -	 * If only 8 bits data are wanted, the 8 bits data lines
> -	 * of the camera interface must be physically connected
> -	 * on the DVP data lines [9:2].
> -	 *
> -	 * Control lines polarity can be configured through
> -	 * devicetree endpoint control lines properties.
> -	 * If no endpoint control lines properties are set,
> -	 * polarity will be as below:
> -	 * - VSYNC:	active high
> -	 * - HREF:	active low
> -	 * - PCLK:	active low
> -	 */
> -
> -	if (on) {
> -		/*
> -		 * configure parallel port control lines polarity
> -		 *
> -		 * POLARITY CTRL0
> -		 * - [5]:	PCLK polarity (0: active low, 1: active high)
> -		 * - [1]:	HREF polarity (0: active low, 1: active high)
> -		 * - [0]:	VSYNC polarity (mismatch here between
> -		 *		datasheet and hardware, 0 is active high
> -		 *		and 1 is active low...)
> -		 */
> -		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -			pclk_pol = 1;
> -		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> -			hsync_pol = 1;
> -		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -			vsync_pol = 1;
> -
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_POLARITY_CTRL00,
> -				       (pclk_pol << 5) |
> -				       (hsync_pol << 1) |
> -				       vsync_pol);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
> -	/*
> -	 * powerdown MIPI TX/RX PHY & disable MIPI
> -	 *
> -	 * MIPI CONTROL 00
> -	 * 4:	 PWDN PHY TX
> -	 * 3:	 PWDN PHY RX
> -	 * 2:	 MIPI enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable VSYNC/HREF/PCLK DVP control lines
> -	 * & D[9:6] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 01
> -	 * - 6:		VSYNC output enable
> -	 * - 5:		HREF output enable
> -	 * - 4:		PCLK output enable
> -	 * - [3:0]:	D[9:6] output enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_PAD_OUTPUT_ENABLE01,
> -			       on ? 0x7f : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable D[5:0] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 02
> -	 * - [7:2]:	D[5:0] output enable
> -	 */
> -	return ov5640_write_reg(sensor,
> -				OV5640_REG_PAD_OUTPUT_ENABLE02,
> -				on ? 0xfc : 0);
> +	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> +				OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
>  }
>
>  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> @@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
>  	clk_disable_unprepare(sensor->xclk);
>  }
>
> +static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)
> +{
> +	int ret = 0;
> +
> +	if (!on) {
> +		/* Reset MIPI bus settings to their default values. */
> +		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> +		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> +		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> +
> +		return ret;
> +	}
> +
> +	/*
> +	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> +	 *
> +	 * 0x300e = 0x40
> +	 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
> +	 *		  "ov5640_set_stream_mipi()")
> +	 * [4] = 0	: Power up MIPI HS Tx
> +	 * [3] = 0	: Power up MIPI LS Rx
> +	 * [2] = 0	: MIPI interface disabled
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Gate clock and set LP11 in 'no packets mode' (idle)
> +	 *
> +	 * 0x4800 = 0x24
> +	 * [5] = 1	: Gate clock when 'no packets'
> +	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Set data lanes and clock in LP11 when 'sleeping'
> +	 *
> +	 * 0x3019 = 0x70
> +	 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
> +	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> +	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> +	if (ret)
> +		return ret;
> +
> +	/* Give lanes some time to coax into LP11 state. */
> +	usleep_range(500, 1000);
> +
> +	return 0;
> +}
> +
> +static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
> +{
> +	unsigned int flags = sensor->ep.bus.parallel.flags;
> +	u8 pclk_pol = 0;
> +	u8 hsync_pol = 0;
> +	u8 vsync_pol = 0;
> +	int ret = 0;
> +
> +	if (!on) {
> +		/* Reset settings to their default values. */
> +		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> +		ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> +		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> +		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> +
> +		return ret;
> +	}
> +
> +	/*
> +	 * Note about parallel port configuration.
> +	 *
> +	 * When configured in parallel mode, the OV5640 will
> +	 * output 10 bits data on DVP data lines [9:0].
> +	 * If only 8 bits data are wanted, the 8 bits data lines
> +	 * of the camera interface must be physically connected
> +	 * on the DVP data lines [9:2].
> +	 *
> +	 * Control lines polarity can be configured through
> +	 * devicetree endpoint control lines properties.
> +	 * If no endpoint control lines properties are set,
> +	 * polarity will be as below:
> +	 * - VSYNC:	active high
> +	 * - HREF:	active low
> +	 * - PCLK:	active low
> +	 */
> +	/*
> +	 * configure parallel port control lines polarity
> +	 *
> +	 * POLARITY CTRL0
> +	 * - [5]:	PCLK polarity (0: active low, 1: active high)
> +	 * - [1]:	HREF polarity (0: active low, 1: active high)
> +	 * - [0]:	VSYNC polarity (mismatch here between
> +	 *		datasheet and hardware, 0 is active high
> +	 *		and 1 is active low...)
> +	 */
> +	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +		pclk_pol = 1;
> +	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +		hsync_pol = 1;
> +	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +		vsync_pol = 1;
> +
> +	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> +			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> +
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * powerdown MIPI TX/RX PHY & disable MIPI
> +	 *
> +	 * MIPI CONTROL 00
> +	 * 4:	 PWDN PHY TX
> +	 * 3:	 PWDN PHY RX
> +	 * 2:	 MIPI enable
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * enable VSYNC/HREF/PCLK DVP control lines
> +	 * & D[9:6] DVP data lines
> +	 *
> +	 * PAD OUTPUT ENABLE 01
> +	 * - 6:		VSYNC output enable
> +	 * - 5:		HREF output enable
> +	 * - 4:		PCLK output enable
> +	 * - [3:0]:	D[9:6] output enable
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * enable D[5:0] DVP data lines
> +	 *
> +	 * PAD OUTPUT ENABLE 02
> +	 * - [7:2]:	D[5:0] output enable
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  {
>  	int ret = 0;
> @@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  		ret = ov5640_restore_mode(sensor);
>  		if (ret)
>  			goto power_off;
> +	}
>
> -		/* We're done here for DVP bus, while CSI-2 needs setup. */
> -		if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> -			return 0;
> -
> -		/*
> -		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> -		 *
> -		 * 0x300e = 0x40
> -		 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
> -		 *		  "ov5640_set_stream_mipi()")
> -		 * [4] = 0	: Power up MIPI HS Tx
> -		 * [3] = 0	: Power up MIPI LS Rx
> -		 * [2] = 0	: MIPI interface disabled
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
> -		if (ret)
> -			goto power_off;
> -
> -		/*
> -		 * Gate clock and set LP11 in 'no packets mode' (idle)
> -		 *
> -		 * 0x4800 = 0x24
> -		 * [5] = 1	: Gate clock when 'no packets'
> -		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_MIPI_CTRL00, 0x24);
> -		if (ret)
> -			goto power_off;
> -
> -		/*
> -		 * Set data lanes and clock in LP11 when 'sleeping'
> -		 *
> -		 * 0x3019 = 0x70
> -		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
> -		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> -		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_PAD_OUTPUT00, 0x70);
> -		if (ret)
> -			goto power_off;
> -
> -		/* Give lanes some time to coax into LP11 state. */
> -		usleep_range(500, 1000);
> -
> -	} else {
> -		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> -			/* Reset MIPI bus settings to their default values. */
> -			ov5640_write_reg(sensor,
> -					 OV5640_REG_IO_MIPI_CTRL00, 0x58);
> -			ov5640_write_reg(sensor,
> -					 OV5640_REG_MIPI_CTRL00, 0x04);
> -			ov5640_write_reg(sensor,
> -					 OV5640_REG_PAD_OUTPUT00, 0x00);
> -		}
> +	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> +		ret = ov5640_set_mipi(sensor, on);
> +	else
> +		ret = ov5640_set_dvp(sensor, on);
> +	if (ret)
> +		goto power_off;
>
> +	if (!on)
>  		ov5640_set_power_off(sensor);
> -	}
>
>  	return 0;
>
> --
> 2.17.1
>

Re: [PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Lad, Prabhakar]

Hi Jacopo,

Thank you for the review.

On Thu, Aug 6, 2020 at 5:27 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Aug 03, 2020 at 03:31:45PM +0100, Lad Prabhakar wrote:
> > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > (with yavta), but for subsequent runs the bridge driver waited for the
> > frame to be captured. Debugging further noticed the data lines were
> > enabled/disabled in stream on/off callback and dumping the register
> > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > values, but yet frame capturing failed.
>
> That's pretty weird, I wonder if that's not an issue in the bridge, as
> I expect someone tryed to capture more than 1 image in DVP mode with
> this driver already.
>
I did try the bridge driver with an ov7725 sensor and it works fine in
both the modes (DVP and BT656).

> I didn't get from your commit message if you have been able to
> identify where the issue is. You said register values are correct, but
> did you try to plug a scope and see if data are actually put on the
> bus ? Does this happen with full parallel too or BT.656 only ?
>
unfortunately I didn't scope the pins, but this issue happened in both
the modes. And with this patch it improves handling the sensor in
s_stream call.

Cheers,
Prabhakar

> >
> > To get around this issue the following actions are performed for
> > parallel mode (DVP):
> > 1: Keeps the sensor in software power down mode and is woken up only in
> >    ov5640_set_stream_dvp() callback.
> > 2: Enables data lines in s_power callback
> > 3: Configures HVP lines in s_power callback instead of configuring
> >    everytime in ov5640_set_stream_dvp().
> > 4: Disables MIPI interface.
> >
> > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
> >  1 file changed, 172 insertions(+), 149 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 2fe4a7ac0592..ec444bee2ce9 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -94,6 +94,9 @@
> >  #define OV5640_REG_SDE_CTRL5         0x5585
> >  #define OV5640_REG_AVG_READOUT               0x56a1
> >
> > +#define OV5640_SOFTWARE_PWDN         0x42
> > +#define OV5640_SOFTWARE_WAKEUP               0x02
> > +
> >  enum ov5640_mode_id {
> >       OV5640_MODE_QCIF_176_144 = 0,
> >       OV5640_MODE_QVGA_320_240,
> > @@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> >  /* YUV422 UYVY VGA@30fps */
> >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > +     {0x3103, 0x03, 0, 0},
> >       {0x3630, 0x36, 0, 0},
> >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > @@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >               val = regs->val;
> >               mask = regs->mask;
> >
> > +             /* remain in power down mode for DVP */
> > +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
> > +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > +                     continue;
> > +
> >               if (mask)
> >                       ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >               else
> > @@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> >
> >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >  {
> > -     int ret;
> > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > -     u8 pclk_pol = 0;
> > -     u8 hsync_pol = 0;
> > -     u8 vsync_pol = 0;
> > -
> > -     /*
> > -      * Note about parallel port configuration.
> > -      *
> > -      * When configured in parallel mode, the OV5640 will
> > -      * output 10 bits data on DVP data lines [9:0].
> > -      * If only 8 bits data are wanted, the 8 bits data lines
> > -      * of the camera interface must be physically connected
> > -      * on the DVP data lines [9:2].
> > -      *
> > -      * Control lines polarity can be configured through
> > -      * devicetree endpoint control lines properties.
> > -      * If no endpoint control lines properties are set,
> > -      * polarity will be as below:
> > -      * - VSYNC:     active high
> > -      * - HREF:      active low
> > -      * - PCLK:      active low
> > -      */
> > -
> > -     if (on) {
> > -             /*
> > -              * configure parallel port control lines polarity
> > -              *
> > -              * POLARITY CTRL0
> > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > -              * - [0]:       VSYNC polarity (mismatch here between
> > -              *              datasheet and hardware, 0 is active high
> > -              *              and 1 is active low...)
> > -              */
> > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > -                     pclk_pol = 1;
> > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > -                     hsync_pol = 1;
> > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > -                     vsync_pol = 1;
> > -
> > -             ret = ov5640_write_reg(sensor,
> > -                                    OV5640_REG_POLARITY_CTRL00,
> > -                                    (pclk_pol << 5) |
> > -                                    (hsync_pol << 1) |
> > -                                    vsync_pol);
> > -
> > -             if (ret)
> > -                     return ret;
> > -     }
> > -
> > -     /*
> > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > -      *
> > -      * MIPI CONTROL 00
> > -      * 4:    PWDN PHY TX
> > -      * 3:    PWDN PHY RX
> > -      * 2:    MIPI enable
> > -      */
> > -     ret = ov5640_write_reg(sensor,
> > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > -     if (ret)
> > -             return ret;
> > -
> > -     /*
> > -      * enable VSYNC/HREF/PCLK DVP control lines
> > -      * & D[9:6] DVP data lines
> > -      *
> > -      * PAD OUTPUT ENABLE 01
> > -      * - 6:         VSYNC output enable
> > -      * - 5:         HREF output enable
> > -      * - 4:         PCLK output enable
> > -      * - [3:0]:     D[9:6] output enable
> > -      */
> > -     ret = ov5640_write_reg(sensor,
> > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > -                            on ? 0x7f : 0);
> > -     if (ret)
> > -             return ret;
> > -
> > -     /*
> > -      * enable D[5:0] DVP data lines
> > -      *
> > -      * PAD OUTPUT ENABLE 02
> > -      * - [7:2]:     D[5:0] output enable
> > -      */
> > -     return ov5640_write_reg(sensor,
> > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > -                             on ? 0xfc : 0);
> > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> > +                             OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
> >  }
> >
> >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > @@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> >       clk_disable_unprepare(sensor->xclk);
> >  }
> >
> > +static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)
> > +{
> > +     int ret = 0;
> > +
> > +     if (!on) {
> > +             /* Reset MIPI bus settings to their default values. */
> > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > +             ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> > +
> > +             return ret;
> > +     }
> > +
> > +     /*
> > +      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > +      *
> > +      * 0x300e = 0x40
> > +      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > +      *                "ov5640_set_stream_mipi()")
> > +      * [4] = 0      : Power up MIPI HS Tx
> > +      * [3] = 0      : Power up MIPI LS Rx
> > +      * [2] = 0      : MIPI interface disabled
> > +      */
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * Gate clock and set LP11 in 'no packets mode' (idle)
> > +      *
> > +      * 0x4800 = 0x24
> > +      * [5] = 1      : Gate clock when 'no packets'
> > +      * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > +      */
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * Set data lanes and clock in LP11 when 'sleeping'
> > +      *
> > +      * 0x3019 = 0x70
> > +      * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > +      * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > +      * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > +      */
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Give lanes some time to coax into LP11 state. */
> > +     usleep_range(500, 1000);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
> > +{
> > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > +     u8 pclk_pol = 0;
> > +     u8 hsync_pol = 0;
> > +     u8 vsync_pol = 0;
> > +     int ret = 0;
> > +
> > +     if (!on) {
> > +             /* Reset settings to their default values. */
> > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > +             ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> > +
> > +             return ret;
> > +     }
> > +
> > +     /*
> > +      * Note about parallel port configuration.
> > +      *
> > +      * When configured in parallel mode, the OV5640 will
> > +      * output 10 bits data on DVP data lines [9:0].
> > +      * If only 8 bits data are wanted, the 8 bits data lines
> > +      * of the camera interface must be physically connected
> > +      * on the DVP data lines [9:2].
> > +      *
> > +      * Control lines polarity can be configured through
> > +      * devicetree endpoint control lines properties.
> > +      * If no endpoint control lines properties are set,
> > +      * polarity will be as below:
> > +      * - VSYNC:     active high
> > +      * - HREF:      active low
> > +      * - PCLK:      active low
> > +      */
> > +     /*
> > +      * configure parallel port control lines polarity
> > +      *
> > +      * POLARITY CTRL0
> > +      * - [5]:       PCLK polarity (0: active low, 1: active high)
> > +      * - [1]:       HREF polarity (0: active low, 1: active high)
> > +      * - [0]:       VSYNC polarity (mismatch here between
> > +      *              datasheet and hardware, 0 is active high
> > +      *              and 1 is active low...)
> > +      */
> > +     if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > +             pclk_pol = 1;
> > +     if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > +             hsync_pol = 1;
> > +     if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > +             vsync_pol = 1;
> > +
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> > +                            (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * powerdown MIPI TX/RX PHY & disable MIPI
> > +      *
> > +      * MIPI CONTROL 00
> > +      * 4:    PWDN PHY TX
> > +      * 3:    PWDN PHY RX
> > +      * 2:    MIPI enable
> > +      */
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * enable VSYNC/HREF/PCLK DVP control lines
> > +      * & D[9:6] DVP data lines
> > +      *
> > +      * PAD OUTPUT ENABLE 01
> > +      * - 6:         VSYNC output enable
> > +      * - 5:         HREF output enable
> > +      * - 4:         PCLK output enable
> > +      * - [3:0]:     D[9:6] output enable
> > +      */
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * enable D[5:0] DVP data lines
> > +      *
> > +      * PAD OUTPUT ENABLE 02
> > +      * - [7:2]:     D[5:0] output enable
> > +      */
> > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> >  {
> >       int ret = 0;
> > @@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> >               ret = ov5640_restore_mode(sensor);
> >               if (ret)
> >                       goto power_off;
> > +     }
> >
> > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > -                     return 0;
> > -
> > -             /*
> > -              * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > -              *
> > -              * 0x300e = 0x40
> > -              * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > -              *                "ov5640_set_stream_mipi()")
> > -              * [4] = 0      : Power up MIPI HS Tx
> > -              * [3] = 0      : Power up MIPI LS Rx
> > -              * [2] = 0      : MIPI interface disabled
> > -              */
> > -             ret = ov5640_write_reg(sensor,
> > -                                    OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > -             if (ret)
> > -                     goto power_off;
> > -
> > -             /*
> > -              * Gate clock and set LP11 in 'no packets mode' (idle)
> > -              *
> > -              * 0x4800 = 0x24
> > -              * [5] = 1      : Gate clock when 'no packets'
> > -              * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > -              */
> > -             ret = ov5640_write_reg(sensor,
> > -                                    OV5640_REG_MIPI_CTRL00, 0x24);
> > -             if (ret)
> > -                     goto power_off;
> > -
> > -             /*
> > -              * Set data lanes and clock in LP11 when 'sleeping'
> > -              *
> > -              * 0x3019 = 0x70
> > -              * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > -              * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > -              * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > -              */
> > -             ret = ov5640_write_reg(sensor,
> > -                                    OV5640_REG_PAD_OUTPUT00, 0x70);
> > -             if (ret)
> > -                     goto power_off;
> > -
> > -             /* Give lanes some time to coax into LP11 state. */
> > -             usleep_range(500, 1000);
> > -
> > -     } else {
> > -             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > -                     /* Reset MIPI bus settings to their default values. */
> > -                     ov5640_write_reg(sensor,
> > -                                      OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > -                     ov5640_write_reg(sensor,
> > -                                      OV5640_REG_MIPI_CTRL00, 0x04);
> > -                     ov5640_write_reg(sensor,
> > -                                      OV5640_REG_PAD_OUTPUT00, 0x00);
> > -             }
> > +     if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > +             ret = ov5640_set_mipi(sensor, on);
> > +     else
> > +             ret = ov5640_set_dvp(sensor, on);
> > +     if (ret)
> > +             goto power_off;
> >
> > +     if (!on)
> >               ov5640_set_power_off(sensor);
> > -     }
> >
> >       return 0;
> >
> > --
> > 2.17.1
> >

Re: [PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Jacopo Mondi]

Hi Prabhakar,
   + Paul who is working with this chip on a parallel setup

On Thu, Aug 06, 2020 at 05:38:57PM +0100, Lad, Prabhakar wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On Thu, Aug 6, 2020 at 5:27 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Prabhakar,
> >
> > On Mon, Aug 03, 2020 at 03:31:45PM +0100, Lad Prabhakar wrote:
> > > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > > (with yavta), but for subsequent runs the bridge driver waited for the
> > > frame to be captured. Debugging further noticed the data lines were
> > > enabled/disabled in stream on/off callback and dumping the register
> > > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > > values, but yet frame capturing failed.
> >
> > That's pretty weird, I wonder if that's not an issue in the bridge, as
> > I expect someone tryed to capture more than 1 image in DVP mode with
> > this driver already.
> >
> I did try the bridge driver with an ov7725 sensor and it works fine in
> both the modes (DVP and BT656).
>
> > I didn't get from your commit message if you have been able to
> > identify where the issue is. You said register values are correct, but
> > did you try to plug a scope and see if data are actually put on the
> > bus ? Does this happen with full parallel too or BT.656 only ?
> >
> unfortunately I didn't scope the pins, but this issue happened in both
> the modes. And with this patch it improves handling the sensor in
> s_stream call.
>

For the record, I tested this one with my CSI-2 setup and capture
still works as expected.

> Cheers,
> Prabhakar
>
> > >
> > > To get around this issue the following actions are performed for
> > > parallel mode (DVP):
> > > 1: Keeps the sensor in software power down mode and is woken up only in
> > >    ov5640_set_stream_dvp() callback.
> > > 2: Enables data lines in s_power callback
> > > 3: Configures HVP lines in s_power callback instead of configuring
> > >    everytime in ov5640_set_stream_dvp().
> > > 4: Disables MIPI interface.
> > >
> > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
> > >  1 file changed, 172 insertions(+), 149 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 2fe4a7ac0592..ec444bee2ce9 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -94,6 +94,9 @@
> > >  #define OV5640_REG_SDE_CTRL5         0x5585
> > >  #define OV5640_REG_AVG_READOUT               0x56a1
> > >
> > > +#define OV5640_SOFTWARE_PWDN         0x42
> > > +#define OV5640_SOFTWARE_WAKEUP               0x02

These two are bitmasks to apply to register 0x3008. I would place them
below the register definition and name them:

#define OV5640_REG_SYS_CTRL0_SW_PWDN         0x42
#define OV5640_REG_SYS_CTRL0_SW_PWUP         0x02

or even:
#define OV5640_REG_SYS_CTRL0_SW_POWER(on)    (0x2 | (on ? 0x40 : 0x00))

Up to you, but I would keep them close to the register definition.

> > > +
> > >  enum ov5640_mode_id {
> > >       OV5640_MODE_QCIF_176_144 = 0,
> > >       OV5640_MODE_QVGA_320_240,
> > > @@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > >  /* YUV422 UYVY VGA@30fps */
> > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > +     {0x3103, 0x03, 0, 0},
> > >       {0x3630, 0x36, 0, 0},

Could you reflow this lines to not leave holes ?

> > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > @@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> > >               val = regs->val;
> > >               mask = regs->mask;
> > >
> > > +             /* remain in power down mode for DVP */
> > > +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
> > > +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > +                     continue;
> > > +

I'm not yet convinced this is a good idea. This will cause the
ov5640_set_dvp() function to be called while the chip is still in
'software power-down' mode. I tried to do the same for CSI-2 as well
and indeed I have LP-11 errors from the CSI-2 receiver, but then
capture works fine (puzzling! it might indicate that register values
as actually retained between software power up/down)

This driver is such a mess I won't mind this 'special' dvp handling,
but it really puzzles me.

From what I see here the bulk of this patch is about moving the
parallel bus configuration from s_stream() to s_power(), is this bit
here really required for that to work or is it a leftover ? Have you
tested it without the above hunk ?

Paul, would you be able to test this series with your parallel setup
as well ?

Also, if not strictly necessary, let's try to remain in the 80 cols limit.


> > >               if (mask)
> > >                       ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> > >               else
> > > @@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > >
> > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > >  {
> > > -     int ret;
> > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > -     u8 pclk_pol = 0;
> > > -     u8 hsync_pol = 0;
> > > -     u8 vsync_pol = 0;
> > > -
> > > -     /*
> > > -      * Note about parallel port configuration.
> > > -      *
> > > -      * When configured in parallel mode, the OV5640 will
> > > -      * output 10 bits data on DVP data lines [9:0].
> > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > -      * of the camera interface must be physically connected
> > > -      * on the DVP data lines [9:2].
> > > -      *
> > > -      * Control lines polarity can be configured through
> > > -      * devicetree endpoint control lines properties.
> > > -      * If no endpoint control lines properties are set,
> > > -      * polarity will be as below:
> > > -      * - VSYNC:     active high
> > > -      * - HREF:      active low
> > > -      * - PCLK:      active low
> > > -      */
> > > -
> > > -     if (on) {
> > > -             /*
> > > -              * configure parallel port control lines polarity
> > > -              *
> > > -              * POLARITY CTRL0
> > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > -              *              datasheet and hardware, 0 is active high
> > > -              *              and 1 is active low...)
> > > -              */
> > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > -                     pclk_pol = 1;
> > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > -                     hsync_pol = 1;
> > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > -                     vsync_pol = 1;
> > > -
> > > -             ret = ov5640_write_reg(sensor,
> > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > -                                    (pclk_pol << 5) |
> > > -                                    (hsync_pol << 1) |
> > > -                                    vsync_pol);
> > > -
> > > -             if (ret)
> > > -                     return ret;
> > > -     }
> > > -
> > > -     /*
> > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > -      *
> > > -      * MIPI CONTROL 00
> > > -      * 4:    PWDN PHY TX
> > > -      * 3:    PWDN PHY RX
> > > -      * 2:    MIPI enable
> > > -      */
> > > -     ret = ov5640_write_reg(sensor,
> > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     /*
> > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > -      * & D[9:6] DVP data lines
> > > -      *
> > > -      * PAD OUTPUT ENABLE 01
> > > -      * - 6:         VSYNC output enable
> > > -      * - 5:         HREF output enable
> > > -      * - 4:         PCLK output enable
> > > -      * - [3:0]:     D[9:6] output enable
> > > -      */
> > > -     ret = ov5640_write_reg(sensor,
> > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > -                            on ? 0x7f : 0);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     /*
> > > -      * enable D[5:0] DVP data lines
> > > -      *
> > > -      * PAD OUTPUT ENABLE 02
> > > -      * - [7:2]:     D[5:0] output enable
> > > -      */
> > > -     return ov5640_write_reg(sensor,
> > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > -                             on ? 0xfc : 0);
> > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> > > +                             OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);

I'm surprised entering/exiting from what is called "Software power
down" in the chip manual retains the registers state!

> > >  }
> > >
> > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > @@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > >       clk_disable_unprepare(sensor->xclk);
> > >  }
> > >
> > > +static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)

Maybe 'ov5640_set_power_mipi()' ? (same for dvp)

> > > +{
> > > +     int ret = 0;
> > > +
> > > +     if (!on) {
> > > +             /* Reset MIPI bus settings to their default values. */
> > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > +             ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> > > +
> > > +             return ret;
> > > +     }

I know this was there already, but I wonder if this is now necessary
(same for the DVP counterpart).

We call this from the power up/down routine, if we get here with on=1
we are exiting from the chip powerdown mode and I expect registers to
be restored to their default values.

> > > +
> > > +     /*
> > > +      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > +      *
> > > +      * 0x300e = 0x40
> > > +      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > +      *                "ov5640_set_stream_mipi()")
> > > +      * [4] = 0      : Power up MIPI HS Tx
> > > +      * [3] = 0      : Power up MIPI LS Rx
> > > +      * [2] = 0      : MIPI interface disabled
> > > +      */
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * Gate clock and set LP11 in 'no packets mode' (idle)
> > > +      *
> > > +      * 0x4800 = 0x24
> > > +      * [5] = 1      : Gate clock when 'no packets'
> > > +      * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > +      */
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * Set data lanes and clock in LP11 when 'sleeping'
> > > +      *
> > > +      * 0x3019 = 0x70
> > > +      * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > +      * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > +      * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > +      */
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* Give lanes some time to coax into LP11 state. */
> > > +     usleep_range(500, 1000);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
> > > +{
> > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > +     u8 pclk_pol = 0;
> > > +     u8 hsync_pol = 0;
> > > +     u8 vsync_pol = 0;
> > > +     int ret = 0;
> > > +
> > > +     if (!on) {
> > > +             /* Reset settings to their default values. */
> > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > +             ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> > > +
> > > +             return ret;
> > > +     }
> > > +
> > > +     /*
> > > +      * Note about parallel port configuration.
> > > +      *
> > > +      * When configured in parallel mode, the OV5640 will
> > > +      * output 10 bits data on DVP data lines [9:0].
> > > +      * If only 8 bits data are wanted, the 8 bits data lines
> > > +      * of the camera interface must be physically connected
> > > +      * on the DVP data lines [9:2].
> > > +      *
> > > +      * Control lines polarity can be configured through
> > > +      * devicetree endpoint control lines properties.
> > > +      * If no endpoint control lines properties are set,
> > > +      * polarity will be as below:
> > > +      * - VSYNC:     active high
> > > +      * - HREF:      active low
> > > +      * - PCLK:      active low
> > > +      */
> > > +     /*
> > > +      * configure parallel port control lines polarity
> > > +      *
> > > +      * POLARITY CTRL0
> > > +      * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > +      * - [1]:       HREF polarity (0: active low, 1: active high)
> > > +      * - [0]:       VSYNC polarity (mismatch here between
> > > +      *              datasheet and hardware, 0 is active high
> > > +      *              and 1 is active low...)
> > > +      */
> > > +     if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > +             pclk_pol = 1;
> > > +     if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > +             hsync_pol = 1;
> > > +     if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > +             vsync_pol = 1;
> > > +
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> > > +                            (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> > > +
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * powerdown MIPI TX/RX PHY & disable MIPI
> > > +      *
> > > +      * MIPI CONTROL 00
> > > +      * 4:    PWDN PHY TX
> > > +      * 3:    PWDN PHY RX
> > > +      * 2:    MIPI enable
> > > +      */
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * enable VSYNC/HREF/PCLK DVP control lines
> > > +      * & D[9:6] DVP data lines
> > > +      *
> > > +      * PAD OUTPUT ENABLE 01
> > > +      * - 6:         VSYNC output enable
> > > +      * - 5:         HREF output enable
> > > +      * - 4:         PCLK output enable
> > > +      * - [3:0]:     D[9:6] output enable
> > > +      */
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * enable D[5:0] DVP data lines
> > > +      *
> > > +      * PAD OUTPUT ENABLE 02
> > > +      * - [7:2]:     D[5:0] output enable
> > > +      */
> > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > >  {
> > >       int ret = 0;
> > > @@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > >               ret = ov5640_restore_mode(sensor);
> > >               if (ret)
> > >                       goto power_off;
> > > +     }
> > >
> > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > -                     return 0;
> > > -
> > > -             /*
> > > -              * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > -              *
> > > -              * 0x300e = 0x40
> > > -              * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > -              *                "ov5640_set_stream_mipi()")
> > > -              * [4] = 0      : Power up MIPI HS Tx
> > > -              * [3] = 0      : Power up MIPI LS Rx
> > > -              * [2] = 0      : MIPI interface disabled
> > > -              */
> > > -             ret = ov5640_write_reg(sensor,
> > > -                                    OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > -             if (ret)
> > > -                     goto power_off;
> > > -
> > > -             /*
> > > -              * Gate clock and set LP11 in 'no packets mode' (idle)
> > > -              *
> > > -              * 0x4800 = 0x24
> > > -              * [5] = 1      : Gate clock when 'no packets'
> > > -              * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > -              */
> > > -             ret = ov5640_write_reg(sensor,
> > > -                                    OV5640_REG_MIPI_CTRL00, 0x24);
> > > -             if (ret)
> > > -                     goto power_off;
> > > -
> > > -             /*
> > > -              * Set data lanes and clock in LP11 when 'sleeping'
> > > -              *
> > > -              * 0x3019 = 0x70
> > > -              * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > -              * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > -              * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > -              */
> > > -             ret = ov5640_write_reg(sensor,
> > > -                                    OV5640_REG_PAD_OUTPUT00, 0x70);
> > > -             if (ret)
> > > -                     goto power_off;
> > > -
> > > -             /* Give lanes some time to coax into LP11 state. */
> > > -             usleep_range(500, 1000);
> > > -
> > > -     } else {
> > > -             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > -                     /* Reset MIPI bus settings to their default values. */
> > > -                     ov5640_write_reg(sensor,
> > > -                                      OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > -                     ov5640_write_reg(sensor,
> > > -                                      OV5640_REG_MIPI_CTRL00, 0x04);
> > > -                     ov5640_write_reg(sensor,
> > > -                                      OV5640_REG_PAD_OUTPUT00, 0x00);
> > > -             }
> > > +     if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > > +             ret = ov5640_set_mipi(sensor, on);
> > > +     else
> > > +             ret = ov5640_set_dvp(sensor, on);
> > > +     if (ret)
> > > +             goto power_off;

If you agree the  if (!on) case in the two set_mipi()/set_dvp() functions
above can be removed, this function should become

                if (!on) {
                        ov5640_set_power_off(sensor)
                        return 0;
                }

                ....

Thanks
  j

> > >
> > > +     if (!on)
> > >               ov5640_set_power_off(sensor);
> > > -     }
> > >
> > >       return 0;
> > >
> > > --
> > > 2.17.1
> > >

Re: [PATCH v2 0/4] media: i2c: ov5640 feature enhancement and fixes

[Lad, Prabhakar]

Hi Jacopo,


On Thu, Aug 6, 2020 at 3:44 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hello,
>
> On Mon, Aug 03, 2020 at 03:31:43PM +0100, Lad Prabhakar wrote:
> > Hi All,
> >
> > This patch series fixes DVP support and enables BT656 mode in
> > the driver.
> >
> > @Jacopo Mondi - patch 1/4 will collide with your patch series [1],
> > feel free to merge it as part of your v2.
>
> This would actually make my life simpler, as one of the issues I had
> was trying to make bus-type required to be able to differentiate
> between different properties.
>
Thank you for taking care of it.

Cheers,
Prabhakar

> >
> > [1] https://www.spinics.net/lists/linux-renesas-soc/msg51236.html
> >
> > Cheers,
> > Prabhakar
> >
> > Changes for v2:
> > * Added support to fallback in parallel mode
> > * Documented bus-type property
> > * Added descriptive commit message for patch 2/4 as pointed
> >   by Sakari
> > * Fixed review comments pointed by Laurent to have separate functions
> >   for mipi and dvp setup
> > * Made sure the sensor is in power down mode during startup too for
> >   DVP mode
> >
> > Lad Prabhakar (4):
> >   dt-bindings: media: i2c: ov5640: Document bus-type property
> >   media: i2c: ov5640: Enable data pins on poweron for DVP mode
> >   media: i2c: ov5640: Add support for BT656 mode
> >   media: i2c: ov5640: Fallback to parallel mode
> >
> >  .../devicetree/bindings/media/i2c/ov5640.txt  |   9 +-
> >  drivers/media/i2c/ov5640.c                    | 333 ++++++++++--------
> >  2 files changed, 198 insertions(+), 144 deletions(-)
> >
> > --
> > 2.17.1
> >

Re: [PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Lad, Prabhakar]

Hi Jacopo,

Thank you for the review.

On Fri, Aug 7, 2020 at 9:42 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Prabhakar,
>    + Paul who is working with this chip on a parallel setup
>
> On Thu, Aug 06, 2020 at 05:38:57PM +0100, Lad, Prabhakar wrote:
> > Hi Jacopo,
> >
> > Thank you for the review.
> >
> > On Thu, Aug 6, 2020 at 5:27 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > On Mon, Aug 03, 2020 at 03:31:45PM +0100, Lad Prabhakar wrote:
> > > > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > > > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > > > (with yavta), but for subsequent runs the bridge driver waited for the
> > > > frame to be captured. Debugging further noticed the data lines were
> > > > enabled/disabled in stream on/off callback and dumping the register
> > > > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > > > values, but yet frame capturing failed.
> > >
> > > That's pretty weird, I wonder if that's not an issue in the bridge, as
> > > I expect someone tryed to capture more than 1 image in DVP mode with
> > > this driver already.
> > >
> > I did try the bridge driver with an ov7725 sensor and it works fine in
> > both the modes (DVP and BT656).
> >
> > > I didn't get from your commit message if you have been able to
> > > identify where the issue is. You said register values are correct, but
> > > did you try to plug a scope and see if data are actually put on the
> > > bus ? Does this happen with full parallel too or BT.656 only ?
> > >
> > unfortunately I didn't scope the pins, but this issue happened in both
> > the modes. And with this patch it improves handling the sensor in
> > s_stream call.
> >
>
> For the record, I tested this one with my CSI-2 setup and capture
> still works as expected.
>
Thank you for testing this.

> > Cheers,
> > Prabhakar
> >
> > > >
> > > > To get around this issue the following actions are performed for
> > > > parallel mode (DVP):
> > > > 1: Keeps the sensor in software power down mode and is woken up only in
> > > >    ov5640_set_stream_dvp() callback.
> > > > 2: Enables data lines in s_power callback
> > > > 3: Configures HVP lines in s_power callback instead of configuring
> > > >    everytime in ov5640_set_stream_dvp().
> > > > 4: Disables MIPI interface.
> > > >
> > > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
> > > >  1 file changed, 172 insertions(+), 149 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > > index 2fe4a7ac0592..ec444bee2ce9 100644
> > > > --- a/drivers/media/i2c/ov5640.c
> > > > +++ b/drivers/media/i2c/ov5640.c
> > > > @@ -94,6 +94,9 @@
> > > >  #define OV5640_REG_SDE_CTRL5         0x5585
> > > >  #define OV5640_REG_AVG_READOUT               0x56a1
> > > >
> > > > +#define OV5640_SOFTWARE_PWDN         0x42
> > > > +#define OV5640_SOFTWARE_WAKEUP               0x02
>
> These two are bitmasks to apply to register 0x3008. I would place them
> below the register definition and name them:
>
> #define OV5640_REG_SYS_CTRL0_SW_PWDN         0x42
> #define OV5640_REG_SYS_CTRL0_SW_PWUP         0x02
>
> or even:
> #define OV5640_REG_SYS_CTRL0_SW_POWER(on)    (0x2 | (on ? 0x40 : 0x00))
>
> Up to you, but I would keep them close to the register definition.
>
Agreed shall pick up the second option.

> > > > +
> > > >  enum ov5640_mode_id {
> > > >       OV5640_MODE_QCIF_176_144 = 0,
> > > >       OV5640_MODE_QVGA_320_240,
> > > > @@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > > >  /* YUV422 UYVY VGA@30fps */
> > > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > > +     {0x3103, 0x03, 0, 0},
> > > >       {0x3630, 0x36, 0, 0},
>
> Could you reflow this lines to not leave holes ?
>
Will do.

> > > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > > @@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> > > >               val = regs->val;
> > > >               mask = regs->mask;
> > > >
> > > > +             /* remain in power down mode for DVP */
> > > > +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
> > > > +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > +                     continue;
> > > > +
>
> I'm not yet convinced this is a good idea. This will cause the
> ov5640_set_dvp() function to be called while the chip is still in
> 'software power-down' mode. I tried to do the same for CSI-2 as well
> and indeed I have LP-11 errors from the CSI-2 receiver, but then
> capture works fine (puzzling! it might indicate that register values
> as actually retained between software power up/down)
>
> This driver is such a mess I won't mind this 'special' dvp handling,
> but it really puzzles me.
>
> From what I see here the bulk of this patch is about moving the
> parallel bus configuration from s_stream() to s_power(), is this bit
> here really required for that to work or is it a leftover ? Have you
> tested it without the above hunk ?
>
Yes I have and it does work. but the idea behind this is to keep the
sensor in powerdon mode all the time when nor streaming for DVP mode.

> Paul, would you be able to test this series with your parallel setup
> as well ?
>
> Also, if not strictly necessary, let's try to remain in the 80 cols limit.
>
OK.

>
> > > >               if (mask)
> > > >                       ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> > > >               else
> > > > @@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > > >
> > > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > > >  {
> > > > -     int ret;
> > > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > -     u8 pclk_pol = 0;
> > > > -     u8 hsync_pol = 0;
> > > > -     u8 vsync_pol = 0;
> > > > -
> > > > -     /*
> > > > -      * Note about parallel port configuration.
> > > > -      *
> > > > -      * When configured in parallel mode, the OV5640 will
> > > > -      * output 10 bits data on DVP data lines [9:0].
> > > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > > -      * of the camera interface must be physically connected
> > > > -      * on the DVP data lines [9:2].
> > > > -      *
> > > > -      * Control lines polarity can be configured through
> > > > -      * devicetree endpoint control lines properties.
> > > > -      * If no endpoint control lines properties are set,
> > > > -      * polarity will be as below:
> > > > -      * - VSYNC:     active high
> > > > -      * - HREF:      active low
> > > > -      * - PCLK:      active low
> > > > -      */
> > > > -
> > > > -     if (on) {
> > > > -             /*
> > > > -              * configure parallel port control lines polarity
> > > > -              *
> > > > -              * POLARITY CTRL0
> > > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > > -              *              datasheet and hardware, 0 is active high
> > > > -              *              and 1 is active low...)
> > > > -              */
> > > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > -                     pclk_pol = 1;
> > > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > -                     hsync_pol = 1;
> > > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > -                     vsync_pol = 1;
> > > > -
> > > > -             ret = ov5640_write_reg(sensor,
> > > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > > -                                    (pclk_pol << 5) |
> > > > -                                    (hsync_pol << 1) |
> > > > -                                    vsync_pol);
> > > > -
> > > > -             if (ret)
> > > > -                     return ret;
> > > > -     }
> > > > -
> > > > -     /*
> > > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > -      *
> > > > -      * MIPI CONTROL 00
> > > > -      * 4:    PWDN PHY TX
> > > > -      * 3:    PWDN PHY RX
> > > > -      * 2:    MIPI enable
> > > > -      */
> > > > -     ret = ov5640_write_reg(sensor,
> > > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > > -     if (ret)
> > > > -             return ret;
> > > > -
> > > > -     /*
> > > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > > -      * & D[9:6] DVP data lines
> > > > -      *
> > > > -      * PAD OUTPUT ENABLE 01
> > > > -      * - 6:         VSYNC output enable
> > > > -      * - 5:         HREF output enable
> > > > -      * - 4:         PCLK output enable
> > > > -      * - [3:0]:     D[9:6] output enable
> > > > -      */
> > > > -     ret = ov5640_write_reg(sensor,
> > > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > > -                            on ? 0x7f : 0);
> > > > -     if (ret)
> > > > -             return ret;
> > > > -
> > > > -     /*
> > > > -      * enable D[5:0] DVP data lines
> > > > -      *
> > > > -      * PAD OUTPUT ENABLE 02
> > > > -      * - [7:2]:     D[5:0] output enable
> > > > -      */
> > > > -     return ov5640_write_reg(sensor,
> > > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > > -                             on ? 0xfc : 0);
> > > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> > > > +                             OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
>
> I'm surprised entering/exiting from what is called "Software power
> down" in the chip manual retains the registers state!
>
In fact as part of the initial register sequence the sensor is put
into power down mode, the required registers are set and then the
sensor is woken up.

> > > >  }
> > > >
> > > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > > @@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > > >       clk_disable_unprepare(sensor->xclk);
> > > >  }
> > > >
> > > > +static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)
>
> Maybe 'ov5640_set_power_mipi()' ? (same for dvp)
>
OK will replace it.

> > > > +{
> > > > +     int ret = 0;
> > > > +
> > > > +     if (!on) {
> > > > +             /* Reset MIPI bus settings to their default values. */
> > > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > +             ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> > > > +
> > > > +             return ret;
> > > > +     }
>
> I know this was there already, but I wonder if this is now necessary
> (same for the DVP counterpart).
>
> We call this from the power up/down routine, if we get here with on=1
> we are exiting from the chip powerdown mode and I expect registers to
> be restored to their default values.
>
No waking up the sensor from power down mode doesnt restore the
register to default values, infact it will retain the values which are
already set to. Fyi I picked up the datasheet from [1]

[1] http://e2e.ti.com/cfs-file.ashx/__key/communityserver-discussions-components-files/100/2604.OV7725_5F00_CSP2_5F00_DS-_2800_1_5B00_1_5D00_.31_2900_.pdf

> > > > +
> > > > +     /*
> > > > +      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > +      *
> > > > +      * 0x300e = 0x40
> > > > +      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > +      *                "ov5640_set_stream_mipi()")
> > > > +      * [4] = 0      : Power up MIPI HS Tx
> > > > +      * [3] = 0      : Power up MIPI LS Rx
> > > > +      * [2] = 0      : MIPI interface disabled
> > > > +      */
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * Gate clock and set LP11 in 'no packets mode' (idle)
> > > > +      *
> > > > +      * 0x4800 = 0x24
> > > > +      * [5] = 1      : Gate clock when 'no packets'
> > > > +      * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > > +      */
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * Set data lanes and clock in LP11 when 'sleeping'
> > > > +      *
> > > > +      * 0x3019 = 0x70
> > > > +      * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > > +      * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > > +      * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > > +      */
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /* Give lanes some time to coax into LP11 state. */
> > > > +     usleep_range(500, 1000);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
> > > > +{
> > > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > +     u8 pclk_pol = 0;
> > > > +     u8 hsync_pol = 0;
> > > > +     u8 vsync_pol = 0;
> > > > +     int ret = 0;
> > > > +
> > > > +     if (!on) {
> > > > +             /* Reset settings to their default values. */
> > > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > +             ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> > > > +
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     /*
> > > > +      * Note about parallel port configuration.
> > > > +      *
> > > > +      * When configured in parallel mode, the OV5640 will
> > > > +      * output 10 bits data on DVP data lines [9:0].
> > > > +      * If only 8 bits data are wanted, the 8 bits data lines
> > > > +      * of the camera interface must be physically connected
> > > > +      * on the DVP data lines [9:2].
> > > > +      *
> > > > +      * Control lines polarity can be configured through
> > > > +      * devicetree endpoint control lines properties.
> > > > +      * If no endpoint control lines properties are set,
> > > > +      * polarity will be as below:
> > > > +      * - VSYNC:     active high
> > > > +      * - HREF:      active low
> > > > +      * - PCLK:      active low
> > > > +      */
> > > > +     /*
> > > > +      * configure parallel port control lines polarity
> > > > +      *
> > > > +      * POLARITY CTRL0
> > > > +      * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > +      * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > +      * - [0]:       VSYNC polarity (mismatch here between
> > > > +      *              datasheet and hardware, 0 is active high
> > > > +      *              and 1 is active low...)
> > > > +      */
> > > > +     if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > +             pclk_pol = 1;
> > > > +     if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > +             hsync_pol = 1;
> > > > +     if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > +             vsync_pol = 1;
> > > > +
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> > > > +                            (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> > > > +
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > +      *
> > > > +      * MIPI CONTROL 00
> > > > +      * 4:    PWDN PHY TX
> > > > +      * 3:    PWDN PHY RX
> > > > +      * 2:    MIPI enable
> > > > +      */
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * enable VSYNC/HREF/PCLK DVP control lines
> > > > +      * & D[9:6] DVP data lines
> > > > +      *
> > > > +      * PAD OUTPUT ENABLE 01
> > > > +      * - 6:         VSYNC output enable
> > > > +      * - 5:         HREF output enable
> > > > +      * - 4:         PCLK output enable
> > > > +      * - [3:0]:     D[9:6] output enable
> > > > +      */
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * enable D[5:0] DVP data lines
> > > > +      *
> > > > +      * PAD OUTPUT ENABLE 02
> > > > +      * - [7:2]:     D[5:0] output enable
> > > > +      */
> > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > >  {
> > > >       int ret = 0;
> > > > @@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > >               ret = ov5640_restore_mode(sensor);
> > > >               if (ret)
> > > >                       goto power_off;
> > > > +     }
> > > >
> > > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > -                     return 0;
> > > > -
> > > > -             /*
> > > > -              * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > -              *
> > > > -              * 0x300e = 0x40
> > > > -              * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > -              *                "ov5640_set_stream_mipi()")
> > > > -              * [4] = 0      : Power up MIPI HS Tx
> > > > -              * [3] = 0      : Power up MIPI LS Rx
> > > > -              * [2] = 0      : MIPI interface disabled
> > > > -              */
> > > > -             ret = ov5640_write_reg(sensor,
> > > > -                                    OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > > -             if (ret)
> > > > -                     goto power_off;
> > > > -
> > > > -             /*
> > > > -              * Gate clock and set LP11 in 'no packets mode' (idle)
> > > > -              *
> > > > -              * 0x4800 = 0x24
> > > > -              * [5] = 1      : Gate clock when 'no packets'
> > > > -              * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > > -              */
> > > > -             ret = ov5640_write_reg(sensor,
> > > > -                                    OV5640_REG_MIPI_CTRL00, 0x24);
> > > > -             if (ret)
> > > > -                     goto power_off;
> > > > -
> > > > -             /*
> > > > -              * Set data lanes and clock in LP11 when 'sleeping'
> > > > -              *
> > > > -              * 0x3019 = 0x70
> > > > -              * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > > -              * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > > -              * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > > -              */
> > > > -             ret = ov5640_write_reg(sensor,
> > > > -                                    OV5640_REG_PAD_OUTPUT00, 0x70);
> > > > -             if (ret)
> > > > -                     goto power_off;
> > > > -
> > > > -             /* Give lanes some time to coax into LP11 state. */
> > > > -             usleep_range(500, 1000);
> > > > -
> > > > -     } else {
> > > > -             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > > -                     /* Reset MIPI bus settings to their default values. */
> > > > -                     ov5640_write_reg(sensor,
> > > > -                                      OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > -                     ov5640_write_reg(sensor,
> > > > -                                      OV5640_REG_MIPI_CTRL00, 0x04);
> > > > -                     ov5640_write_reg(sensor,
> > > > -                                      OV5640_REG_PAD_OUTPUT00, 0x00);
> > > > -             }
> > > > +     if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > > > +             ret = ov5640_set_mipi(sensor, on);
> > > > +     else
> > > > +             ret = ov5640_set_dvp(sensor, on);
> > > > +     if (ret)
> > > > +             goto power_off;
>
> If you agree the  if (!on) case in the two set_mipi()/set_dvp() functions
> above can be removed, this function should become
>
I would keep it, as mentioned above.

Cheers,
Prabhakar

Re: [PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Jacopo Mondi]

Hi Prabhakar,

On Mon, Aug 10, 2020 at 08:40:29AM +0100, Lad, Prabhakar wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On Fri, Aug 7, 2020 at 9:42 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Prabhakar,
> >    + Paul who is working with this chip on a parallel setup
> >
> > On Thu, Aug 06, 2020 at 05:38:57PM +0100, Lad, Prabhakar wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the review.
> > >
> > > On Thu, Aug 6, 2020 at 5:27 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > On Mon, Aug 03, 2020 at 03:31:45PM +0100, Lad Prabhakar wrote:
> > > > > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > > > > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > > > > (with yavta), but for subsequent runs the bridge driver waited for the
> > > > > frame to be captured. Debugging further noticed the data lines were
> > > > > enabled/disabled in stream on/off callback and dumping the register
> > > > > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > > > > values, but yet frame capturing failed.
> > > >
> > > > That's pretty weird, I wonder if that's not an issue in the bridge, as
> > > > I expect someone tryed to capture more than 1 image in DVP mode with
> > > > this driver already.
> > > >
> > > I did try the bridge driver with an ov7725 sensor and it works fine in
> > > both the modes (DVP and BT656).
> > >
> > > > I didn't get from your commit message if you have been able to
> > > > identify where the issue is. You said register values are correct, but
> > > > did you try to plug a scope and see if data are actually put on the
> > > > bus ? Does this happen with full parallel too or BT.656 only ?
> > > >
> > > unfortunately I didn't scope the pins, but this issue happened in both
> > > the modes. And with this patch it improves handling the sensor in
> > > s_stream call.
> > >
> >
> > For the record, I tested this one with my CSI-2 setup and capture
> > still works as expected.
> >
> Thank you for testing this.
>
> > > Cheers,
> > > Prabhakar
> > >
> > > > >
> > > > > To get around this issue the following actions are performed for
> > > > > parallel mode (DVP):
> > > > > 1: Keeps the sensor in software power down mode and is woken up only in
> > > > >    ov5640_set_stream_dvp() callback.
> > > > > 2: Enables data lines in s_power callback
> > > > > 3: Configures HVP lines in s_power callback instead of configuring
> > > > >    everytime in ov5640_set_stream_dvp().
> > > > > 4: Disables MIPI interface.
> > > > >
> > > > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
> > > > >  1 file changed, 172 insertions(+), 149 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > > > index 2fe4a7ac0592..ec444bee2ce9 100644
> > > > > --- a/drivers/media/i2c/ov5640.c
> > > > > +++ b/drivers/media/i2c/ov5640.c
> > > > > @@ -94,6 +94,9 @@
> > > > >  #define OV5640_REG_SDE_CTRL5         0x5585
> > > > >  #define OV5640_REG_AVG_READOUT               0x56a1
> > > > >
> > > > > +#define OV5640_SOFTWARE_PWDN         0x42
> > > > > +#define OV5640_SOFTWARE_WAKEUP               0x02
> >
> > These two are bitmasks to apply to register 0x3008. I would place them
> > below the register definition and name them:
> >
> > #define OV5640_REG_SYS_CTRL0_SW_PWDN         0x42
> > #define OV5640_REG_SYS_CTRL0_SW_PWUP         0x02
> >
> > or even:
> > #define OV5640_REG_SYS_CTRL0_SW_POWER(on)    (0x2 | (on ? 0x40 : 0x00))
> >
> > Up to you, but I would keep them close to the register definition.
> >
> Agreed shall pick up the second option.
>

Thanks

> > > > > +
> > > > >  enum ov5640_mode_id {
> > > > >       OV5640_MODE_QCIF_176_144 = 0,
> > > > >       OV5640_MODE_QVGA_320_240,
> > > > > @@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > > > >  /* YUV422 UYVY VGA@30fps */
> > > > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > > > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > > > +     {0x3103, 0x03, 0, 0},
> > > > >       {0x3630, 0x36, 0, 0},
> >
> > Could you reflow this lines to not leave holes ?
> >
> Will do.
>

Thank you

> > > > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > > > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > > > @@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> > > > >               val = regs->val;
> > > > >               mask = regs->mask;
> > > > >
> > > > > +             /* remain in power down mode for DVP */
> > > > > +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
> > > > > +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > > +                     continue;
> > > > > +
> >
> > I'm not yet convinced this is a good idea. This will cause the
> > ov5640_set_dvp() function to be called while the chip is still in
> > 'software power-down' mode. I tried to do the same for CSI-2 as well
> > and indeed I have LP-11 errors from the CSI-2 receiver, but then
> > capture works fine (puzzling! it might indicate that register values
> > as actually retained between software power up/down)
> >
> > This driver is such a mess I won't mind this 'special' dvp handling,
> > but it really puzzles me.
> >
> > From what I see here the bulk of this patch is about moving the
> > parallel bus configuration from s_stream() to s_power(), is this bit
> > here really required for that to work or is it a leftover ? Have you
> > tested it without the above hunk ?
> >
> Yes I have and it does work. but the idea behind this is to keep the
> sensor in powerdon mode all the time when nor streaming for DVP mode.
>

I see, thanks for testing

> > Paul, would you be able to test this series with your parallel setup
> > as well ?
> >
> > Also, if not strictly necessary, let's try to remain in the 80 cols limit.
> >
> OK.
>
> >
> > > > >               if (mask)
> > > > >                       ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> > > > >               else
> > > > > @@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > > > >
> > > > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > > > >  {
> > > > > -     int ret;
> > > > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > > -     u8 pclk_pol = 0;
> > > > > -     u8 hsync_pol = 0;
> > > > > -     u8 vsync_pol = 0;
> > > > > -
> > > > > -     /*
> > > > > -      * Note about parallel port configuration.
> > > > > -      *
> > > > > -      * When configured in parallel mode, the OV5640 will
> > > > > -      * output 10 bits data on DVP data lines [9:0].
> > > > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > > > -      * of the camera interface must be physically connected
> > > > > -      * on the DVP data lines [9:2].
> > > > > -      *
> > > > > -      * Control lines polarity can be configured through
> > > > > -      * devicetree endpoint control lines properties.
> > > > > -      * If no endpoint control lines properties are set,
> > > > > -      * polarity will be as below:
> > > > > -      * - VSYNC:     active high
> > > > > -      * - HREF:      active low
> > > > > -      * - PCLK:      active low
> > > > > -      */
> > > > > -
> > > > > -     if (on) {
> > > > > -             /*
> > > > > -              * configure parallel port control lines polarity
> > > > > -              *
> > > > > -              * POLARITY CTRL0
> > > > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > > > -              *              datasheet and hardware, 0 is active high
> > > > > -              *              and 1 is active low...)
> > > > > -              */
> > > > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > -                     pclk_pol = 1;
> > > > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > > -                     hsync_pol = 1;
> > > > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > > -                     vsync_pol = 1;
> > > > > -
> > > > > -             ret = ov5640_write_reg(sensor,
> > > > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > > > -                                    (pclk_pol << 5) |
> > > > > -                                    (hsync_pol << 1) |
> > > > > -                                    vsync_pol);
> > > > > -
> > > > > -             if (ret)
> > > > > -                     return ret;
> > > > > -     }
> > > > > -
> > > > > -     /*
> > > > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > > -      *
> > > > > -      * MIPI CONTROL 00
> > > > > -      * 4:    PWDN PHY TX
> > > > > -      * 3:    PWDN PHY RX
> > > > > -      * 2:    MIPI enable
> > > > > -      */
> > > > > -     ret = ov5640_write_reg(sensor,
> > > > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > > > -     if (ret)
> > > > > -             return ret;
> > > > > -
> > > > > -     /*
> > > > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > > > -      * & D[9:6] DVP data lines
> > > > > -      *
> > > > > -      * PAD OUTPUT ENABLE 01
> > > > > -      * - 6:         VSYNC output enable
> > > > > -      * - 5:         HREF output enable
> > > > > -      * - 4:         PCLK output enable
> > > > > -      * - [3:0]:     D[9:6] output enable
> > > > > -      */
> > > > > -     ret = ov5640_write_reg(sensor,
> > > > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > > > -                            on ? 0x7f : 0);
> > > > > -     if (ret)
> > > > > -             return ret;
> > > > > -
> > > > > -     /*
> > > > > -      * enable D[5:0] DVP data lines
> > > > > -      *
> > > > > -      * PAD OUTPUT ENABLE 02
> > > > > -      * - [7:2]:     D[5:0] output enable
> > > > > -      */
> > > > > -     return ov5640_write_reg(sensor,
> > > > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > > > -                             on ? 0xfc : 0);
> > > > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> > > > > +                             OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
> >
> > I'm surprised entering/exiting from what is called "Software power
> > down" in the chip manual retains the registers state!
> >
> In fact as part of the initial register sequence the sensor is put
> into power down mode, the required registers are set and then the
> sensor is woken up.
>

That's funny. So the 'software power down' mode retains the internal
volatile memory content I assume. Nice to know and thanks for digging!

> > > > >  }
> > > > >
> > > > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > > > @@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > > > >       clk_disable_unprepare(sensor->xclk);
> > > > >  }
> > > > >
> > > > > +static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)
> >
> > Maybe 'ov5640_set_power_mipi()' ? (same for dvp)
> >
> OK will replace it.
>

Thanks

> > > > > +{
> > > > > +     int ret = 0;
> > > > > +
> > > > > +     if (!on) {
> > > > > +             /* Reset MIPI bus settings to their default values. */
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> > > > > +
> > > > > +             return ret;
> > > > > +     }
> >
> > I know this was there already, but I wonder if this is now necessary
> > (same for the DVP counterpart).
> >
> > We call this from the power up/down routine, if we get here with on=1
> > we are exiting from the chip powerdown mode and I expect registers to
> > be restored to their default values.
> >
> No waking up the sensor from power down mode doesnt restore the
> register to default values, infact it will retain the values which are
> already set to. Fyi I picked up the datasheet from [1]
>

Careful here, I now understand 'software power down' might retain the
configuration, but this is called by in the s_power() call chain,
which powers the chip up/down by enabling and disabling clocks and
regulator and resetting the chip (see ov5640_set_power_on() which is called
before this function).

I would really be surprised register values are retained after the
regulators have been shut off :)

> [1] http://e2e.ti.com/cfs-file.ashx/__key/communityserver-discussions-components-files/100/2604.OV7725_5F00_CSP2_5F00_DS-_2800_1_5B00_1_5D00_.31_2900_.pdf

Nice, but that's for ov7725 :)

Thanks
  j

>
> > > > > +
> > > > > +     /*
> > > > > +      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > > +      *
> > > > > +      * 0x300e = 0x40
> > > > > +      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > > +      *                "ov5640_set_stream_mipi()")
> > > > > +      * [4] = 0      : Power up MIPI HS Tx
> > > > > +      * [3] = 0      : Power up MIPI LS Rx
> > > > > +      * [2] = 0      : MIPI interface disabled
> > > > > +      */
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /*
> > > > > +      * Gate clock and set LP11 in 'no packets mode' (idle)
> > > > > +      *
> > > > > +      * 0x4800 = 0x24
> > > > > +      * [5] = 1      : Gate clock when 'no packets'
> > > > > +      * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > > > +      */
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /*
> > > > > +      * Set data lanes and clock in LP11 when 'sleeping'
> > > > > +      *
> > > > > +      * 0x3019 = 0x70
> > > > > +      * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > > > +      * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > > > +      * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > > > +      */
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /* Give lanes some time to coax into LP11 state. */
> > > > > +     usleep_range(500, 1000);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
> > > > > +{
> > > > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > > +     u8 pclk_pol = 0;
> > > > > +     u8 hsync_pol = 0;
> > > > > +     u8 vsync_pol = 0;
> > > > > +     int ret = 0;
> > > > > +
> > > > > +     if (!on) {
> > > > > +             /* Reset settings to their default values. */
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> > > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> > > > > +
> > > > > +             return ret;
> > > > > +     }
> > > > > +
> > > > > +     /*
> > > > > +      * Note about parallel port configuration.
> > > > > +      *
> > > > > +      * When configured in parallel mode, the OV5640 will
> > > > > +      * output 10 bits data on DVP data lines [9:0].
> > > > > +      * If only 8 bits data are wanted, the 8 bits data lines
> > > > > +      * of the camera interface must be physically connected
> > > > > +      * on the DVP data lines [9:2].
> > > > > +      *
> > > > > +      * Control lines polarity can be configured through
> > > > > +      * devicetree endpoint control lines properties.
> > > > > +      * If no endpoint control lines properties are set,
> > > > > +      * polarity will be as below:
> > > > > +      * - VSYNC:     active high
> > > > > +      * - HREF:      active low
> > > > > +      * - PCLK:      active low
> > > > > +      */
> > > > > +     /*
> > > > > +      * configure parallel port control lines polarity
> > > > > +      *
> > > > > +      * POLARITY CTRL0
> > > > > +      * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > > +      * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > > +      * - [0]:       VSYNC polarity (mismatch here between
> > > > > +      *              datasheet and hardware, 0 is active high
> > > > > +      *              and 1 is active low...)
> > > > > +      */
> > > > > +     if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > +             pclk_pol = 1;
> > > > > +     if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > > +             hsync_pol = 1;
> > > > > +     if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > > +             vsync_pol = 1;
> > > > > +
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> > > > > +                            (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> > > > > +
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /*
> > > > > +      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > > +      *
> > > > > +      * MIPI CONTROL 00
> > > > > +      * 4:    PWDN PHY TX
> > > > > +      * 3:    PWDN PHY RX
> > > > > +      * 2:    MIPI enable
> > > > > +      */
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /*
> > > > > +      * enable VSYNC/HREF/PCLK DVP control lines
> > > > > +      * & D[9:6] DVP data lines
> > > > > +      *
> > > > > +      * PAD OUTPUT ENABLE 01
> > > > > +      * - 6:         VSYNC output enable
> > > > > +      * - 5:         HREF output enable
> > > > > +      * - 4:         PCLK output enable
> > > > > +      * - [3:0]:     D[9:6] output enable
> > > > > +      */
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /*
> > > > > +      * enable D[5:0] DVP data lines
> > > > > +      *
> > > > > +      * PAD OUTPUT ENABLE 02
> > > > > +      * - [7:2]:     D[5:0] output enable
> > > > > +      */
> > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > > >  {
> > > > >       int ret = 0;
> > > > > @@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > > >               ret = ov5640_restore_mode(sensor);
> > > > >               if (ret)
> > > > >                       goto power_off;
> > > > > +     }
> > > > >
> > > > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > > -                     return 0;
> > > > > -
> > > > > -             /*
> > > > > -              * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > > -              *
> > > > > -              * 0x300e = 0x40
> > > > > -              * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > > -              *                "ov5640_set_stream_mipi()")
> > > > > -              * [4] = 0      : Power up MIPI HS Tx
> > > > > -              * [3] = 0      : Power up MIPI LS Rx
> > > > > -              * [2] = 0      : MIPI interface disabled
> > > > > -              */
> > > > > -             ret = ov5640_write_reg(sensor,
> > > > > -                                    OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > > > -             if (ret)
> > > > > -                     goto power_off;
> > > > > -
> > > > > -             /*
> > > > > -              * Gate clock and set LP11 in 'no packets mode' (idle)
> > > > > -              *
> > > > > -              * 0x4800 = 0x24
> > > > > -              * [5] = 1      : Gate clock when 'no packets'
> > > > > -              * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > > > -              */
> > > > > -             ret = ov5640_write_reg(sensor,
> > > > > -                                    OV5640_REG_MIPI_CTRL00, 0x24);
> > > > > -             if (ret)
> > > > > -                     goto power_off;
> > > > > -
> > > > > -             /*
> > > > > -              * Set data lanes and clock in LP11 when 'sleeping'
> > > > > -              *
> > > > > -              * 0x3019 = 0x70
> > > > > -              * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > > > -              * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > > > -              * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > > > -              */
> > > > > -             ret = ov5640_write_reg(sensor,
> > > > > -                                    OV5640_REG_PAD_OUTPUT00, 0x70);
> > > > > -             if (ret)
> > > > > -                     goto power_off;
> > > > > -
> > > > > -             /* Give lanes some time to coax into LP11 state. */
> > > > > -             usleep_range(500, 1000);
> > > > > -
> > > > > -     } else {
> > > > > -             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > > > -                     /* Reset MIPI bus settings to their default values. */
> > > > > -                     ov5640_write_reg(sensor,
> > > > > -                                      OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > > -                     ov5640_write_reg(sensor,
> > > > > -                                      OV5640_REG_MIPI_CTRL00, 0x04);
> > > > > -                     ov5640_write_reg(sensor,
> > > > > -                                      OV5640_REG_PAD_OUTPUT00, 0x00);
> > > > > -             }
> > > > > +     if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > > > > +             ret = ov5640_set_mipi(sensor, on);
> > > > > +     else
> > > > > +             ret = ov5640_set_dvp(sensor, on);
> > > > > +     if (ret)
> > > > > +             goto power_off;
> >
> > If you agree the  if (!on) case in the two set_mipi()/set_dvp() functions
> > above can be removed, this function should become
> >
> I would keep it, as mentioned above.
>
> Cheers,
> Prabhakar

Re: [PATCH v2 2/4] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Lad, Prabhakar]

Hi Jacopo,

On Mon, Aug 10, 2020 at 9:39 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Aug 10, 2020 at 08:40:29AM +0100, Lad, Prabhakar wrote:
> > Hi Jacopo,
> >
> > Thank you for the review.
> >
> > On Fri, Aug 7, 2020 at 9:42 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Prabhakar,
> > >    + Paul who is working with this chip on a parallel setup
> > >
> > > On Thu, Aug 06, 2020 at 05:38:57PM +0100, Lad, Prabhakar wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the review.
> > > >
> > > > On Thu, Aug 6, 2020 at 5:27 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > > > >
> > > > > Hi Prabhakar,
> > > > >
> > > > > On Mon, Aug 03, 2020 at 03:31:45PM +0100, Lad Prabhakar wrote:
> > > > > > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > > > > > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > > > > > (with yavta), but for subsequent runs the bridge driver waited for the
> > > > > > frame to be captured. Debugging further noticed the data lines were
> > > > > > enabled/disabled in stream on/off callback and dumping the register
> > > > > > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > > > > > values, but yet frame capturing failed.
> > > > >
> > > > > That's pretty weird, I wonder if that's not an issue in the bridge, as
> > > > > I expect someone tryed to capture more than 1 image in DVP mode with
> > > > > this driver already.
> > > > >
> > > > I did try the bridge driver with an ov7725 sensor and it works fine in
> > > > both the modes (DVP and BT656).
> > > >
> > > > > I didn't get from your commit message if you have been able to
> > > > > identify where the issue is. You said register values are correct, but
> > > > > did you try to plug a scope and see if data are actually put on the
> > > > > bus ? Does this happen with full parallel too or BT.656 only ?
> > > > >
> > > > unfortunately I didn't scope the pins, but this issue happened in both
> > > > the modes. And with this patch it improves handling the sensor in
> > > > s_stream call.
> > > >
> > >
> > > For the record, I tested this one with my CSI-2 setup and capture
> > > still works as expected.
> > >
> > Thank you for testing this.
> >
> > > > Cheers,
> > > > Prabhakar
> > > >
> > > > > >
> > > > > > To get around this issue the following actions are performed for
> > > > > > parallel mode (DVP):
> > > > > > 1: Keeps the sensor in software power down mode and is woken up only in
> > > > > >    ov5640_set_stream_dvp() callback.
> > > > > > 2: Enables data lines in s_power callback
> > > > > > 3: Configures HVP lines in s_power callback instead of configuring
> > > > > >    everytime in ov5640_set_stream_dvp().
> > > > > > 4: Disables MIPI interface.
> > > > > >
> > > > > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/ov5640.c | 321 ++++++++++++++++++++-----------------
> > > > > >  1 file changed, 172 insertions(+), 149 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > > > > index 2fe4a7ac0592..ec444bee2ce9 100644
> > > > > > --- a/drivers/media/i2c/ov5640.c
> > > > > > +++ b/drivers/media/i2c/ov5640.c
> > > > > > @@ -94,6 +94,9 @@
> > > > > >  #define OV5640_REG_SDE_CTRL5         0x5585
> > > > > >  #define OV5640_REG_AVG_READOUT               0x56a1
> > > > > >
> > > > > > +#define OV5640_SOFTWARE_PWDN         0x42
> > > > > > +#define OV5640_SOFTWARE_WAKEUP               0x02
> > >
> > > These two are bitmasks to apply to register 0x3008. I would place them
> > > below the register definition and name them:
> > >
> > > #define OV5640_REG_SYS_CTRL0_SW_PWDN         0x42
> > > #define OV5640_REG_SYS_CTRL0_SW_PWUP         0x02
> > >
> > > or even:
> > > #define OV5640_REG_SYS_CTRL0_SW_POWER(on)    (0x2 | (on ? 0x40 : 0x00))
> > >
> > > Up to you, but I would keep them close to the register definition.
> > >
> > Agreed shall pick up the second option.
> >
>
> Thanks
>
> > > > > > +
> > > > > >  enum ov5640_mode_id {
> > > > > >       OV5640_MODE_QCIF_176_144 = 0,
> > > > > >       OV5640_MODE_QVGA_320_240,
> > > > > > @@ -274,7 +277,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > > > > >  /* YUV422 UYVY VGA@30fps */
> > > > > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > > > > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > > > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > > > > +     {0x3103, 0x03, 0, 0},
> > > > > >       {0x3630, 0x36, 0, 0},
> > >
> > > Could you reflow this lines to not leave holes ?
> > >
> > Will do.
> >
>
> Thank you
>
> > > > > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > > > > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > > > > @@ -1120,6 +1123,11 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> > > > > >               val = regs->val;
> > > > > >               mask = regs->mask;
> > > > > >
> > > > > > +             /* remain in power down mode for DVP */
> > > > > > +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_SOFTWARE_WAKEUP &&
> > > > > > +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > > > +                     continue;
> > > > > > +
> > >
> > > I'm not yet convinced this is a good idea. This will cause the
> > > ov5640_set_dvp() function to be called while the chip is still in
> > > 'software power-down' mode. I tried to do the same for CSI-2 as well
> > > and indeed I have LP-11 errors from the CSI-2 receiver, but then
> > > capture works fine (puzzling! it might indicate that register values
> > > as actually retained between software power up/down)
> > >
> > > This driver is such a mess I won't mind this 'special' dvp handling,
> > > but it really puzzles me.
> > >
> > > From what I see here the bulk of this patch is about moving the
> > > parallel bus configuration from s_stream() to s_power(), is this bit
> > > here really required for that to work or is it a leftover ? Have you
> > > tested it without the above hunk ?
> > >
> > Yes I have and it does work. but the idea behind this is to keep the
> > sensor in powerdon mode all the time when nor streaming for DVP mode.
> >
>
> I see, thanks for testing
>
> > > Paul, would you be able to test this series with your parallel setup
> > > as well ?
> > >
> > > Also, if not strictly necessary, let's try to remain in the 80 cols limit.
> > >
> > OK.
> >
> > >
> > > > > >               if (mask)
> > > > > >                       ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> > > > > >               else
> > > > > > @@ -1210,96 +1218,8 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > > > > >
> > > > > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > > > > >  {
> > > > > > -     int ret;
> > > > > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > > > -     u8 pclk_pol = 0;
> > > > > > -     u8 hsync_pol = 0;
> > > > > > -     u8 vsync_pol = 0;
> > > > > > -
> > > > > > -     /*
> > > > > > -      * Note about parallel port configuration.
> > > > > > -      *
> > > > > > -      * When configured in parallel mode, the OV5640 will
> > > > > > -      * output 10 bits data on DVP data lines [9:0].
> > > > > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > > > > -      * of the camera interface must be physically connected
> > > > > > -      * on the DVP data lines [9:2].
> > > > > > -      *
> > > > > > -      * Control lines polarity can be configured through
> > > > > > -      * devicetree endpoint control lines properties.
> > > > > > -      * If no endpoint control lines properties are set,
> > > > > > -      * polarity will be as below:
> > > > > > -      * - VSYNC:     active high
> > > > > > -      * - HREF:      active low
> > > > > > -      * - PCLK:      active low
> > > > > > -      */
> > > > > > -
> > > > > > -     if (on) {
> > > > > > -             /*
> > > > > > -              * configure parallel port control lines polarity
> > > > > > -              *
> > > > > > -              * POLARITY CTRL0
> > > > > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > > > > -              *              datasheet and hardware, 0 is active high
> > > > > > -              *              and 1 is active low...)
> > > > > > -              */
> > > > > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > > -                     pclk_pol = 1;
> > > > > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > > > -                     hsync_pol = 1;
> > > > > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > > > -                     vsync_pol = 1;
> > > > > > -
> > > > > > -             ret = ov5640_write_reg(sensor,
> > > > > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > > > > -                                    (pclk_pol << 5) |
> > > > > > -                                    (hsync_pol << 1) |
> > > > > > -                                    vsync_pol);
> > > > > > -
> > > > > > -             if (ret)
> > > > > > -                     return ret;
> > > > > > -     }
> > > > > > -
> > > > > > -     /*
> > > > > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > > > -      *
> > > > > > -      * MIPI CONTROL 00
> > > > > > -      * 4:    PWDN PHY TX
> > > > > > -      * 3:    PWDN PHY RX
> > > > > > -      * 2:    MIPI enable
> > > > > > -      */
> > > > > > -     ret = ov5640_write_reg(sensor,
> > > > > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > > > > -     if (ret)
> > > > > > -             return ret;
> > > > > > -
> > > > > > -     /*
> > > > > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > > > > -      * & D[9:6] DVP data lines
> > > > > > -      *
> > > > > > -      * PAD OUTPUT ENABLE 01
> > > > > > -      * - 6:         VSYNC output enable
> > > > > > -      * - 5:         HREF output enable
> > > > > > -      * - 4:         PCLK output enable
> > > > > > -      * - [3:0]:     D[9:6] output enable
> > > > > > -      */
> > > > > > -     ret = ov5640_write_reg(sensor,
> > > > > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > > > > -                            on ? 0x7f : 0);
> > > > > > -     if (ret)
> > > > > > -             return ret;
> > > > > > -
> > > > > > -     /*
> > > > > > -      * enable D[5:0] DVP data lines
> > > > > > -      *
> > > > > > -      * PAD OUTPUT ENABLE 02
> > > > > > -      * - [7:2]:     D[5:0] output enable
> > > > > > -      */
> > > > > > -     return ov5640_write_reg(sensor,
> > > > > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > > > > -                             on ? 0xfc : 0);
> > > > > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> > > > > > +                             OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
> > >
> > > I'm surprised entering/exiting from what is called "Software power
> > > down" in the chip manual retains the registers state!
> > >
> > In fact as part of the initial register sequence the sensor is put
> > into power down mode, the required registers are set and then the
> > sensor is woken up.
> >
>
> That's funny. So the 'software power down' mode retains the internal
> volatile memory content I assume. Nice to know and thanks for digging!
>
> > > > > >  }
> > > > > >
> > > > > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > > > > @@ -2001,6 +1921,159 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > > > > >       clk_disable_unprepare(sensor->xclk);
> > > > > >  }
> > > > > >
> > > > > > +static int ov5640_set_mipi(struct ov5640_dev *sensor, bool on)
> > >
> > > Maybe 'ov5640_set_power_mipi()' ? (same for dvp)
> > >
> > OK will replace it.
> >
>
> Thanks
>
> > > > > > +{
> > > > > > +     int ret = 0;
> > > > > > +
> > > > > > +     if (!on) {
> > > > > > +             /* Reset MIPI bus settings to their default values. */
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> > > > > > +
> > > > > > +             return ret;
> > > > > > +     }
> > >
> > > I know this was there already, but I wonder if this is now necessary
> > > (same for the DVP counterpart).
> > >
> > > We call this from the power up/down routine, if we get here with on=1
> > > we are exiting from the chip powerdown mode and I expect registers to
> > > be restored to their default values.
> > >
> > No waking up the sensor from power down mode doesnt restore the
> > register to default values, infact it will retain the values which are
> > already set to. Fyi I picked up the datasheet from [1]
> >
>
> Careful here, I now understand 'software power down' might retain the
> configuration, but this is called by in the s_power() call chain,
> which powers the chip up/down by enabling and disabling clocks and
> regulator and resetting the chip (see ov5640_set_power_on() which is called
> before this function).
>
> I would really be surprised register values are retained after the
> regulators have been shut off :)
>
Agreed disabling the clocks and regulators shouldnt re-store register
contents :)

> > [1] http://e2e.ti.com/cfs-file.ashx/__key/communityserver-discussions-components-files/100/2604.OV7725_5F00_CSP2_5F00_DS-_2800_1_5B00_1_5D00_.31_2900_.pdf
>
> Nice, but that's for ov7725 :)
>
Oops my bad here you go
https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf

Cheers,
Prabhakar

> Thanks
>   j
>
> >
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > > > +      *
> > > > > > +      * 0x300e = 0x40
> > > > > > +      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > > > +      *                "ov5640_set_stream_mipi()")
> > > > > > +      * [4] = 0      : Power up MIPI HS Tx
> > > > > > +      * [3] = 0      : Power up MIPI LS Rx
> > > > > > +      * [2] = 0      : MIPI interface disabled
> > > > > > +      */
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Gate clock and set LP11 in 'no packets mode' (idle)
> > > > > > +      *
> > > > > > +      * 0x4800 = 0x24
> > > > > > +      * [5] = 1      : Gate clock when 'no packets'
> > > > > > +      * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > > > > +      */
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Set data lanes and clock in LP11 when 'sleeping'
> > > > > > +      *
> > > > > > +      * 0x3019 = 0x70
> > > > > > +      * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > > > > +      * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > > > > +      * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > > > > +      */
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /* Give lanes some time to coax into LP11 state. */
> > > > > > +     usleep_range(500, 1000);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
> > > > > > +{
> > > > > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > > > +     u8 pclk_pol = 0;
> > > > > > +     u8 hsync_pol = 0;
> > > > > > +     u8 vsync_pol = 0;
> > > > > > +     int ret = 0;
> > > > > > +
> > > > > > +     if (!on) {
> > > > > > +             /* Reset settings to their default values. */
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> > > > > > +             ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> > > > > > +
> > > > > > +             return ret;
> > > > > > +     }
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Note about parallel port configuration.
> > > > > > +      *
> > > > > > +      * When configured in parallel mode, the OV5640 will
> > > > > > +      * output 10 bits data on DVP data lines [9:0].
> > > > > > +      * If only 8 bits data are wanted, the 8 bits data lines
> > > > > > +      * of the camera interface must be physically connected
> > > > > > +      * on the DVP data lines [9:2].
> > > > > > +      *
> > > > > > +      * Control lines polarity can be configured through
> > > > > > +      * devicetree endpoint control lines properties.
> > > > > > +      * If no endpoint control lines properties are set,
> > > > > > +      * polarity will be as below:
> > > > > > +      * - VSYNC:     active high
> > > > > > +      * - HREF:      active low
> > > > > > +      * - PCLK:      active low
> > > > > > +      */
> > > > > > +     /*
> > > > > > +      * configure parallel port control lines polarity
> > > > > > +      *
> > > > > > +      * POLARITY CTRL0
> > > > > > +      * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > > > +      * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > > > +      * - [0]:       VSYNC polarity (mismatch here between
> > > > > > +      *              datasheet and hardware, 0 is active high
> > > > > > +      *              and 1 is active low...)
> > > > > > +      */
> > > > > > +     if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > > +             pclk_pol = 1;
> > > > > > +     if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > > > +             hsync_pol = 1;
> > > > > > +     if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > > > +             vsync_pol = 1;
> > > > > > +
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> > > > > > +                            (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> > > > > > +
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > > > +      *
> > > > > > +      * MIPI CONTROL 00
> > > > > > +      * 4:    PWDN PHY TX
> > > > > > +      * 3:    PWDN PHY RX
> > > > > > +      * 2:    MIPI enable
> > > > > > +      */
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * enable VSYNC/HREF/PCLK DVP control lines
> > > > > > +      * & D[9:6] DVP data lines
> > > > > > +      *
> > > > > > +      * PAD OUTPUT ENABLE 01
> > > > > > +      * - 6:         VSYNC output enable
> > > > > > +      * - 5:         HREF output enable
> > > > > > +      * - 4:         PCLK output enable
> > > > > > +      * - [3:0]:     D[9:6] output enable
> > > > > > +      */
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * enable D[5:0] DVP data lines
> > > > > > +      *
> > > > > > +      * PAD OUTPUT ENABLE 02
> > > > > > +      * - [7:2]:     D[5:0] output enable
> > > > > > +      */
> > > > > > +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > > > >  {
> > > > > >       int ret = 0;
> > > > > > @@ -2013,67 +2086,17 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > > > >               ret = ov5640_restore_mode(sensor);
> > > > > >               if (ret)
> > > > > >                       goto power_off;
> > > > > > +     }
> > > > > >
> > > > > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > > > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > > > -                     return 0;
> > > > > > -
> > > > > > -             /*
> > > > > > -              * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > > > -              *
> > > > > > -              * 0x300e = 0x40
> > > > > > -              * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > > > -              *                "ov5640_set_stream_mipi()")
> > > > > > -              * [4] = 0      : Power up MIPI HS Tx
> > > > > > -              * [3] = 0      : Power up MIPI LS Rx
> > > > > > -              * [2] = 0      : MIPI interface disabled
> > > > > > -              */
> > > > > > -             ret = ov5640_write_reg(sensor,
> > > > > > -                                    OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > > > > > -             if (ret)
> > > > > > -                     goto power_off;
> > > > > > -
> > > > > > -             /*
> > > > > > -              * Gate clock and set LP11 in 'no packets mode' (idle)
> > > > > > -              *
> > > > > > -              * 0x4800 = 0x24
> > > > > > -              * [5] = 1      : Gate clock when 'no packets'
> > > > > > -              * [2] = 1      : MIPI bus in LP11 when 'no packets'
> > > > > > -              */
> > > > > > -             ret = ov5640_write_reg(sensor,
> > > > > > -                                    OV5640_REG_MIPI_CTRL00, 0x24);
> > > > > > -             if (ret)
> > > > > > -                     goto power_off;
> > > > > > -
> > > > > > -             /*
> > > > > > -              * Set data lanes and clock in LP11 when 'sleeping'
> > > > > > -              *
> > > > > > -              * 0x3019 = 0x70
> > > > > > -              * [6] = 1      : MIPI data lane 2 in LP11 when 'sleeping'
> > > > > > -              * [5] = 1      : MIPI data lane 1 in LP11 when 'sleeping'
> > > > > > -              * [4] = 1      : MIPI clock lane in LP11 when 'sleeping'
> > > > > > -              */
> > > > > > -             ret = ov5640_write_reg(sensor,
> > > > > > -                                    OV5640_REG_PAD_OUTPUT00, 0x70);
> > > > > > -             if (ret)
> > > > > > -                     goto power_off;
> > > > > > -
> > > > > > -             /* Give lanes some time to coax into LP11 state. */
> > > > > > -             usleep_range(500, 1000);
> > > > > > -
> > > > > > -     } else {
> > > > > > -             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > > > > -                     /* Reset MIPI bus settings to their default values. */
> > > > > > -                     ov5640_write_reg(sensor,
> > > > > > -                                      OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > > > > > -                     ov5640_write_reg(sensor,
> > > > > > -                                      OV5640_REG_MIPI_CTRL00, 0x04);
> > > > > > -                     ov5640_write_reg(sensor,
> > > > > > -                                      OV5640_REG_PAD_OUTPUT00, 0x00);
> > > > > > -             }
> > > > > > +     if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > > > > > +             ret = ov5640_set_mipi(sensor, on);
> > > > > > +     else
> > > > > > +             ret = ov5640_set_dvp(sensor, on);
> > > > > > +     if (ret)
> > > > > > +             goto power_off;
> > >
> > > If you agree the  if (!on) case in the two set_mipi()/set_dvp() functions
> > > above can be removed, this function should become
> > >
> > I would keep it, as mentioned above.
> >
> > Cheers,
> > Prabhakar

Re: [PATCH v2 3/4] media: i2c: ov5640: Add support for BT656 mode

[Hugues FRUCHET]

Hi Prabhakar,

I'm currently testing the OV5640 CCIR656 embedded synchronisation mode 
on STM32MP1 running STM32 DCMI camera interface.
Tests not yet fully completed but sounds good, more details below.

On 8/3/20 4:31 PM, Lad Prabhakar wrote:
> Enable support for BT656 mode.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>   drivers/media/i2c/ov5640.c | 40 +++++++++++++++++++++++++++-----------
>   1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index ec444bee2ce9..08c67250042f 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -82,6 +82,7 @@
>   #define OV5640_REG_VFIFO_HSIZE		0x4602
>   #define OV5640_REG_VFIFO_VSIZE		0x4604
>   #define OV5640_REG_JPG_MODE_SELECT	0x4713
> +#define OV5640_REG_CCIR656_CTRL00	0x4730
>   #define OV5640_REG_POLARITY_CTRL00	0x4740
>   #define OV5640_REG_MIPI_CTRL00		0x4800
>   #define OV5640_REG_DEBUG_MODE		0x4814
> @@ -1216,6 +1217,18 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>   			      BIT(1), on ? 0 : BIT(1));
>   }
>   
> +static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
> +{
> +	int ret;
> +
> +	ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00, on ? 0x1 : 0x00);
> +	if (ret)
> +		return ret;

Please add a comment explaining bit fields from datasheet:
Bit[7]: SYNC code selection
0: Automatically generate SYNC code
1: SYNC code from register setting 0x4732~4735
Bit[4:3]: Blank toggle data options
00: Toggle data is 1'h040/1'h200
Bit[1]: Clip data disable (so clip is enabled)
Bit[0]: CCIR656 mode enable

So 0x1 stands for CCIR656 with automatic SYNC codes: SAV/EAV with 
default values ie SAV=0xFF000080 & EAV=0xFF00009d.

On STM32 platform, this correspond to DCMI configuration ESCR=0xff9d80ff 
and ESUR=0xffffffff.
On Renesas platform, I have not seen any configuration in code, this 
SAV/EAV mode seems to be handled by default by hardware, do you confirm ?

Note that another CCIR656 embedded synchro mode could be used with 
custom synchro codes FS/FE/LS/LE in registers 0x4732-0x4735, this
mode is enabled with CCIR656_CTRL00(0x4730) set to 0x81:
Bit[7]: SYNC code selection
1: SYNC code from register setting 0x4732~4735

> +
> +	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> +				OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
> +}
> +
>   static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>   {
>   	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> @@ -2022,18 +2035,20 @@ static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
>   	 *		datasheet and hardware, 0 is active high
>   	 *		and 1 is active low...)
>   	 */
> -	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -		pclk_pol = 1;
> -	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> -		hsync_pol = 1;
> -	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -		vsync_pol = 1;
> +	if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
> +		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +			pclk_pol = 1;
> +		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +			hsync_pol = 1;
> +		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +			vsync_pol = 1;
>   
> -	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> -			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> +		ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> +				       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
>   
> -	if (ret)
> -		return ret;
> +		if (ret)
> +			return ret;
> +	}
>   
>   	/*
>   	 * powerdown MIPI TX/RX PHY & disable MIPI
> @@ -2057,7 +2072,8 @@ static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
>   	 * - 4:		PCLK output enable
>   	 * - [3:0]:	D[9:6] output enable
>   	 */
> -	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
> +			       sensor->ep.bus_type == V4L2_MBUS_PARALLEL ? 0x7f : 0x1f);
>   	if (ret)
>   		return ret;
>   
> @@ -2911,6 +2927,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>   
>   		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
>   			ret = ov5640_set_stream_mipi(sensor, enable);
> +		else if (sensor->ep.bus_type == V4L2_MBUS_BT656)
> +			ret = ov5640_set_stream_bt656(sensor, enable);
>   		else
>   			ret = ov5640_set_stream_dvp(sensor, enable);
>   
> 

BR, Hugues.