[PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues Fruchet]

Enhance OV5640 CSI driver to support also DVP parallel interface.
Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
YUV422 UYVY format.
Some other improvements on chip identifier check and removal
of warnings in powering phase around gpio handling.

===========
= history =
===========
version 5:
  - Refine bindings as per Sakari suggestion:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg124048.html

version 4:
  - Refine bindings as per Sakari suggestion:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg123609.html
  - Parallel port control lines polarity can now be configured through
    devicetree

version 3:
  - Move chip identifier check at probe according to Fabio Estevam comment:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg122575.html
  - Use 16 bits register read for this check as per Steve Longerbeam comment:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg122692.html
  - Update bindings to document parallel mode support as per Fabio Estevam comment:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg122576.html
  - Enable the whole 10 bits parallel output and document 8/10 bits support
    in ov5640_set_stream_dvp() to answer to Steve Longerbeam comment:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg122693.html

version 2:
  - Fix comments from Sakari Ailus:
    https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg122259.html
  - Revisit ov5640_set_stream_dvp() to only configure DVP at streamon
  - Revisit ov5640_set_stream_dvp() implementation with fewer register settings

version 1:
  - Initial submission

Hugues Fruchet (5):
  media: ov5640: switch to gpiod_set_value_cansleep()
  media: ov5640: check chip id
  media: dt-bindings: ov5640: refine CSI-2 and add parallel interface
  media: ov5640: add support of DVP parallel interface
  media: ov5640: add support of RGB565 and YUYV formats

 .../devicetree/bindings/media/i2c/ov5640.txt       |  46 ++-
 drivers/media/i2c/ov5640.c                         | 325 ++++++++++++++++++---
 2 files changed, 324 insertions(+), 47 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/5] media: ov5640: switch to gpiod_set_value_cansleep()

[Hugues Fruchet]

Switch gpiod_set_value to gpiod_set_value_cansleep to avoid
warnings when powering sensor.

Signed-off-by: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
---
 drivers/media/i2c/ov5640.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index c89ed66..61071f5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1524,7 +1524,7 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 
 static void ov5640_power(struct ov5640_dev *sensor, bool enable)
 {
-	gpiod_set_value(sensor->pwdn_gpio, enable ? 0 : 1);
+	gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
 }
 
 static void ov5640_reset(struct ov5640_dev *sensor)
@@ -1532,7 +1532,7 @@ static void ov5640_reset(struct ov5640_dev *sensor)
 	if (!sensor->reset_gpio)
 		return;
 
-	gpiod_set_value(sensor->reset_gpio, 0);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
 
 	/* camera power cycle */
 	ov5640_power(sensor, false);
@@ -1540,10 +1540,10 @@ static void ov5640_reset(struct ov5640_dev *sensor)
 	ov5640_power(sensor, true);
 	usleep_range(5000, 10000);
 
-	gpiod_set_value(sensor->reset_gpio, 1);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
 	usleep_range(1000, 2000);
 
-	gpiod_set_value(sensor->reset_gpio, 0);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
 	usleep_range(5000, 10000);
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/5] media: ov5640: check chip id

[Hugues Fruchet]

Verify that chip identifier is correct when probing.

Signed-off-by: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
---
 drivers/media/i2c/ov5640.c | 95 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 61071f5..9f031f3 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1547,24 +1547,58 @@ static void ov5640_reset(struct ov5640_dev *sensor)
 	usleep_range(5000, 10000);
 }
 
-static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
+static int ov5640_set_power_on(struct ov5640_dev *sensor)
 {
-	int ret = 0;
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
 
-	if (on) {
-		clk_prepare_enable(sensor->xclk);
+	ret = clk_prepare_enable(sensor->xclk);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable clock\n",
+			__func__);
+		return ret;
+	}
 
-		ret = regulator_bulk_enable(OV5640_NUM_SUPPLIES,
-					    sensor->supplies);
-		if (ret)
-			goto xclk_off;
+	ret = regulator_bulk_enable(OV5640_NUM_SUPPLIES,
+				    sensor->supplies);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable regulators\n",
+			__func__);
+		goto xclk_off;
+	}
+
+	ov5640_reset(sensor);
+	ov5640_power(sensor, true);
+
+	ret = ov5640_init_slave_id(sensor);
+	if (ret)
+		goto power_off;
+
+	return 0;
+
+power_off:
+	ov5640_power(sensor, false);
+	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
+xclk_off:
+	clk_disable_unprepare(sensor->xclk);
+	return ret;
+}
+
+static void ov5640_set_power_off(struct ov5640_dev *sensor)
+{
+	ov5640_power(sensor, false);
+	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
+	clk_disable_unprepare(sensor->xclk);
+}
 
-		ov5640_reset(sensor);
-		ov5640_power(sensor, true);
+static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
+{
+	int ret = 0;
 
-		ret = ov5640_init_slave_id(sensor);
+	if (on) {
+		ret = ov5640_set_power_on(sensor);
 		if (ret)
-			goto power_off;
+			return ret;
 
 		ret = ov5640_restore_mode(sensor);
 		if (ret)
@@ -1586,10 +1620,7 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 	}
 
 power_off:
-	ov5640_power(sensor, false);
-	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
-xclk_off:
-	clk_disable_unprepare(sensor->xclk);
+	ov5640_set_power_off(sensor);
 	return ret;
 }
 
@@ -2202,6 +2233,34 @@ static int ov5640_get_regulators(struct ov5640_dev *sensor)
 				       sensor->supplies);
 }
 
+static int ov5640_check_chip_id(struct ov5640_dev *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	int ret = 0;
+	u16 chip_id;
+
+	ret = ov5640_set_power_on(sensor);
+	if (ret)
+		return ret;
+
+	ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to read chip identifier\n",
+			__func__);
+		goto power_off;
+	}
+
+	if (chip_id != 0x5640) {
+		dev_err(&client->dev, "%s: wrong chip identifier, expected 0x5640, got 0x%x\n",
+			__func__, chip_id);
+		ret = -ENXIO;
+	}
+
+power_off:
+	ov5640_set_power_off(sensor);
+	return ret;
+}
+
 static int ov5640_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -2284,6 +2343,10 @@ static int ov5640_probe(struct i2c_client *client,
 
 	mutex_init(&sensor->lock);
 
+	ret = ov5640_check_chip_id(sensor);
+	if (ret)
+		goto entity_cleanup;
+
 	ret = ov5640_init_controls(sensor);
 	if (ret)
 		goto entity_cleanup;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 3/5] media: dt-bindings: ov5640: refine CSI-2 and add parallel interface

[Hugues Fruchet]

Refine CSI-2 endpoint documentation and add bindings
for DVP parallel interface support.

Signed-off-by: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
---
 .../devicetree/bindings/media/i2c/ov5640.txt       | 46 +++++++++++++++++++++-
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
index 540b36c..8e36da0 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
@@ -1,4 +1,4 @@
-* Omnivision OV5640 MIPI CSI-2 sensor
+* Omnivision OV5640 MIPI CSI-2 / parallel sensor
 
 Required Properties:
 - compatible: should be "ovti,ov5640"
@@ -18,7 +18,25 @@ The device node must contain one 'port' child node for its digital output
 video port, in accordance with the video interface bindings defined in
 Documentation/devicetree/bindings/media/video-interfaces.txt.
 
