[PATCH v2 0/4] Add OV5640 parallel interface and RGB565/YUYV support

[PATCH v2 0/4] 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 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 (4):
  media: ov5640: switch to gpiod_set_value_cansleep()
  media: ov5640: check chip id
  media: ov5640: add support of DVP parallel interface
  media: ov5640: add support of RGB565 and YUYV formats

 drivers/media/i2c/ov5640.c | 213 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 183 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] 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@st.com>
---
 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

[PATCH v2 2/4] media: ov5640: check chip id

[Hugues Fruchet]

Verify that chip identifier is correct before starting streaming

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 61071f5..a576d11 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -34,7 +34,8 @@
 
 #define OV5640_DEFAULT_SLAVE_ID 0x3c
 
-#define OV5640_REG_CHIP_ID		0x300a
+#define OV5640_REG_CHIP_ID_HIGH		0x300a
+#define OV5640_REG_CHIP_ID_LOW		0x300b
 #define OV5640_REG_PAD_OUTPUT00		0x3019
 #define OV5640_REG_SC_PLL_CTRL0		0x3034
 #define OV5640_REG_SC_PLL_CTRL1		0x3035
@@ -926,6 +927,29 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
 	return ret;
 }
 
+static int ov5640_check_chip_id(struct ov5640_dev *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
+	u8 chip_id_h, chip_id_l;
+
+	ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_HIGH, &chip_id_h);
+	if (ret)
+		return ret;
+
+	ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_LOW, &chip_id_l);
+	if (ret)
+		return ret;
+
+	if (!(chip_id_h == 0x56 && chip_id_l == 0x40)) {
+		dev_err(&client->dev, "%s: wrong chip identifier, expected 0x5640, got 0x%x%x\n",
+			__func__, chip_id_h, chip_id_l);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* read exposure, in number of line periods */
 static int ov5640_get_exposure(struct ov5640_dev *sensor)
 {
@@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		ov5640_reset(sensor);
 		ov5640_power(sensor, true);
 
+		ret = ov5640_check_chip_id(sensor);
+		if (ret)
+			goto power_off;
+
 		ret = ov5640_init_slave_id(sensor);
 		if (ret)
 			goto power_off;
-- 
1.9.1

[PATCH v2 3/4] 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@st.com>
---
 drivers/media/i2c/ov5640.c | 101 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a576d11..826b102 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -34,14 +34,20 @@
 
 #define OV5640_DEFAULT_SLAVE_ID 0x3c
 
+#define OV5640_REG_SYS_CTRL0		0x3008
 #define OV5640_REG_CHIP_ID_HIGH		0x300a
 #define OV5640_REG_CHIP_ID_LOW		0x300b
+#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
@@ -1006,7 +1012,65 @@ 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;
+
+	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, 0xF, 0);
+		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:2] DVP data lines (D[0:1] are unused with 8 bits
+	 * parallel mode, 8 bits output are mapped on D[9:2])
+	 *
+	 * PAD OUTPUT ENABLE 02
+	 * - [7:4]:	D[5:2] output enable
+	 *		0:1 are unused with 8 bits
+	 *		parallel mode (8 bits output
+	 *		are on D[9:2])
+	 */
+	return ov5640_write_reg(sensor,
+				OV5640_REG_PAD_OUTPUT_ENABLE02, on ? 0xf0 : 0);
+}
+
+static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
@@ -1598,17 +1662,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;
 	}
@@ -2185,7 +2251,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;
 	}
@@ -2270,11 +2340,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

[PATCH v2 4/4] 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 826b102..70239bb 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -77,8 +77,10 @@
 #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_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.
@@ -1787,17 +1801,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;
 }
 
@@ -1840,6 +1860,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.
@@ -2225,15 +2284,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;
 }
 
@@ -2249,6 +2305,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 v2 2/4] media: ov5640: check chip id

[Fabio Estevam]

Hi Hugues,

On Wed, Nov 29, 2017 at 3:11 PM, Hugues Fruchet <hugues.fruchet@st.com> wrote:

