Re: [PATCH v3 3/5] media: ov02c10: Split the PLL registers off the mode tables

[Felipe Calliari <calliarifelipe@gmail.com>]

Hi Sakari,

Thanks for the review.

On Fri, Oct 02, 2026 at 11:46:27PM +0300, Sakari Ailus wrote:
> On Wed, Sep 23, 2026 at 08:40:58PM -0300, Felipe Calliari wrote:
> >  static const struct reg_sequence sensor_1928x1092_30fps_setting[] = {
> > -	{0x0301, 0x08},
> > -	{0x0303, 0x06},
>
> I haven't tried to figure out what this register exatly is, but this patch
> changes how this register is programmed. After this patch, the value of
> this register is always 5.

It was always 5 before this patch too. ov02c10_enable_streams() writes
the common table and then the per-lane table, and both per-lane tables
write 0x0303 and 0x0305 again:

  common table:  0x0303 = 0x06, 0x0305 = 0xe0
  1-lane table:  0x0303 = 0x05, 0x0305 = 0x90
  2-lane table:  0x0303 = 0x05, 0x0305 = 0x90

The tables are written every time streaming starts, after the sensor
is powered up, and the sensor only starts streaming afterwards, when
0x0100 is set. So when streaming starts, 0x0303 is always 0x05 and
0x0305 always 0x90, before and after this patch.

The tables come from the out-of-tree driver [1], where each mode table
has the same pair: 0x06 and 0xe0 at the top, and 0x05 and 0x90 under a
"// plls" comment at its end.

Still, you're right that the patch changes how the register is
programmed, and the commit message didn't say so. In v4 the series no
longer touches these writes: the common table is left alone, 0x0303 =
0x06 and 0x0305 = 0xe0 included, and only the "plls" block of the
per-lane tables moves into the PLL table, which is written right after
the common table. Every register write is kept, with the same value, so
0x0303 still starts at 0x06 and is then set to 0x05, and there is no
change in behaviour.

If you'd like the repeated writes in the common table dropped, I can do
that in a separate patch after this series is merged.

> > +static const struct reg_sequence sensor_pll_19_2mhz_setting[] = {
>
> I guess some of these settings are determining link frequency or pixel
> rate, so they're not generic to anything using 19,2 MHz external clock.

You're right, they set both. I have no PLL documentation, but sweeping
the registers on my 26 MHz unit shows that 0x0304/0x0305 is the
multiplier for the link and 0x0315/0x0316 the one for the pixel clock.
0x031c divides the pixel clock, and 0x0301 and 0x0303 act on the link.

In v4 the PLL table goes with the link frequency instead, as in
ov08x40: a link_freq_configs[] array indexed like
link_freq_menu_items[], each entry holding the PLL table and the
external clock rate it is for. Probe picks the first link frequency
listed by the firmware that the external clock can produce.

There were no comments on patches 4 and 5, so I'll send v4 once I've
tested it on my hardware.

[1] https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ov02c10.c

--
Regards,
Felipe