-Example:
+OV5640 can be connected to a MIPI CSI-2 bus or a parallel bus endpoint.
+
+Endpoint node required properties for CSI-2 connection are:
+- remote-endpoint: a phandle to the bus receiver's endpoint node.
+- clock-lanes: should be set to <0> (clock lane on hardware lane 0)
+- data-lanes: should be set to <1> or <1 2> (one or two CSI-2 lanes supported)
+
+Endpoint node required properties for parallel connection are:
+- remote-endpoint: a phandle to the bus receiver's endpoint node.
+- bus-width: shall be set to <8> for 8 bits parallel bus
+	     or <10> for 10 bits parallel bus
+- 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.
+- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
+- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
+	       signal.
+
+Examples:
 
 &i2c1 {
 	ov5640: camera@3c {
@@ -35,6 +53,7 @@ Example:
 		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
 
 		port {
+			/* MIPI CSI-2 bus endpoint */
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_from_ov5640>;
 				clock-lanes = <0>;
@@ -43,3 +62,26 @@ Example:
 		};
 	};
 };
+
+&i2c1 {
+	ov5640: camera@3c {
+		compatible = "ovti,ov5640";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ov5640>;
+		reg = <0x3c>;
+		clocks = <&clk_ext_camera>;
+		clock-names = "xclk";
+
+		port {
+			/* Parallel bus endpoint */
+			ov5640_to_parallel: endpoint {
+				remote-endpoint = <&parallel_from_ov5640>;
+				bus-width = <8>;
+				data-shift = <2>; /* lines 9:2 are used */
+				hsync-active = <0>;
+				vsync-active = <0>;
+				pclk-sample = <1>;
+			};
+		};
+	};
+};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Hugues Fruchet]

Add support of DVP parallel mode in addition of
existing MIPI CSI mode. The choice between two modes
and configuration is made through device tree.

Signed-off-by: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
---
 drivers/media/i2c/ov5640.c | 148 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 130 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 9f031f3..a44b680 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -34,13 +34,19 @@
 
 #define OV5640_DEFAULT_SLAVE_ID 0x3c
 
+#define OV5640_REG_SYS_CTRL0		0x3008
 #define OV5640_REG_CHIP_ID		0x300a
+#define OV5640_REG_IO_MIPI_CTRL00	0x300e
+#define OV5640_REG_PAD_OUTPUT_ENABLE01	0x3017
+#define OV5640_REG_PAD_OUTPUT_ENABLE02	0x3018
 #define OV5640_REG_PAD_OUTPUT00		0x3019
+#define OV5640_REG_SYSTEM_CONTROL1	0x302e
 #define OV5640_REG_SC_PLL_CTRL0		0x3034
 #define OV5640_REG_SC_PLL_CTRL1		0x3035
 #define OV5640_REG_SC_PLL_CTRL2		0x3036
 #define OV5640_REG_SC_PLL_CTRL3		0x3037
 #define OV5640_REG_SLAVE_ID		0x3100
+#define OV5640_REG_SCCB_SYS_CTRL1	0x3103
 #define OV5640_REG_SYS_ROOT_DIVIDER	0x3108
 #define OV5640_REG_AWB_R_GAIN		0x3400
 #define OV5640_REG_AWB_G_GAIN		0x3402
@@ -70,6 +76,7 @@
 #define OV5640_REG_HZ5060_CTRL01	0x3c01
 #define OV5640_REG_SIGMADELTA_CTRL0C	0x3c0c
 #define OV5640_REG_FRAME_CTRL01		0x4202
+#define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
@@ -982,7 +989,111 @@ static int ov5640_get_gain(struct ov5640_dev *sensor)
 	return gain & 0x3ff;
 }
 
-static int ov5640_set_stream(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) {
+		/*
+		 * reset MIPI PCLK/SERCLK divider
+		 *
+		 * SC PLL CONTRL1 0
+		 * - [3..0]:	MIPI PCLK/SERCLK divider
+		 */
+		ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
+		if (ret)
+			return ret;
+
+		/*
+		 * 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);
+}
+
+static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
@@ -1604,17 +1715,19 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		if (ret)
 			goto power_off;
 
-		/*
-		 * start streaming briefly followed by stream off in
-		 * order to coax the clock lane into LP-11 state.
-		 */
-		ret = ov5640_set_stream(sensor, true);
-		if (ret)
-			goto power_off;
-		usleep_range(1000, 2000);
-		ret = ov5640_set_stream(sensor, false);
-		if (ret)
-			goto power_off;
+		if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
+			/*
+			 * start streaming briefly followed by stream off in
+			 * order to coax the clock lane into LP-11 state.
+			 */
+			ret = ov5640_set_stream_mipi(sensor, true);
+			if (ret)
+				goto power_off;
+			usleep_range(1000, 2000);
+			ret = ov5640_set_stream_mipi(sensor, false);
+			if (ret)
+				goto power_off;
+		}
 
 		return 0;
 	}
@@ -2188,7 +2301,11 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 				goto out;
 		}
 
-		ret = ov5640_set_stream(sensor, enable);
+		if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
+			ret = ov5640_set_stream_mipi(sensor, enable);
+		else
+			ret = ov5640_set_stream_dvp(sensor, enable);
+
 		if (!ret)
 			sensor->streaming = enable;
 	}
@@ -2301,11 +2418,6 @@ static int ov5640_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (sensor->ep.bus_type != V4L2_MBUS_CSI2) {
-		dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
-		return -EINVAL;
-	}
-
 	/* get system clock (xclk) */
 	sensor->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(sensor->xclk)) {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 5/5] media: ov5640: add support of RGB565 and YUYV formats

[Hugues Fruchet]

Add RGB565 (LE & BE) and YUV422 YUYV format in addition
to existing YUV422 UYVY format.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 74 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a44b680..f017742 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -76,9 +76,11 @@
 #define OV5640_REG_HZ5060_CTRL01	0x3c01
 #define OV5640_REG_SIGMADELTA_CTRL0C	0x3c0c
 #define OV5640_REG_FRAME_CTRL01		0x4202
+#define OV5640_REG_FORMAT_CONTROL00	0x4300
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
+#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
 #define OV5640_REG_SDE_CTRL0		0x5580
 #define OV5640_REG_SDE_CTRL1		0x5581
@@ -106,6 +108,18 @@ enum ov5640_frame_rate {
 	OV5640_NUM_FRAMERATES,
 };
 
