[PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[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 v2:
  - Rebased on latest Sakari PR
  - Fixed the issues reported by Hugues: improper FPS returned for
    formats, improper rounding of the FPS, some with his suggestions,
    some by simplifying the logic.
  - Expanded the clock tree comments based on the feedback from Samuel
    Bobrowicz and Loic Poulain
  - Merged some of the changes made by Samuel Bobrowicz to fix the
    MIPI rate computation, fix the call sites of the
    ov5640_set_timings function, the auto-exposure calculation call,
    etc.
  - Split the patches into smaller ones in order to make it more
    readable (hopefully)

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 (11):
  media: ov5640: Adjust the clock based on the expected rate
  media: ov5640: Remove the clocks registers initialization
  media: ov5640: Remove redundant defines
  media: ov5640: Remove redundant register setup
  media: ov5640: Compute the clock rate at runtime
  media: ov5640: Remove pixel clock rates
  media: ov5640: Enhance FPS handling
  media: ov5640: Make the return rate type more explicit
  media: ov5640: Make the FPS clamping / rounding more extendable
  media: ov5640: Add 60 fps support
  media: ov5640: Remove duplicate auto-exposure setup

Samuel Bobrowicz (1):
  media: ov5640: Fix timings setup code

 drivers/media/i2c/ov5640.c | 701 +++++++++++++++++++++----------------
 1 file changed, 392 insertions(+), 309 deletions(-)

-- 
2.17.0

[PATCH v3 01/12] media: ov5640: Fix timings setup code

[Maxime Ripard]

From: Samuel Bobrowicz <sam@elite-embedded.com>

The current code, when changing the mode and changing the scaling or
sampling parameters, will look at the horizontal and vertical total size,
which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
totals") has been moved from the static register initialization to after
the mode change.

That means that the values are no longer set up before the code retrieves
them, which is obviously a bug.

Fixes: 5999f381e023 ("media: ov5640: Add horizontal and vertical totals")
Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e480e53b369b..4bd968b478db 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1462,6 +1462,10 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
 	if (ret < 0)
 		return ret;
 
+	ret = ov5640_set_timings(sensor, mode);
+	if (ret < 0)
+		return ret;
+
 	/* read capture VTS */
 	ret = ov5640_get_vts(sensor);
 	if (ret < 0)
@@ -1589,6 +1593,10 @@ static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
 	if (ret < 0)
 		return ret;
 
+	ret = ov5640_set_timings(sensor, mode);
+	if (ret < 0)
+		return ret;
+
 	/* turn auto gain/exposure back on for direct mode */
 	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
 	if (ret)
@@ -1633,10 +1641,6 @@ 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;
 
-- 
2.17.0

Re: [PATCH v3 01/12] media: ov5640: Fix timings setup code

[Daniel Mack]

Hi,

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> From: Samuel Bobrowicz <sam@elite-embedded.com>
> 
> The current code, when changing the mode and changing the scaling or
> sampling parameters, will look at the horizontal and vertical total size,
> which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
> totals") has been moved from the static register initialization to after
> the mode change.
> 
> That means that the values are no longer set up before the code retrieves
> them, which is obviously a bug.

I tried 'for-4.18-5' before your patch set now and noticed it didn't 
work. I then bisected the regression down to the same commit that you 
mentioned above.

The old code (before 5999f381e023) used to have the timing registers 
embedded in a large register blob. Omitting them during the bulk upload 
and writing them later doesn't work here, even if the values are the 
same. It seems that the order in which registers are written matters.

Hence your patch in this email doesn't fix it for me either. I have to 
move ov5640_set_timings() before ov5640_load_regs() to revive my platform.

One of the subsequent patches in this series introduces a new regression 
for me, unfortunately, possibly for the same reason. I'll dig a bit more.

What cameras are you testing this with? MIPI or parallel?


Thanks,
Daniel



> Fixes: 5999f381e023 ("media: ov5640: Add horizontal and vertical totals")
> Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>   drivers/media/i2c/ov5640.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index e480e53b369b..4bd968b478db 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1462,6 +1462,10 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = ov5640_set_timings(sensor, mode);
> +	if (ret < 0)
> +		return ret;
> +
>   	/* read capture VTS */
>   	ret = ov5640_get_vts(sensor);
>   	if (ret < 0)
> @@ -1589,6 +1593,10 @@ static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = ov5640_set_timings(sensor, mode);
> +	if (ret < 0)
> +		return ret;
> +
>   	/* turn auto gain/exposure back on for direct mode */
>   	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
>   	if (ret)
> @@ -1633,10 +1641,6 @@ 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;
>   
> 

Re: [PATCH v3 01/12] media: ov5640: Fix timings setup code

[Maxime Ripard]

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

Hi Daniel,

On Fri, May 18, 2018 at 10:32:43AM +0200, Daniel Mack wrote:
> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > From: Samuel Bobrowicz <sam@elite-embedded.com>
> > 
> > The current code, when changing the mode and changing the scaling or
> > sampling parameters, will look at the horizontal and vertical total size,
> > which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
> > totals") has been moved from the static register initialization to after
> > the mode change.
> > 
> > That means that the values are no longer set up before the code retrieves
> > them, which is obviously a bug.
> 
> I tried 'for-4.18-5' before your patch set now and noticed it didn't work. I
> then bisected the regression down to the same commit that you mentioned
> above.
> 
> The old code (before 5999f381e023) used to have the timing registers
> embedded in a large register blob. Omitting them during the bulk upload and
> writing them later doesn't work here, even if the values are the same. It
> seems that the order in which registers are written matters.
> 
> Hence your patch in this email doesn't fix it for me either. I have to move
> ov5640_set_timings() before ov5640_load_regs() to revive my platform.

That's kind of weird, it definitely works for parallel. Do you have a
bit more details on what doesn't work? What commands / apps are you
running?

Maxime

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

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

[PATCH v3 02/12] media: ov5640: Adjust the clock based on the expected rate

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

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 289 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 288 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 4bd968b478db..63923d85a31f 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -174,6 +174,7 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
+	u32 pixel_clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -695,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,
+	56000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -704,74 +706,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 28000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 28000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 28000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 28000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 28000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 28000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 21000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 42000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
+		 84000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 56000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 56000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 56000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 56000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 56000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 56000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 42000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 84000000,
 		 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},
 	},
 };
 
@@ -904,6 +923,255 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 	return ov5640_write_reg(sensor, reg, val);
 }
 
