Skip to contentSkip to conversation

media: ov5640: Misc cleanup and improvements

PATCH Applied Maxime Ripard 6 participants 41 messages Apr 16, 2018 – May 18, 2018

Subsystems (3)

v2 of 6 Applied Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v2 cover media: ov5640: Misc cleanup and improvements

Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code, slowly getting rid of the big data array for more understandable code (hopefully).

The biggest addition would be the clock rate computation at runtime, instead of relying on those arrays to setup the clock tree properly. As a side effect, it fixes the framerate that was off by around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v1:
- Integrated Hugues' suggestions to fix v4l2-compliance
- Fixed the bus width with JPEG
- Dropped the clock rate calculation loops for something simpler as suggested by Sakari
- Cache the exposure value instead of using the control value
- Rebased on top of 4.17

Maxime Ripard (10):
media: ov5640: Don't force the auto exposure state at start time
media: ov5640: Init properly the SCLK dividers
media: ov5640: Change horizontal and vertical resolutions name
media: ov5640: Add horizontal and vertical totals
media: ov5640: Program the visible resolution
media: ov5640: Adjust the clock based on the expected rate
media: ov5640: Compute the clock rate at runtime
media: ov5640: Enhance FPS handling
media: ov5640: Add 60 fps support
media: ov5640: Remove duplicate auto-exposure setup

Mylène Josserand (2):
media: ov5640: Add auto-focus feature
media: ov5640: Add light frequency control

 drivers/media/i2c/ov5640.c | 752 +++++++++++++++++++++----------------
 1 file changed, 422 insertions(+), 330 deletions(-)
Signature · 1 line
2.17.0
#1 lore raw

PATCH v2 1/12 media: ov5640: Add auto-focus feature

From: Mylène Josserand <[email protected]>

Add the auto-focus ENABLE/DISABLE feature as V4L2 control. Disabled by default.

---
 drivers/media/i2c/ov5640.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/media/i2c/ov5640.c +15 -1
index 852026baa2e7..a33e45f8e2b0 100644
@@ -82,8 +82,9 @@
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
-#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
+#define OV5640_REG_ISP_CTRL03		0x5003
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
+#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
 #define OV5640_REG_SDE_CTRL0		0x5580
 #define OV5640_REG_SDE_CTRL1		0x5581
 #define OV5640_REG_SDE_CTRL3		0x5583
@@ -186,6 +187,7 @@ struct ov5640_ctrls {
 		struct v4l2_ctrl *auto_gain;
 		struct v4l2_ctrl *gain;
 	};
+	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;
@@ -2155,6 +2157,12 @@ static int ov5640_set_ctrl_test_pattern(struct ov5640_dev *sensor, int value)
 			      0xa4, value ? 0xa4 : 0);
 }
 
+static int ov5640_set_ctrl_focus(struct ov5640_dev *sensor, int value)
+{
+	return ov5640_mod_reg(sensor, OV5640_REG_ISP_CTRL03,
+			      BIT(1), value ? BIT(1) : 0);
+}
+
 static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
@@ -2223,6 +2231,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov5640_set_ctrl_test_pattern(sensor, ctrl->val);
 		break;
+	case V4L2_CID_FOCUS_AUTO:
+		ret = ov5640_set_ctrl_focus(sensor, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2285,6 +2296,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
 					     ARRAY_SIZE(test_pattern_menu) - 1,
 					     0, 0, test_pattern_menu);
 
+	ctrls->auto_focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_AUTO,
+					      0, 1, 1, 0);
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto free_ctrls;
Signature · 1 line
2.17.0
#2 lore raw

Re: [PATCH v2 01/12] media: ov5640: Add auto-focus feature

Hi Maxime,

Thank you for the patch.

Quoted 35 lines · On Monday, 16 April 2018 15:36:50 EEST Maxime Ripard wrote:

From: Mylène Josserand <[email protected]>

Add the auto-focus ENABLE/DISABLE feature as V4L2 control. Disabled by default.

---
 drivers/media/i2c/ov5640.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/media/i2c/ov5640.c +3 -1
index 852026baa2e7..a33e45f8e2b0 100644
@@ -82,8 +82,9 @@
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
-#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
+#define OV5640_REG_ISP_CTRL03		0x5003
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
+#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
 #define OV5640_REG_SDE_CTRL0		0x5580
 #define OV5640_REG_SDE_CTRL1		0x5581
 #define OV5640_REG_SDE_CTRL3		0x5583
@@ -186,6 +187,7 @@ struct ov5640_ctrls {
 		struct v4l2_ctrl *auto_gain;
 		struct v4l2_ctrl *gain;
 	};
+	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;

drivers/media/i2c/ov5640.c static int ov5640_set_ctrl_test_pattern(struct · line 2163

Quoted 7 lines
drivers/media/i2c/ov5640.c +4 -0
@@ -2155,6 +2157,12 @@ static int ov5640_set_ctrl_test_pattern(struct
 ov5640_dev *sensor, int value) 0xa4, value ? 0xa4 : 0);
 }
 
+static int ov5640_set_ctrl_focus(struct ov5640_dev *sensor, int value)
+{
+	return ov5640_mod_reg(sensor, OV5640_REG_ISP_CTRL03,
+			      BIT(1), value ? BIT(1) : 0);

According to the datasheet, bit 1 in register 0x5003 is "Draw window for AFC enable". The draw window module is further documented as being "used to display a window on top of live video. It is usually used by autofocus to display a focus window". Are you sure the bit controls the autofocus itself ?

Furthermore, do all 0V5640 camera modules include a VCM ?

Quoted 25 lines
drivers/media/i2c/ov5640.c +8 -0
+}
+
 static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
@@ -2223,6 +2231,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov5640_set_ctrl_test_pattern(sensor, ctrl->val);
 		break;
+	case V4L2_CID_FOCUS_AUTO:
+		ret = ov5640_set_ctrl_focus(sensor, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2285,6 +2296,9 @@ static int ov5640_init_controls(struct ov5640_dev
 *sensor) ARRAY_SIZE(test_pattern_menu) - 1,
 					     0, 0, test_pattern_menu);
 
+	ctrls->auto_focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_AUTO,
+					      0, 1, 1, 0);
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto free_ctrls;
Signature · 3 lines
Regards,

Laurent Pinchart
#3 lore raw

Re: [PATCH v2 01/12] media: ov5640: Add auto-focus feature

Quoted 53 lines · Laurent Pinchart

Hi Maxime,

Thank you for the patch.

Quoted 35 lines · On Monday, 16 April 2018 15:36:50 EEST Maxime Ripard wrote:

From: Mylène Josserand <[email protected]>

Add the auto-focus ENABLE/DISABLE feature as V4L2 control. Disabled by default.

---
 drivers/media/i2c/ov5640.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/media/i2c/ov5640.c +3 -1
index 852026baa2e7..a33e45f8e2b0 100644
@@ -82,8 +82,9 @@
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
-#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
+#define OV5640_REG_ISP_CTRL03		0x5003
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
+#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
 #define OV5640_REG_SDE_CTRL0		0x5580
 #define OV5640_REG_SDE_CTRL1		0x5581
 #define OV5640_REG_SDE_CTRL3		0x5583
@@ -186,6 +187,7 @@ struct ov5640_ctrls {
 		struct v4l2_ctrl *auto_gain;
 		struct v4l2_ctrl *gain;
 	};
+	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;

drivers/media/i2c/ov5640.c static int ov5640_set_ctrl_test_pattern(struct · line 2163

Quoted 7 lines
drivers/media/i2c/ov5640.c +4 -0
@@ -2155,6 +2157,12 @@ static int ov5640_set_ctrl_test_pattern(struct
 ov5640_dev *sensor, int value) 0xa4, value ? 0xa4 : 0);
 }
 
+static int ov5640_set_ctrl_focus(struct ov5640_dev *sensor, int value)
+{
+	return ov5640_mod_reg(sensor, OV5640_REG_ISP_CTRL03,
+			      BIT(1), value ? BIT(1) : 0);

According to the datasheet, bit 1 in register 0x5003 is "Draw window for AFC enable". The draw window module is further documented as being "used to display a window on top of live video. It is usually used by autofocus to display a focus window". Are you sure the bit controls the autofocus itself ?

Quoted 1 line

Furthermore, do all 0V5640 camera modules include a VCM ?

Hmmm, double checking in the datasheet, it indeed looks like this is not what we want here. And I haven't found something to do this. I'll drop this patch.

Thanks!
Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#4 lore raw

PATCH v2 2/12 media: ov5640: Add light frequency control

In reply to Maxime Ripard

From: Mylène Josserand <[email protected]>

Add the light frequency control to be able to set the frequency to manual (50Hz or 60Hz) or auto.

---
 drivers/media/i2c/ov5640.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
drivers/media/i2c/ov5640.c +24 -0
index a33e45f8e2b0..28122341fc35 100644
@@ -189,6 +189,7 @@ struct ov5640_ctrls {
 	};
 	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
+	struct v4l2_ctrl *light_freq;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;
 	struct v4l2_ctrl *hue;
@@ -2163,6 +2164,21 @@ static int ov5640_set_ctrl_focus(struct ov5640_dev *sensor, int value)
 			      BIT(1), value ? BIT(1) : 0);
 }
 
+static int ov5640_set_ctl_light_freq(struct ov5640_dev *sensor, int value)
+{
+	int ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_HZ5060_CTRL01, BIT(7),
+			     (value == V4L2_CID_POWER_LINE_FREQUENCY_AUTO) ?
+			     0: BIT(7));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_HZ5060_CTRL00, BIT(2),
+			      (value == V4L2_CID_POWER_LINE_FREQUENCY_50HZ) ?
+			      BIT(2): 0);
+}
+
 static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
@@ -2234,6 +2250,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_FOCUS_AUTO:
 		ret = ov5640_set_ctrl_focus(sensor, ctrl->val);
 		break;
+	case V4L2_CID_POWER_LINE_FREQUENCY:
+		ret = ov5640_set_ctl_light_freq(sensor, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2298,6 +2317,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
 
 	ctrls->auto_focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_AUTO,
 					      0, 1, 1, 0);
+	ctrls->light_freq =
+		v4l2_ctrl_new_std_menu(hdl, ops,
+				       V4L2_CID_POWER_LINE_FREQUENCY,
+				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
+				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
 
 	if (hdl->error) {
 		ret = hdl->error;
Signature · 1 line
2.17.0
Reviewed-by on v2 2/12
#5 lore raw

Re: [PATCH v2 02/12] media: ov5640: Add light frequency control

Hi Maxime,

Thank you for the patch.

Quoted 24 lines · On Monday, 16 April 2018 15:36:51 EEST Maxime Ripard wrote:

From: Mylène Josserand <[email protected]>

Add the light frequency control to be able to set the frequency to manual (50Hz or 60Hz) or auto.

---
 drivers/media/i2c/ov5640.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
drivers/media/i2c/ov5640.c +1 -0
index a33e45f8e2b0..28122341fc35 100644
@@ -189,6 +189,7 @@ struct ov5640_ctrls {
 	};
 	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
+	struct v4l2_ctrl *light_freq;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;
 	struct v4l2_ctrl *hue;

drivers/media/i2c/ov5640.c static int ov5640_set_ctrl_focus(struct ov5640_dev · line 2167

Quoted 4 lines
drivers/media/i2c/ov5640.c +1 -0
@@ -2163,6 +2164,21 @@ static int ov5640_set_ctrl_focus(struct ov5640_dev
 *sensor, int value) BIT(1), value ? BIT(1) : 0);
 }
 
+static int ov5640_set_ctl_light_freq(struct ov5640_dev *sensor, int value)

To stay consistent with the other functions, I propose calling this ov5640_set_ctrl_light_freq().

Apart from that,

Quoted 40 lines
drivers/media/i2c/ov5640.c +22 -0
+{
+	int ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_HZ5060_CTRL01, BIT(7),
+			     (value == V4L2_CID_POWER_LINE_FREQUENCY_AUTO) ?
+			     0: BIT(7));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_HZ5060_CTRL00, BIT(2),
+			      (value == V4L2_CID_POWER_LINE_FREQUENCY_50HZ) ?
+			      BIT(2): 0);
+}
+
 static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
@@ -2234,6 +2250,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_FOCUS_AUTO:
 		ret = ov5640_set_ctrl_focus(sensor, ctrl->val);
 		break;
+	case V4L2_CID_POWER_LINE_FREQUENCY:
+		ret = ov5640_set_ctl_light_freq(sensor, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2298,6 +2317,11 @@ static int ov5640_init_controls(struct ov5640_dev
 *sensor)
 
 	ctrls->auto_focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_AUTO,
 					      0, 1, 1, 0);
+	ctrls->light_freq =
+		v4l2_ctrl_new_std_menu(hdl, ops,
+				       V4L2_CID_POWER_LINE_FREQUENCY,
+				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
+				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
 
 	if (hdl->error) {

ret = hdl->error;

Signature · 3 lines
Regards,

Laurent Pinchart
#6 lore raw

Re: [PATCH v2 02/12] media: ov5640: Add light frequency control

Hi Laurent,

Quoted 28 lines · Laurent Pinchart
Quoted 27 lines · On Monday, 16 April 2018 15:36:51 EEST Maxime Ripard wrote:

From: Mylène Josserand <[email protected]>

Add the light frequency control to be able to set the frequency to manual (50Hz or 60Hz) or auto.

---
 drivers/media/i2c/ov5640.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
drivers/media/i2c/ov5640.c +1 -0
index a33e45f8e2b0..28122341fc35 100644
@@ -189,6 +189,7 @@ struct ov5640_ctrls {
 	};
 	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
+	struct v4l2_ctrl *light_freq;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;
 	struct v4l2_ctrl *hue;
@@ -2163,6 +2164,21 @@ static int ov5640_set_ctrl_focus(struct ov5640_dev
 *sensor, int value) BIT(1), value ? BIT(1) : 0);
 }
 
Quoted 8 lines

drivers/media/i2c/ov5640.c static int ov5640_set_ctrl_focus(struct ov5640_dev *sensor, int value) · line 2167

Quoted 1 line
drivers/media/i2c/ov5640.c +1 -0
+static int ov5640_set_ctl_light_freq(struct ov5640_dev *sensor, int value)

To stay consistent with the other functions, I propose calling this ov5640_set_ctrl_light_freq().

Apart from that,

Consider it fixed in the next iteration, thanks!
Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
maintainer Applied
#7 lore raw

Re: [PATCH v2 02/12] media: ov5640: Add light frequency control

Quoted 36 lines · Maxime Ripard

Hi Laurent,