+struct ov5640_pixfmt {
+	u32 code;
+	u32 colorspace;
+};
+
+static const struct ov5640_pixfmt ov5640_formats[] = {
+	{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
+};
+
 /*
  * FIXME: remove this when a subdev API becomes available
  * to set the MIPI CSI-2 virtual channel.
@@ -1837,17 +1851,23 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
 {
 	struct ov5640_dev *sensor = to_ov5640_dev(sd);
 	const struct ov5640_mode_info *mode;
+	int i;
 
 	mode = ov5640_find_mode(sensor, fr, fmt->width, fmt->height, true);
 	if (!mode)
 		return -EINVAL;
-
 	fmt->width = mode->width;
 	fmt->height = mode->height;
-	fmt->code = sensor->fmt.code;
 
 	if (new_mode)
 		*new_mode = mode;
+
+	for (i = 0; i < ARRAY_SIZE(ov5640_formats); i++)
+		if (ov5640_formats[i].code == fmt->code)
+			break;
+	if (i >= ARRAY_SIZE(ov5640_formats))
+		fmt->code = ov5640_formats[0].code;
+
 	return 0;
 }
 
@@ -1890,6 +1910,45 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int ov5640_set_framefmt(struct ov5640_dev *sensor,
+			       struct v4l2_mbus_framefmt *format)
+{
+	int ret = 0;
+	bool is_rgb = false;
+	u8 val;
+
+	switch (format->code) {
+	case MEDIA_BUS_FMT_UYVY8_2X8:
+		/* YUV422, UYVY */
+		val = 0x3f;
+		break;
+	case MEDIA_BUS_FMT_YUYV8_2X8:
+		/* YUV422, YUYV */
+		val = 0x30;
+		break;
+	case MEDIA_BUS_FMT_RGB565_2X8_LE:
+		/* RGB565 {g[2:0],b[4:0]},{r[4:0],g[5:3]} */
+		val = 0x6F;
+		is_rgb = true;
+		break;
+	case MEDIA_BUS_FMT_RGB565_2X8_BE:
+		/* RGB565 {r[4:0],g[5:3]},{g[2:0],b[4:0]} */
+		val = 0x61;
+		is_rgb = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* FORMAT CONTROL00: YUV and RGB formatting */
+	ret = ov5640_write_reg(sensor, OV5640_REG_FORMAT_CONTROL00, val);
+	if (ret)
+		return ret;
+
+	/* FORMAT MUX CONTROL: ISP YUV or RGB */
+	return ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL,
+				is_rgb ? 0x01 : 0x00);
+}
 
 /*
  * Sensor Controls.
@@ -2275,15 +2334,12 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_pad_config *cfg,
 				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	struct ov5640_dev *sensor = to_ov5640_dev(sd);
-
 	if (code->pad != 0)
 		return -EINVAL;
-	if (code->index != 0)
+	if (code->index >= ARRAY_SIZE(ov5640_formats))
 		return -EINVAL;
 
-	code->code = sensor->fmt.code;
-
+	code->code = ov5640_formats[code->index].code;
 	return 0;
 }
 
@@ -2299,6 +2355,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 			ret = ov5640_set_mode(sensor, sensor->current_mode);
 			if (ret)
 				goto out;
+
+			ret = ov5640_set_framefmt(sensor, &sensor->fmt);
+			if (ret)
+				goto out;
 		}
 
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
-- 
1.9.1

Re: [PATCH v5 3/5] media: dt-bindings: ov5640: refine CSI-2 and add parallel interface

[Rob Herring]

On Wed, Jan 03, 2018 at 10:57:30AM +0100, Hugues Fruchet wrote:
> Refine CSI-2 endpoint documentation and add bindings
> for DVP parallel interface support.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  .../devicetree/bindings/media/i2c/ov5640.txt       | 46 +++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 2 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>                 

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Hi Hugues,

On Wed, Jan 03, 2018 at 10:57:27AM +0100, Hugues Fruchet wrote:
> Enhance OV5640 CSI driver to support also DVP parallel interface.
> Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
> YUV422 UYVY format.
> Some other improvements on chip identifier check and removal
> of warnings in powering phase around gpio handling.

I've been trying to use your patches on top of 4.14, but I cannot seem
to get any signal out of it.

What is your test setup and which commands are you running?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues FRUCHET]

Hi Maxime,

I'm using a ST board with OV5640 wired in parallel bus output in order 
to interface to my STM32 DCMI parallel interface.
Perhaps could you describe your setup so I could help on understanding 
the problem on your side. From my past experience with this sensor 
module, you can first check hsync/vsync polarities, the datasheet is 
buggy on VSYNC polarity as documented in patch 4/5.

Best regards,
Hugues.

On 01/08/2018 04:38 PM, Maxime Ripard wrote:
> Hi Hugues,
> 
> On Wed, Jan 03, 2018 at 10:57:27AM +0100, Hugues Fruchet wrote:
>> Enhance OV5640 CSI driver to support also DVP parallel interface.
>> Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
>> YUV422 UYVY format.
>> Some other improvements on chip identifier check and removal
>> of warnings in powering phase around gpio handling.
> 
> I've been trying to use your patches on top of 4.14, but I cannot seem
> to get any signal out of it.
> 
> What is your test setup and which commands are you running?
> 
> Thanks!
> Maxime
> --
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Fabrizio Castro]

Hello Hugues,

thank you for the patch series.
I am having a go with your patches, and although they seem alright, I don't seem to be able to grab a non-black picture on the iWave iwg20d in plain DVP mode, but if I switch to BT656 just by setting register 0x4730 to 0x01 (I know, it's a nasty hack...) I can get something sensible out.

At the moment there is no proper BT656 support in the driver, I was wondering if you have any plans to enhance the ov5640 driver a little bit further to add proper BT656 support as it may be convenient.

Do you know if someone else was able to get DVP to work by means of this patch series on a non-STM32 platform?

Thanks,
Fabrizio


> Subject: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support
>
> Enhance OV5640 CSI driver to support also DVP parallel interface.
> Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
> YUV422 UYVY format.
> Some other improvements on chip identifier check and removal
> of warnings in powering phase around gpio handling.
>
> ===========
> = history =
> ===========
> version 5:
>   - Refine bindings as per Sakari suggestion:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg124048.html
>
> version 4:
>   - Refine bindings as per Sakari suggestion:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg123609.html
>   - Parallel port control lines polarity can now be configured through
>     devicetree
>
> version 3:
>   - Move chip identifier check at probe according to Fabio Estevam comment:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg122575.html
>   - Use 16 bits register read for this check as per Steve Longerbeam comment:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg122692.html
>   - Update bindings to document parallel mode support as per Fabio Estevam comment:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg122576.html
>   - Enable the whole 10 bits parallel output and document 8/10 bits support
>     in ov5640_set_stream_dvp() to answer to Steve Longerbeam comment:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg122693.html
>
> version 2:
>   - Fix comments from Sakari Ailus:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg122259.html
>   - Revisit ov5640_set_stream_dvp() to only configure DVP at streamon
>   - Revisit ov5640_set_stream_dvp() implementation with fewer register settings
>
> version 1:
>   - Initial submission
>
> Hugues Fruchet (5):
>   media: ov5640: switch to gpiod_set_value_cansleep()
>   media: ov5640: check chip id
>   media: dt-bindings: ov5640: refine CSI-2 and add parallel interface
>   media: ov5640: add support of DVP parallel interface
>   media: ov5640: add support of RGB565 and YUYV formats
>
>  .../devicetree/bindings/media/i2c/ov5640.txt       |  46 ++-
>  drivers/media/i2c/ov5640.c                         | 325 ++++++++++++++++++---
>  2 files changed, 324 insertions(+), 47 deletions(-)
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues FRUCHET]

Hi Fabrizio,

Happy to see that this patch series is of interest ;)

As you can see in mail thread, Maxime Ripard is also testing it:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg124322.html

For BT656 support, it was not initially planned but it seems 
straightforward to code it, and anyway I have to add JPEG support, so I 
could add BT656 support in next patch series.

For your symptom, I would say same as I said to Maxime, check first the 
polarity of sync signals (hysnc/vysnc/pclk) between ISP and OV5640.
Note also that several frames are needed to get a non-black picture.
You can also use the colorbar test to validate bus connection between 
ISP and sensor.

Here are the yavta commands that I'm commonly using:

* grab QVGA RGB565 raw frame (note the skip=10 to get a correct image)
yavta -s 320x240 -n 3 --capture=13 --skip=10 --format=RGB565 /dev/video0 
--file=grab-320x240-rgb565-#.raw

* grab QVGA YUV frame
yavta -s 320x240 -n 3 --capture=13 --skip=10 --format=YUYV /dev/video0 
--file=grab-320x240-yuyv-#.raw

* grab QVGA colorbar RGB565 raw frame
yavta -s 320x240 -n 3 -w '0x009f0903 1' --capture=1 --format=RGB565 
/dev/video0 --file=grab-colorbar-320x240-rgb565-#.raw

* disable colorbars
yavta -s 320x240 -n 3 -w '0x009f0903 0' /dev/video0


Hope this will help !

Best regards,
Hugues.

On 01/08/2018 09:54 PM, Fabrizio Castro wrote:
> Hello Hugues,
> 
> thank you for the patch series.
> I am having a go with your patches, and although they seem alright, I don't seem to be able to grab a non-black picture on the iWave iwg20d in plain DVP mode, but if I switch to BT656 just by setting register 0x4730 to 0x01 (I know, it's a nasty hack...) I can get something sensible out.
> 
> At the moment there is no proper BT656 support in the driver, I was wondering if you have any plans to enhance the ov5640 driver a little bit further to add proper BT656 support as it may be convenient.
> 
> Do you know if someone else was able to get DVP to work by means of this patch series on a non-STM32 platform?
> 
> Thanks,
> Fabrizio
> 
> 
>> Subject: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support
>>
>> Enhance OV5640 CSI driver to support also DVP parallel interface.
>> Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
>> YUV422 UYVY format.
>> Some other improvements on chip identifier check and removal
>> of warnings in powering phase around gpio handling.
>>
>> ===========
>> = history =
>> ===========
>> version 5:
>>    - Refine bindings as per Sakari suggestion:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg124048.html
>>
>> version 4:
>>    - Refine bindings as per Sakari suggestion:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg123609.html
>>    - Parallel port control lines polarity can now be configured through
>>      devicetree
>>
>> version 3:
>>    - Move chip identifier check at probe according to Fabio Estevam comment:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122575.html
>>    - Use 16 bits register read for this check as per Steve Longerbeam comment:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122692.html
>>    - Update bindings to document parallel mode support as per Fabio Estevam comment:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122576.html
>>    - Enable the whole 10 bits parallel output and document 8/10 bits support
>>      in ov5640_set_stream_dvp() to answer to Steve Longerbeam comment:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122693.html
>>
>> version 2:
>>    - Fix comments from Sakari Ailus:
>>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122259.html
>>    - Revisit ov5640_set_stream_dvp() to only configure DVP at streamon
>>    - Revisit ov5640_set_stream_dvp() implementation with fewer register settings
>>
>> version 1:
>>    - Initial submission
>>
>> Hugues Fruchet (5):
>>    media: ov5640: switch to gpiod_set_value_cansleep()
>>    media: ov5640: check chip id
>>    media: dt-bindings: ov5640: refine CSI-2 and add parallel interface
>>    media: ov5640: add support of DVP parallel interface
>>    media: ov5640: add support of RGB565 and YUYV formats
>>
>>   .../devicetree/bindings/media/i2c/ov5640.txt       |  46 ++-
>>   drivers/media/i2c/ov5640.c                         | 325 ++++++++++++++++++---
>>   2 files changed, 324 insertions(+), 47 deletions(-)
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> 

RE: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Fabrizio Castro]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5783 bytes --]

Hello Hugues,

thank you for getting back to me, I'll look into your suggestions in the next few days, I'll give you a feedback as soon as I know more.

Thanks,
Fab

> Subject: Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support
>
> Hi Fabrizio,
>
> Happy to see that this patch series is of interest ;)
>
> As you can see in mail thread, Maxime Ripard is also testing it:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg124322.html
>
> For BT656 support, it was not initially planned but it seems
> straightforward to code it, and anyway I have to add JPEG support, so I
> could add BT656 support in next patch series.
>
> For your symptom, I would say same as I said to Maxime, check first the
> polarity of sync signals (hysnc/vysnc/pclk) between ISP and OV5640.
> Note also that several frames are needed to get a non-black picture.
> You can also use the colorbar test to validate bus connection between
> ISP and sensor.
>
> Here are the yavta commands that I'm commonly using:
>
> * grab QVGA RGB565 raw frame (note the skip=10 to get a correct image)
> yavta -s 320x240 -n 3 --capture=13 --skip=10 --format=RGB565 /dev/video0
> --file=grab-320x240-rgb565-#.raw
>
> * grab QVGA YUV frame
> yavta -s 320x240 -n 3 --capture=13 --skip=10 --format=YUYV /dev/video0
> --file=grab-320x240-yuyv-#.raw
>
> * grab QVGA colorbar RGB565 raw frame
> yavta -s 320x240 -n 3 -w '0x009f0903 1' --capture=1 --format=RGB565
> /dev/video0 --file=grab-colorbar-320x240-rgb565-#.raw
>
> * disable colorbars
> yavta -s 320x240 -n 3 -w '0x009f0903 0' /dev/video0
>
>
> Hope this will help !
>
> Best regards,
> Hugues.
>
> On 01/08/2018 09:54 PM, Fabrizio Castro wrote:
> > Hello Hugues,
> >
> > thank you for the patch series.
> > I am having a go with your patches, and although they seem alright, I don't seem to be able to grab a non-black picture on the iWave
> iwg20d in plain DVP mode, but if I switch to BT656 just by setting register 0x4730 to 0x01 (I know, it's a nasty hack...) I can get
> something sensible out.
> >
> > At the moment there is no proper BT656 support in the driver, I was wondering if you have any plans to enhance the ov5640 driver a
> little bit further to add proper BT656 support as it may be convenient.
> >
> > Do you know if someone else was able to get DVP to work by means of this patch series on a non-STM32 platform?
> >
> > Thanks,
> > Fabrizio
> >
> >
> >> Subject: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support
> >>
> >> Enhance OV5640 CSI driver to support also DVP parallel interface.
> >> Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
> >> YUV422 UYVY format.
> >> Some other improvements on chip identifier check and removal
> >> of warnings in powering phase around gpio handling.
> >>
> >> ===========
> >> = history =
> >> ===========
> >> version 5:
> >>    - Refine bindings as per Sakari suggestion:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg124048.html
> >>
> >> version 4:
> >>    - Refine bindings as per Sakari suggestion:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg123609.html
> >>    - Parallel port control lines polarity can now be configured through
> >>      devicetree
> >>
> >> version 3:
> >>    - Move chip identifier check at probe according to Fabio Estevam comment:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122575.html
> >>    - Use 16 bits register read for this check as per Steve Longerbeam comment:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122692.html
> >>    - Update bindings to document parallel mode support as per Fabio Estevam comment:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122576.html
> >>    - Enable the whole 10 bits parallel output and document 8/10 bits support
> >>      in ov5640_set_stream_dvp() to answer to Steve Longerbeam comment:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122693.html
> >>
> >> version 2:
> >>    - Fix comments from Sakari Ailus:
> >>      https://www.mail-archive.com/linux-media@vger.kernel.org/msg122259.html
> >>    - Revisit ov5640_set_stream_dvp() to only configure DVP at streamon
> >>    - Revisit ov5640_set_stream_dvp() implementation with fewer register settings
> >>
> >> version 1:
> >>    - Initial submission
> >>
> >> Hugues Fruchet (5):
> >>    media: ov5640: switch to gpiod_set_value_cansleep()
> >>    media: ov5640: check chip id
> >>    media: dt-bindings: ov5640: refine CSI-2 and add parallel interface
> >>    media: ov5640: add support of DVP parallel interface
> >>    media: ov5640: add support of RGB565 and YUYV formats
> >>
> >>   .../devicetree/bindings/media/i2c/ov5640.txt       |  46 ++-
> >>   drivers/media/i2c/ov5640.c                         | 325 ++++++++++++++++++---
> >>   2 files changed, 324 insertions(+), 47 deletions(-)
> >>
> >> --
> >> 1.9.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> >
> > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England
> & Wales under Registered No. 04586709.
> >



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

Hi Hugues,

On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
> I'm using a ST board with OV5640 wired in parallel bus output in order 
> to interface to my STM32 DCMI parallel interface.
> Perhaps could you describe your setup so I could help on understanding 
> the problem on your side. From my past experience with this sensor 
> module, you can first check hsync/vsync polarities, the datasheet is 
> buggy on VSYNC polarity as documented in patch 4/5.

I'm testing using the driver, using a parallel interface:
https://patchwork.kernel.org/patch/10129463/

The bus is 8-bits wide, and like I was saying, we had a separate
driver for the ov5640 that is working fine with that driver, so the
hardware setup works, and the capture driver should be mostly ok too.

By dumping the registers, I've seen some differences on the clock
setup, I'll try to hack something tomorrow and let you know :)

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

Hi Hugues,

On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
> I'm using a ST board with OV5640 wired in parallel bus output in order 
> to interface to my STM32 DCMI parallel interface.
> Perhaps could you describe your setup so I could help on understanding 
> the problem on your side. From my past experience with this sensor 
> module, you can first check hsync/vsync polarities, the datasheet is 
> buggy on VSYNC polarity as documented in patch 4/5.

It turns out that it was indeed a polarity issue.

It looks like that in order to operate properly, I need to setup the
opposite polarity on HSYNC and VSYNC on the interface. I looked at the
signals under a scope, and VSYNC is obviously inversed as you
described. HSYNC, I'm not so sure since the HBLANK period seems very
long, almost a line.

Since VSYNC at least looks correct, I'd be inclined to think that the
polarity is inversed on at least the SoC I'm using it on.

Yong, did you test the V3S CSI driver with a parallel interface? With
what sensor driver? Have you found some polarities issues like this?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues FRUCHET]

Good news Maxime !

Have you seen that you can adapt the polarities through devicetree ?

+                       /* Parallel bus endpoint */
+                       ov5640_to_parallel: endpoint {
[...]
+                               hsync-active = <0>;
+                               vsync-active = <0>;
+                               pclk-sample = <1>;
+                       };