>  /* read exposure, in number of line periods */
>  static int ov5640_get_exposure(struct ov5640_dev *sensor)
>  {
> @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>                 ov5640_reset(sensor);
>                 ov5640_power(sensor, true);
>
> +               ret = ov5640_check_chip_id(sensor);
> +               if (ret)
> +                       goto power_off;

Wouldn't it make more sense to add this check in ov5640_probe()
function instead?

Re: [PATCH v2 3/4] media: ov5640: add support of DVP parallel interface

[Fabio Estevam]

Hi Hugues,

On Wed, Nov 29, 2017 at 3:11 PM, Hugues Fruchet <hugues.fruchet@st.com> wrote:
> 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.

What about explaining how to select between the two modes in
Documentation/devicetree/bindings/media/i2c/ov5640.txt ?

Thanks

Re: [PATCH v2 2/4] media: ov5640: check chip id

[Steve Longerbeam]

On 11/29/2017 09:11 AM, Hugues Fruchet wrote:
> Verify that chip identifier is correct before starting streaming
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>   drivers/media/i2c/ov5640.c | 30 +++++++++++++++++++++++++++++-
>   1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 61071f5..a576d11 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -34,7 +34,8 @@
>   
>   #define OV5640_DEFAULT_SLAVE_ID 0x3c
>   
> -#define OV5640_REG_CHIP_ID		0x300a
> +#define OV5640_REG_CHIP_ID_HIGH		0x300a
> +#define OV5640_REG_CHIP_ID_LOW		0x300b

There is no need to separate low and high byte addresses.
See below.

>   #define OV5640_REG_PAD_OUTPUT00		0x3019
>   #define OV5640_REG_SC_PLL_CTRL0		0x3034
>   #define OV5640_REG_SC_PLL_CTRL1		0x3035
> @@ -926,6 +927,29 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>   	return ret;
>   }
>   
> +static int ov5640_check_chip_id(struct ov5640_dev *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +	u8 chip_id_h, chip_id_l;
> +
> +	ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_HIGH, &chip_id_h);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_LOW, &chip_id_l);
> +	if (ret)
> +		return ret;
> +
> +	if (!(chip_id_h == 0x56 && chip_id_l == 0x40)) {
> +		dev_err(&client->dev, "%s: wrong chip identifier, expected 0x5640, got 0x%x%x\n",
> +			__func__, chip_id_h, chip_id_l);
> +		return -EINVAL;
> +	}
> +

This should all be be replaced by:

u16 chip_id;

ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);

etc.

> +	return 0;
> +}
> +
>   /* read exposure, in number of line periods */
>   static int ov5640_get_exposure(struct ov5640_dev *sensor)
>   {
> @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>   		ov5640_reset(sensor);
>   		ov5640_power(sensor, true);
>   
> +		ret = ov5640_check_chip_id(sensor);
> +		if (ret)
> +			goto power_off;
> +
>   		ret = ov5640_init_slave_id(sensor);
>   		if (ret)
>   			goto power_off;

Re: [PATCH v2 3/4] media: ov5640: add support of DVP parallel interface

[Steve Longerbeam]

On 11/29/2017 09:11 AM, Hugues Fruchet wrote:
> 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 | 101 +++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 83 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index a576d11..826b102 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -34,14 +34,20 @@
>   
>   #define OV5640_DEFAULT_SLAVE_ID 0x3c
>   
> +#define OV5640_REG_SYS_CTRL0		0x3008
>   #define OV5640_REG_CHIP_ID_HIGH		0x300a
>   #define OV5640_REG_CHIP_ID_LOW		0x300b
> +#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
> @@ -1006,7 +1012,65 @@ 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;
> +
> +	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, 0xF, 0);
> +		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:2] DVP data lines (D[0:1] are unused with 8 bits
> +	 * parallel mode, 8 bits output are mapped on D[9:2])
> +	 *
> +	 * PAD OUTPUT ENABLE 02
> +	 * - [7:4]:	D[5:2] output enable
> +	 *		0:1 are unused with 8 bits
> +	 *		parallel mode (8 bits output
> +	 *		are on D[9:2])
> +	 */

It should be verified in this driver, at probe, that the device tree
endpoint for the OV5640 output parallel interface has specified this
with "bus-width=<8>; data-shift=<2>;"

Steve

> +	return ov5640_write_reg(sensor,
> +				OV5640_REG_PAD_OUTPUT_ENABLE02, on ? 0xf0 : 0);
> +}
> +
> +static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>   {
>   	int ret;
>   
> @@ -1598,17 +1662,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;
>   	}
> @@ -2185,7 +2251,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;
>   	}
> @@ -2270,11 +2340,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)) {

Re: [PATCH v2 2/4] media: ov5640: check chip id

[Hugues FRUCHET]

Hi Steve,
thanks for review, comments below.

On 12/03/2017 10:34 PM, Steve Longerbeam wrote:
> 
> 
> On 11/29/2017 09:11 AM, Hugues Fruchet wrote:
>> Verify that chip identifier is correct before starting streaming
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/i2c/ov5640.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 61071f5..a576d11 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -34,7 +34,8 @@
>>   #define OV5640_DEFAULT_SLAVE_ID 0x3c
>> -#define OV5640_REG_CHIP_ID        0x300a
>> +#define OV5640_REG_CHIP_ID_HIGH        0x300a
>> +#define OV5640_REG_CHIP_ID_LOW        0x300b
> 
> There is no need to separate low and high byte addresses.
> See below.
> 
>>   #define OV5640_REG_PAD_OUTPUT00        0x3019
>>   #define OV5640_REG_SC_PLL_CTRL0        0x3034
>>   #define OV5640_REG_SC_PLL_CTRL1        0x3035
>> @@ -926,6 +927,29 @@ static int ov5640_load_regs(struct ov5640_dev 
>> *sensor,
>>       return ret;
>>   }
>> +static int ov5640_check_chip_id(struct ov5640_dev *sensor)
>> +{
>> +    struct i2c_client *client = sensor->i2c_client;
>> +    int ret;
>> +    u8 chip_id_h, chip_id_l;
>> +
>> +    ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_HIGH, &chip_id_h);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_LOW, &chip_id_l);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (!(chip_id_h == 0x56 && chip_id_l == 0x40)) {
>> +        dev_err(&client->dev, "%s: wrong chip identifier, expected 
>> 0x5640, got 0x%x%x\n",
>> +            __func__, chip_id_h, chip_id_l);
>> +        return -EINVAL;
>> +    }
>> +
> 
> This should all be be replaced by:
> 
> u16 chip_id;
> 
> ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);
> 
> etc.
> 

