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

[Dave Stevenson]

Hi Antoine

On Fri, 30 Oct 2026 at 14:26, Antoine Pichot <antoine.pichot@xxxxxxxxx> wrote:
>
> Hi Dave,
>
> On Thu, Oct 29, 2026 at 07:06:31PM +0000, Dave Stevenson wrote:
> > I have a 24MHz module and had tested with it, which you'd acknowledged
> > in the previous sentence.
>
> You are right, sorry. v3 replaces that sentence: the 24 MHz tables are
> left to a separate change, with your measurement cited.
>
> > 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.
>
> Thanks. For v3 I am taking your "Move the common registers into the
> global init table" as patch 7, under your authorship, in place of my
> strobe-register patch: the strobe registers now sit in
> ov7251_global_init_setting, which answers your question on 5/7 of v1.

All good with me.

> The move has two side effects on the bus, both in the commit message:
>
> - The common registers are written at power-on instead of at every
>   stream start, so a restart within the runtime PM autosuspend delay
>   skips them. The datasheet allows it (table 2-8: software standby
>   keeps the register values).
> - Register 0x3820 holds 0x40 while the sensor streams, where it held
>   0x00 before. The flip control writes back the value probe cached,
>   and probe now caches it after the init table has written 0x40, not
>   right after the reset when it read 0x00. I poked the old value in
>   during a lit stream: brightness, picture, orientation and frame
>   rate did not change.
>
> Tested on the Book 2 against v2: the clock, timing, PLL, flip and
> strobe registers read back while streaming at 30, 60 and 90 fps are
> identical apart from 0x3820. On a sensor kept powered, a restart and
> mode switches from 30 to 90, to 60 and back to 30 fps give a correct
> first frame every time, at 90.45, 60.15 and 30.0 fps.

That all sounds good.
I hadn't noticed that the common registers were written from power on,
so thanks for checking it. It's generally when there are multiple
resolutions supported that using the reset at the start of the common
registers is of most use.

> The rest of your branch (400 MHz, CCI, HBLANK and MIN_HTS, the
> optional enable GPIO, the gain rename, the VBLANK minimum) I leave to
> you as you offered, to keep v3 to the fixes and the IR camera. I can
> test them on the Book 2 (19.2 MHz, 319.2 MHz link) when you post them.

That's fine with me. I'll leave doing any further work until your
patches land, as the CCI change is tiresome to handle conflicts with.

> Two things for that:
>
> - A data point for the VBLANK minimum. A sweep on the Book 2 in the
>   30 fps mode (exposure 400, gain 16, 20 frames per value) gives
>   normal frames down to 41, white frames from 40 to 26 and frames
>   rolled sideways from 25 to 16, both without CSI-2 errors, CRC
>   errors from 13 and a stalled receiver at 10. The same boundary as
>   on your Pi.

Great. I was expecting it to be down to timing within the pixel array
rather than the MIPI configuration, and too few blanking lines results
in things going awry. I'm happy to hear that it does the same for you.

> - At the tip of the branch (fde519da0503, "media: ov7251: Reduce
>   OV7251_TIMING_MIN_HTS"), unless I misread it, ov7251_set_vflip()
>   writes OV7251_TIMING_FORMAT2 (0x3821) rather than 0x3820, probe
>   stores the 0x3821 read in timing_format1 rather than
>   timing_format2, and the return value of devm_cci_regmap_init_i2c()
>   is not checked. In case it helps before you post.

Doh, too much copy/paste. Thanks for picking it up.
That error is actually in "media: ov7251: Convert to use V4L2 CCI
library". I've updated my local branch, but not repushed.

  Dave

> Thanks,
> Antoine