+/*
+ * After trying the various combinations, reading various
+ * documentations spreaded around the net, and from the various
+ * feedback, the clock tree is probably as follows:
+ *
+ *   +--------------+
+ *   |  Ext. Clock  |
+ *   +-+------------+
+ *     |  +----------+
+ *     +->|   PLL1   | - reg 0x3036, for the multiplier
+ *        +-+--------+ - reg 0x3037, bits 0-3 for the pre-divider
+ *          |  +--------------+
+ *          +->| System Clock |  - reg 0x3035, bits 4-7
+ *             +-+------------+
+ *               |  +--------------+
+ *               +->| MIPI Divider | - reg 0x3035, bits 0-3
+ *               |  +-+------------+
+ *               |    +----------------> MIPI SCLK
+ *               |  +--------------+
+ *               +->| PLL Root Div | - reg 0x3037, bit 4
+ *                  +-+------------+
+ *                    |  +---------+
+ *                    +->| Bit Div | - reg 0x3035, bits 0-3
+ *                       +-+-------+
+ *                         |  +-------------+
+ *                         +->| SCLK Div    | - reg 0x3108, bits 0-1
+ *                         |  +-+-----------+
+ *                         |    +---------------> SCLK
+ *                         |  +-------------+
+ *                         +->| SCLK 2X Div | - reg 0x3108, bits 2-3
+ *                         |  +-+-----------+
+ *                         |    +---------------> SCLK 2X
+ *                         |  +-------------+
+ *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
+ *                            +-+-----------+
+ *                              +---------------> PCLK
+ *
+ * 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.
+ *
+ * There seems to be also (unverified) constraints:
+ *  - the PLL pre-divider output rate should be in the 4-27MHz range
+ *  - the PLL multiplier output rate should be in the 500-1000MHz range
+ *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
+ *  - MIPI SCLK = (bpp / lanes) / PCLK
+ *
+ * In the two latter cases, these constraints are met since our
+ * factors are hardcoded. If we were to change that, we would need to
+ * take this into account. The only varying parts are the PLL
+ * multiplier and the system clock divider, which are shared between
+ * all these clocks so won't cause any issue.
+ */
+
+/*
+ * 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 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 16, but the value is always
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_MIPI_DIV		2
+
+/*
+ * This is supposed to be ranging from 1 to 2, but the value is always
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_PLL_ROOT_DIV	2
+
+/*
+ * This is supposed to be either 1, 2 or 2.5, but the value is always
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_BIT_DIV		2
+
+/*
+ * 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 hardcoded so that the consistency is maintained between SCLK and
+ * SCLK 2x.
+ */
+#define OV5640_SCLK2X_ROOT_DIV (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_sys_clk(struct ov5640_dev *sensor,
+					    u8 pll_prediv, u8 pll_mult,
+					    u8 sysdiv)
+{
+	unsigned long rate = clk_get_rate(sensor->xclk);
+
+	return rate / pll_prediv * pll_mult / sysdiv;
+}
+
+static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
+					 unsigned long rate,
+					 u8 *pll_prediv, u8 *pll_mult,
+					 u8 *sysdiv)
+{
+	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_sys_clk(sensor,
+						       OV5640_PLL_PREDIV,
+						       _pll_mult, _sysdiv);
+			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;
+	*pll_prediv = OV5640_PLL_PREDIV;
+	*pll_mult = best_mult;
+	return best;
+}
+
+static unsigned long ov5640_calc_mipi_clk(struct ov5640_dev *sensor,
+					  unsigned long rate,
+					  u8 *pll_prediv, u8 *pll_mult,
+					  u8 *sysdiv, u8 *mipi_div)
+{
+	unsigned long _rate = rate * OV5640_MIPI_DIV;
+
+	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
+				    sysdiv);
+	*mipi_div = OV5640_MIPI_DIV;
+
+	return _rate / *mipi_div;
+}
+
+static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 prediv, mult, sysdiv, mipi_div;
+	int ret;
+
+	ov5640_calc_mipi_clk(sensor, rate, &prediv, &mult, &sysdiv, &mipi_div);
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			     0xff, sysdiv << 4 | (mipi_div - 1));
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3, 0xf, prediv);
+}
+
+static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
+				      unsigned long rate,
+				      u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
+				      u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
+{
+	unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
+				OV5640_PCLK_ROOT_DIV;
+
+	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
+				    sysdiv);
+	*pll_rdiv = OV5640_PLL_ROOT_DIV;
+	*bit_div = OV5640_BIT_DIV;
+	*pclk_div = OV5640_PCLK_ROOT_DIV;
+
+	return _rate / *pll_rdiv / *bit_div / *pclk_div;
+}
+
+static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 prediv, mult, sysdiv, pll_rdiv, bit_div, pclk_div;
+	int ret;
+
+	ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
+			 &bit_div, &pclk_div);
+
+	if (bit_div == 2)
+		bit_div = 8;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
+			     0x0f, bit_div);
+	if (ret)
+		return ret;
+
+	/*
+	 * We need to set sysdiv according to the clock, and to clear
+	 * the MIPI divider.
+	 */
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			     0xff, 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,
+			     0x1f, prediv | ((pll_rdiv - 1) << 4));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x30,
+			      (ilog2(pclk_div) << 4));
+}
+
 /* download ov5640 settings to sensor through i2c */
 static int ov5640_load_regs(struct ov5640_dev *sensor,
 			    const struct ov5640_mode_info *mode)
@@ -1610,6 +1878,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;
 