Quoted 33 lines · Laurent Pinchart
Quoted 24 lines · On Monday, 16 April 2018 15:36:51 EEST Maxime Ripard wrote:

From: Mylène Josserand <[email protected]>

Add the light frequency control to be able to set the frequency to manual (50Hz or 60Hz) or auto.

---
 drivers/media/i2c/ov5640.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
drivers/media/i2c/ov5640.c +1 -0
index a33e45f8e2b0..28122341fc35 100644
@@ -189,6 +189,7 @@ struct ov5640_ctrls {
 	};
 	struct v4l2_ctrl *auto_focus;
 	struct v4l2_ctrl *brightness;
+	struct v4l2_ctrl *light_freq;
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *contrast;
 	struct v4l2_ctrl *hue;

drivers/media/i2c/ov5640.c static int ov5640_set_ctrl_focus(struct ov5640_dev · line 2167

Quoted 4 lines
drivers/media/i2c/ov5640.c +1 -0
@@ -2163,6 +2164,21 @@ static int ov5640_set_ctrl_focus(struct ov5640_dev
 *sensor, int value) BIT(1), value ? BIT(1) : 0);
 }
 
+static int ov5640_set_ctl_light_freq(struct ov5640_dev *sensor, int value)

To stay consistent with the other functions, I propose calling this ov5640_set_ctrl_light_freq().

Quoted 6 lines
Quoted 3 lines

Apart from that,

Consider it fixed in the next iteration, thanks!
Maxime

Applied patches 2--7 with the following diff to the first applied patch, i.e. this one:

drivers/media/i2c/ov5640.c +2 -2
index dc3950c20c62..e480e53b369b 100644
@@ -2178,7 +2178,7 @@ static int ov5640_set_ctrl_test_pattern(struct ov5640_dev *sensor, int value)
 			      0xa4, value ? 0xa4 : 0);
 }
 
-static int ov5640_set_ctl_light_freq(struct ov5640_dev *sensor, int value)
+static int ov5640_set_ctrl_light_freq(struct ov5640_dev *sensor, int value)
 {
 	int ret;
 
@@ -2262,7 +2262,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov5640_set_ctrl_test_pattern(sensor, ctrl->val);
 		break;
 	case V4L2_CID_POWER_LINE_FREQUENCY:
-		ret = ov5640_set_ctl_light_freq(sensor, ctrl->val);
+		ret = ov5640_set_ctrl_light_freq(sensor, ctrl->val);
 		break;
 	default:
 		ret = -EINVAL;

Thanks!

Signature · 2 lines
Sakari Ailus
[email protected]
#8 lore raw

PATCH v2 3/12 media: ov5640: Don't force the auto exposure state at start time

In reply to Maxime Ripard

The sensor needs to have the auto exposure stopped while changing mode. However, when the new mode is set, the driver will force the auto exposure on, disregarding whether the control has been changed or not.

Bypass the controls code entirely to do that, and only use the control value cached when restoring the auto exposure mode.

---
 drivers/media/i2c/ov5640.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/media/i2c/ov5640.c +9 -4
index 28122341fc35..a41e4cd5fd17 100644
@@ -1571,7 +1571,8 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
  * change mode directly
  */
 static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
-				  const struct ov5640_mode_info *mode)
+				  const struct ov5640_mode_info *mode,
+				  s32 exposure)
 {
 	int ret;
 
@@ -1587,7 +1588,8 @@ static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
 	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
 	if (ret)
 		return ret;
-	return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_AUTO);
+
+	return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure);
 }
 
 static int ov5640_set_mode(struct ov5640_dev *sensor,
@@ -1595,6 +1597,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 {
 	const struct ov5640_mode_info *mode = sensor->current_mode;
 	enum ov5640_downsize_mode dn_mode, orig_dn_mode;
+	s32 exposure;
 	int ret;
 
 	dn_mode = mode->dn_mode;
@@ -1604,7 +1607,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 0);
 	if (ret)
 		return ret;
-	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
+
+	exposure = sensor->ctrls.auto_exp->val;
+	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
 	if (ret)
 		return ret;
 
@@ -1620,7 +1625,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		 * change inside subsampling or scaling
 		 * download firmware directly
 		 */
-		ret = ov5640_set_mode_direct(sensor, mode);
+		ret = ov5640_set_mode_direct(sensor, mode, exposure);
 	}
 
 	if (ret < 0)
Signature · 1 line
2.17.0
#9 lore raw

PATCH v2 4/12 media: ov5640: Init properly the SCLK dividers

In reply to Maxime Ripard

The SCLK and SCLK2X dividers are fixed in stone in the initialization array. Let's make explicit what we're doing and move that away from the huge array to the initialization code.

---
 drivers/media/i2c/ov5640.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/media/i2c/ov5640.c +10 -1
index a41e4cd5fd17..5b7995abc0e6 100644
@@ -92,6 +92,9 @@
 #define OV5640_REG_SDE_CTRL5		0x5585
 #define OV5640_REG_AVG_READOUT		0x56a1
 
+#define OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT	1
+#define OV5640_SCLK_ROOT_DIVIDER_DEFAULT	2
+
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
 	OV5640_MODE_QVGA_320_240,
@@ -251,7 +254,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
 	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
 	{0x3034, 0x18, 0, 0}, {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0},
-	{0x3037, 0x13, 0, 0}, {0x3108, 0x01, 0, 0}, {0x3630, 0x36, 0, 0},
+	{0x3037, 0x13, 0, 0}, {0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
 	{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
@@ -1662,6 +1665,12 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 	if (ret < 0)
 		return ret;
 
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
+			     (ilog2(OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT) << 2) |
+			     ilog2(OV5640_SCLK_ROOT_DIVIDER_DEFAULT));
+	if (ret)
+		return ret;
+
 	/* now restore the last capture mode */
 	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
 	if (ret < 0)
Signature · 1 line
2.17.0
#10 lore raw

PATCH v2 5/12 media: ov5640: Change horizontal and vertical resolutions name

In reply to Maxime Ripard

The current width and height parameters in the struct ov5640_mode_info are actually the active horizontal and vertical resolutions.

Since we're going to add a few other parameters, let's pick a better, more precise name for these values.

---
 drivers/media/i2c/ov5640.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/media/i2c/ov5640.c +13 -13
index 5b7995abc0e6..a8158ea9da67 100644
@@ -169,8 +169,8 @@ struct reg_value {
 struct ov5640_mode_info {
 	enum ov5640_mode_id id;
 	enum ov5640_downsize_mode dn_mode;
-	u32 width;
-	u32 height;
+	u32 hact;
+	u32 vact;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -1396,10 +1396,10 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 		if (!mode->reg_data)
 			continue;
 
-		if ((nearest && mode->width <= width &&
-		     mode->height <= height) ||
-		    (!nearest && mode->width == width &&
-		     mode->height == height))
+		if ((nearest && mode->hact <= width &&
+		     mode->vact <= height) ||
+		    (!nearest && mode->hact == width &&
+		     mode->vact == height))
 			break;
 	}
 
@@ -1888,8 +1888,8 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
 	mode = ov5640_find_mode(sensor, fr, fmt->width, fmt->height, true);
 	if (!mode)
 		return -EINVAL;
-	fmt->width = mode->width;
-	fmt->height = mode->height;
+	fmt->width = mode->hact;
+	fmt->height = mode->vact;
 
 	if (new_mode)
 		*new_mode = mode;
@@ -2367,10 +2367,10 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	fse->min_width =
-		ov5640_mode_data[0][fse->index].width;
+		ov5640_mode_data[0][fse->index].hact;
 	fse->max_width = fse->min_width;
 	fse->min_height =
-		ov5640_mode_data[0][fse->index].height;
+		ov5640_mode_data[0][fse->index].vact;
 	fse->max_height = fse->min_height;
 
 	return 0;
@@ -2434,14 +2434,14 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
 	mode = sensor->current_mode;
 
 	frame_rate = ov5640_try_frame_interval(sensor, &fi->interval,
-					       mode->width, mode->height);
+					       mode->hact, mode->vact);
 	if (frame_rate < 0)
 		frame_rate = OV5640_15_FPS;
 
 	sensor->current_fr = frame_rate;
 	sensor->frame_interval = fi->interval;
-	sensor->current_mode = ov5640_find_mode(sensor, frame_rate, mode->width,
-						mode->height, true);
+	sensor->current_mode = ov5640_find_mode(sensor, frame_rate, mode->hact,
+						mode->vact, true);
 	sensor->pending_mode_change = true;
 out:
 	mutex_unlock(&sensor->lock);
Signature · 1 line
2.17.0
#11 lore raw

PATCH v2 6/12 media: ov5640: Add horizontal and vertical totals

In reply to Maxime Ripard

All the initialization arrays are changing the horizontal and vertical totals for some value.

In order to clean up the driver, and since we're going to need that value later on, let's introduce in the ov5640_mode_info structure the horizontal and vertical total sizes, and move these out of the bytes array.

---
 drivers/media/i2c/ov5640.c | 156 +++++++++++++++++++++++--------------
 1 file changed, 97 insertions(+), 59 deletions(-)
drivers/media/i2c/ov5640.c +97 -59
index a8158ea9da67..c90d5fd06563 100644
@@ -170,7 +170,9 @@ struct ov5640_mode_info {
 	enum ov5640_mode_id id;
 	enum ov5640_downsize_mode dn_mode;
 	u32 hact;
+	u32 htot;
 	u32 vact;
+	u32 vtot;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -272,8 +274,8 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -346,8 +348,8 @@ static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x04, 0, 0}, {0x380f, 0x38, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -367,8 +369,8 @@ static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -388,8 +390,8 @@ static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x04, 0, 0}, {0x380f, 0x38, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -411,8 +413,8 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -433,8 +435,8 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x01, 0, 0}, {0x3809, 0x40, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0xf0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xf0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -454,8 +456,8 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x01, 0, 0}, {0x3809, 0x40, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0xf0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xf0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -475,8 +477,8 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x00, 0, 0}, {0x3809, 0xb0, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0x90, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x90, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -496,8 +498,8 @@ static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x00, 0, 0}, {0x3809, 0xb0, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0x90, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x90, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -517,8 +519,8 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x3c, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -538,8 +540,8 @@ static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xe0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x3c, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -559,8 +561,8 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0x40, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x40, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x38, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -580,8 +582,8 @@ static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
 	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0x40, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
-	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x40, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x38, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -602,8 +604,8 @@ static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0xfa, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x06, 0, 0}, {0x3807, 0xa9, 0, 0},
 	{0x3808, 0x05, 0, 0}, {0x3809, 0x00, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0xd0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x64, 0, 0},
-	{0x380e, 0x02, 0, 0}, {0x380f, 0xe4, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xd0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x02, 0, 0},
@@ -624,8 +626,8 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0xfa, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x06, 0, 0}, {0x3807, 0xa9, 0, 0},
 	{0x3808, 0x05, 0, 0}, {0x3809, 0x00, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0xd0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x64, 0, 0},
-	{0x380e, 0x02, 0, 0}, {0x380f, 0xe4, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0xd0, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x02, 0, 0},
@@ -646,8 +648,8 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
 	{0x3808, 0x0a, 0, 0}, {0x3809, 0x20, 0, 0}, {0x380a, 0x07, 0, 0},
