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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi Laurent,

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

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

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

And calculating max_pre_div is probably not right above, I think it
should be min(), 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 <=.

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.

 Tomi