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

[Benjamin Mugnier <benjamin.mugnier@foss.st.com>]

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