-	{0x380b, 0x98, 0, 0}, {0x380c, 0x0b, 0, 0}, {0x380d, 0x1c, 0, 0},
-	{0x380e, 0x07, 0, 0}, {0x380f, 0xb0, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x98, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
 	{0x3709, 0x12, 0, 0}, {0x370c, 0x00, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -663,8 +665,7 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
 	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0}, {0x3808, 0x07, 0, 0},
 	{0x3809, 0x80, 0, 0}, {0x380a, 0x04, 0, 0}, {0x380b, 0x38, 0, 0},
-	{0x380c, 0x09, 0, 0}, {0x380d, 0xc4, 0, 0}, {0x380e, 0x04, 0, 0},
-	{0x380f, 0x60, 0, 0}, {0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
+	{0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
 	{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},
@@ -683,8 +684,8 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
 	{0x3808, 0x0a, 0, 0}, {0x3809, 0x20, 0, 0}, {0x380a, 0x07, 0, 0},
-	{0x380b, 0x98, 0, 0}, {0x380c, 0x0b, 0, 0}, {0x380d, 0x1c, 0, 0},
-	{0x380e, 0x07, 0, 0}, {0x380f, 0xb0, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x98, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
 	{0x3709, 0x12, 0, 0}, {0x370c, 0x00, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -700,8 +701,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
 	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0}, {0x3808, 0x07, 0, 0},
 	{0x3809, 0x80, 0, 0}, {0x380a, 0x04, 0, 0}, {0x380b, 0x38, 0, 0},
-	{0x380c, 0x09, 0, 0}, {0x380d, 0xc4, 0, 0}, {0x380e, 0x04, 0, 0},
-	{0x380f, 0x60, 0, 0}, {0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
+	{0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
 	{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},
@@ -719,8 +719,8 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
 	{0x3808, 0x0a, 0, 0}, {0x3809, 0x20, 0, 0}, {0x380a, 0x07, 0, 0},
-	{0x380b, 0x98, 0, 0}, {0x380c, 0x0b, 0, 0}, {0x380d, 0x1c, 0, 0},
-	{0x380e, 0x07, 0, 0}, {0x380f, 0xb0, 0, 0}, {0x3810, 0x00, 0, 0},
+	{0x380b, 0x98, 0, 0},
+	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
 	{0x3709, 0x12, 0, 0}, {0x370c, 0x00, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -734,66 +734,84 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
-	0, SUBSAMPLING, 640, 480, ov5640_init_setting_30fps_VGA,
+	0, SUBSAMPLING, 640, 1896, 480, 984,
+	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
 
 static const struct ov5640_mode_info
 ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING, 176, 144,
+		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
+		 176, 1896, 144, 984,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING, 320,  240,
+		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
+		 320, 1896, 240, 984,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING, 640,  480,
+		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
+		 640, 1896, 480, 1080,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING, 720, 480,
+		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
+		 720, 1896, 480, 984,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING, 720, 576,
+		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
+		 720, 1896, 576, 984,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING, 1024, 768,
+		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
+		 1024, 1896, 768, 1080,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 720,
+		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
+		 1280, 1892, 720, 740,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING, 1920, 1080,
+		{OV5640_MODE_1080P_1920_1080, SCALING,
+		 1920, 2500, 1080, 1120,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, SCALING, 2592, 1944,
+		{OV5640_MODE_QSXGA_2592_1944, SCALING,
+		 2592, 2844, 1944, 1968,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING, 176, 144,
+		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
+		 176, 1896, 144, 984,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING, 320,  240,
+		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
+		 320, 1896, 240, 984,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING, 640,  480,
+		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
+		 640, 1896, 480, 1080,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING, 720, 480,
+		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
+		 720, 1896, 480, 984,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING, 720, 576,
+		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
+		 720, 1896, 576, 984,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING, 1024, 768,
+		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
+		 1024, 1896, 768, 1080,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 720,
+		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
+		 1280, 1892, 720, 740,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING, 1920, 1080,
+		{OV5640_MODE_1080P_1920_1080, SCALING,
+		 1920, 2500, 1080, 1120,
 		 ov5640_setting_30fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, NULL, 0},
+		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
 	},
 };
 
@@ -1383,6 +1401,22 @@ static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
 	return ov5640_write_reg(sensor, OV5640_REG_DEBUG_MODE, temp);
 }
 
+static int ov5640_set_timings(struct ov5640_dev *sensor,
+			      const struct ov5640_mode_info *mode)
+{
+	int ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static const struct ov5640_mode_info *
 ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 		 int width, int height, bool nearest)
@@ -1631,6 +1665,10 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		ret = ov5640_set_mode_direct(sensor, mode, exposure);
 	}
 
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_set_timings(sensor, mode);
 	if (ret < 0)
 		return ret;
 
Signature · 1 line
2.17.0
#12 lore raw

PATCH v2 7/12 media: ov5640: Program the visible resolution

In reply to Maxime Ripard

The active frame size is set in the initialization arrays, but the value itself is also available in the struct ov5640_mode_info.

Let's move these values out of the big bytes arrays, and program it with the value of the mode that we are given.

---
 drivers/media/i2c/ov5640.c | 58 +++++++++-----------------------------
 1 file changed, 14 insertions(+), 44 deletions(-)
drivers/media/i2c/ov5640.c +14 -44
index c90d5fd06563..4608b8dc6495 100644
@@ -60,6 +60,8 @@
 #define OV5640_REG_AEC_PK_MANUAL	0x3503
 #define OV5640_REG_AEC_PK_REAL_GAIN	0x350a
 #define OV5640_REG_AEC_PK_VTS		0x350c
+#define OV5640_REG_TIMING_DVPHO		0x3808
+#define OV5640_REG_TIMING_DVPVO		0x380a
 #define OV5640_REG_TIMING_HTS		0x380c
 #define OV5640_REG_TIMING_VTS		0x380e
 #define OV5640_REG_TIMING_TC_REG21	0x3821
@@ -273,8 +275,6 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -347,8 +347,6 @@ static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -368,8 +366,6 @@ static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -389,8 +385,6 @@ static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -401,8 +395,7 @@ static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-	{0x3808, 0x04, 0, 0}, {0x3809, 0x00, 0, 0}, {0x380a, 0x03, 0, 0},
-	{0x380b, 0x00, 0, 0}, {0x3035, 0x12, 0, 0},
+	{0x3035, 0x12, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
@@ -412,8 +405,6 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -423,8 +414,7 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3808, 0x04, 0, 0},
-	{0x3809, 0x00, 0, 0}, {0x380a, 0x03, 0, 0}, {0x380b, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
@@ -434,8 +424,6 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x01, 0, 0}, {0x3809, 0x40, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0xf0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -455,8 +443,6 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x01, 0, 0}, {0x3809, 0x40, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0xf0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -476,8 +462,6 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x00, 0, 0}, {0x3809, 0xb0, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0x90, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -497,8 +481,6 @@ static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x00, 0, 0}, {0x3809, 0xb0, 0, 0}, {0x380a, 0x00, 0, 0},
-	{0x380b, 0x90, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -518,8 +500,6 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x3c, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -539,8 +519,6 @@ static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x01, 0, 0},
-	{0x380b, 0xe0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x3c, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -560,8 +538,6 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0x40, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x38, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -581,8 +557,6 @@ static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3808, 0x02, 0, 0}, {0x3809, 0xd0, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0x40, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x38, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -603,8 +577,6 @@ static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0xfa, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x06, 0, 0}, {0x3807, 0xa9, 0, 0},
-	{0x3808, 0x05, 0, 0}, {0x3809, 0x00, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0xd0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -625,8 +597,6 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0xfa, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x06, 0, 0}, {0x3807, 0xa9, 0, 0},
-	{0x3808, 0x05, 0, 0}, {0x3809, 0x00, 0, 0}, {0x380a, 0x02, 0, 0},
-	{0x380b, 0xd0, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
@@ -647,8 +617,6 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
-	{0x3808, 0x0a, 0, 0}, {0x3809, 0x20, 0, 0}, {0x380a, 0x07, 0, 0},
-	{0x380b, 0x98, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
@@ -663,8 +631,7 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
-	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0}, {0x3808, 0x07, 0, 0},
-	{0x3809, 0x80, 0, 0}, {0x380a, 0x04, 0, 0}, {0x380b, 0x38, 0, 0},
+	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0},
 	{0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3a02, 0x04, 0, 0}, {0x3a03, 0x60, 0, 0}, {0x3a08, 0x01, 0, 0},
 	{0x3a09, 0x50, 0, 0}, {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x18, 0, 0},
@@ -683,8 +650,6 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
-	{0x3808, 0x0a, 0, 0}, {0x3809, 0x20, 0, 0}, {0x380a, 0x07, 0, 0},
-	{0x380b, 0x98, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
@@ -699,8 +664,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
-	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0}, {0x3808, 0x07, 0, 0},
-	{0x3809, 0x80, 0, 0}, {0x380a, 0x04, 0, 0}, {0x380b, 0x38, 0, 0},
+	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0},
 	{0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3a02, 0x04, 0, 0}, {0x3a03, 0x60, 0, 0}, {0x3a08, 0x01, 0, 0},
 	{0x3a09, 0x50, 0, 0}, {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x18, 0, 0},
@@ -718,8 +682,6 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
-	{0x3808, 0x0a, 0, 0}, {0x3809, 0x20, 0, 0}, {0x380a, 0x07, 0, 0},
-	{0x380b, 0x98, 0, 0},
 	{0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
 	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
@@ -1406,6 +1368,14 @@ static int ov5640_set_timings(struct ov5640_dev *sensor,
 {
 	int ret;
 
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
+	if (ret < 0)
+		return ret;
+
 	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
 	if (ret < 0)
 		return ret;
Signature · 1 line
2.17.0
#13 lore raw

PATCH v2 8/12 media: ov5640: Adjust the clock based on the expected rate

In reply to Maxime Ripard

The clock structure for the PCLK is quite obscure in the documentation, and was hardcoded through the bytes array of each and every mode.

This is troublesome, since we cannot adjust it at runtime based on other parameters (such as the number of bytes per pixel), and we can't support either framerates that have not been used by the various vendors, since we don't have the needed initialization sequence.

We can however understand how the clock tree works, and then implement some functions to derive the various parameters from a given rate. And now that those parameters are calculated at runtime, we can remove them from the initialization sequence.

The modes also gained a new parameter which is the clock that they are running at, from the register writes they were doing, so for now the switch to the new algorithm should be transparent.

---
 drivers/media/i2c/ov5640.c | 272 +++++++++++++++++++++++++++++++++----
 1 file changed, 245 insertions(+), 27 deletions(-)
drivers/media/i2c/ov5640.c +245 -27
index 4608b8dc6495..8db4fc0f031c 100644
@@ -95,7 +95,6 @@
 #define OV5640_REG_AVG_READOUT		0x56a1
 
 #define OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT	1
-#define OV5640_SCLK_ROOT_DIVIDER_DEFAULT	2
 
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
@@ -175,6 +174,7 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
+	u32 clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -257,8 +257,8 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
 	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
-	{0x3034, 0x18, 0, 0}, {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0},
-	{0x3037, 0x13, 0, 0}, {0x3630, 0x36, 0, 0},
+	{0x3034, 0x18, 0, 0},
+	{0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
 	{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
@@ -341,7 +341,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -360,7 +360,7 @@ static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -379,7 +379,7 @@ static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -395,11 +395,10 @@ static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-	{0x3035, 0x12, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -418,7 +417,7 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -437,7 +436,7 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -456,7 +455,7 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -475,7 +474,7 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -494,7 +493,7 @@ static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
-	{0x3035, 0x12, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -513,7 +512,7 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -532,7 +531,7 @@ static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
-	{0x3035, 0x12, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -551,7 +550,7 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -571,7 +570,7 @@ static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
 
 static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0},
+	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -591,7 +590,7 @@ static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
-	{0x3035, 0x41, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0},
+	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -611,7 +610,7 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 
 static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -626,8 +625,8 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x11, 0, 0},
-	{0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
@@ -644,7 +643,7 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 
 static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -659,8 +658,8 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x21, 0, 0},
-	{0x3036, 0x54, 0, 1}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
@@ -676,7 +675,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 
 static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -697,6 +696,7 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
 	0, SUBSAMPLING, 640, 1896, 480, 984,
+	112000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -706,74 +706,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 56000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 56000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 56000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 56000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 56000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 56000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 42000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 84000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
+		 168000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 112000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 112000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 112000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 112000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 112000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 112000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 84000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 168000000,
 		 ov5640_setting_30fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
+		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
 	},
 };
 
@@ -906,6 +923,199 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 	return ov5640_write_reg(sensor, reg, val);
 }
 
+/*
+ * After spending way too much time trying the various combinations, I
+ * believe the clock tree is as follows:
+ *
+ *   +--------------+
+ *   |  Ext. Clock  |
+ *   +------+-------+
+ *          |
+ *   +------+-------+
+ *   | System clock | - reg 0x3035, bits 4-7
+ *   +------+-------+
+ *          |
+ *   +------+-------+ - reg 0x3036, for the multiplier
+ *   |     PLL      | - reg 0x3037, bits 4 for the root divider
+ *   +------+-------+ - reg 0x3037, bits 0-3 for the pre-divider
+ *          |
+ *   +------+-------+
+ *   |     SCLK     | - reg 0x3108, bits 0-1 for the root divider
+ *   +------+-------+
+ *          |
+ *   +------+-------+
+ *   |    PCLK      | - reg 0x3108, bits 4-5 for the root divider
+ *   +--------------+
+ *
+ * This is deviating from the datasheet at least for the register
+ * 0x3108, since it's said here that the PCLK would be clocked from
+ * the PLL. However, changing the SCLK divider value has a direct
+ * effect on the PCLK rate, which wouldn't be the case if both PCLK
+ * and SCLK were to be sourced from the PLL.
+ *
+ * These parameters also match perfectly the rate that is output by
+ * the sensor, so we shouldn't have too much factors missing (or they
+ * would be set to 1).
+ *
+ * In the vendor kernels, the system clock divider is either 1 or 2.
+ * The PLL pre-divider is set to 3, its root divider to 1. The SCLK
+ * divider is set to 2, and the PCLK divider set to 1.
+ *
+ * The only varying parts are thus the PLL multiplier and the system
+ * clock divider.
+ */
+
+/*
+ * This is supposed to be ranging from 1 to 16, but the value is
+ * always set to either 1 or 2 in the vendor kernels.
+ */
+#define OV5640_SYSDIV_MIN	1
+#define OV5640_SYSDIV_MAX	2
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 3 in the vendor kernels.
+ */
+#define OV5640_PLL_PREDIV	3
+
+#define OV5640_PLL_MULT_MIN	4
+#define OV5640_PLL_MULT_MAX	252
+
+/*
+ * This is supposed to be ranging from 1 to 2, but the value is always
+ * set to 1 in the vendor kernels.
+ */
+#define OV5640_PLL_ROOT_DIV	1
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_SCLK_ROOT_DIV	2
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 1 in the vendor kernels.
+ */
+#define OV5640_PCLK_ROOT_DIV	1
+
+static unsigned long ov5640_compute_pclk(struct ov5640_dev *sensor,
+					 u8 sys_div, u8 pll_prediv,
+					 u8 pll_mult, u8 pll_div,
+					 u8 sclk_div, u8 pclk_div)
+{
+	unsigned long rate = clk_get_rate(sensor->xclk);
+
+	rate = rate / sys_div / pll_prediv * pll_mult / pll_div;
+
+	return rate / sclk_div / pclk_div;
+}
+
+static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
+				      unsigned long rate,
+				      u8 *sysdiv, u8 *prediv, u8 *pll_rdiv,
+				      u8 *mult, u8 *sclk_rdiv, u8 *pclk_rdiv)
+{
+	unsigned long best = ~0;
+	u8 best_sysdiv = 1, best_mult = 1;
+	u8 _sysdiv, _pll_mult;
+
+	for (_sysdiv = OV5640_SYSDIV_MIN;
+	     _sysdiv <= OV5640_SYSDIV_MAX;
+	     _sysdiv++) {
+		for (_pll_mult = OV5640_PLL_MULT_MIN;
+		     _pll_mult <= OV5640_PLL_MULT_MAX;
+		     _pll_mult++) {
+			unsigned long _rate;
+
+			/*
+			 * The PLL multiplier cannot be odd if above
+			 * 127.
+			 */
+			if (_pll_mult > 127 && !(_pll_mult % 2))
+				continue;
+
+			_rate = ov5640_compute_pclk(sensor, _sysdiv,
+						    OV5640_PLL_PREDIV,
+						    _pll_mult,
+						    OV5640_PLL_ROOT_DIV,
+						    OV5640_SCLK_ROOT_DIV,
+						    OV5640_PCLK_ROOT_DIV);
+
+			if (abs(rate - _rate) < abs(rate - best)) {
+				best = _rate;
+				best_sysdiv = _sysdiv;
+				best_mult = _pll_mult;
+			}
+
+			if (_rate == rate)
+				goto out;
+		}
+	}
+
+out:
+	*sysdiv = best_sysdiv;
+	*prediv = OV5640_PLL_PREDIV;
+	*pll_rdiv = OV5640_PLL_ROOT_DIV;
+	*mult = best_mult;
+	*sclk_rdiv = OV5640_SCLK_ROOT_DIV;
+	*pclk_rdiv = OV5640_PCLK_ROOT_DIV;
+	return best;
+}
+
+static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 sysdiv, prediv, mult, pll_rdiv, sclk_rdiv, pclk_rdiv;
+	int ret;
+
+	ov5640_calc_pclk(sensor, rate, &sysdiv, &prediv, &pll_rdiv, &mult,
+			 &sclk_rdiv, &pclk_rdiv);
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			     0xf0, sysdiv << 4);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
+			     0xff, mult);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
+			     0xff, prediv | ((pll_rdiv - 1) << 4));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x33,
+			      (ilog2(pclk_rdiv) << 4) |
+			      ilog2(OV5640_SCLK_ROOT_DIV));
+}
+
+static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 sysdiv, prediv, mult, pll_rdiv, sclk_rdiv, pclk_rdiv;
+	int ret;
+
+	ov5640_calc_pclk(sensor, rate, &sysdiv, &prediv, &pll_rdiv, &mult,
+			 &sclk_rdiv, &pclk_rdiv);
+	ret = ov5640_write_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			       (sysdiv << 4) | pclk_rdiv);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
+			     0xff, mult);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3,
+			     ilog2(sclk_rdiv));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
+			      0xff, prediv | ((pll_rdiv - 1) << 4));
+}
+
 /* download ov5640 settings to sensor through i2c */
 static int ov5640_load_regs(struct ov5640_dev *sensor,
 			    const struct ov5640_mode_info *mode)