Doing so you can adapt to your SoC/board setup easily.

If you don't put those lines in devicetree, the ov5640 default init 
sequence is used which set the polarity as defined in below comment:
ov5640_set_stream_dvp()
[...]
+        * 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
+        */
[...]


Best regards,
Hugues.

On 01/10/2018 04:37 PM, Maxime Ripard wrote:
> Hi Hugues,
> 
> On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
>> I'm using a ST board with OV5640 wired in parallel bus output in order
>> to interface to my STM32 DCMI parallel interface.
>> Perhaps could you describe your setup so I could help on understanding
>> the problem on your side. From my past experience with this sensor
>> module, you can first check hsync/vsync polarities, the datasheet is
>> buggy on VSYNC polarity as documented in patch 4/5.
> 
> It turns out that it was indeed a polarity issue.
> 
> It looks like that in order to operate properly, I need to setup the
> opposite polarity on HSYNC and VSYNC on the interface. I looked at the
> signals under a scope, and VSYNC is obviously inversed as you
> described. HSYNC, I'm not so sure since the HBLANK period seems very
> long, almost a line.
> 
> Since VSYNC at least looks correct, I'd be inclined to think that the
> polarity is inversed on at least the SoC I'm using it on.
> 
> Yong, did you test the V3S CSI driver with a parallel interface? With
> what sensor driver? Have you found some polarities issues like this?
> 
> Thanks!
> Maxime
> 

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Sakari Ailus]

