Re: [PATCH v3 3/4] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli <fernandorimoli11@gmail.com>]

Hi Sakari,

Thanks for the review, and sorry for the slow response.

> > Gating the clock lane was determined to be necessary and sufficient by
> > sweeping the register at runtime on a Surface Pro 9 (IPU6): every value
[...]
> This paragraph fits better to the cover page than to a commit message.

Moved to the cover letter in v4, and expanded there since it now also has
to cover the bit 2 question below.

> How about calling this OV5693_MIPI_CTRL00_LP11?
>
> Didn't IPU6 work with this sensor without setting the 2nd bit?

It did, so I have dropped bit 2 entirely in v4 rather than renaming it.
The macro is gone and only the clock-lane gate is written. The remaining
macro keeps the name ov5647 uses for the same bit.

To answer it properly, from the runtime sweep on my Surface Pro 9 (IPU6),
3 trials of 30 frames plus a 300-frame stability run per value:

  0x20  bit5                    300/300 frames, steady 28.6 fps
  0x24  bit5+bit2               300/300 frames, steady 28.6 fps
  0x00  power-on default        0 frames, "stream stop time out"
  0x04  bit2 alone              0 frames, "stream stop time out"

0x20 and 0x24 are indistinguishable, and bit 2 on its own does nothing
for the link, so bit 5 is both necessary and sufficient here. I had set
bit 2 only because it is part of ov5640's canonical value for this
register, not because anything on IPU6 needed it. That is a bad reason
to write a bit, so it is gone. v4 writes bit 5 only.

While I was in there, the same sweep also covers why the value differs
from ov5647's despite the mechanism being copied from it: bit 4 (line
sync) breaks the link on this receiver rather than being merely
unnecessary.

  0x10  bit4 alone              0 frames, "stream stop time out"
  0x30  bit5+bit4               2 frames, stream collapses
  0x34  bit5+bit4+bit2          2 frames, stream collapses

A bit5-only value recovered to 300/300 later in the same run, after the
bit 4 failures, so these are genuine value effects and not a link that
had got itself wedged.

Since v3 a linux-surface user has reproduced the value question
independently on a Surface Pro 8, a different IPU6 generation (0x9a19,
Tiger Lake) with the INT33BE HID rather than OVTI5693, reading every
value back after writing [1]:

  0x2d  vendor value (control)  30 frames, 28.65 fps
  0x24  bit5+bit2               30 frames, 28.65 fps
  0x20  bit5                    30 frames, 28.65 fps
  0x0d  0x2d with bit 5 clear   0 frames
  0x08  bit3 alone              0 frames
  0x04  bit2 alone              0 frames
  0x01  bit0 alone              0 frames

That 0x0d row is the test I had not run: the vendor value with only bit 5
removed does not stream, so bit 5 is necessary and not just sufficient.
That set has two limits. The register was written over I2C into a stalled
capture rather than by running the patch, and the write lands after
stream on rather than before. So it confirms the value on a second device
and IPU generation, but says nothing about the plumbing. Both sweeps are
in the cover letter.

One further change in v4 that you have not seen: the bit is now set with
cci_update_bits() rather than cci_write(). The ov5693 does not otherwise
program MIPI_CTRL00, and the driver also serves IPU3/CIO2 and Rockchip,
so modifying the single bit leaves whatever the platform left in the
register intact. On the devices in question it reads 0x00 beforehand, so
the two are equivalent in practice and this is just the smaller claim.
It was suggested on the linux-surface thread [1] and seemed right.

One consequence: Dan reviewed and Jakob tested v3's 0x24, and since v4
changes both the value and the form of the write, I dropped both tags
rather than carry them across a behaviour change. Both are asked in the
cover letter to re-confirm.

[1] https://github.com/linux-surface/linux-surface/pull/2171#issuecomment-5298352360

Thanks,
Fernando