@@ -1626,6 +1896,23 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
+	/*
+	 * All the formats we support have 16 bits per pixel, except for JPEG
+	 * which is 8 bits per pixel.
+	 */
+	bpp = sensor->fmt.code == MEDIA_BUS_FMT_JPEG_1X8 ? 8 : 16;
+	rate = mode->pixel_clock * bpp;
+	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 {
+		rate = rate / sensor->ep.bus.parallel.bus_width;
+		ret = ov5640_set_dvp_pclk(sensor, rate);
+	}
+
+	if (ret < 0)
+		return 0;
+
 	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
 	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
 		/*
-- 
2.17.0

[PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Maxime Ripard]

Part of the hardcoded initialization sequence is to set up the proper clock
dividers. However, this is now done dynamically through proper code and as
such, the static one is now redundant.

Let's remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 46 ++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 63923d85a31f..45e5ba32b8d1 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -256,8 +256,7 @@ 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},
+	{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},
@@ -340,7 +339,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},
@@ -359,7 +358,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},
@@ -378,7 +377,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},
@@ -394,11 +393,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},
@@ -417,7 +415,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},
@@ -436,7 +434,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},
@@ -455,7 +453,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},
@@ -474,7 +472,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},
@@ -493,7 +491,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},
@@ -512,7 +510,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},
@@ -531,7 +529,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},
@@ -550,7 +548,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},
@@ -570,7 +568,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},
@@ -590,7 +588,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},
@@ -610,7 +608,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},
@@ -625,8 +623,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},
@@ -643,7 +641,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},
@@ -658,8 +656,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},
@@ -675,7 +673,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},
-- 
2.17.0

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> Part of the hardcoded initialization sequence is to set up the proper clock
> dividers. However, this is now done dynamically through proper code and as
> such, the static one is now redundant.
> 
> Let's remove it.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---

[...]

> @@ -625,8 +623,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},

This is the mode that I'm testing with. Previously, the hard-coded 
registers here were:

  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07

Your new code that calculates the clock rates dynamically ends up with 
different values however:

  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03

Interestingly, leaving the hard-coded values in the array *and* letting 
ov5640_set_mipi_pclk() do its thing later still works. So again it seems 
that writes to registers after 0x3035/0x3036/0x3037 seem to depend on 
the values of these timing registers. You might need to leave these 
values as dummies in the array. Confusing.

Any idea?


Thanks,
Daniel

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Sam Bobrowicz]

On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
>>
>> Part of the hardcoded initialization sequence is to set up the proper
>> clock
>> dividers. However, this is now done dynamically through proper code and as
>> such, the static one is now redundant.
>>
>> Let's remove it.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>
>
> [...]
>
>> @@ -625,8 +623,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},
>
>
> This is the mode that I'm testing with. Previously, the hard-coded registers
> here were:
>
>  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
>  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
>
> Your new code that calculates the clock rates dynamically ends up with
> different values however:
>
>  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
>  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
>
> Interestingly, leaving the hard-coded values in the array *and* letting
> ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> values of these timing registers. You might need to leave these values as
> dummies in the array. Confusing.
>
> Any idea?
>
>
> Thanks,
> Daniel

This set of patches is also not working for my MIPI platform (mine has
a 12 MHz external clock). I am pretty sure is isn't working because it
does not include the following, which my tests have found to be
necessary:

1) Setting pclk period reg in order to correct DPHY timing.
2) Disabling of MIPI lanes when streaming not enabled.
3) setting mipi_div to 1 when the scaler is disabled
4) Doubling ADC clock on faster resolutions.

I will run some more tests to see if anything else is broken and come
back with some suggestions.

I should mention that the upstream driver has never worked with my
platform. I suspect that the driver only ever worked previously with
MIPI platforms that have loose DPHY timing requirements and a specific
xclk (24MHz maybe?). Out of the interest of collecting more data, can
you provide the following info on your platform?

a) External clock frequency
b) List of resolutions (including framerates) that are working with
these patches (and your fix) applied
c) List of resolutions that were working prior to the regression you
experienced with the set_timings function

Sam

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

Hi,

On Saturday, May 19, 2018 04:42 AM, Sam Bobrowicz wrote:
> This set of patches is also not working for my MIPI platform (mine has
> a 12 MHz external clock). I am pretty sure is isn't working because it
> does not include the following, which my tests have found to be
> necessary:
> 
> 1) Setting pclk period reg in order to correct DPHY timing.
> 2) Disabling of MIPI lanes when streaming not enabled.
> 3) setting mipi_div to 1 when the scaler is disabled
> 4) Doubling ADC clock on faster resolutions.
> 
> I will run some more tests to see if anything else is broken and come
> back with some suggestions.
> 
> I should mention that the upstream driver has never worked with my
> platform. I suspect that the driver only ever worked previously with
> MIPI platforms that have loose DPHY timing requirements and a specific
> xclk (24MHz maybe?). Out of the interest of collecting more data, can
> you provide the following info on your platform?
> 
> a) External clock frequency

Mine has a 24MHz oscillator.

> b) List of resolutions (including framerates) that are working with
> these patches (and your fix) applied

I have a somewhat limited support in userspace which is currently 
hard-coded to 1920x1080@30fps. I haven't tested any other resolution, 
but this one is not working with this patch set.

> c) List of resolutions that were working prior to the regression you
> experienced with the set_timings function

The one mentioned above did work before, except for one things: I need a 
patch on top that adds a V4L2_CID_PIXEL_RATE control. The qcom camss 
platform needs that in order to calculate its own clock rates. When I 
tested this patch set, I hard-coded the setting the camss driver.

I can send a patch that adds this control once this patch set has landed.


Thanks,
Daniel

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Maxime Ripard]

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

On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
> > On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> >>
> >> Part of the hardcoded initialization sequence is to set up the proper
> >> clock
> >> dividers. However, this is now done dynamically through proper code and as
> >> such, the static one is now redundant.
> >>
> >> Let's remove it.
> >>
> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> ---
> >
> >
> > [...]
> >
> >> @@ -625,8 +623,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},
> >
> >
> > This is the mode that I'm testing with. Previously, the hard-coded registers
> > here were:
> >
> >  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> >  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
> >  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
> >
> > Your new code that calculates the clock rates dynamically ends up with
> > different values however:
> >
> >  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> >  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
> >  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
> >
> > Interestingly, leaving the hard-coded values in the array *and* letting
> > ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> > that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> > values of these timing registers. You might need to leave these values as
> > dummies in the array. Confusing.
> >
> > Any idea?
> >
> >
> > Thanks,
> > Daniel
> 
> This set of patches is also not working for my MIPI platform (mine has
> a 12 MHz external clock). I am pretty sure is isn't working because it
> does not include the following, which my tests have found to be
> necessary:
> 
> 1) Setting pclk period reg in order to correct DPHY timing.
> 2) Disabling of MIPI lanes when streaming not enabled.
> 3) setting mipi_div to 1 when the scaler is disabled
> 4) Doubling ADC clock on faster resolutions.

Yeah, I left them out because I didn't think this was relevant to this
patchset but should come as future improvements. However, given that
it works with the parallel bus, maybe the two first are needed when
adjusting the rate.

The mipi divider however seems to be a bit more complicated than you
report here. It is indeed set to 1 when the scaler is enabled (all
resolutions > 1280 * 960), but it's also set to 4 in some cases
(640x480@30, 320x240@30, 176x144@30). I couldn't really find any
relationship between the resolution/framerate and whether to use a
divider of 2 or 4.

And the faster resolutions were working already, so I guess the ADC
clock is already fast enough with a 24MHz oscillator?

Maxime

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

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

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Maxime Ripard]

On Mon, May 21, 2018 at 09:39:02AM +0200, Maxime Ripard wrote:
> On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> > On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
> > > On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > >>
> > >> Part of the hardcoded initialization sequence is to set up the proper
> > >> clock
> > >> dividers. However, this is now done dynamically through proper code and as
> > >> such, the static one is now redundant.
> > >>
> > >> Let's remove it.
> > >>
> > >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > >> ---
> > >
> > >
> > > [...]
> > >
> > >> @@ -625,8 +623,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},
> > >
> > >
> > > This is the mode that I'm testing with. Previously, the hard-coded registers
> > > here were:
> > >
> > >  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> > >  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
> > >  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
> > >
> > > Your new code that calculates the clock rates dynamically ends up with
> > > different values however:
> > >
> > >  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> > >  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
> > >  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
> > >
> > > Interestingly, leaving the hard-coded values in the array *and* letting
> > > ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> > > that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> > > values of these timing registers. You might need to leave these values as
> > > dummies in the array. Confusing.
> > >
> > > Any idea?
> > >
> > >
> > > Thanks,
> > > Daniel
> > 
> > This set of patches is also not working for my MIPI platform (mine has
> > a 12 MHz external clock). I am pretty sure is isn't working because it
> > does not include the following, which my tests have found to be
> > necessary:
> > 
> > 1) Setting pclk period reg in order to correct DPHY timing.
> > 2) Disabling of MIPI lanes when streaming not enabled.
> > 3) setting mipi_div to 1 when the scaler is disabled
> > 4) Doubling ADC clock on faster resolutions.
> 
> Yeah, I left them out because I didn't think this was relevant to this
> patchset but should come as future improvements. However, given that
> it works with the parallel bus, maybe the two first are needed when
> adjusting the rate.

I've checked for the pclk period, and it's hardcoded to the same value
all the time, so I guess this is not the reason it doesn't work on
MIPI CSI anymore.

Daniel, could you test:
http://code.bulix.org/ki6kgz-339327?raw

And let us know the results?

Thanks!
Maxime

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

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Daniel Mack]

Hi Maxime,

On Tuesday, May 22, 2018 09:54 PM, Maxime Ripard wrote:
> On Mon, May 21, 2018 at 09:39:02AM +0200, Maxime Ripard wrote:
>> On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:

>>> This set of patches is also not working for my MIPI platform (mine has
>>> a 12 MHz external clock). I am pretty sure is isn't working because it
>>> does not include the following, which my tests have found to be
>>> necessary:
>>>
>>> 1) Setting pclk period reg in order to correct DPHY timing.
>>> 2) Disabling of MIPI lanes when streaming not enabled.
>>> 3) setting mipi_div to 1 when the scaler is disabled
>>> 4) Doubling ADC clock on faster resolutions.
>>
>> Yeah, I left them out because I didn't think this was relevant to this
>> patchset but should come as future improvements. However, given that
>> it works with the parallel bus, maybe the two first are needed when
>> adjusting the rate.
> 
> I've checked for the pclk period, and it's hardcoded to the same value
> all the time, so I guess this is not the reason it doesn't work on
> MIPI CSI anymore.
> 
> Daniel, could you test:
> http://code.bulix.org/ki6kgz-339327?raw

[Note that there's a missing parenthesis in this snippet]

Hmm, no, that doesn't change anything. Streaming doesn't work here, even 
if I move ov5640_load_regs() before any other initialization.

One of my test setup is the following gst pipeline:

   gst-launch-1.0	\
	v4l2src device=/dev/video0 ! \
	videoconvert ! \
	video/x-raw,format=UYVY,width=1920,height=1080 ! \
	glimagesink

With the pixel clock hard-coded to 166600000 in qcom camss, the setup 
works on 4.14, but as I said, it broke already before this series with 
5999f381e023 ("media: ov5640: Add horizontal and vertical
totals").

Frankly, my understanding of these chips is currently limited, so I 
don't really know where to start digging. It seems clear though that the 
timing registers setup is necessary for other register writes to succeed.

Can I help in any other way?


Thanks for all your efforts,
Daniel

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Maxime Ripard]

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

On Wed, May 23, 2018 at 11:31:58AM +0200, Daniel Mack wrote:
> Hi Maxime,
> 
> On Tuesday, May 22, 2018 09:54 PM, Maxime Ripard wrote:
> > On Mon, May 21, 2018 at 09:39:02AM +0200, Maxime Ripard wrote:
> > > On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> 
> > > > This set of patches is also not working for my MIPI platform (mine has
> > > > a 12 MHz external clock). I am pretty sure is isn't working because it
> > > > does not include the following, which my tests have found to be
> > > > necessary:
> > > > 
> > > > 1) Setting pclk period reg in order to correct DPHY timing.
> > > > 2) Disabling of MIPI lanes when streaming not enabled.
> > > > 3) setting mipi_div to 1 when the scaler is disabled
> > > > 4) Doubling ADC clock on faster resolutions.
> > > 
> > > Yeah, I left them out because I didn't think this was relevant to this
> > > patchset but should come as future improvements. However, given that
> > > it works with the parallel bus, maybe the two first are needed when
> > > adjusting the rate.
> > 
> > I've checked for the pclk period, and it's hardcoded to the same value
> > all the time, so I guess this is not the reason it doesn't work on
> > MIPI CSI anymore.
> > 
> > Daniel, could you test:
> > http://code.bulix.org/ki6kgz-339327?raw
> 
> [Note that there's a missing parenthesis in this snippet]

Sorry :/

> Hmm, no, that doesn't change anything. Streaming doesn't work here, even if
> I move ov5640_load_regs() before any other initialization.
> 
> One of my test setup is the following gst pipeline:
> 
>   gst-launch-1.0	\
> 	v4l2src device=/dev/video0 ! \
> 	videoconvert ! \
> 	video/x-raw,format=UYVY,width=1920,height=1080 ! \
> 	glimagesink
> 
> With the pixel clock hard-coded to 166600000 in qcom camss, the setup works
> on 4.14, but as I said, it broke already before this series with
> 5999f381e023 ("media: ov5640: Add horizontal and vertical
> totals").
> 
> Frankly, my understanding of these chips is currently limited, so I don't
> really know where to start digging. It seems clear though that the timing
> registers setup is necessary for other register writes to succeed.
> 
> Can I help in any other way?

If you feel like it, you could go through the various changes
(especially the pclk period I guess) changes Sam pushed in the
previous iteration to his dropbox. That's probably not going to be
quite easy to merge though, so that's going to require some manual
holding.

Sorry for not being able to help more than that :/

Maxime

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

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

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Sam Bobrowicz]

>> On May 21, 2018, at 12:39 AM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>>
>>> On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
>>>> On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
>>>> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
>>>>
>>>> Part of the hardcoded initialization sequence is to set up the proper
>>>> clock
>>>> dividers. However, this is now done dynamically through proper code and as
>>>> such, the static one is now redundant.
>>>>
>>>> Let's remove it.
>>>>
>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>>> ---
>>>
>>>
>>> [...]
>>>
>>>> @@ -625,8 +623,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},
>>>
>>>
>>> This is the mode that I'm testing with. Previously, the hard-coded registers
>>> here were:
>>>
>>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
>>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
>>>
>>> Your new code that calculates the clock rates dynamically ends up with
>>> different values however:
>>>
>>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
>>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
>>>
>>> Interestingly, leaving the hard-coded values in the array *and* letting
>>> ov5640_set_mipi_pclk() do its thing later still works. So again it seems
>>> that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
>>> values of these timing registers. You might need to leave these values as
>>> dummies in the array. Confusing.
>>>
>>> Any idea?
>>>
>>>
>>> Thanks,
>>> Daniel
>>
>> This set of patches is also not working for my MIPI platform (mine has
>> a 12 MHz external clock). I am pretty sure is isn't working because it
>> does not include the following, which my tests have found to be
>> necessary:
>>
>> 1) Setting pclk period reg in order to correct DPHY timing.
>> 2) Disabling of MIPI lanes when streaming not enabled.
>> 3) setting mipi_div to 1 when the scaler is disabled
>> 4) Doubling ADC clock on faster resolutions.
>
> Yeah, I left them out because I didn't think this was relevant to this
> patchset but should come as future improvements. However, given that
> it works with the parallel bus, maybe the two first are needed when
> adjusting the rate.
>
I agree that 1-4 are separate improvements to MIPI mode that may not
affect all modules. They do break mine, but that has been true since
the driver was released (mainly because of my 12 MHz clock and more
stringent CSI RX requirements). So it makes sense for me to address
them in a follow-up series. But I do think that we should get the
clock generation a little closer to what I know works for MIPI so we
don't break things for people that do have MIPI working.

> The mipi divider however seems to be a bit more complicated than you
> report here. It is indeed set to 1 when the scaler is enabled (all
> resolutions > 1280 * 960), but it's also set to 4 in some cases
> (640x480@30, 320x240@30, 176x144@30). I couldn't really find any
> relationship between the resolution/framerate and whether to use a
> divider of 2 or 4.

I didn't notice the divide by 4, interesting. I have a theory
though... it could be that the constraint of PCLK relative to SCLK is:

SCLK*(cpp/scaler ratio)<=PCLK<= ?
cpp=Components/pixel (1 for JPEG, 2 for YUV, e.g.)

Since the scaler is in auto mode, the scaler ratio might automatically
change depending on the resolution selected, something like (using int
math):

(hTotal/hActive) * (vTotal/vActive) = scaler ratio

If SCLK is responsible for reading the data into the scaler, and PCLK
is responsible for reading data out to the physical interface, this
would make sense. It will require more experiments to verify any of
this, though, and unfortunately I don't have a lot of time to put into
this right now.

On my platform I have also run into an upper bound for PCLK, where it
seems that PCLK must be <= SCLK when the scaler is enabled. I think
this may have to do with some of my platform's idiosyncrasies, so I'm
not ready to say that it needs to be addressed in this series. But if
others run into it while testing MIPI, you should consider
implementing #3 above to address it.

> And the faster resolutions were working already, so I guess the ADC
> clock is already fast enough with a 24MHz oscillator?

That's my theory. It seems to have pretty loose requirements as long
as it is fast enough, which is why I did the simple 2x solution. It
doesn't need to be addressed here though. If anyone runs into images
that are all black or bluish, this is a possible culprit.

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

I'm back, sorry for the delay. Here is a patch that should fix a few
things for MIPI users. Just apply it directly after applying the
series. Someone else should definitely verify this on a different MIPI
platform.

https://nofile.io/f/W8J3thK7pOp/clock_fixes.patch

These are the noteworthy changes I made.

*Added writes to init blob for 0x3034 (bit div) and 0x3037 (pll r
div). This is because bit div and pll root div never get written to
the expected values (8 and 2), so they remain as defaults (10 and 1).
It would also be possible to modify set_mipi_pclk to just write these
values there, but I didn't wan't to mess with your functions too much.

*Change MIPI SCLK constraint in comments to match the notes found
here: https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg.
It seems that the pixels are serialized into components when they
cross from SCLK to PCLK, so the MIPI serial clock does not care about
cpp, only bpc (bits/component).

*Lower MIPI DIV to 1 for now. It may be necessary to conditionally set
it later if people are still having trouble, but always using 2 will
make PCLK<SCLK*cpp, and definitely break non-scaled resolutions.

*MIPI div register doesn't need a -1. When set to zero, it actually
divides by 16.

Also, FYI, I've made some improvements to my clock configuration
spreadsheet and incorporated a register dump function into set_mode
that prints the relevant registers so they can be copied into the
spreadsheet for interpretation. Just let me know if anyone wants it.

Sam

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Maxime Ripard]

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

Hi Sam,

On Fri, Jun 01, 2018 at 04:05:58PM -0700, Sam Bobrowicz wrote:
> >> On May 21, 2018, at 12:39 AM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >>
> >>> On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> >>>> On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
> >>>> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> >>>>
> >>>> Part of the hardcoded initialization sequence is to set up the proper
> >>>> clock
> >>>> dividers. However, this is now done dynamically through proper code and as
> >>>> such, the static one is now redundant.
> >>>>
> >>>> Let's remove it.
> >>>>
> >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >>>> ---
> >>>
> >>>
> >>> [...]
> >>>
> >>>> @@ -625,8 +623,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},
> >>>
> >>>
> >>> This is the mode that I'm testing with. Previously, the hard-coded registers
> >>> here were:
> >>>
> >>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> >>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
> >>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
> >>>
> >>> Your new code that calculates the clock rates dynamically ends up with
> >>> different values however:
> >>>
> >>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> >>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
> >>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
> >>>
> >>> Interestingly, leaving the hard-coded values in the array *and* letting
> >>> ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> >>> that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> >>> values of these timing registers. You might need to leave these values as
> >>> dummies in the array. Confusing.
> >>>
> >>> Any idea?
> >>
> >> This set of patches is also not working for my MIPI platform (mine has
> >> a 12 MHz external clock). I am pretty sure is isn't working because it
> >> does not include the following, which my tests have found to be
> >> necessary:
> >>
> >> 1) Setting pclk period reg in order to correct DPHY timing.
> >> 2) Disabling of MIPI lanes when streaming not enabled.
> >> 3) setting mipi_div to 1 when the scaler is disabled
> >> 4) Doubling ADC clock on faster resolutions.
> >
> > Yeah, I left them out because I didn't think this was relevant to this
> > patchset but should come as future improvements. However, given that
> > it works with the parallel bus, maybe the two first are needed when
> > adjusting the rate.
> >
> I agree that 1-4 are separate improvements to MIPI mode that may not
> affect all modules. They do break mine, but that has been true since
> the driver was released (mainly because of my 12 MHz clock and more
> stringent CSI RX requirements). So it makes sense for me to address
> them in a follow-up series. But I do think that we should get the
> clock generation a little closer to what I know works for MIPI so we
> don't break things for people that do have MIPI working.

I guess it's already a bit too late for that :/

> > The mipi divider however seems to be a bit more complicated than you
> > report here. It is indeed set to 1 when the scaler is enabled (all
> > resolutions > 1280 * 960), but it's also set to 4 in some cases
> > (640x480@30, 320x240@30, 176x144@30). I couldn't really find any
> > relationship between the resolution/framerate and whether to use a
> > divider of 2 or 4.
> 
> I didn't notice the divide by 4, interesting. I have a theory
> though... it could be that the constraint of PCLK relative to SCLK is:
> 
> SCLK*(cpp/scaler ratio)<=PCLK<= ?
> cpp=Components/pixel (1 for JPEG, 2 for YUV, e.g.)
> 
> Since the scaler is in auto mode, the scaler ratio might automatically
> change depending on the resolution selected, something like (using int
> math):
> 
> (hTotal/hActive) * (vTotal/vActive) = scaler ratio
> 
> If SCLK is responsible for reading the data into the scaler, and PCLK
> is responsible for reading data out to the physical interface, this
> would make sense. It will require more experiments to verify any of
> this, though, and unfortunately I don't have a lot of time to put into
> this right now.

To be honest, I really couldn't find any correlation. Some have
different dividers for the two framerates (15 and 30), which wouldn't
make sense in your case, and doubling the resolution doesn't always
result in increasing that divider either.

> On my platform I have also run into an upper bound for PCLK, where it
> seems that PCLK must be <= SCLK when the scaler is enabled. I think
> this may have to do with some of my platform's idiosyncrasies, so I'm
> not ready to say that it needs to be addressed in this series. But if
> others run into it while testing MIPI, you should consider
> implementing #3 above to address it.
> 
> > And the faster resolutions were working already, so I guess the ADC
> > clock is already fast enough with a 24MHz oscillator?
> 
> That's my theory. It seems to have pretty loose requirements as long
> as it is fast enough, which is why I did the simple 2x solution. It
> doesn't need to be addressed here though. If anyone runs into images
> that are all black or bluish, this is a possible culprit.
> 
> I'm back, sorry for the delay. Here is a patch that should fix a few
> things for MIPI users. Just apply it directly after applying the
> series. Someone else should definitely verify this on a different MIPI
> platform.
> 
> https://nofile.io/f/W8J3thK7pOp/clock_fixes.patch
> 
> These are the noteworthy changes I made.
> 
> *Added writes to init blob for 0x3034 (bit div) and 0x3037 (pll r
> div). This is because bit div and pll root div never get written to
> the expected values (8 and 2), so they remain as defaults (10 and 1).
> It would also be possible to modify set_mipi_pclk to just write these
> values there, but I didn't wan't to mess with your functions too much.
> 
> *Change MIPI SCLK constraint in comments to match the notes found
> here: https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg.
> It seems that the pixels are serialized into components when they
> cross from SCLK to PCLK, so the MIPI serial clock does not care about
> cpp, only bpc (bits/component).
> 
> *Lower MIPI DIV to 1 for now. It may be necessary to conditionally set
> it later if people are still having trouble, but always using 2 will
> make PCLK<SCLK*cpp, and definitely break non-scaled resolutions.
> 
> *MIPI div register doesn't need a -1. When set to zero, it actually
> divides by 16.
> 
> Also, FYI, I've made some improvements to my clock configuration
> spreadsheet and incorporated a register dump function into set_mode
> that prints the relevant registers so they can be copied into the
> spreadsheet for interpretation. Just let me know if anyone wants it.

Thanks!

Daniel, is this fixing your issue?

Maxime

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

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

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[jacopo mondi]

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

Hi ov5640 people,
    I'm happy to finally jump on the bandwagon...

I had a few days to test the sensor driver (on 2 platforms) and unfortunately
this series breaks MIPI capture for me as well..

On Fri, Jun 01, 2018 at 04:05:58PM -0700, Sam Bobrowicz wrote:
> >> On May 21, 2018, at 12:39 AM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >>
> >>> On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> >>>> On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
> >>>> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> >>>>
> >>>> Part of the hardcoded initialization sequence is to set up the proper
> >>>> clock
> >>>> dividers. However, this is now done dynamically through proper code and as
> >>>> such, the static one is now redundant.
> >>>>
> >>>> Let's remove it.
> >>>>
> >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >>>> ---
> >>>
> >>>
> >>> [...]
> >>>
> >>>> @@ -625,8 +623,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},
> >>>
> >>>
> >>> This is the mode that I'm testing with. Previously, the hard-coded registers
> >>> here were:
> >>>
> >>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> >>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
> >>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
> >>>
> >>> Your new code that calculates the clock rates dynamically ends up with
> >>> different values however:
> >>>
> >>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> >>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
> >>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
> >>>
> >>> Interestingly, leaving the hard-coded values in the array *and* letting
> >>> ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> >>> that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> >>> values of these timing registers. You might need to leave these values as
> >>> dummies in the array. Confusing.
> >>>
> >>> Any idea?
> >>>
> >>>
> >>> Thanks,
> >>> Daniel
> >>
> >> This set of patches is also not working for my MIPI platform (mine has
> >> a 12 MHz external clock). I am pretty sure is isn't working because it
> >> does not include the following, which my tests have found to be
> >> necessary:
> >>
> >> 1) Setting pclk period reg in order to correct DPHY timing.
> >> 2) Disabling of MIPI lanes when streaming not enabled.
> >> 3) setting mipi_div to 1 when the scaler is disabled
> >> 4) Doubling ADC clock on faster resolutions.
> >
> > Yeah, I left them out because I didn't think this was relevant to this
> > patchset but should come as future improvements. However, given that
> > it works with the parallel bus, maybe the two first are needed when
> > adjusting the rate.
> >
> I agree that 1-4 are separate improvements to MIPI mode that may not
> affect all modules. They do break mine, but that has been true since
> the driver was released (mainly because of my 12 MHz clock and more
> stringent CSI RX requirements). So it makes sense for me to address
> them in a follow-up series. But I do think that we should get the
> clock generation a little closer to what I know works for MIPI so we
> don't break things for people that do have MIPI working.
>
> > The mipi divider however seems to be a bit more complicated than you
> > report here. It is indeed set to 1 when the scaler is enabled (all
> > resolutions > 1280 * 960), but it's also set to 4 in some cases
> > (640x480@30, 320x240@30, 176x144@30). I couldn't really find any
> > relationship between the resolution/framerate and whether to use a
> > divider of 2 or 4.
>
> I didn't notice the divide by 4, interesting. I have a theory
> though... it could be that the constraint of PCLK relative to SCLK is:
>
> SCLK*(cpp/scaler ratio)<=PCLK<= ?
> cpp=Components/pixel (1 for JPEG, 2 for YUV, e.g.)
>
> Since the scaler is in auto mode, the scaler ratio might automatically
> change depending on the resolution selected, something like (using int
> math):
>
> (hTotal/hActive) * (vTotal/vActive) = scaler ratio
>
> If SCLK is responsible for reading the data into the scaler, and PCLK
> is responsible for reading data out to the physical interface, this
> would make sense. It will require more experiments to verify any of
> this, though, and unfortunately I don't have a lot of time to put into
> this right now.
>
> On my platform I have also run into an upper bound for PCLK, where it
> seems that PCLK must be <= SCLK when the scaler is enabled. I think
> this may have to do with some of my platform's idiosyncrasies, so I'm
> not ready to say that it needs to be addressed in this series. But if
> others run into it while testing MIPI, you should consider
> implementing #3 above to address it.
>
> > And the faster resolutions were working already, so I guess the ADC
> > clock is already fast enough with a 24MHz oscillator?
>
> That's my theory. It seems to have pretty loose requirements as long
> as it is fast enough, which is why I did the simple 2x solution. It
> doesn't need to be addressed here though. If anyone runs into images
> that are all black or bluish, this is a possible culprit.
>
> > Maxime
> >
> > --
> > Maxime Ripard, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>
> I'm back, sorry for the delay. Here is a patch that should fix a few
> things for MIPI users. Just apply it directly after applying the
> series. Someone else should definitely verify this on a different MIPI
> platform.

It doesn't help in my case sorry.

Without this series applied I can capture in all resolutions <
1280x800, with Maxime series (and your patch on top) capture breaks...

>
> https://nofile.io/f/W8J3thK7pOp/clock_fixes.patch
>

Sam, I remember you had shared several patches based on a previous
version of this series that did not only address what this last one
does but also took care of programming the rest of the clock tree even when
running in MIPI mode.

For my understanding (based on the PLL clock tree drawing Daniel shared and
from the few informations on the datasheet), SCLK goes to the ISP so it needs
to be programmed even when running in MIPI mode (through the
configuration of PLL_ROOT_DIV and BIT_DIV dividers, which this series
does not take into account when running in MIPI mode).

I went down the same path you took to unify the DVP and MIPI clock
tree programming (the only thing that actually changes is the input
rate calculation) and I got some mixed results. Do those patches still
apply here or they got obsoleted by later findings?


> These are the noteworthy changes I made.
>
> *Added writes to init blob for 0x3034 (bit div) and 0x3037 (pll r
> div). This is because bit div and pll root div never get written to
> the expected values (8 and 2), so they remain as defaults (10 and 1).
> It would also be possible to modify set_mipi_pclk to just write these
> values there, but I didn't wan't to mess with your functions too much.
>
> *Change MIPI SCLK constraint in comments to match the notes found
> here: https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg.
> It seems that the pixels are serialized into components when they
> cross from SCLK to PCLK, so the MIPI serial clock does not care about
> cpp, only bpc (bits/component).
>
> *Lower MIPI DIV to 1 for now. It may be necessary to conditionally set
> it later if people are still having trouble, but always using 2 will
> make PCLK<SCLK*cpp, and definitely break non-scaled resolutions.
>
> *MIPI div register doesn't need a -1. When set to zero, it actually
> divides by 16.
>
> Also, FYI, I've made some improvements to my clock configuration
> spreadsheet and incorporated a register dump function into set_mode
> that prints the relevant registers so they can be copied into the
> spreadsheet for interpretation. Just let me know if anyone wants it.

I would like to have the updated spreadsheet, yes... seems like we all have
different sources of informations, it would be nice to clear the table
a bit...

Also, I had some issues with capture start stability, it failed time
to time, and I guess this is due to the awkward MIPI interface
initialization in the current driver version, that requires a stream
start/stop sequence at power up time to force lanes in LP11 mode.
Depending on the receiver, this may cause troubles or not, apparently.

I'm about to share a few patches that addresses this (something Sam tried
to address as well in the previous series shared on dropbox by powering down
the individual CSI-2 lanes at streamoff time). Anyone that could test
those patches, if of course very welcome.

Thanks
   j

>
> Sam

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

[PATCH v3 04/12] media: ov5640: Remove redundant defines

[Maxime Ripard]

The OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT and OV5640_SCLK_ROOT_DIVIDER_DEFAULT
defines represent exactly the same setup, and are at the same value, than
the more consistent with the rest of the driver OV5640_SCLK2X_ROOT_DIV and
OV5640_SCLK_ROOT_DIV.

Remove them.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 45e5ba32b8d1..ce9bfaafb675 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -93,9 +93,6 @@
 #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,
@@ -1961,8 +1958,8 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 		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));
+			     (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
+			     ilog2(OV5640_SCLK_ROOT_DIV));
 	if (ret)
 		return ret;
 
-- 
2.17.0

[PATCH v3 05/12] media: ov5640: Remove redundant register setup

[Maxime Ripard]

The MIPI divider is also cleared as part of the clock setup sequence, so we
can remove that code.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index ce9bfaafb675..77864a1a5eb0 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1281,16 +1281,6 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 	 */
 
 	if (on) {
-		/*
-		 * reset MIPI PCLK/SERCLK divider
-		 *
-		 * SC PLL CONTRL1 0
-		 * - [3..0]:	MIPI PCLK/SERCLK divider
-		 */
-		ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
-		if (ret)
-			return ret;
-
 		/*
 		 * configure parallel port control lines polarity
 		 *
-- 
2.17.0

[PATCH v3 06/12] media: ov5640: Compute the clock rate at runtime

[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 change.
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, and never really
tested with a DVP bus.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 77864a1a5eb0..e9bd0aa55409 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1886,7 +1886,8 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	 * which is 8 bits per pixel.
 	 */
 	bpp = sensor->fmt.code == MEDIA_BUS_FMT_JPEG_1X8 ? 8 : 16;
-	rate = mode->pixel_clock * bpp;
+	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);
-- 
2.17.0

