linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] media: ov5640: JPEG and test pattern improvements
@ 2019-01-18  8:52 Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 1/6] media: ov5640: Move test_pattern_menu before ov5640_set_ctrl_test_pattern Chen-Yu Tsai
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

Hi everyone,

This series improves the JPEG compression output mode handling of the
OV5640 sensor, and adds some more test patterns for users to use.

The JPEG compression output mode of the sensor works, but the data
framing is incorrect. Additional registers need to be set to configure
the framing / timing of the data. This was found when adding support for
JPEG_1X8 for the sun6i CSI sensor interface driver (improvements which
I'll send in a separate series). Because the default timings might not
be the same as the resolution timings, the capture device would either
truncate or pad each line as it is captured. This corrupts the JPEG
data. With the timings fixed, the captured data is once again a
continuous JPEG file.

While using the test pattern for testing, I found that the test pattern
was superimposed on top of an actual captured image. This made it
slightly harder to compare results, as the underlying image would change
with scenery and static from the sensor. Disabling the transparency
feature makes the result a "true" test pattern.

I also added the "rolling bar" variant of the color bar test pattern,
which is much better for testing video captures, as the image changes
in a predictable way, instead of being static.

Finally, another test pattern, "color squares", along with a rolling bar
variant, were added as an alternative.

Patch 1 is some simple code movement, in preparation for the next patch.

Patch 2 adds definitions for the test pattern configuration register, to
be used in the next patch.

Patch 3 disables the transparency feature of the test pattern generator.

Patch 4 adds more test pattern options.

Patch 5 fixes the JPEG compression output data framing.

Patch 6 consolidates the JPEG compression mode setting in one place,
instead of being listed in the register dumps.

Please have a look.


Regards
ChenYu


Chen-Yu Tsai (6):
  media: ov5640: Move test_pattern_menu before
    ov5640_set_ctrl_test_pattern
  media: ov5640: Add register definition for test pattern register
  media: ov5640: Disable transparent feature for test pattern
  media: ov5640: Add three more test patterns
  media: ov5640: Set JPEG output timings when outputting JPEG data
  media: ov5640: Consolidate JPEG compression mode setting

 drivers/media/i2c/ov5640.c | 95 ++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 18 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/6] media: ov5640: Move test_pattern_menu before ov5640_set_ctrl_test_pattern
  2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
@ 2019-01-18  8:52 ` Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 2/6] media: ov5640: Add register definition for test pattern register Chen-Yu Tsai
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

The OV5640 has many options for generating test patterns. Unfortunately
there is only one V4L2 control for it. Thus the driver would need to
list some or all combinations.

Move the test_pattern_menu list before the ov5640_set_ctrl_test_pattern
function that programs the hardware. This would allow us to add a
matching list of values to program into the hardware, while keeping the
two lists together for ease of maintenance.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/i2c/ov5640.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 5a909abd0a2d..8e4e8fa3685f 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2441,6 +2441,11 @@ static int ov5640_set_ctrl_gain(struct ov5640_dev *sensor, bool auto_gain)
 	return ret;
 }
 
+static const char * const test_pattern_menu[] = {
+	"Disabled",
+	"Color bars",
+};
+
 static int ov5640_set_ctrl_test_pattern(struct ov5640_dev *sensor, int value)
 {
 	return ov5640_mod_reg(sensor, OV5640_REG_PRE_ISP_TEST_SET1,
@@ -2585,11 +2590,6 @@ static const struct v4l2_ctrl_ops ov5640_ctrl_ops = {
 	.s_ctrl = ov5640_s_ctrl,
 };
 
-static const char * const test_pattern_menu[] = {
-	"Disabled",
-	"Color bars",
-};
-
 static int ov5640_init_controls(struct ov5640_dev *sensor)
 {
 	const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/6] media: ov5640: Add register definition for test pattern register
  2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 1/6] media: ov5640: Move test_pattern_menu before ov5640_set_ctrl_test_pattern Chen-Yu Tsai
@ 2019-01-18  8:52 ` Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 3/6] media: ov5640: Disable transparent feature for test pattern Chen-Yu Tsai
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

