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

[PATCH v1 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.

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 | 229 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 198 insertions(+), 31 deletions(-)

-- 
1.9.1

[PATCH v1 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 v1 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 v1 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 | 112 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 94 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a576d11..fb519ad 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,71 @@ 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)
+{
+	int ret;
+
+	/*
+	 * MIPI CONTROL 00
+	 * 6:mipi lane debug
+	 * 4:PWDN PHY TX
+	 * 3:PWDN PHY RX
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
+	if (ret)
+		return ret;
+
+	/* SYSTEM CONTROL, not recommended... */
+	ret = ov5640_write_reg(sensor, OV5640_REG_SYSTEM_CONTROL1, 0x00);
+	if (ret)
+		return ret;
+
+	/* SCCB SYSTEM CTRL1 1:system input clk from PLL, 0:debug enable */
+	ret = ov5640_write_reg(sensor, OV5640_REG_SCCB_SYS_CTRL1, 0x03);
+	if (ret)
+		return ret;
+
+	/* SYSTEM CTRL0 1:debug enable */
+	ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, 0x02);
+	if (ret)
+		return ret;
+
+	/*
+	 * SC PLL CONTRL1 0
+	 * - [7..4]:	System clock divider = 4
+	 * - [3..0]:	MIPI PCLK/SERCLK divider = 1
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x41);
+	if (ret)
+		return ret;
+
+	/* OV5640_REG_SC_PLL_CTRL2 [7:0]: PLL multiplier (4~252) = 0x60 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0x60);
+	if (ret)
+		return ret;
+
+	/*
+	 * PAD OUTPUT ENABLE 01:
+	 * - 6:		VSYNC output enable
+	 * - 5:		HREF output enable
+	 * - 4:		PCLK output enable
+	 * - [3:0]:	D[9:6] output enable
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+	if (ret)
+		return ret;
+
+	/*
+	 * 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, 0xf0);
+}
+
+static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
@@ -1598,17 +1668,24 @@ 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;
+		} else {
+			ret = ov5640_set_stream_dvp(sensor);
+			if (ret)
+				goto power_off;
+			usleep_range(40000, 60000);
+		}
 
 		return 0;
 	}
@@ -2185,7 +2262,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);
+
 		if (!ret)
 			sensor->streaming = enable;
 	}
@@ -2270,11 +2351,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

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

[Sakari Ailus]

Hi Hugues,

On Thu, Nov 16, 2017 at 02:41:41PM +0100, Hugues Fruchet wrote:
> @@ -2185,7 +2262,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);

Hmm. Do you want to configure it even when you're disabling streaming?

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

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

[Hugues FRUCHET]

Thanks Sakari for review,

On 11/24/2017 03:06 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Thu, Nov 16, 2017 at 02:41:41PM +0100, Hugues Fruchet wrote:
>> @@ -2185,7 +2262,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);
> 
> Hmm. Do you want to configure it even when you're disabling streaming?
> 
I will fix this by only setting stream when enabling streaming.

Best regards,
Hugues.

[PATCH v1 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 | 79 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index fb519ad..086a451 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.
@@ -1798,17 +1812,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;
 }
 
@@ -1851,6 +1871,49 @@ 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 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL,
+			       is_rgb ? 0x01 : 0x00);
+	if (ret)
+		return ret;
+
+	return ret;
+}
 
 /*
  * Sensor Controls.
@@ -2236,15 +2299,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;
 }
 
@@ -2254,12 +2314,15 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret = 0;
 
 	mutex_lock(&sensor->lock);
-
 	if (sensor->streaming == !enable) {
 		if (enable && sensor->pending_mode_change) {
 			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 v1 4/4] media: ov5640: add support of RGB565 and YUYV formats

[Sakari Ailus]

Hi Hugues,

On Thu, Nov 16, 2017 at 02:41:42PM +0100, Hugues Fruchet wrote:
> 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 | 79 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 71 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index fb519ad..086a451 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.
> @@ -1798,17 +1812,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;
>  }
>  
> @@ -1851,6 +1871,49 @@ 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 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL,
> +			       is_rgb ? 0x01 : 0x00);

return ov5640...;

> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
>  
>  /*
>   * Sensor Controls.
> @@ -2236,15 +2299,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;
>  }
>  
> @@ -2254,12 +2314,15 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret = 0;
>  
>  	mutex_lock(&sensor->lock);
> -

I rather liked this newline!

>  	if (sensor->streaming == !enable) {
>  		if (enable && sensor->pending_mode_change) {
>  			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)

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

Re: [PATCH v1 4/4] media: ov5640: add support of RGB565 and YUYV formats

[Hugues FRUCHET]

Thanks for review Sakari,

On 11/24/2017 03:09 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Thu, Nov 16, 2017 at 02:41:42PM +0100, Hugues Fruchet wrote:
>> 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 | 79 +++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 71 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index fb519ad..086a451 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.
>> @@ -1798,17 +1812,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;
>>   }
>>   
>> @@ -1851,6 +1871,49 @@ 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 */
>> +	ret = ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL,
>> +			       is_rgb ? 0x01 : 0x00);
> 
> return ov5640...;
> 
OK.

>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
>> +}
>>   
>>   /*
>>    * Sensor Controls.
>> @@ -2236,15 +2299,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;
>>   }
>>   
>> @@ -2254,12 +2314,15 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>>   	int ret = 0;
>>   
>>   	mutex_lock(&sensor->lock);
>> -
> 
> I rather liked this newline!
> 
OK.

>>   	if (sensor->streaming == !enable) {
>>   		if (enable && sensor->pending_mode_change) {
>>   			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)
>