Hi Hugues,

On Wed, Jan 10, 2018 at 03:51:07PM +0000, Hugues FRUCHET wrote:
> Good news Maxime !
> 
> Have you seen that you can adapt the polarities through devicetree ?
> 
> +                       /* Parallel bus endpoint */
> +                       ov5640_to_parallel: endpoint {
> [...]
> +                               hsync-active = <0>;
> +                               vsync-active = <0>;
> +                               pclk-sample = <1>;
> +                       };
> 
> Doing so you can adapt to your SoC/board setup easily.
> 
> If you don't put those lines in devicetree, the ov5640 default init 
> sequence is used which set the polarity as defined in below comment:
> ov5640_set_stream_dvp()
> [...]
> +        * 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
> +        */
> [...]

The properties are at the moment documented as mandatory in DT binding
documentation.

-- 
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Yong]

Hi Maxime,

On Wed, 10 Jan 2018 16:37:24 +0100
Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Hi Hugues,
> 
> On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
> > I'm using a ST board with OV5640 wired in parallel bus output in order 
> > to interface to my STM32 DCMI parallel interface.
> > Perhaps could you describe your setup so I could help on understanding 
> > the problem on your side. From my past experience with this sensor 
> > module, you can first check hsync/vsync polarities, the datasheet is 
> > buggy on VSYNC polarity as documented in patch 4/5.
> 
> It turns out that it was indeed a polarity issue.
> 
> It looks like that in order to operate properly, I need to setup the
> opposite polarity on HSYNC and VSYNC on the interface. I looked at the
> signals under a scope, and VSYNC is obviously inversed as you
> described. HSYNC, I'm not so sure since the HBLANK period seems very
> long, almost a line.
> 
> Since VSYNC at least looks correct, I'd be inclined to think that the
> polarity is inversed on at least the SoC I'm using it on.
> 
> Yong, did you test the V3S CSI driver with a parallel interface? With
> what sensor driver? Have you found some polarities issues like this?

Did you try it with Allwinner SoCs?

No. I only tested with a BT1120 signal generated by FPGA or ADV7611. HSYNC
and VSYNC are not used.

For V3s CSI driver, I will add the following to dt-bindings:
Endpoint node properties for CSI1
---------------------------------

- remote-endpoint      : (required) a phandle to the bus receiver's endpoint
                          node
- bus-width:           : (required) must be 8, 10, 12 or 16
- pclk-sample          : (optional) (default: sample on falling edge)
- hsync-active         : (only required for parallel)
- vsync-active         : (only required for parallel)

You could try diffrent hsync-active/vsync-active values here.

> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues FRUCHET]

Hi Sakari,

On 01/10/2018 11:25 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Wed, Jan 10, 2018 at 03:51:07PM +0000, Hugues FRUCHET wrote:
>> Good news Maxime !
>>
>> Have you seen that you can adapt the polarities through devicetree ?
>>
>> +                       /* Parallel bus endpoint */
>> +                       ov5640_to_parallel: endpoint {
>> [...]
>> +                               hsync-active = <0>;
>> +                               vsync-active = <0>;
>> +                               pclk-sample = <1>;
>> +                       };
>>
>> Doing so you can adapt to your SoC/board setup easily.
>>
>> If you don't put those lines in devicetree, the ov5640 default init
>> sequence is used which set the polarity as defined in below comment:
>> ov5640_set_stream_dvp()
>> [...]
>> +        * 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
>> +        */
>> [...]
> 
> The properties are at the moment documented as mandatory in DT binding
> documentation.
> 
of course, it was just to ask Maxime to check the devicetree on its 
side, the symptom observed by Maxime with hsync/vsync inversed is the 
same than the one observed if we stick to just default init sequence.

BR,
Hugues.

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Sakari Ailus]

On Thu, Jan 11, 2018 at 08:12:11AM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
> 
> On 01/10/2018 11:25 PM, Sakari Ailus wrote:
> > Hi Hugues,
> > 
> > On Wed, Jan 10, 2018 at 03:51:07PM +0000, Hugues FRUCHET wrote:
> >> Good news Maxime !
> >>
> >> Have you seen that you can adapt the polarities through devicetree ?
> >>
> >> +                       /* Parallel bus endpoint */
> >> +                       ov5640_to_parallel: endpoint {
> >> [...]
> >> +                               hsync-active = <0>;
> >> +                               vsync-active = <0>;
> >> +                               pclk-sample = <1>;
> >> +                       };
> >>
> >> Doing so you can adapt to your SoC/board setup easily.
> >>
> >> If you don't put those lines in devicetree, the ov5640 default init
> >> sequence is used which set the polarity as defined in below comment:
> >> ov5640_set_stream_dvp()
> >> [...]
> >> +        * 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
> >> +        */
> >> [...]
> > 
> > The properties are at the moment documented as mandatory in DT binding
> > documentation.
> > 
> of course, it was just to ask Maxime to check the devicetree on its 
> side, the symptom observed by Maxime with hsync/vsync inversed is the 
> same than the one observed if we stick to just default init sequence.

I wonder if the driver should be changed to require hsync and vsync. These
signals won't be there at all in Bt.656 mode.