The OV5640 can generate many types of test patterns, some with
additional modifiers, such as a rolling bar, or gamma gradients.

Add the bit definitions for all bits in the test pattern register,
and use them to compose the values to be written to the register.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/i2c/ov5640.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8e4e8fa3685f..22d07b3cc8a2 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2446,10 +2446,30 @@ static const char * const test_pattern_menu[] = {
 	"Color bars",
 };
 
+#define OV5640_TEST_ENABLE		BIT(7)
+#define OV5640_TEST_ROLLING		BIT(6)	/* rolling horizontal bar */
+#define OV5640_TEST_TRANSPARENT		BIT(5)
+#define OV5640_TEST_SQUARE_BW		BIT(4)	/* black & white squares */
+#define OV5640_TEST_BAR_STANDARD	(0 << 2)
+#define OV5640_TEST_BAR_VERT_CHANGE_1	(1 << 2)
+#define OV5640_TEST_BAR_HOR_CHANGE	(2 << 2)
+#define OV5640_TEST_BAR_VERT_CHANGE_2	(3 << 2)
+#define OV5640_TEST_BAR			(0 << 0)
+#define OV5640_TEST_RANDOM		(1 << 0)
+#define OV5640_TEST_SQUARE		(2 << 0)
+#define OV5640_TEST_BLACK		(3 << 0)
+
+static const u8 test_pattern_val[] = {
+	0,
+	OV5640_TEST_ENABLE | OV5640_TEST_TRANSPARENT |
+		OV5640_TEST_BAR_VERT_CHANGE_1 |
+		OV5640_TEST_BAR,
+};
+
 static int ov5640_set_ctrl_test_pattern(struct ov5640_dev *sensor, int value)
 {
-	return ov5640_mod_reg(sensor, OV5640_REG_PRE_ISP_TEST_SET1,
-			      0xa4, value ? 0xa4 : 0);
+	return ov5640_write_reg(sensor, OV5640_REG_PRE_ISP_TEST_SET1,
+				test_pattern_val[value]);
 }
 
 static int ov5640_set_ctrl_light_freq(struct ov5640_dev *sensor, int value)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/6] media: ov5640: Disable transparent feature for test pattern
  2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 1/6] media: ov5640: Move test_pattern_menu before ov5640_set_ctrl_test_pattern Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 2/6] media: ov5640: Add register definition for test pattern register Chen-Yu Tsai
@ 2019-01-18  8:52 ` Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 4/6] media: ov5640: Add three more test patterns Chen-Yu Tsai
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

The transparent feature for test patterns blends the test pattern with
an actual captured image. This makes the result non-static, subject to
changes in the sensor's field of view.

Test patterns should be predictable and deterministic, even if they are
dynamic patterns. Disable the transparent feature of the test pattern.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/i2c/ov5640.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 22d07b3cc8a2..a1fd69a21df1 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2461,8 +2461,7 @@ static const char * const test_pattern_menu[] = {
 
 static const u8 test_pattern_val[] = {
 	0,
-	OV5640_TEST_ENABLE | OV5640_TEST_TRANSPARENT |
-		OV5640_TEST_BAR_VERT_CHANGE_1 |
+	OV5640_TEST_ENABLE | OV5640_TEST_BAR_VERT_CHANGE_1 |
 		OV5640_TEST_BAR,
 };
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/6] media: ov5640: Add three more test patterns
  2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
                   ` (2 preceding siblings ...)
  2019-01-18  8:52 ` [PATCH 3/6] media: ov5640: Disable transparent feature for test pattern Chen-Yu Tsai
@ 2019-01-18  8:52 ` Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 5/6] media: ov5640: Set JPEG output timings when outputting JPEG data Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting Chen-Yu Tsai
  5 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

The OV5640 driver currently supports a static color bar pattern with a
small vertical gamma gradient. The hardware also supports a color square
pattern, as well as having a rolling bar for dynamic sequences.