@@ -1620,6 +1830,14 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
+		ret = ov5640_set_mipi_pclk(sensor, mode->clock);
+	else
+		ret = ov5640_set_dvp_pclk(sensor, mode->clock);
+
+	if (ret < 0)
+		return 0;
+
 	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
 	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
 		/*
@@ -1675,7 +1893,7 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
 			     (ilog2(OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT) << 2) |
-			     ilog2(OV5640_SCLK_ROOT_DIVIDER_DEFAULT));
+			     ilog2(OV5640_SCLK_ROOT_DIV));
 	if (ret)
 		return ret;
 
Signature · 1 line
2.17.0
maintainer
#14 lore raw

Re: [PATCH v2 08/12] media: ov5640: Adjust the clock based on the expected rate

Hi Maxime,

Quoted 534 lines · Maxime Ripard

The clock structure for the PCLK is quite obscure in the documentation, and was hardcoded through the bytes array of each and every mode.

This is troublesome, since we cannot adjust it at runtime based on other parameters (such as the number of bytes per pixel), and we can't support either framerates that have not been used by the various vendors, since we don't have the needed initialization sequence.

We can however understand how the clock tree works, and then implement some functions to derive the various parameters from a given rate. And now that those parameters are calculated at runtime, we can remove them from the initialization sequence.

The modes also gained a new parameter which is the clock that they are running at, from the register writes they were doing, so for now the switch to the new algorithm should be transparent.

---
 drivers/media/i2c/ov5640.c | 272 +++++++++++++++++++++++++++++++++----
 1 file changed, 245 insertions(+), 27 deletions(-)
drivers/media/i2c/ov5640.c +236 -26
index 4608b8dc6495..8db4fc0f031c 100644
@@ -95,7 +95,6 @@
 #define OV5640_REG_AVG_READOUT		0x56a1
 
 #define OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT	1
-#define OV5640_SCLK_ROOT_DIVIDER_DEFAULT	2
 
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
@@ -175,6 +174,7 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
+	u32 clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -257,8 +257,8 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
 	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
-	{0x3034, 0x18, 0, 0}, {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0},
-	{0x3037, 0x13, 0, 0}, {0x3630, 0x36, 0, 0},
+	{0x3034, 0x18, 0, 0},
+	{0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
 	{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
@@ -341,7 +341,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -360,7 +360,7 @@ static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -379,7 +379,7 @@ static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -395,11 +395,10 @@ static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-	{0x3035, 0x12, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -418,7 +417,7 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -437,7 +436,7 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -456,7 +455,7 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -475,7 +474,7 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -494,7 +493,7 @@ static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
-	{0x3035, 0x12, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -513,7 +512,7 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -532,7 +531,7 @@ static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
-	{0x3035, 0x12, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -551,7 +550,7 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -571,7 +570,7 @@ static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
 
 static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0},
+	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -591,7 +590,7 @@ static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
-	{0x3035, 0x41, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0},
+	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -611,7 +610,7 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 
 static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -626,8 +625,8 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x11, 0, 0},
-	{0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
@@ -644,7 +643,7 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 
 static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -659,8 +658,8 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x21, 0, 0},
-	{0x3036, 0x54, 0, 1}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
@@ -676,7 +675,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 
 static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -697,6 +696,7 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
 	0, SUBSAMPLING, 640, 1896, 480, 984,
+	112000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -706,74 +706,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 56000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 56000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 56000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 56000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 56000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 56000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 42000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 84000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
+		 168000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 112000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 112000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 112000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 112000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 112000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 112000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 84000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 168000000,
 		 ov5640_setting_30fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
+		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
 	},
 };
 
@@ -906,6 +923,199 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 	return ov5640_write_reg(sensor, reg, val);
 }
 
+/*
+ * After spending way too much time trying the various combinations, I
+ * believe the clock tree is as follows:
+ *
+ *   +--------------+
+ *   |  Ext. Clock  |
+ *   +------+-------+
+ *          |
+ *   +------+-------+
+ *   | System clock | - reg 0x3035, bits 4-7
+ *   +------+-------+
+ *          |
+ *   +------+-------+ - reg 0x3036, for the multiplier
+ *   |     PLL      | - reg 0x3037, bits 4 for the root divider
+ *   +------+-------+ - reg 0x3037, bits 0-3 for the pre-divider
+ *          |
+ *   +------+-------+
+ *   |     SCLK     | - reg 0x3108, bits 0-1 for the root divider
+ *   +------+-------+
+ *          |
+ *   +------+-------+
+ *   |    PCLK      | - reg 0x3108, bits 4-5 for the root divider
+ *   +--------------+
+ *
+ * This is deviating from the datasheet at least for the register
+ * 0x3108, since it's said here that the PCLK would be clocked from
+ * the PLL. However, changing the SCLK divider value has a direct
+ * effect on the PCLK rate, which wouldn't be the case if both PCLK
+ * and SCLK were to be sourced from the PLL.
+ *
+ * These parameters also match perfectly the rate that is output by
+ * the sensor, so we shouldn't have too much factors missing (or they
+ * would be set to 1).
+ *
+ * In the vendor kernels, the system clock divider is either 1 or 2.
+ * The PLL pre-divider is set to 3, its root divider to 1. The SCLK
+ * divider is set to 2, and the PCLK divider set to 1.
+ *
+ * The only varying parts are thus the PLL multiplier and the system
+ * clock divider.
+ */
+
+/*
+ * This is supposed to be ranging from 1 to 16, but the value is
+ * always set to either 1 or 2 in the vendor kernels.
+ */
+#define OV5640_SYSDIV_MIN	1
+#define OV5640_SYSDIV_MAX	2
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 3 in the vendor kernels.
+ */
+#define OV5640_PLL_PREDIV	3
+
+#define OV5640_PLL_MULT_MIN	4
+#define OV5640_PLL_MULT_MAX	252
+
+/*
+ * This is supposed to be ranging from 1 to 2, but the value is always
+ * set to 1 in the vendor kernels.
+ */
+#define OV5640_PLL_ROOT_DIV	1
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_SCLK_ROOT_DIV	2
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 1 in the vendor kernels.
+ */
+#define OV5640_PCLK_ROOT_DIV	1
+
+static unsigned long ov5640_compute_pclk(struct ov5640_dev *sensor,
+					 u8 sys_div, u8 pll_prediv,
+					 u8 pll_mult, u8 pll_div,
+					 u8 sclk_div, u8 pclk_div)
+{
+	unsigned long rate = clk_get_rate(sensor->xclk);
+
+	rate = rate / sys_div / pll_prediv * pll_mult / pll_div;
+
+	return rate / sclk_div / pclk_div;
+}
+
+static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
+				      unsigned long rate,
+				      u8 *sysdiv, u8 *prediv, u8 *pll_rdiv,
+				      u8 *mult, u8 *sclk_rdiv, u8 *pclk_rdiv)
+{
+	unsigned long best = ~0;
+	u8 best_sysdiv = 1, best_mult = 1;
+	u8 _sysdiv, _pll_mult;
+
+	for (_sysdiv = OV5640_SYSDIV_MIN;
+	     _sysdiv <= OV5640_SYSDIV_MAX;
+	     _sysdiv++) {
+		for (_pll_mult = OV5640_PLL_MULT_MIN;
+		     _pll_mult <= OV5640_PLL_MULT_MAX;
+		     _pll_mult++) {
+			unsigned long _rate;
+
+			/*
+			 * The PLL multiplier cannot be odd if above
+			 * 127.
+			 */
+			if (_pll_mult > 127 && !(_pll_mult % 2))
+				continue;
+
+			_rate = ov5640_compute_pclk(sensor, _sysdiv,
+						    OV5640_PLL_PREDIV,
+						    _pll_mult,
+						    OV5640_PLL_ROOT_DIV,
+						    OV5640_SCLK_ROOT_DIV,
+						    OV5640_PCLK_ROOT_DIV);
+
+			if (abs(rate - _rate) < abs(rate - best)) {
+				best = _rate;
+				best_sysdiv = _sysdiv;
+				best_mult = _pll_mult;
+			}
+
+			if (_rate == rate)
+				goto out;
+		}
+	}
+
+out:
+	*sysdiv = best_sysdiv;
+	*prediv = OV5640_PLL_PREDIV;
+	*pll_rdiv = OV5640_PLL_ROOT_DIV;
+	*mult = best_mult;
+	*sclk_rdiv = OV5640_SCLK_ROOT_DIV;
+	*pclk_rdiv = OV5640_PCLK_ROOT_DIV;
+	return best;
+}
+
+static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 sysdiv, prediv, mult, pll_rdiv, sclk_rdiv, pclk_rdiv;
+	int ret;
+
+	ov5640_calc_pclk(sensor, rate, &sysdiv, &prediv, &pll_rdiv, &mult,
+			 &sclk_rdiv, &pclk_rdiv);
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			     0xf0, sysdiv << 4);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
+			     0xff, mult);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
+			     0xff, prediv | ((pll_rdiv - 1) << 4));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x33,
+			      (ilog2(pclk_rdiv) << 4) |
+			      ilog2(OV5640_SCLK_ROOT_DIV));
+}
+
+static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 sysdiv, prediv, mult, pll_rdiv, sclk_rdiv, pclk_rdiv;
+	int ret;
+
+	ov5640_calc_pclk(sensor, rate, &sysdiv, &prediv, &pll_rdiv, &mult,
+			 &sclk_rdiv, &pclk_rdiv);
+	ret = ov5640_write_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			       (sysdiv << 4) | pclk_rdiv);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
+			     0xff, mult);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3,
+			     ilog2(sclk_rdiv));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
+			      0xff, prediv | ((pll_rdiv - 1) << 4));
+}
+
 /* download ov5640 settings to sensor through i2c */
 static int ov5640_load_regs(struct ov5640_dev *sensor,
 			    const struct ov5640_mode_info *mode)

