Re: [PATCH v2] media: ipu-bridge: add the OV13858 rear sensor

[Sergey Lebedev <lsa.uz@pm.me>]

Thank you for the tag, and for replacing your own entry with mine so that my
line was the only difference. That is more care than a Tested-by usually
costs.

Your question is a better one than it looks, because our two failures produce
the same string:

  ov13858 i2c-OVTID858:00: failed to find sensor: -5
  ov13858 i2c-OVTID858:00: probe with driver ov13858 failed with error -5

Same -EIO, and necessarily so: ov13858_identify_module() has one caller and
one dev_err, so every cause that makes the chip-id read fail prints exactly
that. On this machine that line has already had two causes. In July it was
int3472 not knowing GPIO type 0x08, so none of the regulators existed; since
that was fixed upstream it has been the driver not asking for the ones that
do. Yours would be a third. So I checked rather than answering from memory.

They are independent, and neither subsumes the other.

Mine is that the sensor is never powered. The driver's assumption holds where
the rails are ACPI power resources. Here an INT3472 companion registers them
as regulators, a clock and a reset GPIO for the sensor driver to consume, and
ov13858 consumes none of them. The patch requests dovdd, avdd and dvdd and
the reset GPIO, and sequences them with the clock in the runtime PM
callbacks. It retries nothing.

What rules your mechanism out for mine is that there was no neighbour. The
before-and-after was measured on a media/next build, and no media/next build
carries a vd55g0 module - that driver is still in review - so SMO55F0 was
unbound while the read was failing. There was nothing on the bus to corrupt
the transfer, it failed anyway, and adding the supplies fixed it. The build I
am on today is the same in that respect:

  /sys/bus/i2c/devices, adapter i2c-1
    i2c-OVTID858:00   driver=ov13858
    i2c-SMO55F0:00    driver=none

which is your topology exactly - the rear sensor sharing a controller with
the infrared one - with the neighbour silent because nothing claims its HID.

Retrying a read on an unpowered part returns -EIO five times, and powering a
part that is already powered does not stop a neighbour corrupting a transfer.
So both, and your commit message can say so without it reading as a duplicate
of mine.

Since more than one build is involved above, which is which:

  media/next f9536a806, no vd55g0 module in the tree
    stock ov13858    failed to find sensor: -5 at boot
    with my patch    supply dovdd not found, using dummy regulator, binds
  a 7.3.0-rc1 built from media/next, same absence of vd55g0
    the bus listing above; SMO55F0 unbound
  Ubuntu 7.0.0-30-generic, my patch backported, out-of-tree vd55g0 installed
    SMO55F0 bound to vd55g0, and the rear sensor binds

All on one Surface Pro 11 for Business (Intel), firmware 17.105.143.

The last of those is the useful one for you: I have a kernel here where your
neighbour is live on the same controller, and one where it is absent. If you
would like the retry tested against either when you post it, say so.

Thank you also for the link-frequency observation, which I checked and which
holds in the source: ov13858 calls v4l2_fwnode_device_parse() and neither
v4l2_fwnode_endpoint_parse() nor the _alloc_ variant anywhere, so the array
the bridge publishes is indeed never read. I had ordered the two as measured
rather than ascending and wondered whether that would be queried. It cannot
be.

Sergey