Done, thanks.

>> +    return 0;
>> +}
>> +
>>   /* read exposure, in number of line periods */
>>   static int ov5640_get_exposure(struct ov5640_dev *sensor)
>>   {
>> @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev 
>> *sensor, bool on)
>>           ov5640_reset(sensor);
>>           ov5640_power(sensor, true);
>> +        ret = ov5640_check_chip_id(sensor);
>> +        if (ret)
>> +            goto power_off;
>> +
>>           ret = ov5640_init_slave_id(sensor);
>>           if (ret)
>>               goto power_off;
> 

Best regards,
Hugues.

Re: [PATCH v2 3/4] media: ov5640: add support of DVP parallel interface

[Hugues FRUCHET]

Hi Steve,

On 12/03/2017 10:58 PM, Steve Longerbeam wrote:
> 
> 
> On 11/29/2017 09:11 AM, Hugues Fruchet wrote:
>> 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 | 101 
>> +++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 83 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index a576d11..826b102 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -34,14 +34,20 @@
>>   #define OV5640_DEFAULT_SLAVE_ID 0x3c
>> +#define OV5640_REG_SYS_CTRL0        0x3008
>>   #define OV5640_REG_CHIP_ID_HIGH        0x300a
>>   #define OV5640_REG_CHIP_ID_LOW        0x300b
>> +#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
>> @@ -1006,7 +1012,65 @@ 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;
>> +
>> +    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, 0xF, 0);
>> +        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:2] DVP data lines (D[0:1] are unused with 8 bits
>> +     * parallel mode, 8 bits output are mapped on D[9:2])
>> +     *
>> +     * PAD OUTPUT ENABLE 02
>> +     * - [7:4]:    D[5:2] output enable
>> +     *        0:1 are unused with 8 bits
>> +     *        parallel mode (8 bits output
>> +     *        are on D[9:2])
>> +     */
> 
> It should be verified in this driver, at probe, that the device tree
> endpoint for the OV5640 output parallel interface has specified this
> with "bus-width=<8>; data-shift=<2>;"
> 
> Steve
> 

I have changed the code enabling the whole 10 bits:
	/*
	 * 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);

doing so, no need to verify bus-width/data-shift, and sensor is ready 
for 10 bits output.
In addition to this I can do a check at probe verifying that 
bus-width/data-shift are valid, ie 8/2 or 10/0, what do you think about 
this ?


>> +    return ov5640_write_reg(sensor,
>> +                OV5640_REG_PAD_OUTPUT_ENABLE02, on ? 0xf0 : 0);
>> +}
>> +
>> +static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>   {
>>       int ret;
>> @@ -1598,17 +1662,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;
>>       }
>> @@ -2185,7 +2251,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;
>>       }
>> @@ -2270,11 +2340,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)) {
> 

Best regards,
Hugues.

Re: [PATCH v2 2/4] media: ov5640: check chip id

[Hugues FRUCHET]

Thanks Fabio for review,

This make sense, I'll try to change my code that way.

On 11/30/2017 08:07 PM, Fabio Estevam wrote:
> Hi Hugues,
> 
> On Wed, Nov 29, 2017 at 3:11 PM, Hugues Fruchet <hugues.fruchet@st.com> wrote:
> 
>>   /* read exposure, in number of line periods */
>>   static int ov5640_get_exposure(struct ov5640_dev *sensor)
>>   {
>> @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>>                  ov5640_reset(sensor);
>>                  ov5640_power(sensor, true);
>>
>> +               ret = ov5640_check_chip_id(sensor);
>> +               if (ret)
>> +                       goto power_off;
> 
> Wouldn't it make more sense to add this check in ov5640_probe()
> function instead?
> 

Best regards,
Hugues.

Re: [PATCH v2 3/4] media: ov5640: add support of DVP parallel interface

[Hugues FRUCHET]

Hi Fabio,

I will do.

On 11/30/2017 08:09 PM, Fabio Estevam wrote:
> Hi Hugues,
> 
> On Wed, Nov 29, 2017 at 3:11 PM, Hugues Fruchet <hugues.fruchet@st.com> wrote:
>> 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.
> 
> What about explaining how to select between the two modes in
> Documentation/devicetree/bindings/media/i2c/ov5640.txt ?
> 
> Thanks
> 

Best regards,
Hugues.