Add three more test patterns:

  - color bars with a rolling bar (but without the gamma gradient)
  - static color squares
  - color squares with a rolling bar

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/i2c/ov5640.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a1fd69a21df1..13311483792c 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2444,6 +2444,9 @@ static int ov5640_set_ctrl_gain(struct ov5640_dev *sensor, bool auto_gain)
 static const char * const test_pattern_menu[] = {
 	"Disabled",
 	"Color bars",
+	"Color bars w/ rolling bar",
+	"Color squares",
+	"Color squares w/ rolling bar",
 };
 
 #define OV5640_TEST_ENABLE		BIT(7)
@@ -2463,6 +2466,10 @@ static const u8 test_pattern_val[] = {
 	0,
 	OV5640_TEST_ENABLE | OV5640_TEST_BAR_VERT_CHANGE_1 |
 		OV5640_TEST_BAR,
+	OV5640_TEST_ENABLE | OV5640_TEST_ROLLING |
+		OV5640_TEST_BAR_VERT_CHANGE_1 | OV5640_TEST_BAR,
+	OV5640_TEST_ENABLE | OV5640_TEST_SQUARE,
+	OV5640_TEST_ENABLE | OV5640_TEST_ROLLING | OV5640_TEST_SQUARE,
 };
 
 static int ov5640_set_ctrl_test_pattern(struct ov5640_dev *sensor, int value)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/6] media: ov5640: Set JPEG output timings when outputting JPEG data
  2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
                   ` (3 preceding siblings ...)
  2019-01-18  8:52 ` [PATCH 4/6] media: ov5640: Add three more test patterns Chen-Yu Tsai
@ 2019-01-18  8:52 ` Chen-Yu Tsai
  2019-01-18  8:52 ` [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting Chen-Yu Tsai
  5 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

When compression is turned on, the on-bus data is framed according to
the compression mode, and the height and width set in VFIFO_VSIZE and
VFIFO_HSIZE. If these are not updated correctly, the sensor will send
data framed in a manner unexpected by the capture interface, such as
having more bytes per line than expected, and having the extra data
dropped. This ultimately results in corrupted data.

Set the two values when the media bus is configured for JPEG data,
meaning the sensor would be in JPEG mode.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/i2c/ov5640.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 13311483792c..1c1dc401c678 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -83,6 +83,8 @@
 #define OV5640_REG_SIGMADELTA_CTRL0C	0x3c0c
 #define OV5640_REG_FRAME_CTRL01		0x4202
 #define OV5640_REG_FORMAT_CONTROL00	0x4300
+#define OV5640_REG_VFIFO_HSIZE		0x4602
+#define OV5640_REG_VFIFO_VSIZE		0x4604
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
@@ -1043,12 +1045,31 @@ static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
 			      (ilog2(pclk_div) << 4));
 }
 
