linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support
@ 2026-09-02 20:45 Peter Marshall
  2026-09-04 12:06 ` Benjamin Mugnier
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Marshall @ 2026-09-02 20:45 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Benjamin Mugnier,
	Sylvain Petinot
  Cc: linux-kernel, devicetree, linux-media, platform-driver-x86,
	Peter Marshall, Conor Dooley, Daniel Scally, Hans de Goede,
	Ilpo Järvinen, Krzysztof Kozlowski, Rob Herring

This patch series refactors the STMicroelectronics VD55G1 and VD65G4 image
sensor driver (vd55g1) into a generic VD55G family driver (vd55g) and adds
support for the VD55G0 monochrome sensor.

The VD55G0 sensor is found in devices such as the Microsoft Surface Pro 9 under
ACPI HID `SMO55F0`. It features a native resolution of 644 x 604 pixels and
differs from later variants in the family in several ways:

* It uses a different native resolution and register addresses mapping,
  requiring specification of generational feature differences.
* It lacks support for spatial and temporal HDR.
* It requires an exposure duty cycle limit to keep the illuminator current safe.

Tested on a Microsoft Surface Pro 9 (an Intel IPU6 platform with ACPI
HID "SMO55F0") running libcamera.

Peter Marshall (7):
  media: i2c: vd55g: move patch array to a separate firmware header
  dt-bindings: media: i2c: st,vd55g: rename and add VD55G0 variant
  media: ipu-bridge: Add VD55G0 to the list of supported sensors
  platform/x86: int3472: Add VD55G0 supply GPIO mapping
  media: i2c: st-vd55g: Add generic driver and VD55G0 support
  media: i2c: st-vd55g: Remove legacy vd55g1 remnants
  MAINTAINERS: Update ST VD55G camera driver entry

 .../i2c/{st,vd55g1.yaml => st,vd55g.yaml}     |   30 +-
 MAINTAINERS                                   |    7 +-
 drivers/media/i2c/Kconfig                     |   11 +
 drivers/media/i2c/Makefile                    |    2 +-
 drivers/media/i2c/vd55g-fw.h                  |  305 +++
 drivers/media/i2c/vd55g.c                     | 2166 +++++++++++++++++
 drivers/media/i2c/vd55g1.c                    | 2061 ----------------
 drivers/media/pci/intel/ipu-bridge.c          |    2 +
 drivers/platform/x86/intel/int3472/discrete.c |   18 +
 9 files changed, 2531 insertions(+), 2071 deletions(-)
 rename Documentation/devicetree/bindings/media/i2c/{st,vd55g1.yaml => st,vd55g.yaml} (76%)
 create mode 100644 drivers/media/i2c/vd55g-fw.h
 create mode 100644 drivers/media/i2c/vd55g.c
 delete mode 100644 drivers/media/i2c/vd55g1.c

-- 
2.55.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support
  2026-09-02 20:45 Peter Marshall
@ 2026-09-04 12:06 ` Benjamin Mugnier
       [not found]   ` <DL6TUYCM6AE9.3SFNKPI5T7CVS@petermarshall.ca>
  0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Mugnier @ 2026-09-04 12:06 UTC (permalink / raw)
  To: Peter Marshall, Sakari Ailus, Mauro Carvalho Chehab,
	Sylvain Petinot
  Cc: linux-kernel, devicetree, linux-media, platform-driver-x86,
	Conor Dooley, Daniel Scally, Hans de Goede, Ilpo Järvinen,
	Krzysztof Kozlowski, Rob Herring

Hi Peter,

Le 02/09/2026 à 22:45, Peter Marshall a écrit :
> This patch series refactors the STMicroelectronics VD55G1 and VD65G4 image
> sensor driver (vd55g1) into a generic VD55G family driver (vd55g) and adds
> support for the VD55G0 monochrome sensor.
> 
> The VD55G0 sensor is found in devices such as the Microsoft Surface Pro 9 under
> ACPI HID `SMO55F0`. It features a native resolution of 644 x 604 pixels and
> differs from later variants in the family in several ways:
> 
> * It uses a different native resolution and register addresses mapping,
>   requiring specification of generational feature differences.
> * It lacks support for spatial and temporal HDR.
> * It requires an exposure duty cycle limit to keep the illuminator current safe.
> 
> Tested on a Microsoft Surface Pro 9 (an Intel IPU6 platform with ACPI
> HID "SMO55F0") running libcamera.

I'm sorry I haven't read through your whole serie yet.

First thing first what is the point in refactoring both drivers into the
same one ? I guess it should be addressed in your commit messages. To be
fair it even looks to add complexity because even if the sensors share
some IPs you might have notice they are in fact very different. I could
even argue that the vd56g3 is closer to the vd55g1 than the vd55g0 is.

Plus having your commits merged will also require changes in libcamera.

Otherwise I'm interested in your patches 3 and 4. If I read it correctly
they allow the Microsoft Surface Pro 9 to use the embedded vd55g0 right ?

> 
> Peter Marshall (7):
>   media: i2c: vd55g: move patch array to a separate firmware header
>   dt-bindings: media: i2c: st,vd55g: rename and add VD55G0 variant
>   media: ipu-bridge: Add VD55G0 to the list of supported sensors
>   platform/x86: int3472: Add VD55G0 supply GPIO mapping
>   media: i2c: st-vd55g: Add generic driver and VD55G0 support
>   media: i2c: st-vd55g: Remove legacy vd55g1 remnants
>   MAINTAINERS: Update ST VD55G camera driver entry
> 
>  .../i2c/{st,vd55g1.yaml => st,vd55g.yaml}     |   30 +-
>  MAINTAINERS                                   |    7 +-
>  drivers/media/i2c/Kconfig                     |   11 +
>  drivers/media/i2c/Makefile                    |    2 +-
>  drivers/media/i2c/vd55g-fw.h                  |  305 +++
>  drivers/media/i2c/vd55g.c                     | 2166 +++++++++++++++++
>  drivers/media/i2c/vd55g1.c                    | 2061 ----------------
>  drivers/media/pci/intel/ipu-bridge.c          |    2 +
>  drivers/platform/x86/intel/int3472/discrete.c |   18 +
>  9 files changed, 2531 insertions(+), 2071 deletions(-)
>  rename Documentation/devicetree/bindings/media/i2c/{st,vd55g1.yaml => st,vd55g.yaml} (76%)
>  create mode 100644 drivers/media/i2c/vd55g-fw.h
>  create mode 100644 drivers/media/i2c/vd55g.c
>  delete mode 100644 drivers/media/i2c/vd55g1.c
> 

-- 
Regards,
Benjamin


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support
@ 2026-09-07 20:59 Sergey Lebedev
  2026-09-08  8:54 ` Benjamin Mugnier
  0 siblings, 1 reply; 6+ messages in thread
