Re: [PATCH 2/2] media: i2c: Add OV1063x sensor driver

Hi,

On 20/01/2021 10:26, Laurent Pinchart wrote:

Hi Tomi,

On Mon, Jan 18, 2021 at 04:04:47PM +0200, Tomi Valkeinen wrote:

quoted

On 04/01/2021 07:39, Laurent Pinchart wrote:

quoted

+static int ov1063x_pll_setup(unsigned int clk_rate,
+			     unsigned int *htsmin, unsigned int vts,
+			     unsigned int fps_numerator,
+			     unsigned int fps_denominator,
+			     struct ov1063x_pll_config *cfg)
+{
+	static const unsigned int pre_divs[] = { 2, 3, 4, 6, 8, 10, 12, 14 };
+
+	unsigned int best_pclk = UINT_MAX;
+	unsigned int best_pre_div;
+	unsigned int best_mult;
+	unsigned int best_div;
+	unsigned int best_hts;
+	unsigned int max_pre_div;
+	unsigned int pre_div;
+	unsigned int hts;
+
+	/*
+	 *  XVCLK --> pre-div -------> mult ----------> div --> output
+	 * 6-27 MHz           3-27 MHz      200-500 MHz       Max 96 MHz
+	 *
+	 * Valid pre-divider values are 1, 1.5, 2, 3, 4, 5, 6 and 7. The
+	 * pre_divs array stores the pre-dividers multiplied by two, indexed by
+	 * register values.
+	 *
+	 * Valid multiplier values are [1, 63], stored as-is in registers.
+	 *
+	 * Valid divider values are 2 to 16 with a step of 2, stored in
+	 * registers as (div / 2) - 1.
+	 */
+
+	if (clk_rate < 6 * 1000 * 1000 || clk_rate > 27 * 1000 * 1000)
+		return -EINVAL;
+
+	/*
+	 * We try all valid combinations of settings for the 3 blocks to get
+	 * the pixel clock, and from that calculate the actual hts/vts to use.
+	 * The vts is extended so as to achieve the required frame rate.
+	 */
+
+	max_pre_div = max(clk_rate / (3 * 1000 * 1000),
+			  ARRAY_SIZE(pre_divs) - 1);
+
+	for (pre_div = 0; pre_div <= max_pre_div; pre_div++) {
+		unsigned int clk1 = clk_rate * 2 / pre_divs[pre_div];
+		unsigned int min_mult;
+		unsigned int max_mult;
+		unsigned int mult;

This PLL setup is a bit confusing to understand, because:

- "pre_div" is not the divider value, but an index to the pre_divs array
and a value to be written into the register, and pre_div is also stored
into the pll_cfg.

Correct.

quoted

- "div" _is_ the divider value but it's not stored into the pll_cfg,
instead (div / 2) - 1 is stored there.

Correct too. cfg->div stores the register value.

quoted

And calculating max_pre_div is probably not right above, I think it
should be min(),

Indeed. Good catch, I'll fix that.

quoted

and additionally "clk_rate / (3 * 1000 * 1000)" is
calculating the divider value, not the index, but it's then used as a
max to the index loop... And even if it were the index, it should be -1,
as the loop check uses <=.

Those are real issues too, I'll address them.

quoted

Suggestions:

- Redefine the PLL formula. Instead of having fractional pre_divider (I
wonder if it's actually fractional in the HW... Aren't dividers usually
integer dividers?), redefine the formula as pclk = xvclk * 2 / pre_div *
mul / div, and say that the possible pre_dividers are what's currently
in pre_divs array. Or pclk = xvclk / pre_div * 2 * mul / div, which
gives a different result with integer divisions. I don't know which one
is the correct one (or is it either of those... If the HW handles
fractionals, both are wrong).

- Clearly separate divider value and index/regvalue variables. The
iteration loop should use the plain divider values, and I think it would
be best to store the values as such to pll_config. And map the divider
values to register values only when writing to the register.

I'd agree if it wasn't for the pre-div value. Storing the pre-div value
in pll_config would make it more complicated to calculate the register
value in the caller. And mixing register values and multiplier/divider
values in the structure isn't nice :-S That's why I've stored register
values only in pll_config.

Yes, I wouldn't mix them. But why is pre-div complicating it? Isn't all
that's needed a table with prediv values, which you already have? A
simple lookup to that table will give the needed register value.

Works for me and looks relatively clean, so I'm ok with this fix.

 Tomi