+/* set JPEG framing sizes */
+static int ov5640_set_jpeg_timings(struct ov5640_dev *sensor,
+				   const struct ov5640_mode_info *mode)
+{
+	int ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_VFIFO_HSIZE, mode->hact);
+	if (ret < 0)
+		return ret;
+
+	return ov5640_write_reg16(sensor, OV5640_REG_VFIFO_VSIZE, mode->vact);
+}
+
 /* download ov5640 settings to sensor through i2c */
 static int ov5640_set_timings(struct ov5640_dev *sensor,
 			      const struct ov5640_mode_info *mode)
 {
 	int ret;
 
+	if (sensor->fmt.code == MEDIA_BUS_FMT_JPEG_1X8) {
+		ret = ov5640_set_jpeg_timings(sensor, mode);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
 	if (ret < 0)
 		return ret;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting
  2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
                   ` (4 preceding siblings ...)
  2019-01-18  8:52 ` [PATCH 5/6] media: ov5640: Set JPEG output timings when outputting JPEG data Chen-Yu Tsai
@ 2019-01-18  8:52 ` Chen-Yu Tsai
  2019-02-05  8:55   ` Sakari Ailus
  5 siblings, 1 reply; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-01-18  8:52 UTC (permalink / raw)
  To: Steve Longerbeam, Mauro Carvalho Chehab, Sakari Ailus
  Cc: Chen-Yu Tsai, linux-media, linux-kernel, Maxime Ripard

The register value lists for all the supported resolution settings all
include a register address/value pair for setting the JPEG compression
mode. With the exception of 1080p (which sets mode 2), all resolutions
use mode 3.

The only difference between mode 2 and mode 3 is that mode 2 may have
padding data on the last line, while mode 3 does not add padding data.

As these register values were from dumps of running systems, and the
difference between the modes is quite small, using mode 3 for all
configurations should be OK.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/i2c/ov5640.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1c1dc401c678..3d2c5de73283 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -85,6 +85,7 @@
 #define OV5640_REG_FORMAT_CONTROL00	0x4300
 #define OV5640_REG_VFIFO_HSIZE		0x4602
 #define OV5640_REG_VFIFO_VSIZE		0x4604
+#define OV5640_REG_JPG_MODE_SELECT	0x4713
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
@@ -303,7 +304,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
 	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
 	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
-	{0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
+	{0x501f, 0x00, 0, 0}, {0x4407, 0x04, 0, 0},
 	{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
 	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
@@ -372,7 +373,7 @@ static const struct reg_value ov5640_setting_VGA_640_480[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
@@ -391,7 +392,7 @@ static const struct reg_value ov5640_setting_XGA_1024_768[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
@@ -410,7 +411,7 @@ static const struct reg_value ov5640_setting_QVGA_320_240[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
@@ -429,7 +430,7 @@ static const struct reg_value ov5640_setting_QCIF_176_144[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
@@ -448,7 +449,7 @@ static const struct reg_value ov5640_setting_NTSC_720_480[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
@@ -467,7 +468,7 @@ static const struct reg_value ov5640_setting_PAL_720_576[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
@@ -486,7 +487,7 @@ static const struct reg_value ov5640_setting_720P_1280_720[] = {
 	{0x3a03, 0xe4, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0xbc, 0, 0},
 	{0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x72, 0, 0}, {0x3a0e, 0x01, 0, 0},
 	{0x3a0d, 0x02, 0, 0}, {0x3a14, 0x02, 0, 0}, {0x3a15, 0xe4, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0},
 	{0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0},
 };
@@ -506,7 +507,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
 	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
@@ -518,7 +519,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
 	{0x3a02, 0x04, 0, 0}, {0x3a03, 0x60, 0, 0}, {0x3a08, 0x01, 0, 0},
 	{0x3a09, 0x50, 0, 0}, {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x18, 0, 0},
 	{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
-	{0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 0, 0}, {0x4407, 0x04, 0, 0},
+	{0x3a15, 0x60, 0, 0}, {0x4407, 0x04, 0, 0},
 	{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
 	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
 };
@@ -537,7 +538,7 @@ static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
 	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
+	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 70},
 };
@@ -1051,6 +1052,17 @@ static int ov5640_set_jpeg_timings(struct ov5640_dev *sensor,
 {
 	int ret;
 
+	/*
+	 * compression mode 3 timing
+	 *
+	 * Data is transmitted with programmable width (VFIFO_HSIZE).
+	 * No padding done. Last line may have less data. Varying
+	 * number of lines per frame, depending on amount of data.
+	 */
+	ret = ov5640_mod_reg(sensor, OV5640_REG_JPEG_MODE_SELECT, 0x7, 0x3);
+	if (ret < 0)
+		return ret;
+
 	ret = ov5640_write_reg16(sensor, OV5640_REG_VFIFO_HSIZE, mode->hact);
 	if (ret < 0)
 		return ret;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting
  2019-01-18  8:52 ` [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting Chen-Yu Tsai
@ 2019-02-05  8:55   ` Sakari Ailus
  2019-02-05 13:50     ` Chen-Yu Tsai
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2019-02-05  8:55 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Steve Longerbeam, Mauro Carvalho Chehab, linux-media,
	linux-kernel, Maxime Ripard

Hi Chen-Yu,

On Fri, Jan 18, 2019 at 04:52:06PM +0800, Chen-Yu Tsai wrote:
> The register value lists for all the supported resolution settings all
> include a register address/value pair for setting the JPEG compression
> mode. With the exception of 1080p (which sets mode 2), all resolutions
> use mode 3.
> 
> The only difference between mode 2 and mode 3 is that mode 2 may have
> padding data on the last line, while mode 3 does not add padding data.
> 
> As these register values were from dumps of running systems, and the
> difference between the modes is quite small, using mode 3 for all
> configurations should be OK.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/media/i2c/ov5640.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 1c1dc401c678..3d2c5de73283 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -85,6 +85,7 @@
>  #define OV5640_REG_FORMAT_CONTROL00	0x4300
>  #define OV5640_REG_VFIFO_HSIZE		0x4602
>  #define OV5640_REG_VFIFO_VSIZE		0x4604
> +#define OV5640_REG_JPG_MODE_SELECT	0x4713

How has this been tested?

The register is referred to as "OV5640_REG_JPEG_MODE_SELECT" below. I can
fix it if it's just a typo, but please confirm.

Thanks.

>  #define OV5640_REG_POLARITY_CTRL00	0x4740
>  #define OV5640_REG_MIPI_CTRL00		0x4800
>  #define OV5640_REG_DEBUG_MODE		0x4814
> @@ -303,7 +304,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
>  	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
>  	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
>  	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> -	{0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
> +	{0x501f, 0x00, 0, 0}, {0x4407, 0x04, 0, 0},
>  	{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
>  	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
> @@ -372,7 +373,7 @@ static const struct reg_value ov5640_setting_VGA_640_480[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
> @@ -391,7 +392,7 @@ static const struct reg_value ov5640_setting_XGA_1024_768[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
> @@ -410,7 +411,7 @@ static const struct reg_value ov5640_setting_QVGA_320_240[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
> @@ -429,7 +430,7 @@ static const struct reg_value ov5640_setting_QCIF_176_144[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
> @@ -448,7 +449,7 @@ static const struct reg_value ov5640_setting_NTSC_720_480[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
> @@ -467,7 +468,7 @@ static const struct reg_value ov5640_setting_PAL_720_576[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
> @@ -486,7 +487,7 @@ static const struct reg_value ov5640_setting_720P_1280_720[] = {
>  	{0x3a03, 0xe4, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0xbc, 0, 0},
>  	{0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x72, 0, 0}, {0x3a0e, 0x01, 0, 0},
>  	{0x3a0d, 0x02, 0, 0}, {0x3a14, 0x02, 0, 0}, {0x3a15, 0xe4, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0},
>  	{0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0},
>  };
> @@ -506,7 +507,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
>  	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
> @@ -518,7 +519,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
>  	{0x3a02, 0x04, 0, 0}, {0x3a03, 0x60, 0, 0}, {0x3a08, 0x01, 0, 0},
>  	{0x3a09, 0x50, 0, 0}, {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x18, 0, 0},
>  	{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
> -	{0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 0, 0}, {0x4407, 0x04, 0, 0},
> +	{0x3a15, 0x60, 0, 0}, {0x4407, 0x04, 0, 0},
>  	{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
>  	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
>  };
> @@ -537,7 +538,7 @@ static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
>  	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> -	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
> +	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0},
>  	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 70},
>  };
> @@ -1051,6 +1052,17 @@ static int ov5640_set_jpeg_timings(struct ov5640_dev *sensor,
>  {
>  	int ret;
>  
> +	/*
> +	 * compression mode 3 timing
> +	 *
> +	 * Data is transmitted with programmable width (VFIFO_HSIZE).
> +	 * No padding done. Last line may have less data. Varying
> +	 * number of lines per frame, depending on amount of data.
> +	 */
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_JPEG_MODE_SELECT, 0x7, 0x3);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = ov5640_write_reg16(sensor, OV5640_REG_VFIFO_HSIZE, mode->hact);
>  	if (ret < 0)
>  		return ret;

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting
  2019-02-05  8:55   ` Sakari Ailus
@ 2019-02-05 13:50     ` Chen-Yu Tsai
  2019-02-05 16:08       ` Sakari Ailus
  0 siblings, 1 reply; 10+ messages in thread
From: Chen-Yu Tsai @ 2019-02-05 13:50 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Steve Longerbeam, Mauro Carvalho Chehab,
	Linux Media Mailing List, linux-kernel, Maxime Ripard

On Tue, Feb 5, 2019 at 4:55 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Chen-Yu,
>
> On Fri, Jan 18, 2019 at 04:52:06PM +0800, Chen-Yu Tsai wrote:
> > The register value lists for all the supported resolution settings all
> > include a register address/value pair for setting the JPEG compression
> > mode. With the exception of 1080p (which sets mode 2), all resolutions
> > use mode 3.
> >
> > The only difference between mode 2 and mode 3 is that mode 2 may have
> > padding data on the last line, while mode 3 does not add padding data.
> >
> > As these register values were from dumps of running systems, and the
> > difference between the modes is quite small, using mode 3 for all
> > configurations should be OK.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >  drivers/media/i2c/ov5640.c | 34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 1c1dc401c678..3d2c5de73283 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -85,6 +85,7 @@
> >  #define OV5640_REG_FORMAT_CONTROL00  0x4300
> >  #define OV5640_REG_VFIFO_HSIZE               0x4602
> >  #define OV5640_REG_VFIFO_VSIZE               0x4604
> > +#define OV5640_REG_JPG_MODE_SELECT   0x4713
>
> How has this been tested?
>
> The register is referred to as "OV5640_REG_JPEG_MODE_SELECT" below. I can
> fix it if it's just a typo, but please confirm.

It's a typo. The datasheet uses the abbreviated form, JPG_MODE_SELECT,
but all the bitfield names are the full JPEG form. I believe I missed
the other occurrence while fixing up the names to match the datasheet.
I appologize for not doing a final compile test.

Thanks
ChenYu

> Thanks.
>
> >  #define OV5640_REG_POLARITY_CTRL00   0x4740
> >  #define OV5640_REG_MIPI_CTRL00               0x4800
> >  #define OV5640_REG_DEBUG_MODE                0x4814
> > @@ -303,7 +304,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> >       {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
> >       {0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
> >       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > -     {0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
> > +     {0x501f, 0x00, 0, 0}, {0x4407, 0x04, 0, 0},
> >       {0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
> >       {0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
> > @@ -372,7 +373,7 @@ static const struct reg_value ov5640_setting_VGA_640_480[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> > @@ -391,7 +392,7 @@ static const struct reg_value ov5640_setting_XGA_1024_768[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> > @@ -410,7 +411,7 @@ static const struct reg_value ov5640_setting_QVGA_320_240[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> > @@ -429,7 +430,7 @@ static const struct reg_value ov5640_setting_QCIF_176_144[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> > @@ -448,7 +449,7 @@ static const struct reg_value ov5640_setting_NTSC_720_480[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> > @@ -467,7 +468,7 @@ static const struct reg_value ov5640_setting_PAL_720_576[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> > @@ -486,7 +487,7 @@ static const struct reg_value ov5640_setting_720P_1280_720[] = {
> >       {0x3a03, 0xe4, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0xbc, 0, 0},
> >       {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x72, 0, 0}, {0x3a0e, 0x01, 0, 0},
> >       {0x3a0d, 0x02, 0, 0}, {0x3a14, 0x02, 0, 0}, {0x3a15, 0xe4, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0},
> >       {0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0},
> >  };
> > @@ -506,7 +507,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
> >       {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
> > @@ -518,7 +519,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
> >       {0x3a02, 0x04, 0, 0}, {0x3a03, 0x60, 0, 0}, {0x3a08, 0x01, 0, 0},
> >       {0x3a09, 0x50, 0, 0}, {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x18, 0, 0},
> >       {0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
> > -     {0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 0, 0}, {0x4407, 0x04, 0, 0},
> > +     {0x3a15, 0x60, 0, 0}, {0x4407, 0x04, 0, 0},
> >       {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
> >       {0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
> >  };
> > @@ -537,7 +538,7 @@ static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
> >       {0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > -     {0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
> > +     {0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0},
> >       {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> >       {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 70},
> >  };
> > @@ -1051,6 +1052,17 @@ static int ov5640_set_jpeg_timings(struct ov5640_dev *sensor,
> >  {
> >       int ret;
> >
> > +     /*
> > +      * compression mode 3 timing
> > +      *
> > +      * Data is transmitted with programmable width (VFIFO_HSIZE).
> > +      * No padding done. Last line may have less data. Varying
> > +      * number of lines per frame, depending on amount of data.
> > +      */
> > +     ret = ov5640_mod_reg(sensor, OV5640_REG_JPEG_MODE_SELECT, 0x7, 0x3);
> > +     if (ret < 0)
> > +             return ret;
> > +
> >       ret = ov5640_write_reg16(sensor, OV5640_REG_VFIFO_HSIZE, mode->hact);
> >       if (ret < 0)
> >               return ret;
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting
  2019-02-05 13:50     ` Chen-Yu Tsai
@ 2019-02-05 16:08       ` Sakari Ailus
  0 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2019-02-05 16:08 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Steve Longerbeam, Mauro Carvalho Chehab,
	Linux Media Mailing List, linux-kernel, Maxime Ripard

On Tue, Feb 05, 2019 at 09:50:47PM +0800, Chen-Yu Tsai wrote:
> On Tue, Feb 5, 2019 at 4:55 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Chen-Yu,
> >
> > On Fri, Jan 18, 2019 at 04:52:06PM +0800, Chen-Yu Tsai wrote:
> > > The register value lists for all the supported resolution settings all
> > > include a register address/value pair for setting the JPEG compression
> > > mode. With the exception of 1080p (which sets mode 2), all resolutions
> > > use mode 3.
> > >
> > > The only difference between mode 2 and mode 3 is that mode 2 may have
> > > padding data on the last line, while mode 3 does not add padding data.
> > >
> > > As these register values were from dumps of running systems, and the
> > > difference between the modes is quite small, using mode 3 for all
> > > configurations should be OK.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > ---
> > >  drivers/media/i2c/ov5640.c | 34 +++++++++++++++++++++++-----------
> > >  1 file changed, 23 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 1c1dc401c678..3d2c5de73283 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -85,6 +85,7 @@
> > >  #define OV5640_REG_FORMAT_CONTROL00  0x4300
> > >  #define OV5640_REG_VFIFO_HSIZE               0x4602
> > >  #define OV5640_REG_VFIFO_VSIZE               0x4604
> > > +#define OV5640_REG_JPG_MODE_SELECT   0x4713
> >
> > How has this been tested?
> >
> > The register is referred to as "OV5640_REG_JPEG_MODE_SELECT" below. I can
> > fix it if it's just a typo, but please confirm.
> 
> It's a typo. The datasheet uses the abbreviated form, JPG_MODE_SELECT,
> but all the bitfield names are the full JPEG form. I believe I missed
> the other occurrence while fixing up the names to match the datasheet.
> I appologize for not doing a final compile test.

Thanks, and no problem.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-02-05 16:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18  8:52 [PATCH 0/6] media: ov5640: JPEG and test pattern improvements Chen-Yu Tsai
2019-01-18  8:52 ` [PATCH 1/6] media: ov5640: Move test_pattern_menu before ov5640_set_ctrl_test_pattern Chen-Yu Tsai
2019-01-18  8:52 ` [PATCH 2/6] media: ov5640: Add register definition for test pattern register Chen-Yu Tsai
2019-01-18  8:52 ` [PATCH 3/6] media: ov5640: Disable transparent feature for test pattern Chen-Yu Tsai
2019-01-18  8:52 ` [PATCH 4/6] media: ov5640: Add three more test patterns Chen-Yu Tsai
2019-01-18  8:52 ` [PATCH 5/6] media: ov5640: Set JPEG output timings when outputting JPEG data Chen-Yu Tsai
2019-01-18  8:52 ` [PATCH 6/6] media: ov5640: Consolidate JPEG compression mode setting Chen-Yu Tsai
2019-02-05  8:55   ` Sakari Ailus
2019-02-05 13:50     ` Chen-Yu Tsai
2019-02-05 16:08       ` Sakari Ailus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).