From: Sergey Lebedev @ 2026-09-07 20:59 UTC (permalink / raw)
  To: Peter Marshall
  Cc: Benjamin Mugnier, Sylvain Petinot, Sakari Ailus,
	Mauro Carvalho Chehab, Hans de Goede, Daniel Scally, linux-media,
	platform-driver-x86, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support
       [not found]   ` <DL6TUYCM6AE9.3SFNKPI5T7CVS@petermarshall.ca>
@ 2026-09-08  8:48     ` Benjamin Mugnier
  0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Mugnier @ 2026-09-08  8:48 UTC (permalink / raw)
  To: Peter Marshall
  Cc: Sakari Ailus, Mauro Carvalho Chehab, Sylvain Petinot,
	linux-kernel, devicetree, linux-media, platform-driver-x86,
	Laurent Pinchart

Hi Peter,

Adding back everyone to the CC list so they can see our discussion ;)

Le 04/09/2026 à 22:51, Peter Marshall a écrit :
> Hi Benjamin,
> 
> Thanks for your initial review.
> 
> I chose to merge the new hardware support into the existing driver to avoid code
> duplication and improve long-term maintainability. It looks like mainline is
> moving toward generic interfaces, and away from the "copy-and-modify" status quo
> common in vendor kernel trees.

I'm not aware of such thing in the mainline tree, we can take for
example imx sensors and every sensor tends to have it's own driver, but
I may be wrong.

