Re: [PATCH 2/3] media: i2c: Add driver for OmniVision OV32C4

[Robert Bozik <robertbozik@gmail.com>]

Hi Sakari,

Thank you for the review - that is a lot of detail for a 2800 line
driver and I am grateful for it.

One note first: v2 crossed with your mail. It only carries Conor's
binding comments, two fixes the Sashiko bot found (pm_ptr() instead of
pm_sleep_ptr(), and the control handler freed on the probe error path)
and a trailer reorder. Nothing below is addressed there; it all goes
into v3.

> Nearly all of this belongs to the cover letter -- here you're expected to
> say just what the patch does [...]

Will do. The commit message will say what the patch adds, and the
measurement detail moves to the cover letter, along with the long
provenance comments in the file that you flagged further down.

> Is this needed? The DT bindings suggest otherwise. :-)

No. Dropping the ACPI dependency.

> Please calculate this instead of adding a comment.

Will do.

> This macro name suggests it's a register address, not value. The register
> address should have a macro associated with it, with a human-readable name.

Will rename it, and add macros for the timing register addresses,
including the ones behind the fields in struct ov32c4_mode.

> This isn't really specific to the sensor, is it? It'd be nice to know what
> this required write is really about but it sounds like there's another
> device there that needs its own driver. Is it found in the system as a
> device (see what's under /sys/bus/i2c/devices)?

It is, but as the wrong device, and I think there is a problem in
ipu-bridge underneath this.

ipu-bridge takes the VCM model from ACPI and does not verify it:

	sensor->vcm_type = ipu_vcm_types[ssdb.vcmtype - 1];

Here SSDB.vcmtype is 2, so it instantiates a "dw9714" client on _CRS
resource 1, which is the same address my driver writes to (0x3e). dw9714
has no identification register, so its driver binds unconditionally and
cannot notice.

The chip at that address is not a dw9714. With the sensor powered and
the dw9714 driver unbound:

	w2@0x3e 0x10 0x01  r1@0x3e   ->  0x04   (what my driver wrote)
	w2@0x3e 0x10 0x00  r8@0x3e   ->  00 04 00 00 00 00 00 00
	w2@0x3e 0x00 0x00 r32@0x3e   ->  all zeroes

repeatably. So it has a 16-bit addressed register file and retains what
is written to it, which a dw9714 - a write-only DAC with no register
addressing at all - does not. Its lens subdev has zero pads and zero
links.

I could not identify the chip. Only a sparse block around 0x1000 and
0x1010-0x1018 reads back non-zero, and I found no ID register. It has no
ACPI device of its own; the only other unbound I2C node on this machine
is TXNW3643 at \_SB.FLM1, which is the flash module.

Without the write the sensor NACKs everything at 0x36, so it does gate
it somehow. The Windows driver issues the same write from its sensor
driver, so there is no separate driver for it there either.

So I agree this does not belong in the sensor driver. What I do not know
is what shape you would prefer instead: a regulator provider consumed as
dvdd-supply would mean writing a driver for a chip I cannot name, and
something would have to stop ipu-bridge from claiming the address as a
VCM first. Guidance welcome.

> These belong to the int3472 driver or the ipu-bridge.

Agreed. They go away with whatever the answer to the above turns out to
be.

> These are 0 and 5 ms, respectiely.

I am not sure I follow - could you say what you would like here? The
values in the patch are 5 ms after the supply and 20 ms after reset,
arrived at during bring-up rather than from a datasheet, as there is
none.

> Is this required?

I will test without the retries and drop them if the power-up sequence
is enough on its own.

> That's not right, these are in pixels in terms of the value of the
> PIXEL_RATE control, that reflects timing on the sensor's pixel array.

Thank you - I will correct the comment and re-check the arithmetic
against that definition.

> There's (typically) no need for get_frame_desc() op if you have a single
> stream.

That was my assumption too, but it measured otherwise here: without the
op the call returns -ENOIOCTLCMD, the receiver is configured from the
media bus code alone, and the IPU7 then reports one oversized packet per
frame. I will re-test that for v3 in case something else I changed since
has made it moot; if it still holds I would rather explain why the op is
there than drop it silently.

> No need to check for errors here -- v4l2_fwnode_endpoint_alloc_parse()
> already does.

You are right - __v4l2_fwnode_endpoint_parse() returns -EPROBE_DEFER for
a NULL fwnode, and the comment there names the IPU bridge case
explicitly. Dropping the check.

Everything else - pm_runtime_get_if_active(), setting the control flags
after the handler error check, v4l2_fwnode_device_parse() moved up, the
error checks on __v4l2_ctrl_modify_range() and in disable_streams(), the
definitions from mipi-csi2.h, unsigned int for the loop variable, and the
debug prints and redundant comments - will be as you say in v3.

Thanks,
Robert