-- 
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues FRUCHET]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2015 bytes --]

On 01/11/2018 09:19 AM, Sakari Ailus wrote:
> On Thu, Jan 11, 2018 at 08:12:11AM +0000, Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>> On 01/10/2018 11:25 PM, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> On Wed, Jan 10, 2018 at 03:51:07PM +0000, Hugues FRUCHET wrote:
>>>> Good news Maxime !
>>>>
>>>> Have you seen that you can adapt the polarities through devicetree ?
>>>>
>>>> +                       /* Parallel bus endpoint */
>>>> +                       ov5640_to_parallel: endpoint {
>>>> [...]
>>>> +                               hsync-active = <0>;
>>>> +                               vsync-active = <0>;
>>>> +                               pclk-sample = <1>;
>>>> +                       };
>>>>
>>>> Doing so you can adapt to your SoC/board setup easily.
>>>>
>>>> If you don't put those lines in devicetree, the ov5640 default init
>>>> sequence is used which set the polarity as defined in below comment:
>>>> ov5640_set_stream_dvp()
>>>> [...]
>>>> +        * 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
>>>> +        */
>>>> [...]
>>>
>>> The properties are at the moment documented as mandatory in DT binding
>>> documentation.
>>>
>> of course, it was just to ask Maxime to check the devicetree on its
>> side, the symptom observed by Maxime with hsync/vsync inversed is the
>> same than the one observed if we stick to just default init sequence.
> 
> I wonder if the driver should be changed to require hsync and vsync. These
> signals won't be there at all in Bt.656 mode.
> 
I will revisit this when pushing Bt.656 mode.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Sakari Ailus]

On Thu, Jan 11, 2018 at 08:25:41AM +0000, Hugues FRUCHET wrote:
> On 01/11/2018 09:19 AM, Sakari Ailus wrote:
> > On Thu, Jan 11, 2018 at 08:12:11AM +0000, Hugues FRUCHET wrote:
> >> Hi Sakari,
> >>
> >> On 01/10/2018 11:25 PM, Sakari Ailus wrote:
> >>> Hi Hugues,
> >>>
> >>> On Wed, Jan 10, 2018 at 03:51:07PM +0000, Hugues FRUCHET wrote:
> >>>> Good news Maxime !
> >>>>
> >>>> Have you seen that you can adapt the polarities through devicetree ?
> >>>>
> >>>> +                       /* Parallel bus endpoint */
> >>>> +                       ov5640_to_parallel: endpoint {
> >>>> [...]
> >>>> +                               hsync-active = <0>;
> >>>> +                               vsync-active = <0>;
> >>>> +                               pclk-sample = <1>;
> >>>> +                       };
> >>>>
> >>>> Doing so you can adapt to your SoC/board setup easily.
> >>>>
> >>>> If you don't put those lines in devicetree, the ov5640 default init
> >>>> sequence is used which set the polarity as defined in below comment:
> >>>> ov5640_set_stream_dvp()
> >>>> [...]
> >>>> +        * 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
> >>>> +        */
> >>>> [...]
> >>>
> >>> The properties are at the moment documented as mandatory in DT binding
> >>> documentation.
> >>>
> >> of course, it was just to ask Maxime to check the devicetree on its
> >> side, the symptom observed by Maxime with hsync/vsync inversed is the
> >> same than the one observed if we stick to just default init sequence.
> > 
> > I wonder if the driver should be changed to require hsync and vsync. These
> > signals won't be there at all in Bt.656 mode.
> > 
> I will revisit this when pushing Bt.656 mode.

That may lead to a situation where DT source which currently intends to use
parallel bus with sync signals will automatically switch to Bt.656. That
could be an issue for the receiver.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

Hi Hugues,

On Wed, Jan 10, 2018 at 03:51:07PM +0000, Hugues FRUCHET wrote:
> Good news Maxime !
> 
> Have you seen that you can adapt the polarities through devicetree ?
> 
> +                       /* Parallel bus endpoint */
> +                       ov5640_to_parallel: endpoint {
> [...]
> +                               hsync-active = <0>;
> +                               vsync-active = <0>;
> +                               pclk-sample = <1>;
> +                       };

It's what I did, with the polarity infos on both side of the
endpoints.
Here it is:
http://code.bulix.org/4vgchd-257344?raw

You can see that the polarity are inverted on both sides of the link,
which seems weird :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

Hi Yong,

On Thu, Jan 11, 2018 at 09:15:08AM +0800, Yong wrote:
> > On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
> > > I'm using a ST board with OV5640 wired in parallel bus output in order 
> > > to interface to my STM32 DCMI parallel interface.
> > > Perhaps could you describe your setup so I could help on understanding 
> > > the problem on your side. From my past experience with this sensor 
> > > module, you can first check hsync/vsync polarities, the datasheet is 
> > > buggy on VSYNC polarity as documented in patch 4/5.
> > 
> > It turns out that it was indeed a polarity issue.
> > 
> > It looks like that in order to operate properly, I need to setup the
> > opposite polarity on HSYNC and VSYNC on the interface. I looked at the
> > signals under a scope, and VSYNC is obviously inversed as you
> > described. HSYNC, I'm not so sure since the HBLANK period seems very
> > long, almost a line.
> > 
> > Since VSYNC at least looks correct, I'd be inclined to think that the
> > polarity is inversed on at least the SoC I'm using it on.
> > 
> > Yong, did you test the V3S CSI driver with a parallel interface? With
> > what sensor driver? Have you found some polarities issues like this?
> 
> Did you try it with Allwinner SoCs?

Yes, on an H3. Looking at all the Allwinner datasheet I could get my
hands on, they are all documented in the same way. However, I really
start to wonder whether the polarity shouldn't be reversed.

At least the fact that VSYNC is clearly active low on the
oscilloscope, while I have to set it active high in the controller
seems like a strong hint :)

> No. I only tested with a BT1120 signal generated by FPGA or ADV7611. HSYNC
> and VSYNC are not used.

Ok, that's good to know :)

> For V3s CSI driver, I will add the following to dt-bindings:
> Endpoint node properties for CSI1
> ---------------------------------
> 
> - remote-endpoint      : (required) a phandle to the bus receiver's endpoint
>                           node
> - bus-width:           : (required) must be 8, 10, 12 or 16
> - pclk-sample          : (optional) (default: sample on falling edge)
> - hsync-active         : (only required for parallel)
> - vsync-active         : (only required for parallel)
> 
> You could try diffrent hsync-active/vsync-active values here.

I did already, and the only combination that works is the one that is
the inversed polarity on HSYNC and VSYNC than what the sensor setup.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Yong]

Hi Maxime,

On Thu, 11 Jan 2018 13:40:18 +0100
Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Hi Yong,
> 
> On Thu, Jan 11, 2018 at 09:15:08AM +0800, Yong wrote:
> > > On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
> > > > I'm using a ST board with OV5640 wired in parallel bus output in order 
> > > > to interface to my STM32 DCMI parallel interface.
> > > > Perhaps could you describe your setup so I could help on understanding 
> > > > the problem on your side. From my past experience with this sensor 
> > > > module, you can first check hsync/vsync polarities, the datasheet is 
> > > > buggy on VSYNC polarity as documented in patch 4/5.
> > > 
> > > It turns out that it was indeed a polarity issue.
> > > 
> > > It looks like that in order to operate properly, I need to setup the
> > > opposite polarity on HSYNC and VSYNC on the interface. I looked at the
> > > signals under a scope, and VSYNC is obviously inversed as you
> > > described. HSYNC, I'm not so sure since the HBLANK period seems very
> > > long, almost a line.
> > > 
> > > Since VSYNC at least looks correct, I'd be inclined to think that the
> > > polarity is inversed on at least the SoC I'm using it on.
> > > 
> > > Yong, did you test the V3S CSI driver with a parallel interface? With
> > > what sensor driver? Have you found some polarities issues like this?
> > 
> > Did you try it with Allwinner SoCs?
> 
> Yes, on an H3. Looking at all the Allwinner datasheet I could get my
> hands on, they are all documented in the same way. However, I really
> start to wonder whether the polarity shouldn't be reversed.
> 
> At least the fact that VSYNC is clearly active low on the
> oscilloscope, while I have to set it active high in the controller
> seems like a strong hint :)