[PATCH v3 07/12] media: ov5640: Remove pixel clock rates

[Maxime Ripard]

The pixel clock rates were introduced to report the initially static clock
rate.

Since this is now handled dynamically, we can remove them entirely.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e9bd0aa55409..48f3c9e640ed 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -171,7 +171,6 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
-	u32 pixel_clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -691,7 +690,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,
-	56000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -701,91 +699,74 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
-		 28000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
-		 28000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
-		 28000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
-		 28000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
-		 28000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
-		 28000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
-		 21000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
-		 42000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
-		 84000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
-		 56000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
-		 56000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
-		 56000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
-		 56000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
-		 56000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
-		 56000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
-		 42000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
-		 84000000,
 		 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},
 	},
 };
 
-- 
2.17.0

[PATCH v3 08/12] media: ov5640: Enhance FPS handling

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

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 316 +++++++------------------------------
 1 file changed, 60 insertions(+), 256 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 48f3c9e640ed..785a692946b6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -334,64 +334,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},
@@ -410,7 +353,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},
@@ -429,7 +372,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},
@@ -448,7 +391,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},
@@ -467,26 +410,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},
@@ -505,26 +429,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},
@@ -543,47 +448,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},
@@ -602,40 +467,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},
@@ -667,7 +499,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},
@@ -695,79 +527,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)
@@ -1627,7 +1423,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;
@@ -1635,12 +1431,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;
 }
