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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Dropping the mandatory V4L2_CID_LINK_FREQ control breaks compatibility with downstream MIPI CSI-2 receivers.
- [High] Uninitialized clock pointer access causes the device tree link frequency validation to be silently bypassed.
- [High] Integer division precision loss in the link frequency calculation causes mismatches with exact device tree specifications.
- [Medium] The commit message claims to drop the mclk clock rate check, but the corresponding code remains unmodified.
--

commit 92606ae6195a0cd5f3f660281d12726034b8af33
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: fix link frequency handling

This commit modifies link frequency handling to use runtime calculation
based on PLL configuration, lane count, and external clock rate, removing
hardcoded link frequencies. It drops the exposed link_freq control and
adjusts pixel rate calculations.

>     3. dropped mclk clock rate check.

The commit message says the mclk clock rate check was dropped, but looking at
the driver, the check if (mclk_freq != 25000000) appears to still be present
in hi846_probe(). Was this change accidentally omitted from the patch?

[ ... ]

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce5..61297ef66a0eb 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);
>  
> -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;

Could this integer division cause a precision loss?

If mclk is 25000000, 25000000 / 3 truncates to 8333333. Multiplied by 90 and
divided by 4 (the post dividers), this evaluates to 187499992 Hz instead of
the exact 187500000 Hz.

This truncation could cause hi846_check_link_freqs() to fail the equality
check against the frequencies specified in the device tree, falsely rejecting
valid hardware configurations.

Would it be better to reorder the operations to multiply before dividing
(e.g., mclk * 90 / 3 / ...)?

[ ... ]

> @@ -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;
> -

Will dropping the V4L2_CID_LINK_FREQ control break compatibility with
downstream MIPI CSI-2 receivers?

The V4L2 specification mandates the V4L2_CID_LINK_FREQ control for MIPI CSI-2
transmitters. Downstream receiver hardware queries this control via
v4l2_get_link_freq() to dynamically configure their own PLLs and lane timing
constraints. Removing it might prevent receivers from successfully locking onto
the transmitter's data lanes.

[ ... ]

> @@ -1950,16 +1950,17 @@ static int hi846_identify_module(struct hi846 *hi846)
>  static s64 hi846_check_link_freqs(struct hi846 *hi846,
>  				  struct v4l2_fwnode_endpoint *ep)
>  {
> -	const s64 *freqs = hi846_link_freqs;
> -	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
> +	int freqs_count = ARRAY_SIZE(supported_modes);
> +	u64 link_freq;
>  	int i, j;
>  
>  	for (i = 0; i < freqs_count; i++) {
> +		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);

Can this access hi846->clock before it is initialized?

If hi846_check_link_freqs() is called during probe via hi846_parse_dt() before
the clock is initialized, hi846->clock will be NULL.

Calling clk_get_rate(NULL) safely returns 0, leading to a computed link_freq
of 0. When this 0 frequency is not found in the DT endpoints, this function
returns 0. The caller checks if (fq), which evaluates to false for 0,
incorrectly treating the mismatch as a success and bypassing the DT hardware
capability validation.

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