> 
> The vd55g1 appears to be a generational successor of the vd55g0 and the two are
> similar enough that the driver only needs a few feature flags passed through
> the ACPI/OF match table to support both of them. The added abstraction does not
> obscure operational logic, although it might make it harder to verify according
> to the datasheets.

I tend to disagree, I still think it adds complexity and slowdown code
iteration as we will have to test both drivers at the same time to see
any regressions, which is not ideal.

I'd like to have an advice from Sakari and or Laurent on this matter,
what do you think ?

> 
> Patches 3 and 4 add the required identifiers for the Microsoft Surface Pro 9 to
> use its embedded vd55g0. The sensor isn't quite usable yet because of remaining
> issues in the IPU6 subsystem. Most notably the stream fails to start if the
> sensor sends less than a certain number of bytes per line (384 bytes or less
> on my hardware), and the frame gets cut off causing flickering when the CPU is
> idle. The latter issue was already documented by Hans de Goede on linux-media
> (Oct 2025).

Gotcha.

> 
> libcamera comes with generic defaults for unknown sensors and doesn't require
> any modifications to stream successfully.

Well yeah, the sensor will stream but will lack its medata to make it
stream cleanly. Please note that for Raspberry Pi this is a hard
requirement and it won't stream at without a cam_helper. Honestly that's
not that big of a deal and I consider it a side effect, we could fix
that easilly. I'm more concerned about the huge refactoring part.

> 
> Unfortunately, patch 5 in my series is not split into logical steps and does
> way too much at once. The renaming from vd55g1 to vd55g was unnecessary, and the
> following patches 6 and 7 should have been squashed into it as they represent
> cleanup steps instead of isolated logical changes. That makes this series
> completely unbisectable and not suitable for merging. The next version will

Indeed.

> focus on abstracting vd55g1.c in logical steps before supporting
> the vd55g0 revision.

Thank you for your work. I'm eager to see this.

> 
> Best regards,
> Peter Marshall

