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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Matthias,

On Mon, Dec 22, 2025 at 04:58:28PM +0100, Matthias Fend wrote:
> 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

Correct.

> v4l2 controls V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ are then also based
> on the DT link freuqency.

Yes, they should be.

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

Those drivers have no PLL calculators. As you can use an exact frequencies,
please do so. Any error would be visible in frame rates, for instance.

-- 
Kind regards,

Sakari Ailus