The BSP code of Allwinner also treat V4L2_MBUS_VSYNC_ACTIVE_HIGH as
they documented 'positive'.
Maybe there need some more tests to confirm if the datasheet and BSP
code are both wrong.

> 
> > No. I only tested with a BT1120 signal generated by FPGA or ADV7611. HSYNC
> > and VSYNC are not used.
> 
> Ok, that's good to know :)
> 
> > For V3s CSI driver, I will add the following to dt-bindings:
> > Endpoint node properties for CSI1
> > ---------------------------------
> > 
> > - remote-endpoint      : (required) a phandle to the bus receiver's endpoint
> >                           node
> > - bus-width:           : (required) must be 8, 10, 12 or 16
> > - pclk-sample          : (optional) (default: sample on falling edge)
> > - hsync-active         : (only required for parallel)
> > - vsync-active         : (only required for parallel)
> > 
> > You could try diffrent hsync-active/vsync-active values here.
> 
> I did already, and the only combination that works is the one that is
> the inversed polarity on HSYNC and VSYNC than what the sensor setup.
> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2438 bytes --]

Hi,

On Fri, Jan 12, 2018 at 10:18:39AM +0800, Yong wrote:
> > On Thu, Jan 11, 2018 at 09:15:08AM +0800, Yong wrote:
> > > > On Mon, Jan 08, 2018 at 05:13:39PM +0000, Hugues FRUCHET wrote:
> > > > > I'm using a ST board with OV5640 wired in parallel bus output in order 
> > > > > to interface to my STM32 DCMI parallel interface.
> > > > > Perhaps could you describe your setup so I could help on understanding 
> > > > > the problem on your side. From my past experience with this sensor 
> > > > > module, you can first check hsync/vsync polarities, the datasheet is 
> > > > > buggy on VSYNC polarity as documented in patch 4/5.
> > > > 
> > > > It turns out that it was indeed a polarity issue.
> > > > 
> > > > It looks like that in order to operate properly, I need to setup the
> > > > opposite polarity on HSYNC and VSYNC on the interface. I looked at the
> > > > signals under a scope, and VSYNC is obviously inversed as you
> > > > described. HSYNC, I'm not so sure since the HBLANK period seems very
> > > > long, almost a line.
> > > > 
> > > > Since VSYNC at least looks correct, I'd be inclined to think that the
> > > > polarity is inversed on at least the SoC I'm using it on.
> > > > 
> > > > Yong, did you test the V3S CSI driver with a parallel interface? With
> > > > what sensor driver? Have you found some polarities issues like this?
> > > 
> > > Did you try it with Allwinner SoCs?
> > 
> > Yes, on an H3. Looking at all the Allwinner datasheet I could get my
> > hands on, they are all documented in the same way. However, I really
> > start to wonder whether the polarity shouldn't be reversed.
> > 
> > At least the fact that VSYNC is clearly active low on the
> > oscilloscope, while I have to set it active high in the controller
> > seems like a strong hint :)
> 
> The BSP code of Allwinner also treat V4L2_MBUS_VSYNC_ACTIVE_HIGH as
> they documented 'positive'.
> Maybe there need some more tests to confirm if the datasheet and BSP
> code are both wrong.

Indeed, and at the same time they do the same on the ov5640 driver
they have.

I really think they are in a situation where they report that both the
interface and the sensor are active high, while they are actually
active low, but since both sides still use the same polarity it works.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Fabrizio Castro]

Hello Hugues,

> Subject: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface
>
> Add support of DVP parallel mode in addition of
> existing MIPI CSI mode. The choice between two modes
> and configuration is made through device tree.
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
> ---
>  drivers/media/i2c/ov5640.c | 148 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 130 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 9f031f3..a44b680 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -34,13 +34,19 @@
>
>  #define OV5640_DEFAULT_SLAVE_ID 0x3c
>
> +#define OV5640_REG_SYS_CTRL00x3008
>  #define OV5640_REG_CHIP_ID0x300a
> +#define OV5640_REG_IO_MIPI_CTRL000x300e
> +#define OV5640_REG_PAD_OUTPUT_ENABLE010x3017
> +#define OV5640_REG_PAD_OUTPUT_ENABLE020x3018
>  #define OV5640_REG_PAD_OUTPUT000x3019
> +#define OV5640_REG_SYSTEM_CONTROL10x302e
>  #define OV5640_REG_SC_PLL_CTRL00x3034
>  #define OV5640_REG_SC_PLL_CTRL10x3035
>  #define OV5640_REG_SC_PLL_CTRL20x3036
>  #define OV5640_REG_SC_PLL_CTRL30x3037
>  #define OV5640_REG_SLAVE_ID0x3100
> +#define OV5640_REG_SCCB_SYS_CTRL10x3103
>  #define OV5640_REG_SYS_ROOT_DIVIDER0x3108
>  #define OV5640_REG_AWB_R_GAIN0x3400
>  #define OV5640_REG_AWB_G_GAIN0x3402
> @@ -70,6 +76,7 @@
>  #define OV5640_REG_HZ5060_CTRL010x3c01
>  #define OV5640_REG_SIGMADELTA_CTRL0C0x3c0c
>  #define OV5640_REG_FRAME_CTRL010x4202
> +#define OV5640_REG_POLARITY_CTRL000x4740
>  #define OV5640_REG_MIPI_CTRL000x4800
>  #define OV5640_REG_DEBUG_MODE0x4814
>  #define OV5640_REG_PRE_ISP_TEST_SET10x503d
> @@ -982,7 +989,111 @@ static int ov5640_get_gain(struct ov5640_dev *sensor)
>  return gain & 0x3ff;
>  }
>
> -static int ov5640_set_stream(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) {
> +/*
> + * reset MIPI PCLK/SERCLK divider
> + *
> + * SC PLL CONTRL1 0
> + * - [3..0]:MIPI PCLK/SERCLK divider
> + */
> +ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
> +if (ret)
> +return ret;
> +
> +/*
> + * 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...)

I know that yourself and Maxime have both confirmed that VSYNC polarity is inverted, but I am looking at HSYNC and VSYNC with a logic analyser and I am dumping the values written to OV5640_REG_POLARITY_CTRL00 and to me it looks like that HSYNC is active HIGH when hsync_pol == 0, and VSYNC is active HIGH when vsync_pol == 1.
Between the SoC and the sensor I have a voltage translator, 2.8V input -> 3.3V output, I am measuring the signals at the translator outputs.
Register 0x302A (chip revision register) on my sensor contains 0xb0.

Could you please double check this? How is it possible that this works differently on my sensor?

Thanks,
Fabrizio

> + */
> +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);
> +}
> +
> +static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>  {
>  int ret;
>
> @@ -1604,17 +1715,19 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  if (ret)
>  goto power_off;
>
> -/*
> - * start streaming briefly followed by stream off in
> - * order to coax the clock lane into LP-11 state.
> - */
> -ret = ov5640_set_stream(sensor, true);
> -if (ret)
> -goto power_off;
> -usleep_range(1000, 2000);
> -ret = ov5640_set_stream(sensor, false);
> -if (ret)
> -goto power_off;
> +if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
> +/*
> + * start streaming briefly followed by stream off in
> + * order to coax the clock lane into LP-11 state.
> + */
> +ret = ov5640_set_stream_mipi(sensor, true);
> +if (ret)
> +goto power_off;
> +usleep_range(1000, 2000);
> +ret = ov5640_set_stream_mipi(sensor, false);
> +if (ret)
> +goto power_off;
> +}
>
>  return 0;
>  }
> @@ -2188,7 +2301,11 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  goto out;
>  }
>
> -ret = ov5640_set_stream(sensor, enable);
> +if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
> +ret = ov5640_set_stream_mipi(sensor, enable);
> +else
> +ret = ov5640_set_stream_dvp(sensor, enable);
> +
>  if (!ret)
>  sensor->streaming = enable;
>  }
> @@ -2301,11 +2418,6 @@ static int ov5640_probe(struct i2c_client *client,
>  return ret;
>  }
>
> -if (sensor->ep.bus_type != V4L2_MBUS_CSI2) {
> -dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
> -return -EINVAL;
> -}
> -
>  /* get system clock (xclk) */
>  sensor->xclk = devm_clk_get(dev, "xclk");
>  if (IS_ERR(sensor->xclk)) {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4273 bytes --]

