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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aUXRsv-r9-sQvpAm@kekkonen.localdomain>
Date: Sat, 20 Dec 2025 00:29:06 +0200
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Matthias Fend <matthias.fend@...end.at>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Hans Verkuil <hverkuil@...nel.org>,
	Hans de Goede <hansg@...nel.org>,
	Ricardo Ribalda <ribalda@...omium.org>,
	André Apitzsch <git@...tzsch.eu>,
	Tarang Raval <tarang.raval@...iconsignals.io>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
	Sylvain Petinot <sylvain.petinot@...s.st.com>,
	Dongcheng Yan <dongcheng.yan@...el.com>,
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Jingjing Xiong <jingjing.xiong@...el.com>,
	Heimir Thor Sverrisson <heimir.sverrisson@...il.com>,
	Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
	Svyatoslav Ryhel <clamor95@...il.com>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Hao Yao <hao.yao@...el.com>
Subject: Re: [PATCH v6 2/2] media: i2c: add Himax HM1246 image sensor driver

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.

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

-- 
Kind regards,

Sakari Ailus