drivers/media/i2c/ov5640.c static int ov5640_set_mode(struct ov5640_dev *sensor, · line 1834

Quoted 5 lines
drivers/media/i2c/ov5640.c +2 -0
@@ -1620,6 +1830,14 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
+		ret = ov5640_set_mipi_pclk(sensor, mode->clock);

What is the value of the mode->clock expected to signify? It'd seem like that this changes from this patch to the next. Which one is correct?

Please also add a comment or two documenting this; it'll be otherwise difficult to find out later on.

Quoted 18 lines
drivers/media/i2c/ov5640.c +7 -1
+	else
+		ret = ov5640_set_dvp_pclk(sensor, mode->clock);
+
+	if (ret < 0)
+		return 0;
+
 	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
 	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
 		/*
@@ -1675,7 +1893,7 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
 			     (ilog2(OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT) << 2) |
-			     ilog2(OV5640_SCLK_ROOT_DIVIDER_DEFAULT));
+			     ilog2(OV5640_SCLK_ROOT_DIV));
 	if (ret)
 		return ret;
 
Signature · 4 lines
Kind regards,

Sakari Ailus
[email protected]
#15 lore raw

Re: [PATCH v2 08/12] media: ov5640: Adjust the clock based on the expected rate

Hi Sakari,

Quoted 7 lines · Sakari Ailus
Quoted 7 lines

/* download ov5640 settings to sensor through i2c */ static int ov5640_load_regs(struct ov5640_dev *sensor,

drivers/media/i2c/ov5640.c +0 -0
 			    const struct ov5640_mode_info *mode)
@@ -1620,6 +1830,14 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
Quoted 5 lines

drivers/media/i2c/ov5640.c static int ov5640_set_mode(struct ov5640_dev *sensor, · line 1834

Quoted 2 lines
drivers/media/i2c/ov5640.c +2 -0
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
+		ret = ov5640_set_mipi_pclk(sensor, mode->clock);

What is the value of the mode->clock expected to signify? It'd seem like that this changes from this patch to the next. Which one is correct?

It doesn't, this is the clock rate computed through the formula described above (and that might be incorrect for MIPI-CSI, given Samuel feedback) from the way the registers are initialized in the arrays.

This shouldn't bring any change to the clock rate, but instead of hardcoding it, we now have the infrastructure to calculate the factors for any given rate.

The subsequent patch will remove that hardcoded clock rate and generate it dynamically from the timings / format.

Does that make sense? Or maybe I should split this some other way?

Quoted 2 lines

Please also add a comment or two documenting this; it'll be otherwise difficult to find out later on.

I'm not sure there's a point in documenting that intermediate step that is just there for a single commit. Maybe I should expand the commit log to make it clearer?

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
maintainer
#16 lore raw

Re: [PATCH v2 08/12] media: ov5640: Adjust the clock based on the expected rate

Quoted 21 lines · Maxime Ripard

Hi Sakari,

Quoted 7 lines · Sakari Ailus
Quoted 7 lines

/* download ov5640 settings to sensor through i2c */ static int ov5640_load_regs(struct ov5640_dev *sensor,

drivers/media/i2c/ov5640.c +0 -0
 			    const struct ov5640_mode_info *mode)
@@ -1620,6 +1830,14 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
Quoted 5 lines

drivers/media/i2c/ov5640.c static int ov5640_set_mode(struct ov5640_dev *sensor, · line 1834

Quoted 2 lines
drivers/media/i2c/ov5640.c +2 -0
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
+		ret = ov5640_set_mipi_pclk(sensor, mode->clock);

What is the value of the mode->clock expected to signify? It'd seem like that this changes from this patch to the next. Which one is correct?

It doesn't, this is the clock rate computed through the formula described above (and that might be incorrect for MIPI-CSI, given Samuel feedback) from the way the registers are initialized in the arrays.

Quoted 8 lines

This shouldn't bring any change to the clock rate, but instead of hardcoding it, we now have the infrastructure to calculate the factors for any given rate.

The subsequent patch will remove that hardcoded clock rate and generate it dynamically from the timings / format.

Does that make sense? Or maybe I should split this some other way?

I guess it's ok as it is. I think I must have misread a chunk in the following patch --- sorry about the noise.

Signature · 2 lines
Sakari Ailus
[email protected]
#17 lore raw

PATCH v2 9/12 media: ov5640: Compute the clock rate at runtime

In reply to Maxime Ripard

The clock rate, while hardcoded until now, is actually a function of the resolution, framerate and bytes per pixel. Now that we have an algorithm to adjust our clock rate, we can select it dynamically when we change the mode.

This changes a bit the clock rate being used, with the following effect:

+------+------+------+------+-----+-----------------+----------------+-----------+
| Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation |
+------+------+------+------+-----+-----------------+----------------+-----------+
|  640 |  480 | 1896 | 1080 |  15 |        56000000 |       61430400 | 8.84 %    |
|  640 |  480 | 1896 | 1080 |  30 |       112000000 |      122860800 | 8.84 %    |
| 1024 |  768 | 1896 | 1080 |  15 |        56000000 |       61430400 | 8.84 %    |
| 1024 |  768 | 1896 | 1080 |  30 |       112000000 |      122860800 | 8.84 %    |
|  320 |  240 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  320 |  240 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
|  176 |  144 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  176 |  144 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
|  720 |  480 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  720 |  480 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
|  720 |  576 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  720 |  576 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
| 1280 |  720 | 1892 |  740 |  15 |        42000000 |       42002400 | 0.01 %    |
| 1280 |  720 | 1892 |  740 |  30 |        84000000 |       84004800 | 0.01 %    |
| 1920 | 1080 | 2500 | 1120 |  15 |        84000000 |       84000000 | 0.00 %    |
| 1920 | 1080 | 2500 | 1120 |  30 |       168000000 |      168000000 | 0.00 %    |
| 2592 | 1944 | 2844 | 1944 |  15 |        84000000 |      165862080 | 49.36 %   |
+------+------+------+------+-----+-----------------+----------------+-----------+

Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected by the new formula.

In this case, 640x480 and 1024x768 are actually fixed by this driver. Indeed, the sensor was sending data at, for example, 27.33fps instead of 30fps. This is -9%, which is roughly what we're seeing in the array. Testing these modes with the new clock setup actually fix that error, and data are now sent at around 30fps.

2592x1944, on the other hand, is probably due to the fact that this mode can only be used using MIPI-CSI2, in a two lane mode. This would have to be tested though.

---
 drivers/media/i2c/ov5640.c | 41 ++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 24 deletions(-)
drivers/media/i2c/ov5640.c +17 -24
index 8db4fc0f031c..62938e9dabc5 100644
@@ -174,7 +174,6 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
-	u32 clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -696,7 +695,6 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
 	0, SUBSAMPLING, 640, 1896, 480, 984,
-	112000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -706,91 +704,74 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
-		 56000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
-		 56000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
-		 56000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
-		 56000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
-		 56000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
-		 56000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
-		 42000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
-		 84000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
-		 168000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
-		 112000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
-		 112000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
-		 112000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
-		 112000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
-		 112000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
-		 112000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
-		 84000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
-		 168000000,
 		 ov5640_setting_30fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
+		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
 	},
 };
 
@@ -1814,6 +1795,8 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 {
 	const struct ov5640_mode_info *mode = sensor->current_mode;
 	enum ov5640_downsize_mode dn_mode, orig_dn_mode;
+	unsigned long rate;
+	unsigned char bpp;
 	s32 exposure;
 	int ret;
 
@@ -1830,10 +1813,20 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
-	if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
-		ret = ov5640_set_mipi_pclk(sensor, mode->clock);
-	else
-		ret = ov5640_set_dvp_pclk(sensor, mode->clock);
+	/*
+	 * All the formats we support have 2 bytes per pixel, except for JPEG
+	 * which is 1 byte per pixel.
+	 */
+	bpp = sensor->fmt.code == MEDIA_BUS_FMT_JPEG_1X8 ? 1 : 2;
+	rate = mode->vtot * mode->htot * bpp;
+	rate *= ov5640_framerates[sensor->current_fr];
+
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
+		rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
+		ret = ov5640_set_mipi_pclk(sensor, rate);
+	} else {
+		ret = ov5640_set_dvp_pclk(sensor, rate);
+	}
 
 	if (ret < 0)
 		return 0;
Signature · 1 line
2.17.0
#18 lore raw

PATCH v2 10/12 media: ov5640: Enhance FPS handling

In reply to Maxime Ripard

Now that we have moved the clock generation logic out of the bytes array, these arrays are identical between the 15fps and 30fps variants.

Remove the duplicate entries, and convert the code accordingly.

---
 drivers/media/i2c/ov5640.c | 316 +++++++------------------------------
 1 file changed, 60 insertions(+), 256 deletions(-)
drivers/media/i2c/ov5640.c +60 -256
index 62938e9dabc5..690ed0238763 100644
@@ -339,64 +339,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
 };
 
-static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x0e, 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},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{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},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x0e, 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},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
+static const struct reg_value ov5640_setting_VGA_640_480[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -415,7 +358,7 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
+static const struct reg_value ov5640_setting_XGA_1024_768[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -434,7 +377,7 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
+static const struct reg_value ov5640_setting_QVGA_320_240[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -453,7 +396,7 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
+static const struct reg_value ov5640_setting_QCIF_176_144[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -472,26 +415,7 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{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},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
+static const struct reg_value ov5640_setting_NTSC_720_480[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -510,26 +434,7 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x3c, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{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},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
+static const struct reg_value ov5640_setting_PAL_720_576[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -548,47 +453,7 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x38, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{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},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
-	{0x3008, 0x42, 0, 0},
-	{0x3c07, 0x07, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0xfa, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x06, 0, 0}, {0x3807, 0xa9, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x02, 0, 0},
-	{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, 0x02, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0},
-	{0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0}, {0x4005, 0x1a, 0, 0},
-	{0x3008, 0x02, 0, 0}, {0x3503, 0,    0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
+static const struct reg_value ov5640_setting_720P_1280_720[] = {
 	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3820, 0x41, 0, 0}, {0x3821, 0x07, 0, 0}, {0x3814, 0x31, 0, 0},
@@ -607,40 +472,7 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 	{0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
-	{0x3008, 0x42, 0, 0},
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0}, {0x3814, 0x11, 0, 0},
-	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
-	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
-	{0x3709, 0x12, 0, 0}, {0x370c, 0x00, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{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},
-	{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},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
-	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
-	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0},
-	{0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
-	{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},
-	{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
-	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
-	{0x3503, 0, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
+static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
@@ -672,7 +504,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3503, 0, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
+static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
 	{0x3820, 0x40, 0, 0}, {0x3821, 0x06, 0, 0},
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
@@ -700,79 +532,43 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 };
 
 static const struct ov5640_mode_info
-ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
-	{
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
-		 176, 1896, 144, 984,
-		 ov5640_setting_15fps_QCIF_176_144,
-		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
-		 320, 1896, 240, 984,
-		 ov5640_setting_15fps_QVGA_320_240,
-		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
-		 640, 1896, 480, 1080,
-		 ov5640_setting_15fps_VGA_640_480,
-		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
-		 720, 1896, 480, 984,
-		 ov5640_setting_15fps_NTSC_720_480,
-		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
-		 720, 1896, 576, 984,
-		 ov5640_setting_15fps_PAL_720_576,
-		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
-		 1024, 1896, 768, 1080,
-		 ov5640_setting_15fps_XGA_1024_768,
-		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
-		 1280, 1892, 720, 740,
-		 ov5640_setting_15fps_720P_1280_720,
-		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING,
-		 1920, 2500, 1080, 1120,
-		 ov5640_setting_15fps_1080P_1920_1080,
-		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, SCALING,
-		 2592, 2844, 1944, 1968,
-		 ov5640_setting_15fps_QSXGA_2592_1944,
-		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
-	}, {
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
-		 176, 1896, 144, 984,
-		 ov5640_setting_30fps_QCIF_176_144,
-		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
-		 320, 1896, 240, 984,
-		 ov5640_setting_30fps_QVGA_320_240,
-		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
-		 640, 1896, 480, 1080,
-		 ov5640_setting_30fps_VGA_640_480,
-		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
-		 720, 1896, 480, 984,
-		 ov5640_setting_30fps_NTSC_720_480,
-		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
-		 720, 1896, 576, 984,
-		 ov5640_setting_30fps_PAL_720_576,
-		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
-		 1024, 1896, 768, 1080,
-		 ov5640_setting_30fps_XGA_1024_768,
-		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
-		 1280, 1892, 720, 740,
-		 ov5640_setting_30fps_720P_1280_720,
-		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING,
-		 1920, 2500, 1080, 1120,
-		 ov5640_setting_30fps_1080P_1920_1080,
-		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
-	},
+ov5640_mode_data[OV5640_NUM_MODES] = {
+	{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
+	 176, 1896, 144, 984,
+	 ov5640_setting_QCIF_176_144,
+	 ARRAY_SIZE(ov5640_setting_QCIF_176_144)},
+	{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
+	 320, 1896, 240, 984,
+	 ov5640_setting_QVGA_320_240,
+	 ARRAY_SIZE(ov5640_setting_QVGA_320_240)},
+	{OV5640_MODE_VGA_640_480, SUBSAMPLING,
+	 640, 1896, 480, 1080,
+	 ov5640_setting_VGA_640_480,
+	 ARRAY_SIZE(ov5640_setting_VGA_640_480)},
+	{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
+	 720, 1896, 480, 984,
+	 ov5640_setting_NTSC_720_480,
+	 ARRAY_SIZE(ov5640_setting_NTSC_720_480)},
+	{OV5640_MODE_PAL_720_576, SUBSAMPLING,
+	 720, 1896, 576, 984,
+	 ov5640_setting_PAL_720_576,
+	 ARRAY_SIZE(ov5640_setting_PAL_720_576)},
+	{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
+	 1024, 1896, 768, 1080,
+	 ov5640_setting_XGA_1024_768,
+	 ARRAY_SIZE(ov5640_setting_XGA_1024_768)},
+	{OV5640_MODE_720P_1280_720, SUBSAMPLING,
+	 1280, 1892, 720, 740,
+	 ov5640_setting_720P_1280_720,
+	 ARRAY_SIZE(ov5640_setting_720P_1280_720)},
+	{OV5640_MODE_1080P_1920_1080, SCALING,
+	 1920, 2500, 1080, 1120,
+	 ov5640_setting_1080P_1920_1080,
+	 ARRAY_SIZE(ov5640_setting_1080P_1920_1080)},
+	{OV5640_MODE_QSXGA_2592_1944, SCALING,
+	 2592, 2844, 1944, 1968,
+	 ov5640_setting_QSXGA_2592_1944,
+	 ARRAY_SIZE(ov5640_setting_QSXGA_2592_1944)},
 };
 
 static int ov5640_init_slave_id(struct ov5640_dev *sensor)
@@ -1586,7 +1382,7 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 	int i;
 
 	for (i = OV5640_NUM_MODES - 1; i >= 0; i--) {
-		mode = &ov5640_mode_data[fr][i];
+		mode = &ov5640_mode_data[i];
 
 		if (!mode->reg_data)
 			continue;
@@ -1594,12 +1390,20 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 		if ((nearest && mode->hact <= width &&
 		     mode->vact <= height) ||
 		    (!nearest && mode->hact == width &&
-		     mode->vact == height))
+		     mode->vact == height)) {
+
+			/* 2592x1944 can only operate at 15fps */
+			if (width == 2592 && height == 1944 &&
+			    fr != OV5640_15_FPS)
+				/* try to find another mode */
+				continue;
+
 			break;
+		}
 	}
 
 	if (nearest && i < 0)
-		mode = &ov5640_mode_data[fr][0];
+		mode = &ov5640_mode_data[0];
 
 	return mode;
 }
@@ -2586,10 +2390,10 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	fse->min_width =
-		ov5640_mode_data[0][fse->index].hact;
+		ov5640_mode_data[fse->index].hact;
 	fse->max_width = fse->min_width;
 	fse->min_height =
-		ov5640_mode_data[0][fse->index].vact;
+		ov5640_mode_data[fse->index].vact;
 	fse->max_height = fse->min_height;
 
 	return 0;
@@ -2802,7 +2606,7 @@ static int ov5640_probe(struct i2c_client *client,
 	sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS];
 	sensor->current_fr = OV5640_30_FPS;
 	sensor->current_mode =
-		&ov5640_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
+		&ov5640_mode_data[OV5640_MODE_VGA_640_480];
 	sensor->pending_mode_change = true;
 
 	sensor->ae_target = 52;
Signature · 1 line
2.17.0
#19 lore raw

PATCH v2 11/12 media: ov5640: Add 60 fps support

In reply to Maxime Ripard

Now that we have everything in place to compute the clock rate at runtime, we can enable the 60fps framerate for the mode we tested it with.

---
 drivers/media/i2c/ov5640.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)
drivers/media/i2c/ov5640.c +25 -8
index 690ed0238763..c01bbc5f9f34 100644
@@ -112,6 +112,7 @@ enum ov5640_mode_id {
 enum ov5640_frame_rate {
 	OV5640_15_FPS = 0,
 	OV5640_30_FPS,
+	OV5640_60_FPS,
 	OV5640_NUM_FRAMERATES,
 };
 
@@ -140,6 +141,7 @@ MODULE_PARM_DESC(virtual_channel,
 static const int ov5640_framerates[] = {
 	[OV5640_15_FPS] = 15,
 	[OV5640_30_FPS] = 30,
+	[OV5640_60_FPS] = 60,
 };
 
 /* regulator supplies */
@@ -1398,12 +1400,19 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 				/* try to find another mode */
 				continue;
 
+			/* Only 640x480 can operate at 60fps (for now) */
+			if (fr == OV5640_60_FPS &&
+			    width != 640 && height != 480)
+				/* try to find another mode */
+				continue;
+
 			break;
 		}
 	}
 
+	/* VGA is the only mode that supports all the framerates */
 	if (nearest && i < 0)
-		mode = &ov5640_mode_data[0];
+		mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480];
 
 	return mode;
 }
@@ -1848,12 +1857,13 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	int ret;
 
 	minfps = ov5640_framerates[OV5640_15_FPS];
-	maxfps = ov5640_framerates[OV5640_30_FPS];
+	maxfps = ov5640_framerates[OV5640_60_FPS];
 
 	if (fi->numerator == 0) {
 		fi->denominator = maxfps;
 		fi->numerator = 1;
-		return OV5640_30_FPS;
+		ret = OV5640_60_FPS;
+		goto find_mode;
 	}
 
 	fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
@@ -1865,11 +1875,15 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 		fi->denominator = minfps;
 	else if (2 * fps >= 2 * minfps + (maxfps - minfps))
 		fi->denominator = maxfps;
+
+	if (fi->denominator == minfps)
+		ret = OV5640_15_FPS;
+	else if (fi->denominator == maxfps)
+		ret = OV5640_60_FPS;
 	else
-		fi->denominator = minfps;
-
-	ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+		ret = OV5640_30_FPS;
 
+find_mode:
 	mode = ov5640_find_mode(sensor, ret, width, height, false);
 	return mode ? ret : -EINVAL;
 }
@@ -2458,8 +2472,11 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
 
 	frame_rate = ov5640_try_frame_interval(sensor, &fi->interval,
 					       mode->hact, mode->vact);
-	if (frame_rate < 0)
-		frame_rate = OV5640_15_FPS;
+	if (frame_rate < 0) {
+		/* Always return a valid frame interval value */
+		fi->interval = sensor->frame_interval;
+		goto out;
+	}
 
 	sensor->current_fr = frame_rate;
 	sensor->frame_interval = fi->interval;
Signature · 1 line
2.17.0
#20 lore raw

Re: [PATCH v2 11/12] media: ov5640: Add 60 fps support

Hi Maxime,

I've taken the whole serie and made some tests on STM32 platform using DVP parallel interface.
Now JPEG is OK and I've not seen any regressions appart on framerate control linked to this current patchset.

Here are issues observed around framerate control:
1) Framerate enumeration is buggy and all resolutions are claimed supporting 15/30/60:
v4l2-ctl --list-formats-ext
ioctl: VIDIOC_ENUM_FMT

         Index       : 0
         Type        : Video Capture
         Pixel Format: 'JPEG' (compressed)
         Name        : JFIF JPEG
                 Size: Discrete 176x144
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 320x240
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 640x480
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 720x480
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 720x576
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 1024x768
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 1280x720
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 1920x1080
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 2592x1944
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)

