Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli <fernandorimoli11@gmail.com>]

Hi Dan,

Thanks for the reviews on v3, and sorry for the slow follow-up.

> Hm, I think this is functionally fine, but matching on PCI ID and sensor
> does seem a bit quirky...do you know if the IPU3 case is fine with the
> clock-noncontinuous flag too? If not I can test it tomorrow.

You and Sakari landed on the same objection, and v4 drops that helper
entirely. Instead struct ipu_sensor_config gains an optional IPU PCI
product ID and a flags field (Sakari's suggestion), so the ov5693 on IPU6
becomes table entries rather than a special case in code:

    IPU_SENSOR_CONFIG("INT33BE", 1, 419200000),
    IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6,
                               CSI2_CLK_NONCONTINUOUS, 1, 419200000),
    IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6EP_ADLP,
                               CSI2_CLK_NONCONTINUOUS, 1, 419200000),

That also answers your IPU3 question without needing the test: IPU3 has
no matching entry, so it keeps using the generic one and never sees the
flag, by construction rather than by a PCI check. So please don't spend
hardware time on it on my account. If you are curious anyway I would
still be interested in the result, since knowing IPU3 tolerates the flag
would let a later patch collapse those three entries back into one, but
it is not blocking anything now.

Since a PCI ID in the table is new, one semantic came with it that I would
value your view on as the bridge's author: a sensor with both a specific
and a generic entry for the same HID matches twice on the specific IPU, and
ipu_bridge_connect_sensor() would then enumerate the same ACPI device twice
and consume two of the four IPU ports. v4 therefore skips the generic entry
when a specific one matches, as an order-independent filter in
ipu_bridge_connect_sensors() rather than a rule about table ordering.

Two other things you should know about v4:

- Patch 3 now writes only bit 5 (clock-lane gate), not bit 5 + bit 2.
  Sakari asked whether IPU6 needed bit 2; it does not, and my sweep data
  agreed, so it is gone. It is also now set with cci_update_bits() rather
  than a full-register cci_write(): the ov5693 does not otherwise program
  MIPI_CTRL00, and since this driver serves IPU3/CIO2 and Rockchip too,
  touching the one bit leaves anything the platform left there intact.
  Because that changes behaviour I dropped your Reviewed-by from that
  patch rather than carry it. Happy to add it back if you are still
  content with the narrower write.

  Since v3 a linux-surface user also reproduced the value question
  independently on a Surface Pro 8 (Tiger Lake IPU6, INT33BE rather than
  OVTI5693) and got the one result I was missing: the vendor value with
  only bit 5 cleared does not stream, so bit 5 is necessary and not just
  sufficient. Details and the caveats in the cover letter.

- Your Reviewed-by on v3 2/4 is carried forward unchanged onto v4 2/6, as
  that patch is untouched. Thank you for it.

There is also a new no-op refactor as patch 4/6 (endpoint property indices
assigned dynamically, per NEXT_PROPERTY() in mipi-disco-img.c). It turned
out to fix a latent issue in my v3: because the property array is
NULL-terminated, a conditional property at a fixed index would have been
silently dropped for any sensor with nr_link_freqs == 0, INTC10C5 being
the in-tree example.

Thanks,
Fernando