Hi Fabrizio,

On Thu, Feb 01, 2018 at 05:53:18PM +0000, Fabrizio Castro wrote:
> > Subject: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface
> >
> > Add support of DVP parallel mode in addition of
> > existing MIPI CSI mode. The choice between two modes
> > and configuration is made through device tree.
> >
> > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 148 +++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 130 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 9f031f3..a44b680 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -34,13 +34,19 @@
> >
> >  #define OV5640_DEFAULT_SLAVE_ID 0x3c
> >
> > +#define OV5640_REG_SYS_CTRL00x3008
> >  #define OV5640_REG_CHIP_ID0x300a
> > +#define OV5640_REG_IO_MIPI_CTRL000x300e
> > +#define OV5640_REG_PAD_OUTPUT_ENABLE010x3017
> > +#define OV5640_REG_PAD_OUTPUT_ENABLE020x3018
> >  #define OV5640_REG_PAD_OUTPUT000x3019
> > +#define OV5640_REG_SYSTEM_CONTROL10x302e
> >  #define OV5640_REG_SC_PLL_CTRL00x3034
> >  #define OV5640_REG_SC_PLL_CTRL10x3035
> >  #define OV5640_REG_SC_PLL_CTRL20x3036
> >  #define OV5640_REG_SC_PLL_CTRL30x3037
> >  #define OV5640_REG_SLAVE_ID0x3100
> > +#define OV5640_REG_SCCB_SYS_CTRL10x3103
> >  #define OV5640_REG_SYS_ROOT_DIVIDER0x3108
> >  #define OV5640_REG_AWB_R_GAIN0x3400
> >  #define OV5640_REG_AWB_G_GAIN0x3402
> > @@ -70,6 +76,7 @@
> >  #define OV5640_REG_HZ5060_CTRL010x3c01
> >  #define OV5640_REG_SIGMADELTA_CTRL0C0x3c0c
> >  #define OV5640_REG_FRAME_CTRL010x4202
> > +#define OV5640_REG_POLARITY_CTRL000x4740
> >  #define OV5640_REG_MIPI_CTRL000x4800
> >  #define OV5640_REG_DEBUG_MODE0x4814
> >  #define OV5640_REG_PRE_ISP_TEST_SET10x503d
> > @@ -982,7 +989,111 @@ static int ov5640_get_gain(struct ov5640_dev *sensor)
> >  return gain & 0x3ff;
> >  }
> >
> > -static int ov5640_set_stream(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) {
> > +/*
> > + * reset MIPI PCLK/SERCLK divider
> > + *
> > + * SC PLL CONTRL1 0
> > + * - [3..0]:MIPI PCLK/SERCLK divider
> > + */
> > +ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
> > +if (ret)
> > +return ret;
> > +
> > +/*
> > + * 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...)
> 
> I know that yourself and Maxime have both confirmed that VSYNC
> polarity is inverted, but I am looking at HSYNC and VSYNC with a
> logic analyser and I am dumping the values written to
> OV5640_REG_POLARITY_CTRL00 and to me it looks like that HSYNC is
> active HIGH when hsync_pol == 0, and VSYNC is active HIGH when
> vsync_pol == 1.

Which mode are you testing this on?

The non-active periods are insanely high in most modes (1896 for an
active horizontal length of 640 in 640x480 for example), especially
hsync, and it's really easy to invert the two.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Fabrizio Castro]

Hello Maxime,

thank you for your feedback.

> > > +/*
> > > + * 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...)
> >
> > I know that yourself and Maxime have both confirmed that VSYNC
> > polarity is inverted, but I am looking at HSYNC and VSYNC with a
> > logic analyser and I am dumping the values written to
> > OV5640_REG_POLARITY_CTRL00 and to me it looks like that HSYNC is
> > active HIGH when hsync_pol == 0, and VSYNC is active HIGH when
> > vsync_pol == 1.
>
> Which mode are you testing this on?

My testing environment is made of the sensor connected to a SoC with 8 data lines, vsync, hsync, and pclk, and of course I am specifying hsync-active, vsync-active, and pclk-sample in the device tree on both ends so that they configure themselves accordingly to work in DVP mode (V4L2_MBUS_PARALLEL), with the correct polarities.
Between the sensor and the SoC there is a noninverting bus transceiver (voltage translator), for my experiments I have plugged myself onto the outputs of this transceiver only to be compliant with the voltage level of my logic analyser.

>
> The non-active periods are insanely high in most modes (1896 for an
> active horizontal length of 640 in 640x480 for example), especially
> hsync, and it's really easy to invert the two.

I am looking at all the data lines, so that I don't confuse the non-active periods with the active periods, and with my setup what I reported is what I get. I wonder if this difference comes from the sensor revision at all? Unfortunately I can only test the one camera I have got.

Thanks,
Fab

>
> Maxime
>
> --
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> http://bootlin.com



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v5 4/5] media: ov5640: add support of DVP parallel interface

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

On Mon, Feb 05, 2018 at 11:42:11AM +0000, Fabrizio Castro wrote:
> Hello Maxime,
> 
> thank you for your feedback.
> 
> > > > +/*
> > > > + * 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...)
> > >
> > > I know that yourself and Maxime have both confirmed that VSYNC
> > > polarity is inverted, but I am looking at HSYNC and VSYNC with a
> > > logic analyser and I am dumping the values written to
> > > OV5640_REG_POLARITY_CTRL00 and to me it looks like that HSYNC is
> > > active HIGH when hsync_pol == 0, and VSYNC is active HIGH when
> > > vsync_pol == 1.
> >
> > Which mode are you testing this on?
> 
> My testing environment is made of the sensor connected to a SoC with
> 8 data lines, vsync, hsync, and pclk, and of course I am specifying
> hsync-active, vsync-active, and pclk-sample in the device tree on
> both ends so that they configure themselves accordingly to work in
> DVP mode (V4L2_MBUS_PARALLEL), with the correct polarities.
>
> Between the sensor and the SoC there is a noninverting bus
> transceiver (voltage translator), for my experiments I have plugged
> myself onto the outputs of this transceiver only to be compliant
> with the voltage level of my logic analyser.

Sorry, my question was more about the resolution, refresh rate, etc.

> > The non-active periods are insanely high in most modes (1896 for an
> > active horizontal length of 640 in 640x480 for example), especially
> > hsync, and it's really easy to invert the two.
> 
> I am looking at all the data lines, so that I don't confuse the
> non-active periods with the active periods, and with my setup what I
> reported is what I get. I wonder if this difference comes from the
> sensor revision at all? Unfortunately I can only test the one camera
> I have got.

I don't really know then. I've had issues with the polarities on my
side, but it was on the receiver side and the sensor part looked like
what is documented.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]