Re: [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support

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

Peter,

Tested on a second machine: Surface Pro 11 for Business, Intel Lunar Lake,
IPU7 — same SMO55F0 HID as your Surface Pro 9, one IPU generation later.
Two results worth having before you respin, one good and one blocking.

The good one: the series applies and builds clean, and the sensor works on
IPU7. All seven patches applied in order to an Ubuntu 7.0.0 tree with no
conflicts (its vd55g1.c is 2061 lines, exactly what your diffstat deletes),
and media/i2c, media/pci/intel and int3472 rebuilt without a warning.
With firmware present the driver probes, the bridge finds the sensor and
the subdev registers:

  intel-ipu7: Found supported sensor SMO55F0:00
  intel_ipu7_isys: bind vd55g 1-0060 nlanes is 1 port is 1
  entity 233: vd55g 1-0060 (1 pad, 1 link, 0 routes)

So nothing here is IPU6-specific. That is the part I could add that you
could not.

The blocking one: without that firmware nobody can use VD55G0 at all.

  vd55g i2c-SMO55F0:00: Direct firmware load for vd55g0-cut1.bin failed
                        with error -2
  vd55g i2c-SMO55F0:00: Failed to load required firmware vd55g0-cut1.bin: -2

Probe fails and the camera disappears. The driver has a fallback for
exactly this case, but only the older part gets one:

  static const struct firmware vd55g1_builtin_fw = {
          .data = vd55g1_patch_array, .size = ARRAY_SIZE(vd55g1_patch_array)
  };
  ...
  if (ret == -ENOENT && fw_info->builtin_fw)

vd55g0_fw_maps[] sets fw_name and leaves builtin_fw NULL, and neither
vd55g0-cut1.bin nor vd55g0-cut2.bin is in linux-firmware. So the series
works for you and for nobody else, which I assume is not the intent. Either
a built-in like VD55G1's, or the files submitted to linux-firmware, or at
minimum a note in the cover letter saying where they come from.

To get past it I used the same ST patch arrays that André Gilerson's
out-of-tree VD55G0 driver compiles in (vd55g0_patches.h, "Copyright (C)
2024 STMicroelectronics SA"), written out as a file. cut1 is 6868 bytes and
its header parses correctly against struct vd55g_patch_header — 0x0681,
version 2.11 — so the format is right. That is how I tested, not a
distribution path I am proposing.

Second, and this one reaches further than this series. Benjamin's warning
about libcamera is concrete and it is a regression for machines that work
today. libcamera keys the sensor-properties database, the soft-ISP helper
and the tuning file on the media entity name, which is the driver name:

  WARN  No static properties available for 'vd55g'
  WARN  'vd55g 1-0060': No sensor delays found in static properties
  WARN  Configuration file 'vd55g.yaml' not found for IPA module 'simple'
  WARN  IPASoft: Failed to create camera sensor helper for vd55g

and the installed libcamera 0.7.0 does carry a vd55g1 entry — `strings
libcamera.so.0.7 | grep -x vd55g1` finds it. So renaming the driver does
not only fail to help VD55G0; it silently takes the properties and the
helper away from VD55G1 users who have them now. Whatever is decided about
one driver or two, that rename needs libcamera patches landing alongside,
or the entity name keeping its old form.

Two smaller things, neither chased:

  - `supply vcore not found, using dummy regulator` persists here despite
    4/7's SMO55F0 mapping. It was not the blocker, so I did not dig.
  - 3/7 advertises 380 MHz for SMO55F0. This machine has been running 402 MHz
    ("MIPI output freq is 804MHz / 2") from the out-of-tree driver. Your
    driver takes the endpoint's value rather than validating against a list,
    so it accepted 380 without complaint, and I have not established which is
    correct for either model. Worth knowing that two people measured
    different numbers for the same HID on different Surfaces.

Happy to run v2 here when you have one, and to report anything specific you
would like measured on IPU7.

Sergey