Re: [PATCH v5 2/3] media: i2c: Add Sony IMX681 sensor driver

[Sergey Lebedev]

Hi Sakari,

Thank you. v6 is on the list with all of this in it bar two points, below.

The dual PLL was the remark that mattered, and the driver contradicted
itself in a way I could have caught without you (sorry): the link frequency
is taken off the output PLL and IMX681_PIXEL_RATE was then taken off the same
side, giving 387,840,000, which is 16.16 fps against a cover letter claiming
thirty. Rather than pick the arithmetic that lands on thirty, I read the
part. At its operating point the video-timing chain is PRE_PLL_CLK_DIV 2 and
PLL_MULTIPLIER 225, so a 2160 MHz VCO, then VT_SYS_CLK_DIV 2 and
VT_PIX_CLK_DIV 6 - by the CCS formula 180 MHz, which would be 7.5 fps. It
runs at 30. Those registers are at the CCS addresses without carrying the
CCS meaning, so the second derivation is no better than the first, and v6
states the rate as measured: LINE_LENGTH_PCK x FRAME_LENGTH_LINES x 29.95
fps is 720,000,000, with a comment saying why it is not derived.

Naming the CCS registers, which your imx355 pointer prompted, turned up two
you had not flagged: 0x0307 is the low byte of the 16-bit PLL_MULTIPLIER and
0x030d the low byte of OP_PRE_PLL_CLK_DIV. Same partial-write fault as the
crop registers, one block further along. Both are whole in v6.

It also settled the shape of the map, which the driver had never stated:
geometry, both PLLs, the external clock and the signalling mode are at their
CCS addresses, while exposure and frame length are 24-bit at 0x0229 and
0x033d. 0x0229 falls inside CCS's SHORT_ANALOG_EXPONENTIAL_GAIN_GLOBAL, so I
checked it on the part - with the blanking raised, exposure reads 6636 in
both the control and 0x0229, frame length 6640 in both, while 0x0340 reads
zero and 0x0202 does not follow the control.

Two I have not applied.

.set_fmt stays. Dropping it and making the bus code follow the flips cannot
both hold: init_state() runs once, so after a flip the stored format still
names a code the sensor has stopped delivering and the pipeline rejects it
with EPIPE at stream on. I found that by applying both. Every driver in the
tree that picks its Bayer code from the flips keeps set_fmt - fourteen of
them, imx219 and imx355 among them - and thirteen carry MODIFY_LAYOUT on
both flips as this one does. imx219 recomputes the code inside set_fmt,
which is what v6 does too.

The comment above pm_runtime_get_if_active() stays, if you will have it. It
is there because getting that three-valued return wrong cost a regression in
v2 that v3 had to undo - the -EINVAL case, where runtime PM is absent, the
device is powered from probe and the write must still go out with no
reference to drop. gc08a3, gc05a2 and imx283 all comment that call site
too, though about a different thing - when controls reach the part rather
than what the return value means.

One number on the exposure default, which is applied. With it at the minimum,
libcamera's generic AGC needs more than a hundred frames to leave it: a
hundred-frame capture ends at exposure 42 of 3173 and a black frame, three
hundred at 136 and still black, four hundred reaches an image. imx219 and
imx283 default mid-scale; ov08x40, imx415 and gc05a2 default to the maximum.
None of them starts at the minimum. Your reasoning holds - userspace does
set it - so v6 has the minimum, and the number is here in case it changes
your mind.

The "Uh-oh." is mine (sorry again): I converted thirteen sites to
dev_err_probe() in v4 and left a `ret =` standing above one of them.

Andre Gilerson wrote this driver and is on Cc. And the binding ack Krzysztof
asked for is properly his.

Sergey