2) Frame interval setting is returning incorrect value (*1000):
v4l2-ctl --set-parm=15
<
Frame rate set to 15000.000 fps

3) After having fixed 1) and 2), 720x480 60fps is still supported:

                 Size: Discrete 640x480
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)
                 Size: Discrete 720x480
                         Interval: Discrete 0.067s (15.000 fps)
                         Interval: Discrete 0.033s (30.000 fps)
                         Interval: Discrete 0.017s (60.000 fps)

Some fixes are proposed below:

Quoted 29 lines · Maxime Ripard

Now that we have everything in place to compute the clock rate at runtime, we can enable the 60fps framerate for the mode we tested it with.

---
  drivers/media/i2c/ov5640.c | 33 +++++++++++++++++++++++++--------
  1 file changed, 25 insertions(+), 8 deletions(-)
drivers/media/i2c/ov5640.c +2 -0
index 690ed0238763..c01bbc5f9f34 100644
@@ -112,6 +112,7 @@ enum ov5640_mode_id {
  enum ov5640_frame_rate {
  	OV5640_15_FPS = 0,
  	OV5640_30_FPS,
+	OV5640_60_FPS,
  	OV5640_NUM_FRAMERATES,
  };
 
@@ -140,6 +141,7 @@ MODULE_PARM_DESC(virtual_channel,
  static const int ov5640_framerates[] = {
  	[OV5640_15_FPS] = 15,
  	[OV5640_30_FPS] = 30,
+	[OV5640_60_FPS] = 60,
  };
 
  /* regulator supplies */

drivers/media/i2c/ov5640.c ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr, · line 1405

Quoted 6 lines
drivers/media/i2c/ov5640.c +3 -0
@@ -1398,12 +1400,19 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
  				/* try to find another mode */
  				continue;
 
+			/* Only 640x480 can operate at 60fps (for now) */
+			if (fr == OV5640_60_FPS &&
+			    width != 640 && height != 480)

Should be
+ !(width == 640 && height == 480))
otherwise 720x480 is also supported (bug 3))

drivers/media/i2c/ov5640.c ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr, · line 1415

Quoted 11 lines
drivers/media/i2c/ov5640.c +5 -1
+				/* try to find another mode */
+				continue;
+
  			break;
  		}
  	}
 
+	/* VGA is the only mode that supports all the framerates */
  	if (nearest && i < 0)
-		mode = &ov5640_mode_data[0];
+		mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480];

Code is missing to reject unsupported framerates when nearest is not set, this fix enumeration bug 1):
- if (nearest && i < 0)
+ if (i < 0) {
+ /* no match */
+ if (!nearest)
+ return NULL;
mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480];
+ }

Anyway I need to rework this code using new Sakari v4l2_find_nearest_size() code as stated in:
https://patchwork.linuxtv.org/patch/47767/

Quoted 20 lines
drivers/media/i2c/ov5640.c +3 -2
  	return mode;
  }
@@ -1848,12 +1857,13 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
  	int ret;
 
  	minfps = ov5640_framerates[OV5640_15_FPS];
-	maxfps = ov5640_framerates[OV5640_30_FPS];
+	maxfps = ov5640_framerates[OV5640_60_FPS];
 
  	if (fi->numerator == 0) {
  		fi->denominator = maxfps;
  		fi->numerator = 1;
-		return OV5640_30_FPS;
+		ret = OV5640_60_FPS;
+		goto find_mode;
  	}
 
  	fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);

drivers/media/i2c/ov5640.c static int ov5640_try_frame_interval(struct ov5640_dev *sensor, · line 1877

Quoted 3 lines
drivers/media/i2c/ov5640.c +0 -0
@@ -1865,11 +1875,15 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
  		fi->denominator = minfps;
  	else if (2 * fps >= 2 * minfps + (maxfps - minfps))
  		fi->denominator = maxfps;

else is missing here leading to denominator untouched while numerator has been reset to 1, leading to bug 2).
In fact, original code is valid for only 2 values, min and max, by adding a third fps value, rework of the conditions are needed using a "mid" value, see proposed code:

static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
struct v4l2_fract *fi, u32 width, u32 height)
{
const struct ov5640_mode_info *mode;
- u32 minfps, maxfps, fps;
+ u32 minfps, midfps, maxfps, fps;
int ret;

minfps = ov5640_framerates[OV5640_15_FPS];
- maxfps = ov5640_framerates[OV5640_30_FPS];
+ midfps = ov5640_framerates[OV5640_30_FPS];
+ maxfps = ov5640_framerates[OV5640_60_FPS];

if (fi->numerator == 0) {
fi->denominator = maxfps;
fi->numerator = 1;
- return OV5640_30_FPS;
+ ret = OV5640_60_FPS;
+ goto find_mode;
}

  	fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);

fi->numerator = 1;
- if (fps > maxfps)
+ if ((fps >= maxfps) ||
+ ((2 * fps >= (midfps + maxfps)) && (fps < maxfps)))
fi->denominator = maxfps;
- else if (fps < minfps)
+ else if ((fps <= minfps) ||
+ ((2 * fps <= (minfps + midfps)) && (fps > minfps)))
fi->denominator = minfps;
- else if (2 * fps >= 2 * minfps + (maxfps - minfps))
- fi->denominator = maxfps;
else
- fi->denominator = minfps;
+ fi->denominator = midfps;

- ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+ if (fi->denominator == minfps)
+ ret = OV5640_15_FPS;
+ else if (fi->denominator == maxfps)
+ ret = OV5640_60_FPS;
+ else
+ ret = OV5640_30_FPS;

+find_mode:
mode = ov5640_find_mode(sensor, ret, width, height, false); return mode ? ret : -EINVAL;
}

This code has been tested using v4l2-ctl:

v4l2-ctl --set-fmt-video=width=640,height=480,pixelformat=RGBP v4l2-ctl --set-parm=14
v4l2-ctl --set-parm=15
v4l2-ctl --set-parm=16
v4l2-ctl --set-parm=22
v4l2-ctl --set-parm=23
v4l2-ctl --set-parm=24
v4l2-ctl --set-parm=29
v4l2-ctl --set-parm=30
v4l2-ctl --set-parm=31
v4l2-ctl --set-parm=36
v4l2-ctl --set-parm=42
v4l2-ctl --set-parm=45
v4l2-ctl --set-parm=46
v4l2-ctl --set-parm=59
v4l2-ctl --set-parm=60
v4l2-ctl --set-parm=61

root@stm32mp1:~# v4l2-ctl --set-parm=14
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=15
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=16
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=22
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=23
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=24
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=29
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=30
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=31
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=36
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=42
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=45
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=46
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=59
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=60
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=61
Frame rate set to 60.000 fps
Quoted 5 lines
drivers/media/i2c/ov5640.c +0 -0
@@ -2458,8 +2472,11 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
 
  	frame_rate = ov5640_try_frame_interval(sensor, &fi->interval,
  					       mode->hact, mode->vact);

drivers/media/i2c/ov5640.c static int ov5640_s_frame_interval(struct v4l2_subdev *sd, · line 2482

Quoted 10 lines
drivers/media/i2c/ov5640.c +5 -2
-	if (frame_rate < 0)
-		frame_rate = OV5640_15_FPS;
+	if (frame_rate < 0) {
+		/* Always return a valid frame interval value */
+		fi->interval = sensor->frame_interval;
+		goto out;
+	}
 
  	sensor->current_fr = frame_rate;
  	sensor->frame_interval = fi->interval;

Best regards,
Hugues.

#21 lore raw

Re: [PATCH v2 11/12] media: ov5640: Add 60 fps support

Hi Hugues,

Quoted 4 lines · Hugues FRUCHET

I've taken the whole serie and made some tests on STM32 platform using DVP parallel interface.
Now JPEG is OK and I've not seen any regressions appart on framerate control linked to this current patchset.

Thanks a lot for your feedback, I've (hopefully) fixed all the issues you reported here, most of the time taking your fix directly, except for 2 where I reworked the code instead.

Quoted 5 lines

Here are issues observed around framerate control:
1) Framerate enumeration is buggy and all resolutions are claimed supporting 15/30/60:
v4l2-ctl --list-formats-ext
ioctl: VIDIOC_ENUM_FMT

Quoted 8 lines

Index : 0
Type : Video Capture
Pixel Format: 'JPEG' (compressed)
Name : JFIF JPEG
Size: Discrete 176x144
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)

One small question though, I don't seem to have that output for v4l2-ctl, is some hook needed in the v4l2 device for it to work?

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#22 lore raw

Re: [PATCH v2 11/12] media: ov5640: Add 60 fps support

Hi Maxime,

Thanks for fixes !

No special modification of v4l2-ctl, I'm using currently v4l-utils 1.12.3. What output do you have ?

Best regards,
Hugues.

Quoted 31 lines · Maxime Ripard

Hi Hugues,

Quoted 4 lines · Hugues FRUCHET

I've taken the whole serie and made some tests on STM32 platform using DVP parallel interface.
Now JPEG is OK and I've not seen any regressions appart on framerate control linked to this current patchset.

Thanks a lot for your feedback, I've (hopefully) fixed all the issues you reported here, most of the time taking your fix directly, except for 2 where I reworked the code instead.

Quoted 5 lines

Here are issues observed around framerate control:
1) Framerate enumeration is buggy and all resolutions are claimed supporting 15/30/60:
v4l2-ctl --list-formats-ext
ioctl: VIDIOC_ENUM_FMT

Quoted 8 lines

Index : 0
Type : Video Capture
Pixel Format: 'JPEG' (compressed)
Name : JFIF JPEG
Size: Discrete 176x144
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)

One small question though, I don't seem to have that output for v4l2-ctl, is some hook needed in the v4l2 device for it to work?

Maxime

#23 lore raw

Re: [PATCH v2 11/12] media: ov5640: Add 60 fps support

Hi Hugues,

Quoted 2 lines · Hugues FRUCHET

No special modification of v4l2-ctl, I'm using currently v4l-utils 1.12.3. What output do you have ?

The same one, without the resolution and framerate. I'm pretty sure this is a driver issue and not an usperspace one.

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#24 lore raw

PATCH v2 12/12 media: ov5640: Remove duplicate auto-exposure setup

In reply to Maxime Ripard

The autoexposure setup in the 1080p init array is redundant with the default value of the sensor.

Remove it.

---
 drivers/media/i2c/ov5640.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/i2c/ov5640.c +1 -1
index c01bbc5f9f34..e7e167a515f2 100644
@@ -503,7 +503,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
 	{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
 	{0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 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}, {0x3503, 0, 0, 0},
+	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
Signature · 1 line
2.17.0
#25 lore raw
In reply to Maxime Ripard

I've been digging around the ov5640.c code for a few weeks now, these look like some solid improvements. I'll give them a shot and let you know how they work.

On that note, I'm bringing up a module that uses dual lane MIPI with a 12MHz fixed oscillator for xclk (Digilent's Pcam 5c). The mainline version of the driver seems to only support xclk of 22MHz (or maybe 24MHz), despite allowing xclk values from 6-24MHz. Will any of these patches add support for a 12MHz xclk while in MIPI mode?

Sam
-----------------------
Sam Bobrowicz
Elite Embedded Consulting LLC
elite-embedded.com

Quoted 46 lines · Maxime Ripard

Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code, slowly getting rid of the big data array for more understandable code (hopefully).

The biggest addition would be the clock rate computation at runtime, instead of relying on those arrays to setup the clock tree properly. As a side effect, it fixes the framerate that was off by around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v1:
- Integrated Hugues' suggestions to fix v4l2-compliance
- Fixed the bus width with JPEG
- Dropped the clock rate calculation loops for something simpler as suggested by Sakari
- Cache the exposure value instead of using the control value
- Rebased on top of 4.17

Maxime Ripard (10):
media: ov5640: Don't force the auto exposure state at start time
media: ov5640: Init properly the SCLK dividers
media: ov5640: Change horizontal and vertical resolutions name
media: ov5640: Add horizontal and vertical totals
media: ov5640: Program the visible resolution
media: ov5640: Adjust the clock based on the expected rate
media: ov5640: Compute the clock rate at runtime
media: ov5640: Enhance FPS handling
media: ov5640: Add 60 fps support
media: ov5640: Remove duplicate auto-exposure setup

Mylène Josserand (2):
media: ov5640: Add auto-focus feature
media: ov5640: Add light frequency control

 drivers/media/i2c/ov5640.c | 752 +++++++++++++++++++++----------------
 1 file changed, 422 insertions(+), 330 deletions(-)

--
2.17.0

