Re: [PATCH v6 2/2] media: i2c: add Himax HM1246 image sensor driver

[Matthias Fend]

Hi Sakari,

Am 19.12.2025 um 23:29 schrieb Sakari Ailus:
> Hi Matthias,
>
> Thanks for the update.
>
> On Tue, Dec 02, 2025 at 04:26:06PM +0100, Matthias Fend wrote:
>
> ...
>
> > +static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq,
> > +			   u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3)
> > +{
> > +	const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 };
> > +	const u8 sysclk_div_table[] = { 1, 2, 3, 4 };
> > +	const u8 post_div_table[] = { 1, 2, 4, 8 };
> > +	const int sysclk_pclk_ratio = 3; /* Recommended value */
> > +	u32 pclk, vco_out, best_vco_diff;
> > +	int pclk_div_index, sysclk_div_index, post_div_index;
> > +	u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0;
> > +	bool sysclk_pclk_ratio_found = false;
> > +
> > +	if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * In raw mode (1 pixel per clock) the pixel clock is internally
> > +	 * divided by two.
> > +	 */
> > +	pclk = 2 * link_freq / clocks_per_pixel;
> > +
> > +	/* Find suitable PCLK and SYSCLK dividers. */
> > +	for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
> > +	     pclk_div_index++) {
> > +		for (sysclk_div_index = 0;
> > +		     sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
> > +		     sysclk_div_index++) {
> > +			if (sysclk_div_table[sysclk_div_index] *
> > +				    sysclk_pclk_ratio ==
> > +			    pclk_div_table[pclk_div_index]) {
> > +				sysclk_pclk_ratio_found = true;
> > +				break;
> > +			}
> > +		}
> > +		if (sysclk_pclk_ratio_found)
> > +			break;
> > +	}
> > +
> > +	if (!sysclk_pclk_ratio_found)
> > +		return -EINVAL;
> > +
> > +	/* Determine an appropriate post divider. */
> > +	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
> > +	     post_div_index++) {
> > +		vco_out = pclk * pclk_div_table[pclk_div_index] *
> > +			  post_div_table[post_div_index];
> > +
> > +		if (vco_out >= HM1246_PLL_VCO_MIN &&
> > +		    vco_out <= HM1246_PLL_VCO_MAX)
> > +			break;
> > +	}
> > +	if (post_div_index >= ARRAY_SIZE(post_div_table))
> > +		return -EINVAL;
> > +
> > +	/* Find best pre-divider and multiplier values. */
> > +	best_vco_diff = U32_MAX;
> > +	for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
> > +	     div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
> > +		u32 multi, multi_h, multi_l, vco, diff;
> > +
> > +		multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
> > +		if (multi < HM1246_PLL_MULTI_MIN ||
> > +		    multi > HM1246_PLL_MULTI_MAX)
> > +			continue;
> > +
> > +		multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
> > +				   HM1246_PLL_MULTI_L_MAX) +
> > +			  2;
> > +		multi_l = multi / multi_h;
> > +		vco = div_u64((u64)xclk * multi_h * multi_l, div);
> > +
> > +		diff = abs_diff(vco_out, vco);
> > +
> > +		if (diff < best_vco_diff) {
> > +			best_vco_diff = diff;
> > +			pre_div = div;
> > +			multiplier_h = multi_h;
> > +			multiplier_l = multi_l;
> > +		}
> > +
> > +		if (!diff)
> > +			break;
> > +	}
> > +
> > +	if (best_vco_diff == U32_MAX)
> > +		return -EINVAL;
>
> How much difference is acceptable? Isn't any difference a bug either in DT
> or the code above? In other words, I'd return an error in that case.

Hard to tell, but almost every input clock will result in a slight 
difference. Even the recommended reference register configuration 
doesn't create a perfect match. Therefore, I don't think it's a good 
idea to treat every deviation as an error.

However, every supported input frequency (6-27MHz) will result in a 
pixel clock deviation of less than 0.5%. Since the sensor uses a 
parallel interface, these frequencies will all work without any 
problems. The frame timings may, of course, be slightly different.

To change this and prevent any deviation, one would probably have to 
replace the PLL calculation with one or more dedicated frequency-setup 
pairs (with adjusted pixelclocks). Which wouldn't be ideal, as the 
solution isn't very flexible - and I've invested quite a bit of effort 
in the PLL calculation ;)

Thanks
 ~Matthias

> > +
> > +	*pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1);
> > +	*pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) |
> > +		HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2);
> > +	*pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) |
> > +		HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) |
> > +		HM1246_PLL3CFG_PCLK_DIV(pclk_div_index);
> > +
> > +	return 0;
> > +}