Re: [PATCH v3 2/5] media: hi846: fix link frequency handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Uninitialized clock usage completely bypasses Device Tree link frequency validation.
- [High] Precision loss in link frequency calculation falsely rejects valid Device Tree configurations.
- [High] Native 64-bit integer division breaks compilation on 32-bit platforms.
- [High] Dynamic doubling of `link_freq` for 2-lane modes introduces a severe framerate regression.
- [Medium] Removal of `V4L2_CID_LINK_FREQ` control without a replacement violates V4L2 subsystem APIs.
- [Low] Commit message falsely claims to have dropped the `mclk` clock rate check.
--

commit 42a7ca82b9bb88db227afdc90d80e44ac458930b
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: fix link frequency handling

This commit modifies the link frequency handling in the hi846 driver to
calculate it at runtime based on the PLL configuration, lane count, and
external clock rate instead of using hardcoded values. It also removes
the exposed link frequency control and attaches the configuration to the
current mode.

> 3. dropped mclk clock rate check.

This isn't a bug, but the commit message states the mclk clock rate check
was dropped.

Looking at hi846_probe(), the code block checking if mclk_freq != 25000000
appears to have been left untouched by this patch. Is this description
accurate?

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce5..befd54553df2e 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1192,21 +1185,37 @@ static const struct hi846_datafmt *hi846_find_datafmt(u32 code)
>  	return NULL;
>  }
>  
> -static inline u8 hi846_get_link_freq_index(struct hi846 *hi846)
> +static u64
> +hi846_get_link_freq(struct hi846 *hi846, const struct hi846_mode *cur_mode)
>  {
> -	return hi846->cur_mode->link_freq_index;
> -}
> +	u16 cfg = cur_mode->pll_cfg_4lane;
> +	u64 mclk = clk_get_rate(hi846->clock);

Can mclk be zero here during probe?

Looking at hi846_probe(), hi846_parse_dt() is called before hi846->clock
is requested via devm_v4l2_sensor_clk_get().

When hi846_parse_dt() calls hi846_check_link_freqs(), hi846->clock will be
NULL, causing clk_get_rate() to return 0.

Will this result in a computed frequency of 0, which fails to match the DT
endpoints and completely bypasses the device tree link frequency validation?

> -static u64 hi846_get_link_freq(struct hi846 *hi846)
> -{
> -	u8 index = hi846_get_link_freq_index(hi846);
> +	/* NOTE: 6, 7 actually map to 8, 10, but this won't be that big */
> +	u8 post_div1 = 1 + FIELD_GET(GENMASK(10, 8), cfg);
> +	u8 post_div2 = 1 << FIELD_GET(GENMASK(12, 11), cfg);
>  
> -	return hi846_link_freqs[index];
> +	/*
> +	 * HI846_REG_PLL_CFG_MIPI1_H = 0x025a, it is fixed in listed modes
> +	 * [11:8]: 0x02 => pre_div = 3
> +	 * [7:0]: 0x5a => multiplier = 90
> +	 */
> +	u64 link_freq = mclk / 3 * 90 / post_div1 / post_div2;

Does this native 64-bit division break compilation on 32-bit platforms?

Since mclk is a u64, using the native division operator / with 32-bit values
will emit a call to __udivdi3, which is intentionally absent from the kernel
libgcc. Should this use div_u64() or do_div() instead?

Also, does the order of operations cause a precision loss that falsely
rejects valid device tree configurations?

For the expected 25MHz external clock, dividing by 3 first evaluates to
8333333, truncating the fractional part early. The final calculated frequency
becomes 187499992 instead of 187500000.

Since hi846_check_link_freqs() strictly checks for equality against the DT
values, will this cause validation to fail and abort the probe?

> +	/*
> +	 * for shared modes, since lane count is halved for 2-lane, then getting
> +	 * link_freq doubled to match the same data rate. Since 720x480 is
> +	 * 2-lane only(reg_list_4lane.num_of_regs == 0), no pll cfg for 4-lane,
> +	 * the pll cfg is for 2-lane, so use the cfg as is.
> +	 */
> +	if (2 == hi846->nr_lanes && cur_mode->reg_list_4lane.num_of_regs)
> +		link_freq *= 2;
> +
> +	return link_freq;
>  }
[ ... ]
> @@ -1426,14 +1435,6 @@ static int hi846_init_controls(struct hi846 *hi846)
>  
>  	ctrl_hdlr->lock = &hi846->mutex;
>  
> -	hi846->link_freq =
> -		v4l2_ctrl_new_int_menu(ctrl_hdlr, &hi846_ctrl_ops,
> -				       V4L2_CID_LINK_FREQ,
> -				       ARRAY_SIZE(hi846_link_freqs) - 1,
> -				       0, hi846_link_freqs);
> -	if (hi846->link_freq)
> -		hi846->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -

Does the removal of the V4L2_CID_LINK_FREQ control violate the V4L2 subsystem
APIs?

The V4L2 MIPI CSI-2 specification mandates that transmitter drivers report
their link frequency so receiver PHYs can dynamically configure their timings.

Since the .get_mbus_config pad operation isn't implemented as an alternative,
does this force the V4L2 core to fall back to an inaccurate estimation based
on pixel rate, potentially misconfiguring receiver hardware?

[ ... ]
> @@ -1503,10 +1504,10 @@ static int hi846_set_video_mode(struct hi846 *hi846, int fps)
>  	u64 frame_length;
>  	int ret = 0;
>  	int dummy_lines;
> -	u64 link_freq = hi846_get_link_freq(hi846);
> +	u64 link_freq = hi846_get_link_freq(hi846, hi846->cur_mode);
>  
>  	dev_dbg(&client->dev, "%s: link freq: %llu\n", __func__,
> -		hi846_get_link_freq(hi846));
> +		link_freq);
>  
>  	do_div(link_freq, fps);
>  	frame_length = link_freq;

Does this calculation introduce a framerate regression for 2-lane modes?

Earlier in hi846_get_link_freq(), the link_freq is dynamically doubled
for 2-lane modes to match the data rate.

Because frame_length relies directly on link_freq here, the resulting
frame_length programmed into the HI846_REG_FLL register will also be doubled.

Since FLL dictates the duration of a frame, will doubling it halve the actual
physical output framerate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511090924.269106-1-mitltlatltl@gmail.com?part=2