-- 
Regards,
Benjamin


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support
  2026-09-07 20:59 [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support Sergey Lebedev
@ 2026-09-08  8:54 ` Benjamin Mugnier
  2026-09-08 10:07   ` Sergey Lebedev
  0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Mugnier @ 2026-09-08  8:54 UTC (permalink / raw)
  To: Sergey Lebedev, Peter Marshall
  Cc: Sylvain Petinot, Sakari Ailus, Mauro Carvalho Chehab,
	Hans de Goede, Daniel Scally, linux-media, platform-driver-x86,
	linux-kernel

Hi Sergey,

Le 07/09/2026 à 22:59, Sergey Lebedev a écrit :
> 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

I'm not aware of André's work. Could you link it to me ?

The only vd55g0 out of tree driver I know is our own version on
Github[1]. Is it a fork of it ?

[1] https://github.com/STMicroelectronics/vd55g0-linux-driver

> 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
> 

-- 
Regards,
Benjamin


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support
  2026-09-08  8:54 ` Benjamin Mugnier
@ 2026-09-08 10:07   ` Sergey Lebedev
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Lebedev @ 2026-09-08 10:07 UTC (permalink / raw)
  To: Benjamin Mugnier
  Cc: Peter Marshall, Sylvain Petinot, Sakari Ailus,
	Mauro Carvalho Chehab, Hans de Goede, Daniel Scally, linux-media,
	platform-driver-x86, linux-kernel

Benjamin,

Yes — it is a fork of yours, and André says so himself in the header he
kept:

  https://github.com/AndreGilerson/linux-surface-kernel
  branch v7.0-surface-devel, drivers/media/i2c/vd55g0.c

  // SPDX-License-Identifier: GPL-2.0
  /*
   * Driver for VD55G0 global shutter sensor family driver
   *
   * Copyright (C) 2024 STMicroelectronics SA
   *
   * Adapter with small changes by André Gilerson
   */

Against your master it is 127 changed lines out of 2114, and all 59
vd55g0_* function names are the same set — nothing added, nothing removed.
vd55g0_patches.h differs in four places: his attribution line, uint ->
unsigned int, and two structs made static. The firmware arrays are
byte-identical, cut1_patch 6868 bytes and cut2_patch 260.

That last point matters for this series. The blob I wrote to
/lib/firmware/vd55g0-cut1.bin to get past the missing-firmware failure was
taken from his header, so it is your firmware, published by you under
GPL-2.0 in your own repository. Peter's driver needs those bytes and
cannot get them from anywhere in-tree; you are the one who can say how
they should reach linux-firmware, or whether VD55G0 should keep a built-in
array the way VD55G1 does.

What the 127 lines actually change, since it is a fair summary of what a
laptop needed that your driver did not have:

  - ACPI. Yours is DT-only; his adds acpi.h and an ACPI match on SMO55F0.
  - Supply names. Yours asks for VCORE, VDDIO and VANA. His asks for avdd
    and dovdd, two rather than three, with the comment that INT3472 on the
    Surface Pro 11 provides GPIO type 0x0B as avdd and 0x10 as dovdd.
  - SGRBG aliases beside the mono formats, commented "allows IPU7 Bayer
    pipeline".
  - illuminator GPIO handling.
  - an unknown revision warns and continues instead of returning -EINVAL.

The second of those explains something I reported yesterday and had not
chased: on this machine Peter's 4/7 leaves `supply vcore not found, using
dummy regulator`. His mapping gives the SMO55F0 GPIOs the names your
driver wants, while the machine's INT3472 appears to describe what André
named avdd and dovdd. One of the two mappings does not match this
hardware, and I have not established which — I can measure it here if that
is useful.

Two things I should retire from my own report, having now seen the
off-list exchange you added back. Peter had already withdrawn the rename
on 4 September, so the libcamera breakage I raised - the sensor-properties
and helper lookups keying on the entity name, which vd55g1 machines have
today - does not arise in a v2 that keeps the name. And the metadata point
you make to him is the same one from the other side: unknown to libcamera,
the sensor streams but on defaults.

What is not retired is the firmware. Nothing in the exchange touches it,
and it is the one thing that stops anyone but Peter using VD55G0 at all.

For contrast rather than contradiction: he reports the sensor is not quite
usable on his Surface Pro 9 because of IPU6 stream problems, short lines
and frames cut off when the CPU idles. I saw none of that on IPU7 - it
probed, bound and registered, and streamed 60 frames at 30 fps. Different
bridge, different result, which may be worth knowing when the two are
compared.

I should be clear that I am not speaking for André. I have written to him
separately about upstreaming his IMX681 driver, he answered that mainline
is his goal, and he is away until 28 September. If you want to reach him
directly his address is in the MODULE_AUTHOR line of that file.

Sergey


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-08 10:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-07 20:59 [PATCH 0/7] media: i2c: st-vd55g1: Genericize driver and add VD55G0 support Sergey Lebedev
2026-09-08  8:54 ` Benjamin Mugnier
2026-09-08 10:07   ` Sergey Lebedev
  -- strict thread matches above, loose matches on Subject: below --
2026-09-02 20:45 Peter Marshall
2026-09-04 12:06 ` Benjamin Mugnier
     [not found]   ` <DL6TUYCM6AE9.3SFNKPI5T7CVS@petermarshall.ca>
2026-09-08  8:48     ` Benjamin Mugnier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).