@@ -2620,10 +2424,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;
@@ -2836,7 +2640,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;
-- 
2.17.0

[PATCH v3 09/12] media: ov5640: Make the return rate type more explicit

[Maxime Ripard]

In the ov5640_try_frame_interval function, the ret variable actually holds
the frame rate index to use, which is represented by the enum
ov5640_frame_rate in the driver.

Make it more obvious.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 785a692946b6..b9a8f3feed74 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1889,8 +1889,8 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 				     u32 width, u32 height)
 {
 	const struct ov5640_mode_info *mode;
+	enum ov5640_frame_rate rate = OV5640_30_FPS;
 	u32 minfps, maxfps, fps;
-	int ret;
 
 	minfps = ov5640_framerates[OV5640_15_FPS];
 	maxfps = ov5640_framerates[OV5640_30_FPS];
@@ -1913,10 +1913,10 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	else
 		fi->denominator = minfps;
 
-	ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+	rate = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
 
-	mode = ov5640_find_mode(sensor, ret, width, height, false);
-	return mode ? ret : -EINVAL;
+	mode = ov5640_find_mode(sensor, rate, width, height, false);
+	return mode ? rate : -EINVAL;
 }
 
 static int ov5640_get_fmt(struct v4l2_subdev *sd,
-- 
2.17.0

[PATCH v3 10/12] media: ov5640: Make the FPS clamping / rounding more extendable

[Maxime Ripard]

The current code uses an algorithm to clamp the FPS values and round them
to the closest supported one that isn't really allows to be extended to
more than two values.

Rework it a bit to make it much easier to extend the amount of FPS options
we support.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index b9a8f3feed74..0f6c39080d69 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1890,7 +1890,8 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 {
 	const struct ov5640_mode_info *mode;
 	enum ov5640_frame_rate rate = OV5640_30_FPS;
-	u32 minfps, maxfps, fps;
+	int minfps, maxfps, best_fps, fps;
+	int i;
 
 	minfps = ov5640_framerates[OV5640_15_FPS];
 	maxfps = ov5640_framerates[OV5640_30_FPS];
@@ -1901,19 +1902,21 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 		return OV5640_30_FPS;
 	}
 
-	fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
+	fps = clamp_val(DIV_ROUND_CLOSEST(fi->denominator, fi->numerator),
+			minfps, maxfps);
+
+	best_fps = minfps;
+	for (i = 0; i < ARRAY_SIZE(ov5640_framerates); i++) {
+		int curr_fps = ov5640_framerates[i];
+
+		if (abs(curr_fps - fps) < abs(best_fps - fps)) {
+			best_fps = curr_fps;
+			rate = i;
+		}
+	}
 
 	fi->numerator = 1;
-	if (fps > maxfps)
-		fi->denominator = maxfps;
-	else if (fps < minfps)
-		fi->denominator = minfps;
-	else if (2 * fps >= 2 * minfps + (maxfps - minfps))
-		fi->denominator = maxfps;
-	else
-		fi->denominator = minfps;
-
-	rate = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+	fi->denominator = best_fps;
 
 	mode = ov5640_find_mode(sensor, rate, width, height, false);
 	return mode ? rate : -EINVAL;
-- 
2.17.0

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

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

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 0f6c39080d69..a8852ded60b6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -109,6 +109,7 @@ enum ov5640_mode_id {
 enum ov5640_frame_rate {
 	OV5640_15_FPS = 0,
 	OV5640_30_FPS,
+	OV5640_60_FPS,
 	OV5640_NUM_FRAMERATES,
 };
 
@@ -137,6 +138,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 */
@@ -1439,12 +1441,23 @@ 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;
 		}
 	}
 
