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

[Fernando Rimoli]

Hi Sakari,

Thanks. Noted on the line length, I will keep prose to 75.

> The support for non-contiguous clock isn't mandatory on either side
> (whereas free-running clock is and should always "just work") so as a
> whole this is weird. But as we know the sensor works with IPU6 with
> non-continous clock, that's what I guess we'll just have to do then.

Agreed that it is odd. The sensor side is not optional in practice here:
with the free-running default the IPU6 receiver never locks and capture
times out, so on these devices the "should just work" case is the one
that does not. I have no visibility into why the receiver behaves that
way, only that gating the clock lane is what makes it lock.

> How about adding PCI IDs (for matching the particualr IPU) and flags to
> struct ipu_sensor_config? I have a feeling we'll need this elsewhere,
> too.

Done in v4, and it is a much better shape than what I had. The quirk
helper is gone; struct ipu_sensor_config gains pci_id and flags, and the
ov5693 becomes table entries rather than code.

Two small deviations from your sketch, in case they were deliberate and I
have missed the point:

- IPU_SENSOR_CONFIG's expansion referenced _NR without it being in the
  parameter list, and forwarded "..." rather than __VA_ARGS__, so I wrote
  it as:

      #define IPU_SENSOR_CONFIG(_HID, _NR, ...)                       \
              IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, NONE, _NR, __VA_ARGS__)

- .flags = IPU_BR_FL_##_FLAGS does not paste to anything for a plain 0, so
  there is an IPU_BR_FL_NONE for the generic case. That keeps every
  existing table entry unchanged, which seemed worth having.

The one thing I would like your opinion on is a semantic that comes with
putting a PCI ID in the table. 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. So in v4 the more specific
entry wins and the generic one is skipped. It is implemented as a filter
in ipu_bridge_connect_sensors() rather than by requiring the table to be
ordered, so it does not depend on entry order. If you would rather have
this expressed differently, say so and I will rework it. An explicit
"generic" marker, or resolving it at match time, would both work.

ipu_bridge_ivsc_is_ready() also walks the table, but it runs before the
bridge exists and is an idempotent readiness check, so duplicate HIDs are
harmless there and I left it alone.

On scope: v4 sets the flag for IPU6 (0x9a19, Tiger Lake) and IPU6EP_ADLP
(0x465d), for both HIDs. To be exact about what hardware stands behind
each: OVTI5693 on ADL-P is this series running on my Surface Pro 9;
INT33BE on Tiger Lake is the register value, confirmed on a Surface Pro 8
and a Pro 7+ by linux-surface users. Jakob's Tested-by from v2 covered
the Pro 7+ but at the old value, which is why I dropped it. v3 matched
all of ipu6_pci_tbl, but now that the IDs are spelled out per entry I
would rather list only what is confirmed on hardware and add the rest as
reports arrive, Surface Go 4 (ADL-N) being the likely next one. Happy
to broaden it if you would prefer the whole family up front.

> You could also switch to dynamically assigning the property index so
> there's no need to rely on a particular device having a list of link
> frequencies. See NEXT_PROPERTY() macro in drivers/acpi/mipi-disco-img.c
> . That should go to a separate patch, like adding the above mechanism.

Also done, as patch 4/6, before the mechanism. It is a no-op refactor:
the endpoint property slots are named in an enum and assigned through a
bounds-checked running index, so the array is sized by the enum and a
conditional property no longer has to sit at a fixed slot.

This was a latent bug and not just untidiness. Because the property array
is NULL-terminated, v3's approach would have silently dropped the
property for any sensor with nr_link_freqs == 0 (INTC10C5 is the one
in-tree example): the empty link-frequencies slot terminated the array
before anything after it. With the running index that cannot happen.

v4 is 6 patches:

  1-2  HID enumeration, unchanged (both have Dan's Reviewed-by)
  3    ov5693 clock-lane gate, now bit 5 only, set with cci_update_bits()
  4    ipu-bridge: dynamic endpoint property indices
  5    ipu-bridge: per-IPU config matching + flags
  6    ipu-bridge: use them for the ov5693 on IPU6

Each patch builds without warnings on its own, at W=1 as well, on top of
v7.3-rc1.

Thanks,
Fernando