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

[Matthias Fend]

Hi Laurent,

Am 22.12.2025 um 14:30 schrieb Laurent Pinchart:
> On Mon, Dec 22, 2025 at 12:17:56PM +0100, Matthias Fend wrote:
> > 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.
>
> I understand that the PLL won't be able to produce the exact nominal
> expected frequency, but can't we require the link-frequencies property
> in DT to match the PLL output exactly ? That's what we do with other
> sensors.

You mean that any link frequency (within the allowed range) can be 
specified in the device tree, which the PLL can generate exactly? The 
values ​​for the v4l2 controls V4L2_CID_PIXEL_RATE and 
V4L2_CID_LINK_FREQ are then also based on the DT link freuqency.

The recently added IMX111 does something similar. For many other 
sensors, the link frequencies are rather fixed.

Thanks
 ~Matthias

> > 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 ;)
> >
> > > > +
> > > > +	*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;
> > > > +}