-	if (nearest && i < 0)
-		mode = &ov5640_mode_data[0];
+	/* VGA is the only mode that supports all the framerates */
+	if (i < 0) {
+		if (!nearest)
+			return NULL;
+
+		mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480];
+	}
 
 	return mode;
 }
@@ -1894,12 +1907,13 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	int i;
 
 	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;
+		rate = OV5640_60_FPS;
+		goto find_mode;
 	}
 
 	fps = clamp_val(DIV_ROUND_CLOSEST(fi->denominator, fi->numerator),
@@ -1918,6 +1932,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	fi->numerator = 1;
 	fi->denominator = best_fps;
 
+find_mode:
 	mode = ov5640_find_mode(sensor, rate, width, height, false);
 	return mode ? rate : -EINVAL;
 }
@@ -2495,8 +2510,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;
-- 
2.17.0

[PATCH v3 12/12] media: ov5640: Remove duplicate auto-exposure setup

[Maxime Ripard]

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

Remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a8852ded60b6..6df227b22303 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -498,7 +498,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[] = {
-- 
2.17.0

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Daniel Mack]

Hi,

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> 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.

I'd like to give this a try. What tree should this patch set be applied 
on? I had no luck with media_tree/for-4.18-6.

Thanks,
Daniel


> 
> Let me know what you think,
> Maxime
> 
> Changes from v2:
>    - Rebased on latest Sakari PR
>    - Fixed the issues reported by Hugues: improper FPS returned for
>      formats, improper rounding of the FPS, some with his suggestions,
>      some by simplifying the logic.
>    - Expanded the clock tree comments based on the feedback from Samuel
>      Bobrowicz and Loic Poulain
>    - Merged some of the changes made by Samuel Bobrowicz to fix the
>      MIPI rate computation, fix the call sites of the
>      ov5640_set_timings function, the auto-exposure calculation call,
>      etc.
>    - Split the patches into smaller ones in order to make it more
>      readable (hopefully)
> 
> 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 (11):
>    media: ov5640: Adjust the clock based on the expected rate
>    media: ov5640: Remove the clocks registers initialization
>    media: ov5640: Remove redundant defines
>    media: ov5640: Remove redundant register setup
>    media: ov5640: Compute the clock rate at runtime
>    media: ov5640: Remove pixel clock rates
>    media: ov5640: Enhance FPS handling
>    media: ov5640: Make the return rate type more explicit
>    media: ov5640: Make the FPS clamping / rounding more extendable
>    media: ov5640: Add 60 fps support
>    media: ov5640: Remove duplicate auto-exposure setup
> 
> Samuel Bobrowicz (1):
>    media: ov5640: Fix timings setup code
> 
>   drivers/media/i2c/ov5640.c | 701 +++++++++++++++++++++----------------
>   1 file changed, 392 insertions(+), 309 deletions(-)
> 

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

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

