Re: [PATCH v2 1/9] media: i2c: ov7251: Use in-range PLL1 dividers for the 319.2 MHz link

[Dave Stevenson]

Hi Antoine

On Wed, 28 Oct 2026 at 11:22, Antoine Pichot <antoine.pichot@xxxxxxxxx> wrote:
>
> The 19.2 MHz PLL1 table for the 319.2 MHz link frequency puts a
> pre-divider of 1 before the multiplier of 133 and a divider of 4 after
> it, so the PLL1 multiplier output (the VCO) runs at 19.2 x 133 =
> 2553.6 MHz before being divided down to 638.4 MHz. The OV7750/OV7251
> datasheet (rev 2.12, table 2-11) limits it to 300 to 800 MHz.
>
> Use pre-divider 4 (register 0x30B4 = 0x04) and divider 1 (0x30B1 =
> 0x01). The multiplier output is then 4.8 x 133 = 638.4 MHz, inside the
> range, and the MIPI clock stays 638.4 Mbps, so the link frequency does
> not change. The Windows driver programs these values for the Surface
> Book 2's Windows Hello mode.
>
> On ACPI systems this table has been unreachable since it was added,
> because ov7251_check_hwcfg() selects the wrong index. The next patch
> fixes the index, so the two must go in together. With the index fix
> alone, a Surface Pro 7+ (IPU6) had CSI-2 errors in 7 of 10 150-frame
> runs, 3 of them stalls; with this change 0 of 13. A Surface Book 2
> (IPU3) streams clean with either table. Dave Stevenson measured on a
> 24 MHz Raspberry Pi board that the out-of-range tables miss their
> nominal frequency and proposed the same values. The 24 MHz tables are
> left alone because nobody in the thread can test a change to them.

I have a 24MHz module and had tested with it, which you'd acknowledged
in the previous sentence.

You've got me sidetracked into doing the cleanups, and that has
included validating the link frequencies on a 24MHz module.
I've pushed where I am to
https://github.com/6by9/linux/tree/rpi-7.3.y-ov7251. Patch "media:
ov7251: Add support for a 400MHz link freq" fixes up all the 24MHz PLL
settings. I've also done the conversion to use the V4L2 CCI library.

The cleanups do raise the question of what registers 0x3016-0x301c do,
and which of the 2 variants is preferable (60 and 90fps both use the
same values). The datasheet SC_CLKRSTx naming isn't exactly
descriptive. I don't suppose you have any information?
It would be lovely to ditch those tables / merge them into
global_init_settings, retaining ov7251_mode_info_data only to provide
backwards compatibility with those clients that use
set_frame_interval.

Feel free to pick up any or all of those patches, otherwise I'll come
along behind you and submit them.

  Dave

> Fixes: ed9566ce1946 ("media: i2c: Add support for new frequencies to ov7251")
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/linux-media/6ae0e944.b6db0abd.16b13d.d261@xxxxxxxxxxxxx/
> Link: https://lore.kernel.org/linux-media/CAPY8ntBFJJPg3y46gC=bNq_55QifC7mKmeq8+pAEDVN42fGUXw@xxxxxxxxxxxxxx/
> Signed-off-by: Antoine Pichot <antoine.pichot@xxxxxxxxx>
> ---
>  drivers/media/i2c/ov7251.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index fce6344b5120..f429de5f1f7c 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -174,9 +174,9 @@ static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_240_mhz = {
>  };
>
>  static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_319_2_mhz = {
> -       .pre_div = 0x01,
> +       .pre_div = 0x04,
>         .mult = 0x85,
> -       .div = 0x04,
> +       .div = 0x01,
>         .pix_div = 0x0a,
>         .mipi_div = 0x05,
>  };
> --
> 2.56.0
>