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

[Sergey Lebedev]

What this is answering, since most of you cannot see it
=======================================================

These replies answer sashiko-bot, an automated reviewer at sashiko.dev. Where
its mail goes is worth stating, because it is not here: devicetree, robh,
conor+dt and media-ci, and not linux-media at all. So the findings I am
quoting have not reached most people reading this, and the review of the
btintel series that Luiz pointed me at earlier tonight reached no list at all
and exists only on the site. I am quoting rather than linking for that reason.

Credit before I start disagreeing with it, because it has earned some. This
series is at v3 because of that reviewer. Its pass on v1 raised five things
and four were real: devm_regulator_bulk_get() ignored, which on a partial
failure leaves freed regulators in supplies[] rather than NULLs; a
pm_runtime_get_if_active() tested so that -EINVAL read as success and a
reference never taken was put; ANALOGUE_GAIN and DIGITAL_GAIN fighting over
one register, which I then measured at 0x020E and which is real; and a missing
pm_runtime_put_noidle(). All four are fixed. Its pass on v2 caught a
regression I introduced while fixing the second of those, and that is the
whole reason a v3 exists. None of those would have been found by the testing I
had listed in the v1 cover letter.

The three things below are where I cannot follow it, or where I can and it is
not this series' to change. Set out at length so the reasoning can be checked
rather than taken on trust. If I have any of it wrong I would rather hear it.

Correcting myself on ipu-bridge
===============================

In my answer to the v2 review I wrote that the dangling secondary fwnodes are
reached on "the ordinary unbind path rather than an error path". That is
wrong. ipu_bridge_unregister_sensors() has exactly two callers and both are
inside ipu_bridge_init()'s error unwind; there is no module_exit, no remove
callback and no devm_add_action in the file, and on success struct ipu_bridge
is deliberately left alive, as its own comment says. I described the reach
without grepping for the callers.

So both halves of that finding are error-path only, and the one I called
narrow and the one I called wider are the same width. The pointers are still
real - nothing clears primary->secondary or the csi_dev's secondary anywhere,
and software_node_unregister_node_group() frees what they point at - but
reaching them needs ipu_bridge_init() to fail after sensors are connected.
Smaller than what I claimed, and the correct claim.

The VBLANK/exposure finding: I do not think it holds
====================================================

The review asks whether clamping the exposure range on a VBLANK change leaves
the hardware register out of sync, because the two controls are not clustered.

__v4l2_ctrl_modify_range() writes it itself. From v4l2-ctrls-api.c:

    cur_to_new(ctrl);
    if (validate_new(ctrl, ctrl->p_new)) { ... = def; }
    ...
    value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
    if (value_changed)
            ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);

validate_new() reaches std_validate, which clamps an integer control with
ROUND_TO_RANGE - clamp_t(val, minimum, maximum) - rather than rejecting it. So
a clamped exposure is a changed value, set_ctrl() runs the driver's own
s_ctrl for V4L2_CID_EXPOSURE, and the register is written. It also happens in
the right order: exposure shrinks before the frame length that forced it to.

If I have misread the core I would rather be told than leave it, but as far as
I can follow it there is nothing to fix here.

The group hold finding: fair, and not a bug
===========================================

Correct as an observation. Each control's handler opens and closes group hold
around its own write, so setting exposure, gain and VBLANK together through
VIDIOC_S_EXT_CTRLS gives three separate groups that the sensor may apply on
different frames.

Two reasons it is not in this version. V4L2 calls s_ctrl once per control
unless they are clustered, so nothing is being lost that the current shape
could have kept - clustering is what would add the guarantee, not what would
restore it. And ov5675, the nearest driver in-tree that uses a group-hold
register at all, uses it the other way: to make one logical value spread over
several registers atomic, not to group controls.

It is a real improvement and it is the author's design to change. Andre is
back on 28 September; I would rather put it to him than reshape his control
handling on his behalf for a second time in one night. If a maintainer wants
it sooner, say so and it goes in the next version.

One last thing about the reviewer, since it will reach your patches too
=======================================================================

It has now reviewed this series' 3/3 twice, on v2 and on v3, and that patch is
byte-identical between the two - I diffed the files. The v2 run raised an ACPI
reference leak and the fwnode pointers; the v3 run raised only the fwnode
pointers. Same input, so that is run-to-run variance rather than anything
having changed. Read the other way round, two independent runs agreeing on the
fwnode half is the stronger signal in it.

Sergey