Hi,

On Thu, May 17, 2018 at 11:24:04AM +0200, Daniel Mack wrote:
> Hi,
> 
> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > 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.
> 
> I'd like to give this a try. What tree should this patch set be applied on?
> I had no luck with media_tree/for-4.18-6.

Maybe it wasn't the latest after all, sorry. It's based on Sakari's
for-4.18-3 PR (67f76c65e94f).

Maxime

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

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

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Sakari Ailus]

On Thu, May 17, 2018 at 01:22:07PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, May 17, 2018 at 11:24:04AM +0200, Daniel Mack wrote:
> > Hi,
> > 
> > On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > > 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.
> > 
> > I'd like to give this a try. What tree should this patch set be applied on?
> > I had no luck with media_tree/for-4.18-6.
> 
> Maybe it wasn't the latest after all, sorry. It's based on Sakari's
> for-4.18-3 PR (67f76c65e94f).

I've pushed these here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.18-5>

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

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Hugues FRUCHET]

Hi Maxime & all OV5640 stakeholders,

I've just pushed a new patchset also related to rate/pixel clock 
handling [1], based on your V3 great work:
 >    media: ov5640: Adjust the clock based on the expected rate
 >    media: ov5640: Remove the clocks registers initialization
 >    media: ov5640: Remove redundant defines
 >    media: ov5640: Remove redundant register setup
 >    media: ov5640: Compute the clock rate at runtime
 >    media: ov5640: Remove pixel clock rates
 >    media: ov5640: Enhance FPS handling

This is working perfectly fine on my parallel setup and allows me to 
well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps 
that I never seen working before.
So at least for the parallel setup, this serie is working fine for all 
the discrete resolutions and framerate exposed by the driver for the moment:
* QCIF 176x144 15/30fps
* QVGA 320x240 15/30fps
* VGA 640x480 15/30fps
* 480p 720x480 15/30fps
* XGA 1024x768 15/30fps
* 720p 1280x720 15/30fps
* 1080p 1920x1080 15/30fps
* 5Mp 2592x1944 15fps

Moreover I'm not clear on relationship between rate and pixel clock 
frequency.
I've understood that to DVP_PCLK_DIVIDER (0x3824) register
and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
All the resolutions up to 720x576 are forcing a manual value of 2 for 
divider (0x460c=0x22), but including 720p and more, the divider value is 
controlled by "auto-mode" (0x460c=0x20)... from what I measured and
understood, for those resolutions, the divider must be set to 1 in order 
that your rate computation match the effective pixel clock on output, 
see [2].

So I wonder if this PCLK divider register should be included
or not into your rate computation, what do you think ?


[1] OV5640: reduce rate according to maximum pixel clock 
https://www.spinics.net/lists/linux-media/msg140958.html:
[2] media: ov5640: move parallel port pixel clock divider out of 
registers set https://www.spinics.net/lists/linux-media/msg140960.html

BR,
Hugues.

On 05/17/2018 10:53 AM, Maxime Ripard wrote:
> 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 v2:
>    - Rebased on latest Sakari PR
>    - Fixed the issues reported by Hugues: improper FPS returned for
>      formats, improper rounding of the FPS, some with his suggestions,
>      some by simplifying the logic.
>    - Expanded the clock tree comments based on the feedback from Samuel
>      Bobrowicz and Loic Poulain
>    - Merged some of the changes made by Samuel Bobrowicz to fix the
>      MIPI rate computation, fix the call sites of the
>      ov5640_set_timings function, the auto-exposure calculation call,
>      etc.
>    - Split the patches into smaller ones in order to make it more
>      readable (hopefully)
> 
> 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 (11):
>    media: ov5640: Adjust the clock based on the expected rate
>    media: ov5640: Remove the clocks registers initialization
>    media: ov5640: Remove redundant defines
>    media: ov5640: Remove redundant register setup
>    media: ov5640: Compute the clock rate at runtime
>    media: ov5640: Remove pixel clock rates
>    media: ov5640: Enhance FPS handling
>    media: ov5640: Make the return rate type more explicit
>    media: ov5640: Make the FPS clamping / rounding more extendable
>    media: ov5640: Add 60 fps support
>    media: ov5640: Remove duplicate auto-exposure setup
> 
> Samuel Bobrowicz (1):
>    media: ov5640: Fix timings setup code
> 
>   drivers/media/i2c/ov5640.c | 701 +++++++++++++++++++++----------------
>   1 file changed, 392 insertions(+), 309 deletions(-)
> 

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

Hi Hugues,

On Thu, Sep 27, 2018 at 03:59:04PM +0000, Hugues FRUCHET wrote:
> Hi Maxime & all OV5640 stakeholders,
> 
> I've just pushed a new patchset also related to rate/pixel clock 
> handling [1], based on your V3 great work:
>  >    media: ov5640: Adjust the clock based on the expected rate
>  >    media: ov5640: Remove the clocks registers initialization
>  >    media: ov5640: Remove redundant defines
>  >    media: ov5640: Remove redundant register setup
>  >    media: ov5640: Compute the clock rate at runtime
>  >    media: ov5640: Remove pixel clock rates
>  >    media: ov5640: Enhance FPS handling
> 
> This is working perfectly fine on my parallel setup and allows me to 
> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps 
> that I never seen working before.
> So at least for the parallel setup, this serie is working fine for all 
> the discrete resolutions and framerate exposed by the driver for the moment:
> * QCIF 176x144 15/30fps
> * QVGA 320x240 15/30fps
> * VGA 640x480 15/30fps
> * 480p 720x480 15/30fps
> * XGA 1024x768 15/30fps
> * 720p 1280x720 15/30fps
> * 1080p 1920x1080 15/30fps
> * 5Mp 2592x1944 15fps

I'm glad this is working for you as well. I guess I'll resubmit these
patches, but this time making sure someone with a CSI setup tests
before merging. I crtainly don't want to repeat the previous disaster.

Do you have those patches rebased somewhere? I'm not quite sure how to
fix the conflict with the v4l2_find_nearest_size addition.

> Moreover I'm not clear on relationship between rate and pixel clock 
> frequency.
> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
> All the resolutions up to 720x576 are forcing a manual value of 2 for 
> divider (0x460c=0x22), but including 720p and more, the divider value is 
> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
> understood, for those resolutions, the divider must be set to 1 in order 
> that your rate computation match the effective pixel clock on output, 
> see [2].
> 
> So I wonder if this PCLK divider register should be included
> or not into your rate computation, what do you think ?