#26 lore raw
Quoted 3 lines · Samuel Bobrowicz

I've been digging around the ov5640.c code for a few weeks now, these look like some solid improvements. I'll give them a shot and let you know how they work.

Great, thanks!

Quoted 5 lines

On that note, I'm bringing up a module that uses dual lane MIPI with a 12MHz fixed oscillator for xclk (Digilent's Pcam 5c). The mainline version of the driver seems to only support xclk of 22MHz (or maybe 24MHz), despite allowing xclk values from 6-24MHz. Will any of these patches add support for a 12MHz xclk while in MIPI mode?

My setup has a 24MHz crystal, and work with a parallel bus so I haven't been able to test yours. However, yeah, I guess my patches will improve your situation a lot.

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#27 lore raw

Hi Maxime,

I applied your patches, and they are a big improvement for what I am trying to do, but things still aren't working right on my platform.

How confident are you that the MIPI mode will work with this version of the driver? I am having issues that I believe are due to incorrect clock generation. Our engineers did some reverse engineering of the clock tree themselves, and came up with a slightly different model. I've captured their model in a spreadsheet here:
https://tinyurl.com/pll-calc . Just modify the register and xclk values to see the clocks change. Do your tests disagree with this potential model?

I'm not sure which model is more correct, but my tests suggest the high speed MIPI clock is generated directly off the PLL. This means the PLL multiplier you are generating in your algorithm is not high enough to satisfy the bandwidth. If this is the case, MIPI mode will require a different set of parameters that enable some of the downstream dividers, so that the PLL multiplier can be higher while the PCLK value still matches the needed rate calculated from the resolution.

Any thoughts on this before I dive in and start tweaking the algorithm in mipi mode?

Sam
-----------------------
Sam Bobrowicz
Elite Embedded Consulting LLC
elite-embedded.com

Quoted 23 lines · Maxime Ripard
Quoted 3 lines · Samuel Bobrowicz

I've been digging around the ov5640.c code for a few weeks now, these look like some solid improvements. I'll give them a shot and let you know how they work.

Great, thanks!

Quoted 5 lines

On that note, I'm bringing up a module that uses dual lane MIPI with a 12MHz fixed oscillator for xclk (Digilent's Pcam 5c). The mainline version of the driver seems to only support xclk of 22MHz (or maybe 24MHz), despite allowing xclk values from 6-24MHz. Will any of these patches add support for a 12MHz xclk while in MIPI mode?

My setup has a 24MHz crystal, and work with a parallel bus so I haven't been able to test yours. However, yeah, I guess my patches will improve your situation a lot.

Maxime

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

#28 lore raw

Hi Samuel,

Quoted 5 lines · Samuel Bobrowicz

I applied your patches, and they are a big improvement for what I am trying to do, but things still aren't working right on my platform.

How confident are you that the MIPI mode will work with this version of the driver?

Not too confident. Like I said, I did all my tests on a parallel camera with a scope, so I'm pretty confident for the parallel bus. But I haven't been able to test the MIPI-CSI side of things and tried to deduce it from the datasheet.

tl; dr: I might very well be wrong.

Quoted 7 lines

I am having issues that I believe are due to incorrect clock generation. Our engineers did some reverse engineering of the clock tree themselves, and came up with a slightly different model. I've captured their model in a spreadsheet here:
https://tinyurl.com/pll-calc . Just modify the register and xclk values to see the clocks change. Do your tests disagree with this potential model?

At least on the parallel side, it looks fairly similar, so I guess we can come to an agreement :)

There's just the SCLK2x divider that is no longer in the path to PCLK but has been replaced with BIT Divider that has the same value, so it should work as well.

Quoted 11 lines

I'm not sure which model is more correct, but my tests suggest the high speed MIPI clock is generated directly off the PLL. This means the PLL multiplier you are generating in your algorithm is not high enough to satisfy the bandwidth. If this is the case, MIPI mode will require a different set of parameters that enable some of the downstream dividers, so that the PLL multiplier can be higher while the PCLK value still matches the needed rate calculated from the resolution.

Any thoughts on this before I dive in and start tweaking the algorithm in mipi mode?

Like I said, I did that analysis by plugging the camera to a scope and look at the PCLK generated for various combinations. Your analysis seems not too far off for the setup I've tested, so I guess this makes sense. And let me know how it works for MIPI-CSI2 so that I can update the patches :)

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#29 lore raw

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

--Sam
-----------------------
Sam Bobrowicz
Elite Embedded Consulting LLC
elite-embedded.com

Quoted 55 lines · Maxime Ripard

Hi Samuel,

Quoted 5 lines · Samuel Bobrowicz

I applied your patches, and they are a big improvement for what I am trying to do, but things still aren't working right on my platform.

How confident are you that the MIPI mode will work with this version of the driver?

Not too confident. Like I said, I did all my tests on a parallel camera with a scope, so I'm pretty confident for the parallel bus. But I haven't been able to test the MIPI-CSI side of things and tried to deduce it from the datasheet.

tl; dr: I might very well be wrong.

Quoted 7 lines

I am having issues that I believe are due to incorrect clock generation. Our engineers did some reverse engineering of the clock tree themselves, and came up with a slightly different model. I've captured their model in a spreadsheet here:
https://tinyurl.com/pll-calc . Just modify the register and xclk values to see the clocks change. Do your tests disagree with this potential model?

At least on the parallel side, it looks fairly similar, so I guess we can come to an agreement :)

There's just the SCLK2x divider that is no longer in the path to PCLK but has been replaced with BIT Divider that has the same value, so it should work as well.

Quoted 11 lines

I'm not sure which model is more correct, but my tests suggest the high speed MIPI clock is generated directly off the PLL. This means the PLL multiplier you are generating in your algorithm is not high enough to satisfy the bandwidth. If this is the case, MIPI mode will require a different set of parameters that enable some of the downstream dividers, so that the PLL multiplier can be higher while the PCLK value still matches the needed rate calculated from the resolution.

Any thoughts on this before I dive in and start tweaking the algorithm in mipi mode?

Like I said, I did that analysis by plugging the camera to a scope and look at the PCLK generated for various combinations. Your analysis seems not too far off for the setup I've tested, so I guess this makes sense. And let me know how it works for MIPI-CSI2 so that I can update the patches :)

Maxime

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

#30 lore raw

Hi Samuel,

Quoted 2 lines · Sam Bobrowicz

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet.

Ok, good, I'll send an updated version next week taking this into account then. Thanks!

Quoted 5 lines

It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk.

It might be quite simple to support actually. Most of the other dividers were hardcoded in the driver, so maybe it's the case for those as well. I'll check and see how it goes.

Quoted 7 lines

Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

Unfortunately, I don't have an answer to that one.

Quoted 7 lines

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

I was planning on sending a new version based on your feedback for the MIPI-CSI2 formula, most likely next week. I guess you could test them and see how it goes. Or send patches on top of this version if you prefer :)

You have more documentation on how to do that here:
https://www.kernel.org/doc/Documentation/process/submitting-patches.rst

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#31 lore raw
In reply to Sam Bobrowicz

Hi Sam,

Quoted 14 lines · On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

Quoted 7 lines

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

I'm also interested in getting the ov5640 driver working with MIPI CSI-2. Studying the datasheet and the driver code, I found the stream on sequence to be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.

OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init mode data and never touched for MIPI CSI-2 (the register is only touched in ov5640_set_stream_dvp). The value means

- mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 for one lane or 001 for two lanes.

- MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY HS TX", so I suppose 0 is correct.

- MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a RX PHY, it could be a typo.

- mipi_en: 1 means MIPI enable, which should be correct.

- BIT(0) is undocumented.

OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains its default value of 0x00, and is controlled when starting and stopping the stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should be LP11 in my opinion (that's the PHY stop state). However, setting them to 0x00 when starting the stream mean that LP00 is selected as the sleep state at stream start, and LP11 when stopping the stream. Maybe "sleep mode" means LPDT, but I would expect that to be controlled by the idle status bit in register 0x4800.

OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode data, and BIT(5) is then cleared at stream on time and set at stream off time. This means:

- Clock lane gate enable: Clock lane is free running
- Line sync enable: Do not send line short packets for each line (I assume that's LS/LE)
- Lane select: Use lane1 as default data lane.
- Idle status: MIPI bus will be LP11 when no packet to transmit. I would have expected the idle status to correspond to LPDT, and thus be LP00 (as opposed to the stop state that should be LP11, which I believe is named "sleep mode" in the datasheet and controlled in register 0x3019).

BIT(5) is the clock lane gate enable, so at stream on time the clock is set to free running, and at stream off time to "Gate clock lane when no packet to transmit". Couldn't we always enable clock gating ?

Do you have any insight on this ? Have you modified the MIPI CSI-2 configuration to get the CSI-2 output working ?

Signature · 3 lines
Regards,

Laurent Pinchart
#32 lore raw
Quoted 30 lines · Laurent Pinchart

Hi Sam,

Quoted 14 lines · On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

Quoted 7 lines

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

I'm also interested in getting the ov5640 driver working with MIPI CSI-2. Studying the datasheet and the driver code, I found the stream on sequence to be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.

Quoted 6 lines

OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init mode data and never touched for MIPI CSI-2 (the register is only touched in ov5640_set_stream_dvp). The value means

- mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 for one lane or 001 for two lanes.

I noticed this too, but it seems that 010 is the correct value for two lane mode. I think this is a typo in the datasheet.

Quoted 12 lines

- MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY HS TX", so I suppose 0 is correct.

- MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a RX PHY, it could be a typo.

- mipi_en: 1 means MIPI enable, which should be correct.

- BIT(0) is undocumented.

Quoted 10 lines

OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains its default value of 0x00, and is controlled when starting and stopping the stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should be LP11 in my opinion (that's the PHY stop state). However, setting them to 0x00 when starting the stream mean that LP00 is selected as the sleep state at stream start, and LP11 when stopping the stream. Maybe "sleep mode" means LPDT, but I would expect that to be controlled by the idle status bit in register 0x4800.

I did not need to mess with the accesses to 0x3019 in order to get things working on my system. I'm not sure of this registers actual behavior, but it might affect idling while not streaming (after power on). My pipeline currently only powers the sensor while streaming, so I might be missing some ramifications of this register.

Quoted 13 lines

OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode data, and BIT(5) is then cleared at stream on time and set at stream off time. This means:

- Clock lane gate enable: Clock lane is free running
- Line sync enable: Do not send line short packets for each line (I assume that's LS/LE)
- Lane select: Use lane1 as default data lane.
- Idle status: MIPI bus will be LP11 when no packet to transmit. I would have expected the idle status to correspond to LPDT, and thus be LP00 (as opposed to the stop state that should be LP11, which I believe is named "sleep mode" in the datasheet and controlled in register 0x3019).

Quoted 3 lines

BIT(5) is the clock lane gate enable, so at stream on time the clock is set to free running, and at stream off time to "Gate clock lane when no packet to transmit". Couldn't we always enable clock gating ?

Good question, it might be worth testing. Same as above, I didn't need to mess with this reg either.

Quoted 10 lines

Do you have any insight on this ? Have you modified the MIPI CSI-2 configuration to get the CSI-2 output working ?

--
Regards,

Laurent Pinchart

Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful.

I will try to get some patches out today or tomorrow that should get you up and running.

Maxime, I'd prefer to create the patches myself for the experience. I've read all of the submission guidelines and I understand the general process, but how should I submit them to the mailing list since they are based to your patches, which are still under review? Should I send the patch series to the mailing list myself and just mention this patch series, maybe with the In-Reply-To header? Or should I just post a link to them here so you can review them and add them to a new version of your patch series?

Sam

#33 lore raw

Hi,

Quoted 98 lines · Sam Bobrowicz
Quoted 28 lines · On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:
Quoted 14 lines

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

Quoted 7 lines

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

I'm also interested in getting the ov5640 driver working with MIPI CSI-2. Studying the datasheet and the driver code, I found the stream on sequence to be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.

Quoted 6 lines

OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init mode data and never touched for MIPI CSI-2 (the register is only touched in ov5640_set_stream_dvp). The value means

- mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 for one lane or 001 for two lanes.

I noticed this too, but it seems that 010 is the correct value for two lane mode. I think this is a typo in the datasheet.

Quoted 12 lines

- MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY HS TX", so I suppose 0 is correct.

- MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a RX PHY, it could be a typo.

- mipi_en: 1 means MIPI enable, which should be correct.

- BIT(0) is undocumented.

Quoted 10 lines

OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains its default value of 0x00, and is controlled when starting and stopping the stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should be LP11 in my opinion (that's the PHY stop state). However, setting them to 0x00 when starting the stream mean that LP00 is selected as the sleep state at stream start, and LP11 when stopping the stream. Maybe "sleep mode" means LPDT, but I would expect that to be controlled by the idle status bit in register 0x4800.

I did not need to mess with the accesses to 0x3019 in order to get things working on my system. I'm not sure of this registers actual behavior, but it might affect idling while not streaming (after power on). My pipeline currently only powers the sensor while streaming, so I might be missing some ramifications of this register.

Quoted 13 lines

OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode data, and BIT(5) is then cleared at stream on time and set at stream off time. This means:

- Clock lane gate enable: Clock lane is free running
- Line sync enable: Do not send line short packets for each line (I assume that's LS/LE)
- Lane select: Use lane1 as default data lane.
- Idle status: MIPI bus will be LP11 when no packet to transmit. I would have expected the idle status to correspond to LPDT, and thus be LP00 (as opposed to the stop state that should be LP11, which I believe is named "sleep mode" in the datasheet and controlled in register 0x3019).

Quoted 3 lines

BIT(5) is the clock lane gate enable, so at stream on time the clock is set to free running, and at stream off time to "Gate clock lane when no packet to transmit". Couldn't we always enable clock gating ?

Good question, it might be worth testing. Same as above, I didn't need to mess with this reg either.

Quoted 2 lines

Do you have any insight on this ? Have you modified the MIPI CSI-2 configuration to get the CSI-2 output working ?

Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful.

I will try to get some patches out today or tomorrow that should get you up and running.

Quoted 8 lines

Maxime, I'd prefer to create the patches myself for the experience. I've read all of the submission guidelines and I understand the general process, but how should I submit them to the mailing list since they are based to your patches, which are still under review? Should I send the patch series to the mailing list myself and just mention this patch series, maybe with the In-Reply-To header? Or should I just post a link to them here so you can review them and add them to a new version of your patch series?

That's definitely something you can do if you want to try it out :)

However, we shouldn't break the bisectability either, so this will need to be folded into this serie eventually.

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#34 lore raw

Hi,

Quoted 115 lines · Maxime Ripard

Hi,

Quoted 98 lines · Sam Bobrowicz
Quoted 28 lines · On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:
Quoted 14 lines

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

Quoted 7 lines

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

I'm also interested in getting the ov5640 driver working with MIPI CSI-2. Studying the datasheet and the driver code, I found the stream on sequence to be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.

Quoted 6 lines

OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init mode data and never touched for MIPI CSI-2 (the register is only touched in ov5640_set_stream_dvp). The value means

- mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 for one lane or 001 for two lanes.

I noticed this too, but it seems that 010 is the correct value for two lane mode. I think this is a typo in the datasheet.

Quoted 12 lines

- MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY HS TX", so I suppose 0 is correct.

- MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a RX PHY, it could be a typo.

- mipi_en: 1 means MIPI enable, which should be correct.

- BIT(0) is undocumented.

Quoted 10 lines

OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains its default value of 0x00, and is controlled when starting and stopping the stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should be LP11 in my opinion (that's the PHY stop state). However, setting them to 0x00 when starting the stream mean that LP00 is selected as the sleep state at stream start, and LP11 when stopping the stream. Maybe "sleep mode" means LPDT, but I would expect that to be controlled by the idle status bit in register 0x4800.

I did not need to mess with the accesses to 0x3019 in order to get things working on my system. I'm not sure of this registers actual behavior, but it might affect idling while not streaming (after power on). My pipeline currently only powers the sensor while streaming, so I might be missing some ramifications of this register.

Quoted 13 lines

OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode data, and BIT(5) is then cleared at stream on time and set at stream off time. This means:

- Clock lane gate enable: Clock lane is free running
- Line sync enable: Do not send line short packets for each line (I assume that's LS/LE)
- Lane select: Use lane1 as default data lane.
- Idle status: MIPI bus will be LP11 when no packet to transmit. I would have expected the idle status to correspond to LPDT, and thus be LP00 (as opposed to the stop state that should be LP11, which I believe is named "sleep mode" in the datasheet and controlled in register 0x3019).

Quoted 3 lines

BIT(5) is the clock lane gate enable, so at stream on time the clock is set to free running, and at stream off time to "Gate clock lane when no packet to transmit". Couldn't we always enable clock gating ?

Good question, it might be worth testing. Same as above, I didn't need to mess with this reg either.

Quoted 2 lines

Do you have any insight on this ? Have you modified the MIPI CSI-2 configuration to get the CSI-2 output working ?

Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful.

I will try to get some patches out today or tomorrow that should get you up and running.

Quoted 8 lines

Maxime, I'd prefer to create the patches myself for the experience. I've read all of the submission guidelines and I understand the general process, but how should I submit them to the mailing list since they are based to your patches, which are still under review? Should I send the patch series to the mailing list myself and just mention this patch series, maybe with the In-Reply-To header? Or should I just post a link to them here so you can review them and add them to a new version of your patch series?

That's definitely something you can do if you want to try it out :)

However, we shouldn't break the bisectability either, so this will need to be folded into this serie eventually.

Quoted 6 lines

Maxime

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

FYI, I've found the following document:
https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg

So it seems that clock tree is a bit different.

So, from my understanding we need to calculate dividers so that:
sclk = vtot * htot * fps
pclk = sclk * byte-per-pixel
mipisclk = 8 * pclk / num-lanes

Regards,
Loic

#35 lore raw
Quoted 125 lines

Hi,

Quoted 115 lines · Maxime Ripard

Hi,

Quoted 98 lines · Sam Bobrowicz
Quoted 28 lines · On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:
Quoted 14 lines

FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful.

Quoted 7 lines

I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :)

I'm also interested in getting the ov5640 driver working with MIPI CSI-2. Studying the datasheet and the driver code, I found the stream on sequence to be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.

Quoted 6 lines

OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init mode data and never touched for MIPI CSI-2 (the register is only touched in ov5640_set_stream_dvp). The value means

- mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 for one lane or 001 for two lanes.

I noticed this too, but it seems that 010 is the correct value for two lane mode. I think this is a typo in the datasheet.

Quoted 12 lines

- MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY HS TX", so I suppose 0 is correct.

- MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a RX PHY, it could be a typo.

- mipi_en: 1 means MIPI enable, which should be correct.

- BIT(0) is undocumented.

Quoted 10 lines

OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains its default value of 0x00, and is controlled when starting and stopping the stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should be LP11 in my opinion (that's the PHY stop state). However, setting them to 0x00 when starting the stream mean that LP00 is selected as the sleep state at stream start, and LP11 when stopping the stream. Maybe "sleep mode" means LPDT, but I would expect that to be controlled by the idle status bit in register 0x4800.

I did not need to mess with the accesses to 0x3019 in order to get things working on my system. I'm not sure of this registers actual behavior, but it might affect idling while not streaming (after power on). My pipeline currently only powers the sensor while streaming, so I might be missing some ramifications of this register.

Quoted 13 lines

OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode data, and BIT(5) is then cleared at stream on time and set at stream off time. This means:

- Clock lane gate enable: Clock lane is free running
- Line sync enable: Do not send line short packets for each line (I assume that's LS/LE)
- Lane select: Use lane1 as default data lane.
- Idle status: MIPI bus will be LP11 when no packet to transmit. I would have expected the idle status to correspond to LPDT, and thus be LP00 (as opposed to the stop state that should be LP11, which I believe is named "sleep mode" in the datasheet and controlled in register 0x3019).

Quoted 3 lines

BIT(5) is the clock lane gate enable, so at stream on time the clock is set to free running, and at stream off time to "Gate clock lane when no packet to transmit". Couldn't we always enable clock gating ?

Good question, it might be worth testing. Same as above, I didn't need to mess with this reg either.

Quoted 2 lines

Do you have any insight on this ? Have you modified the MIPI CSI-2 configuration to get the CSI-2 output working ?

Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful.

I will try to get some patches out today or tomorrow that should get you up and running.

Quoted 8 lines

Maxime, I'd prefer to create the patches myself for the experience. I've read all of the submission guidelines and I understand the general process, but how should I submit them to the mailing list since they are based to your patches, which are still under review? Should I send the patch series to the mailing list myself and just mention this patch series, maybe with the In-Reply-To header? Or should I just post a link to them here so you can review them and add them to a new version of your patch series?

That's definitely something you can do if you want to try it out :)

However, we shouldn't break the bisectability either, so this will need to be folded into this serie eventually.

Quoted 6 lines

Maxime

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

Quoted 4 lines

FYI, I've found the following document:
https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg

So it seems that clock tree is a bit different.

So that is where the Digilent RnD team got their information. I was working with an ASCII representation of the same diagram, thanks for providing that.

Quoted 8 lines

So, from my understanding we need to calculate dividers so that:
sclk = vtot * htot * fps
pclk = sclk * byte-per-pixel
mipisclk = 8 * pclk / num-lanes

Regards,
Loic

Unfortunately there are more factors at play depending on what features you are using in the ISP. Easiest way for me to describe them is to just complete my patches, then we can discuss further.

Sam

#36 lore raw
Quoted 10 lines · Sam Bobrowicz
Quoted 4 lines

FYI, I've found the following document:
https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg

So it seems that clock tree is a bit different.

So that is where the Digilent RnD team got their information. I was working with an ASCII representation of the same diagram, thanks for providing that.

Quoted 1 line
Quoted 8 lines
Quoted 4 lines

So, from my understanding we need to calculate dividers so that:
sclk = vtot * htot * fps
pclk = sclk * byte-per-pixel
mipisclk = 8 * pclk / num-lanes

Unfortunately there are more factors at play depending on what features you are using in the ISP. Easiest way for me to describe them is to just complete my patches, then we can discuss further.

Do we support any of those features at the moment? If not, then we shouldn't worry about it too much for now.

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#37 lore raw
In reply to Sam Bobrowicz

Hi,

Quoted 4 lines

Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful.

Great, I also try to make it work with MIPI-CSI2, If you have found the magic formula to configure the registers, I would be pleased to test it on my side.

Quoted 4 lines

I will try to get some patches out today or tomorrow that should get you up and running.

Quoted 8 lines

Maxime, I'd prefer to create the patches myself for the experience. I've read all of the submission guidelines and I understand the general process, but how should I submit them to the mailing list since they are based to your patches, which are still under review? Should I send the patch series to the mailing list myself and just mention this patch series, maybe with the In-Reply-To header? Or should I just post a link to them here so you can review them and add them to a new version of your patch series?

Yes, I think your patch(es) should be integrated in the Maxime's series.

Regards,
Loic

#38 lore raw
Quoted 24 lines

Hi,

Quoted 4 lines

Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful.

Great, I also try to make it work with MIPI-CSI2, If you have found the magic formula to configure the registers, I would be pleased to test it on my side.

Quoted 4 lines

I will try to get some patches out today or tomorrow that should get you up and running.

Quoted 8 lines

Maxime, I'd prefer to create the patches myself for the experience. I've read all of the submission guidelines and I understand the general process, but how should I submit them to the mailing list since they are based to your patches, which are still under review? Should I send the patch series to the mailing list myself and just mention this patch series, maybe with the In-Reply-To header? Or should I just post a link to them here so you can review them and add them to a new version of your patch series?

Quoted 4 lines

Yes, I think your patch(es) should be integrated in the Maxime's series.

Regards,
Loic

Based on Maxime's reply, I have decided to make the patches based off of that series, post it on dropbox, and provide a link here. Then we can discuss how to integrate them into v2 of the series.

The patches are taking longer than expected, my tree is pretty chaotic since I've been running rapid-fire experiments for weeks now. I'm still working on getting the changes organized into an appropriate set of patches. I think they will go out over the weekend.

Sam

#39 lore raw
In reply to Maxime Ripard
Quoted 40 lines

Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code, slowly getting rid of the big data array for more understandable code (hopefully).

The biggest addition would be the clock rate computation at runtime, instead of relying on those arrays to setup the clock tree properly. As a side effect, it fixes the framerate that was off by around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v1:
- Integrated Hugues' suggestions to fix v4l2-compliance
- Fixed the bus width with JPEG
- Dropped the clock rate calculation loops for something simpler as suggested by Sakari
- Cache the exposure value instead of using the control value
- Rebased on top of 4.17

Maxime Ripard (10):
media: ov5640: Don't force the auto exposure state at start time
media: ov5640: Init properly the SCLK dividers
media: ov5640: Change horizontal and vertical resolutions name
media: ov5640: Add horizontal and vertical totals
media: ov5640: Program the visible resolution
media: ov5640: Adjust the clock based on the expected rate
media: ov5640: Compute the clock rate at runtime
media: ov5640: Enhance FPS handling
media: ov5640: Add 60 fps support
media: ov5640: Remove duplicate auto-exposure setup

Mylène Josserand (2):
media: ov5640: Add auto-focus feature
media: ov5640: Add light frequency control

Quoted 6 lines
 drivers/media/i2c/ov5640.c | 752 +++++++++++++++++++++----------------
 1 file changed, 422 insertions(+), 330 deletions(-)

--
2.17.0

As discussed, MIPI required some additional work. Please see the patches here which add support for MIPI:
https://www.dropbox.com/s/73epty7808yzq1t/ov5640_mipi_fixes.zip?dl=0

The first 3 patches are fixes I believe should be made to earlier patches prior to submitting v2 of this series. The remaining 4 patches should probably just be added onto the end of this series as-is (or with feedback incorporated if needed).

I will note that this is still not working correctly on my system for any resolution that requires a 672 Mbps mipi rate. This includes 1080p@30hz, full@15hz, and 720p@60hz. My CSI2 receiver is reporting CRC errors though, so this could be an integrity issue on my module. I'm curious to hear if others have success at these resolutions.

Please try this out on other MIPI and DVP platforms with as many different resolutions as possible and let me know if it works.

I'd also recommend that someone add a patch to the series that enables 720p@60hz. I didn't have time to do it yet, but it should be really easy.

Sam

#40 lore raw

Hi,

Quoted 56 lines · Sam Bobrowicz
Quoted 40 lines

Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code, slowly getting rid of the big data array for more understandable code (hopefully).

The biggest addition would be the clock rate computation at runtime, instead of relying on those arrays to setup the clock tree properly. As a side effect, it fixes the framerate that was off by around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v1:
- Integrated Hugues' suggestions to fix v4l2-compliance
- Fixed the bus width with JPEG
- Dropped the clock rate calculation loops for something simpler as suggested by Sakari
- Cache the exposure value instead of using the control value
- Rebased on top of 4.17

Maxime Ripard (10):
media: ov5640: Don't force the auto exposure state at start time
media: ov5640: Init properly the SCLK dividers
media: ov5640: Change horizontal and vertical resolutions name
media: ov5640: Add horizontal and vertical totals
media: ov5640: Program the visible resolution
media: ov5640: Adjust the clock based on the expected rate
media: ov5640: Compute the clock rate at runtime
media: ov5640: Enhance FPS handling
media: ov5640: Add 60 fps support
media: ov5640: Remove duplicate auto-exposure setup

Mylène Josserand (2):
media: ov5640: Add auto-focus feature
media: ov5640: Add light frequency control

Quoted 6 lines
 drivers/media/i2c/ov5640.c | 752 +++++++++++++++++++++----------------
 1 file changed, 422 insertions(+), 330 deletions(-)

--
2.17.0

As discussed, MIPI required some additional work. Please see the patches here which add support for MIPI:
https://www.dropbox.com/s/73epty7808yzq1t/ov5640_mipi_fixes.zip?dl=0

The first 3 patches are fixes I believe should be made to earlier patches prior to submitting v2 of this series. The remaining 4 patches should probably just be added onto the end of this series as-is (or with feedback incorporated if needed).

Quoted 8 lines

I will note that this is still not working correctly on my system for any resolution that requires a 672 Mbps mipi rate. This includes 1080p@30hz, full@15hz, and 720p@60hz. My CSI2 receiver is reporting CRC errors though, so this could be an integrity issue on my module. I'm curious to hear if others have success at these resolutions.

Please try this out on other MIPI and DVP platforms with as many different resolutions as possible and let me know if it works.

I've took some of your changes to remain as feature stable as possible in my series. Some other changes (like the PLL2 setup), while totally welcome, should be in a separate, subsequent series.

DVP works as expected, after looking at the feedback from Loic (and the clock tree especially), some of the comments you made (like the bit divider being meaningless for DVP) are not totally accurate, so I've tried to make the best blend of all the feedback you gave. It still works properly with DVP, but I still don't have a MIPI camera to test, so I'm not sure this works, even though I should have the same setup than the one you reported.

Thanks!
Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com