Have you tried change the PCLK divider while in auto-mode? IIRC, I did
that and it was affecting the PCLK rate on my scope, but I wouldn't be
definitive about it.

Can we always set the mode to auto and divider to 1, even for the
lower resolutions?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Hugues FRUCHET]

Hi Maxime,

On 09/28/2018 06:05 PM, Maxime Ripard wrote:
> Hi Hugues,
> 
> On Thu, Sep 27, 2018 at 03:59:04PM +0000, Hugues FRUCHET wrote:
>> Hi Maxime & all OV5640 stakeholders,
>>
>> I've just pushed a new patchset also related to rate/pixel clock
>> handling [1], based on your V3 great work:
>>   >    media: ov5640: Adjust the clock based on the expected rate
>>   >    media: ov5640: Remove the clocks registers initialization
>>   >    media: ov5640: Remove redundant defines
>>   >    media: ov5640: Remove redundant register setup
>>   >    media: ov5640: Compute the clock rate at runtime
>>   >    media: ov5640: Remove pixel clock rates
>>   >    media: ov5640: Enhance FPS handling
>>
>> This is working perfectly fine on my parallel setup and allows me to
>> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps
>> that I never seen working before.
>> So at least for the parallel setup, this serie is working fine for all
>> the discrete resolutions and framerate exposed by the driver for the moment:
>> * QCIF 176x144 15/30fps
>> * QVGA 320x240 15/30fps
>> * VGA 640x480 15/30fps
>> * 480p 720x480 15/30fps
>> * XGA 1024x768 15/30fps
>> * 720p 1280x720 15/30fps
>> * 1080p 1920x1080 15/30fps
>> * 5Mp 2592x1944 15fps
> 
> I'm glad this is working for you as well. I guess I'll resubmit these
> patches, but this time making sure someone with a CSI setup tests
> before merging. I crtainly don't want to repeat the previous disaster.
> 
> Do you have those patches rebased somewhere? I'm not quite sure how to
> fix the conflict with the v4l2_find_nearest_size addition.
> 
>> Moreover I'm not clear on relationship between rate and pixel clock
>> frequency.
>> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
>> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
>> All the resolutions up to 720x576 are forcing a manual value of 2 for
>> divider (0x460c=0x22), but including 720p and more, the divider value is
>> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
>> understood, for those resolutions, the divider must be set to 1 in order
>> that your rate computation match the effective pixel clock on output,
>> see [2].
>>
>> So I wonder if this PCLK divider register should be included
>> or not into your rate computation, what do you think ?
> 
> Have you tried change the PCLK divider while in auto-mode? IIRC, I did
> that and it was affecting the PCLK rate on my scope, but I wouldn't be
> definitive about it.

I have tested to change PCLK divider while in auto mode but no effect.

> 
> Can we always set the mode to auto and divider to 1, even for the
> lower resolutions?
This is breaking 176x144@30fps on my side, because of pixel clock too 
high (112MHz vs 70 MHz max).

Instead of using auto mode, my proposal was the inverse: use manual mode 
with the proper divider to fit the max pixel clock constraint.


BR,
Hugues.

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Maxime Ripard]

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

Hi!

On Mon, Oct 01, 2018 at 02:12:31PM +0000, Hugues FRUCHET wrote:
> >> This is working perfectly fine on my parallel setup and allows me to
> >> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps
> >> that I never seen working before.
> >> So at least for the parallel setup, this serie is working fine for all
> >> the discrete resolutions and framerate exposed by the driver for the moment:
> >> * QCIF 176x144 15/30fps
> >> * QVGA 320x240 15/30fps
> >> * VGA 640x480 15/30fps
> >> * 480p 720x480 15/30fps
> >> * XGA 1024x768 15/30fps
> >> * 720p 1280x720 15/30fps
> >> * 1080p 1920x1080 15/30fps
> >> * 5Mp 2592x1944 15fps
> > 
> > I'm glad this is working for you as well. I guess I'll resubmit these
> > patches, but this time making sure someone with a CSI setup tests
> > before merging. I crtainly don't want to repeat the previous disaster.
> > 
> > Do you have those patches rebased somewhere? I'm not quite sure how to
> > fix the conflict with the v4l2_find_nearest_size addition.
> > 
> >> Moreover I'm not clear on relationship between rate and pixel clock
> >> frequency.
> >> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
> >> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
> >> All the resolutions up to 720x576 are forcing a manual value of 2 for
> >> divider (0x460c=0x22), but including 720p and more, the divider value is
> >> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
> >> understood, for those resolutions, the divider must be set to 1 in order
> >> that your rate computation match the effective pixel clock on output,
> >> see [2].
> >>
> >> So I wonder if this PCLK divider register should be included
> >> or not into your rate computation, what do you think ?
> > 
> > Have you tried change the PCLK divider while in auto-mode? IIRC, I did
> > that and it was affecting the PCLK rate on my scope, but I wouldn't be
> > definitive about it.
> 
> I have tested to change PCLK divider while in auto mode but no effect.
> 
> > Can we always set the mode to auto and divider to 1, even for the
> > lower resolutions?
>
> This is breaking 176x144@30fps on my side, because of pixel clock too 
> high (112MHz vs 70 MHz max).

Ok.

> Instead of using auto mode, my proposal was the inverse: use manual mode 
> with the proper divider to fit the max pixel clock constraint.

Oh. That would work for me too yeah. How do you want to deal with it?
Should I send your rebased patches, and you add that change as a
subsequent patch?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements

[Hugues FRUCHET]

Hi Maxime,

On 10/04/2018 05:04 PM, Maxime Ripard wrote:
> Hi!
> 
> On Mon, Oct 01, 2018 at 02:12:31PM +0000, Hugues FRUCHET wrote:
>>>> This is working perfectly fine on my parallel setup and allows me to
>>>> well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps
>>>> that I never seen working before.
>>>> So at least for the parallel setup, this serie is working fine for all
>>>> the discrete resolutions and framerate exposed by the driver for the moment:
>>>> * QCIF 176x144 15/30fps
>>>> * QVGA 320x240 15/30fps
>>>> * VGA 640x480 15/30fps
>>>> * 480p 720x480 15/30fps
>>>> * XGA 1024x768 15/30fps
>>>> * 720p 1280x720 15/30fps
>>>> * 1080p 1920x1080 15/30fps
>>>> * 5Mp 2592x1944 15fps
>>>
>>> I'm glad this is working for you as well. I guess I'll resubmit these
>>> patches, but this time making sure someone with a CSI setup tests
>>> before merging. I crtainly don't want to repeat the previous disaster.
>>>
>>> Do you have those patches rebased somewhere? I'm not quite sure how to
>>> fix the conflict with the v4l2_find_nearest_size addition.
>>>
>>>> Moreover I'm not clear on relationship between rate and pixel clock
>>>> frequency.
>>>> I've understood that to DVP_PCLK_DIVIDER (0x3824) register
>>>> and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency.
>>>> All the resolutions up to 720x576 are forcing a manual value of 2 for
>>>> divider (0x460c=0x22), but including 720p and more, the divider value is
>>>> controlled by "auto-mode" (0x460c=0x20)... from what I measured and
>>>> understood, for those resolutions, the divider must be set to 1 in order
>>>> that your rate computation match the effective pixel clock on output,
>>>> see [2].
>>>>
>>>> So I wonder if this PCLK divider register should be included
>>>> or not into your rate computation, what do you think ?
>>>
>>> Have you tried change the PCLK divider while in auto-mode? IIRC, I did
>>> that and it was affecting the PCLK rate on my scope, but I wouldn't be
>>> definitive about it.
>>
>> I have tested to change PCLK divider while in auto mode but no effect.
>>
>>> Can we always set the mode to auto and divider to 1, even for the
>>> lower resolutions?
>>
>> This is breaking 176x144@30fps on my side, because of pixel clock too
>> high (112MHz vs 70 MHz max).
> 
> Ok.
> 
>> Instead of using auto mode, my proposal was the inverse: use manual mode
>> with the proper divider to fit the max pixel clock constraint.
> 
> Oh. That would work for me too yeah. How do you want to deal with it?
> Should I send your rebased patches, and you add that change as a
> subsequent patch?

Yes, this is the best option, and we can then ask people having CSI 
setup to check for non-regression after having applied this important 
clock serie patch.
Hoping that this will also work on their setup so that we can move 
forward on next OV5640 improvements.

> 
> Thanks!
> Maxime
> 

BR,
Hugues.