[PATCH 0/5] media: i2c: vd55g1: Add vd55g4 support and various fixes

[PATCH 0/5] media: i2c: vd55g1: Add vd55g4 support and various fixes

[Benjamin Mugnier]

The vd55g4 is the monochrome variant of the vd56g4. This series adds the
necessary code in the driver to probe and stream from the sensor
in the correct format, and a new compatible in device tree bindings.

This series also fixes some issues I encountered while developing.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
Benjamin Mugnier (5):
      media: i2c: vd55g1: Fix media bus code initialization
      media: i2c: vd55g1: Remove spurious pad format update on init_state()
      media: i2c: vd55g1: Fix manual digital gain on color variant
      media: i2c: vd55g1: Add support for vd55g4
      media: dt-bindings: vd55g1: Add vd55g4 compatible

 .../devicetree/bindings/media/i2c/st,vd55g1.yaml   |   3 +-
 drivers/media/i2c/vd55g1.c                         | 141 ++++++++++++++-------
 2 files changed, 99 insertions(+), 45 deletions(-)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260428-vd55g4_and_fixes-97dc23b6f266

Best regards,
--  
Benjamin Mugnier <benjamin.mugnier@foss.st.com>

[PATCH 1/5] media: i2c: vd55g1: Fix media bus code initialization

[Benjamin Mugnier]

In the driver initialization, the index of the default media bus code
from the supported media bus code array is passed directly to the
vd55g1_get_fmt_code() function instead of the proper media bus code.

This works correctly as a proper media bus code is set after
initialization but could not have been the case. This also resulted in
mutliple "Unsupported mbus format" error messages.

Retrieve the media bus code from the media bus code array, and pass this
media bus code to vd55g1_get_fmt_code() instead of the code index.

Rename VD55G1_MBUS_CODE_DEF to VD55G1_MBUS_CODE_IDX_DEF and
VD55G1_MODE_DEF to VD55G1_MODE_IDX_DEF while at it to avoid future
confusions. Display the guilty error code in warning message.

Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 78d18c028154..1e9db21322e3 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -114,9 +114,9 @@
 
 #define VD55G1_WIDTH					804
 #define VD55G1_HEIGHT					704
-#define VD55G1_MODE_DEF					0
+#define VD55G1_MODE_IDX_DEF				0
 #define VD55G1_NB_GPIOS					4
-#define VD55G1_MBUS_CODE_DEF				0
+#define VD55G1_MBUS_CODE_IDX_DEF			0
 #define VD55G1_DGAIN_DEF				256
 #define VD55G1_AGAIN_DEF				19
 #define VD55G1_EXPO_MAX_TERM				64
@@ -634,7 +634,7 @@ static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)
 				goto adapt_bayer_pattern;
 		}
 	}
-	dev_warn(sensor->dev, "Unsupported mbus format\n");
+	dev_warn(sensor->dev, "Unsupported mbus format: 0x%x\n", code);
 
 	return code;
 
@@ -1347,6 +1347,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
 {
 	struct vd55g1 *sensor = to_vd55g1(sd);
 	struct v4l2_subdev_format fmt = { 0 };
+	int code;
 	struct v4l2_subdev_route routes[] = {
 		{ .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE }
 	};
@@ -1361,9 +1362,13 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
-	vd55g1_update_pad_fmt(sensor, &vd55g1_supported_modes[VD55G1_MODE_DEF],
-			      vd55g1_get_fmt_code(sensor, VD55G1_MBUS_CODE_DEF),
-			      &fmt.format);
+	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
+		code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
+	else
+		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
+	vd55g1_update_pad_fmt(sensor,
+			      &vd55g1_supported_modes[VD55G1_MODE_IDX_DEF],
+			      vd55g1_get_fmt_code(sensor, code), &fmt.format);
 
 	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
 }

-- 
2.43.0

[PATCH 2/5] media: i2c: vd55g1: Remove spurious pad format update on init_state()

[Benjamin Mugnier]

vd55g1_update_pad_fmt() is called in vd55g1_init_state(). But
vd55g1_set_pad_fmt(), called at the end of vd55g1_init_state(), also
calls vd55g1_update_pad_fmt() itself.

Enhance readability and clear confusion by only preparing the format in
vd55g1_init_state() and let vd55g1_set_pad_fmt() update it instead,
effectively calling it only 1 time instead of 2.

Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 1e9db21322e3..e44174056ace 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1366,9 +1366,9 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
 		code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
 	else
 		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
-	vd55g1_update_pad_fmt(sensor,
-			      &vd55g1_supported_modes[VD55G1_MODE_IDX_DEF],
-			      vd55g1_get_fmt_code(sensor, code), &fmt.format);
+	fmt.format.code = vd55g1_get_fmt_code(sensor, code);
+	fmt.format.width = vd55g1_supported_modes[VD55G1_MODE_IDX_DEF].width;
+	fmt.format.height = vd55g1_supported_modes[VD55G1_MODE_IDX_DEF].height;
 
 	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
 }

-- 
2.43.0

[PATCH 3/5] media: i2c: vd55g1: Fix manual digital gain on color variant

[Benjamin Mugnier]

Apply digital gain to all channels, each channel representing a color.

Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index e44174056ace..2c962fcb41d2 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -60,7 +60,10 @@
 #define VD55G1_PATGEN_ENABLE				BIT(0)
 #define VD55G1_REG_MANUAL_ANALOG_GAIN			CCI_REG8(0x0501)
 #define VD55G1_REG_MANUAL_COARSE_EXPOSURE		CCI_REG16_LE(0x0502)
-#define VD55G1_REG_MANUAL_DIGITAL_GAIN			CCI_REG16_LE(0x0504)
+#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH0		CCI_REG16_LE(0x0504)
+#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH1		CCI_REG16_LE(0x0506)
+#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH2		CCI_REG16_LE(0x0508)
+#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH3		CCI_REG16_LE(0x050a)
 #define VD55G1_REG_APPLIED_COARSE_EXPOSURE		CCI_REG16_LE(0x00e8)
 #define VD55G1_REG_APPLIED_ANALOG_GAIN			CCI_REG16_LE(0x00ea)
 #define VD55G1_REG_APPLIED_DIGITAL_GAIN			CCI_REG16_LE(0x00ec)
@@ -850,9 +853,16 @@ static int vd55g1_update_expo_cluster(struct vd55g1 *sensor, bool is_auto)
 		vd55g1_write(sensor, VD55G1_REG_MANUAL_ANALOG_GAIN,
 			     sensor->again_ctrl->val, &ret);
 
-	if (!is_auto && sensor->dgain_ctrl->is_new)
-		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN,
+	if (!is_auto && sensor->dgain_ctrl->is_new) {
+		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH0,
 			     sensor->dgain_ctrl->val, &ret);
+		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH1,
+			     sensor->dgain_ctrl->val, &ret);
+		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH2,
+			     sensor->dgain_ctrl->val, &ret);
+		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH3,
+			     sensor->dgain_ctrl->val, &ret);
+	}
 
 	return ret;
 }

-- 
2.43.0

[PATCH 4/5] media: i2c: vd55g1: Add support for vd55g4

[Benjamin Mugnier]

vd55g4 is the same device as vd65g4 but outputs in monochrome instead of
RGB. Adapt the driver structure according to this new variant, and add
its support.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 110 ++++++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 36 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 2c962fcb41d2..9f62fc0428a1 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -29,11 +29,7 @@
 
 /* Register Map */
 #define VD55G1_REG_MODEL_ID				CCI_REG32_LE(0x0000)
-#define VD55G1_MODEL_ID_VD55G1				0x53354731 /* Mono */
-#define VD55G1_MODEL_ID_VD65G4				0x53354733 /* RGB */
-#define VD55G1_REG_REVISION				CCI_REG16_LE(0x0004)
-#define VD55G1_REVISION_CCB				0x2020
-#define VD55G1_REVISION_BAYER				0x3030
+#define VD55G1_REG_COLOR_VERSION			CCI_REG32_LE(0x0670)
 #define VD55G1_REG_FWPATCH_REVISION			CCI_REG16_LE(0x0012)
 #define VD55G1_REG_FWPATCH_START_ADDR			CCI_REG8(0x2000)
 #define VD55G1_REG_SYSTEM_FSM				CCI_REG8(0x001c)
@@ -138,8 +134,39 @@
 #define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
 #define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)
 
-#define VD55G1_MODEL_ID_NAME(id) \
-	((id) == VD55G1_MODEL_ID_VD55G1 ? "vd55g1" : "vd65g4")
+enum vd55g1_model_id {
+	VD55G1_MODEL_ID_2 = 0x53354731,
+	VD55G1_MODEL_ID_3 = 0x53354733,
+};
+
+enum vd55g1_color_version {
+	VD55G1_COLOR_VERSION_MONO = 0x0,
+	VD55G1_COLOR_VERSION_BAYER = 0x1,
+};
+
+struct vd55g1_version {
+	char *name;
+	enum vd55g1_model_id id;
+	enum vd55g1_color_version color;
+};
+
+static const struct vd55g1_version vd55g1_versions[] = {
+	{
+		.name  = "vd55g1",
+		.id    = VD55G1_MODEL_ID_2,
+		.color = VD55G1_COLOR_VERSION_MONO,
+	},
+	{
+		.name  = "vd55g4",
+		.id    = VD55G1_MODEL_ID_3,
+		.color = VD55G1_COLOR_VERSION_MONO,
+	},
+	{
+		.name  = "vd65g4",
+		.id    = VD55G1_MODEL_ID_3,
+		.color = VD55G1_COLOR_VERSION_BAYER,
+	},
+};
 
 static const u8 vd55g1_patch_array[] = {
 	0x44, 0x03, 0x09, 0x02, 0xe6, 0x01, 0x42, 0x00, 0xea, 0x01, 0x42, 0x00,
@@ -535,7 +562,7 @@ struct vd55g1_vblank_limits {
 
 struct vd55g1 {
 	struct device *dev;
-	unsigned int id;
+	const struct vd55g1_version *version;
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(vd55g1_supply_name)];
@@ -628,7 +655,7 @@ static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)
 {
 	unsigned int i, j;
 
-	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
+	if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
 		return code;
 
 	for (i = 0; i < ARRAY_SIZE(vd55g1_mbus_formats_bayer); i++) {
@@ -1183,8 +1210,8 @@ static int vd55g1_patch(struct vd55g1 *sensor)
 	u64 patch;
 	int ret = 0;
 
-	/* vd55g1 needs a patch while vd65g4 does not */
-	if (sensor->id == VD55G1_MODEL_ID_VD55G1) {
+	/* Version 2 needs a patch while version 3 does not */
+	if (sensor->version->id == VD55G1_MODEL_ID_2) {
 		vd55g1_write_array(sensor, VD55G1_REG_FWPATCH_START_ADDR,
 				   sizeof(vd55g1_patch_array),
 				   vd55g1_patch_array, &ret);
@@ -1256,7 +1283,7 @@ static int vd55g1_enum_mbus_code(struct v4l2_subdev *sd,
 	struct vd55g1 *sensor = to_vd55g1(sd);
 	u32 base_code;
 
-	if (sensor->id == VD55G1_MODEL_ID_VD55G1) {
+	if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER) {
 		if (code->index >= ARRAY_SIZE(vd55g1_mbus_formats_mono))
 			return -EINVAL;
 		base_code = vd55g1_mbus_formats_mono[code->index];
@@ -1372,7 +1399,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
-	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
+	if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
 		code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
 	else
 		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
@@ -1659,38 +1686,48 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 	return ret;
 }
 
+static const struct vd55g1_version *
+	vd55g1_get_version(enum vd55g1_model_id id,
+			   enum vd55g1_color_version color)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(vd55g1_versions); i++) {
+		if (vd55g1_versions[i].id == id &&
+		    vd55g1_versions[i].color == color)
+			return &vd55g1_versions[i];
+	}
+
+	return NULL;
+}
+
 static int vd55g1_detect(struct vd55g1 *sensor)
 {
-	unsigned int dt_id = (uintptr_t)device_get_match_data(sensor->dev);
-	u64 rev, id;
-	int ret;
+	const struct vd55g1_version *dt_version =
+		device_get_match_data(sensor->dev);
+	const struct vd55g1_version *version;
+	u64 color, id;
+	int ret = 0;
 
-	ret = vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, NULL);
+	vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, &ret);
+	vd55g1_read(sensor, VD55G1_REG_COLOR_VERSION, &color, &ret);
 	if (ret)
 		return ret;
 
-	if (id != VD55G1_MODEL_ID_VD55G1 && id != VD55G1_MODEL_ID_VD65G4) {
-		dev_warn(sensor->dev, "Unsupported sensor id 0x%x\n",
-			 (u32)id);
+	version = vd55g1_get_version(id, color);
+	if (!version) {
+		dev_warn(sensor->dev, "Unsupported sensor version, expected %s\n",
+			 dt_version->name);
 		return -ENODEV;
 	}
-	if (id != dt_id) {
-		dev_err(sensor->dev, "Probed sensor %s and device tree definition (%s) mismatch",
-			VD55G1_MODEL_ID_NAME(id), VD55G1_MODEL_ID_NAME(dt_id));
+	if (version->id != dt_version->id ||
+	    version->color != dt_version->color) {
+		dev_err(sensor->dev, "Probed sensor version %s and device tree definition %s mismatch",
+			version->name, dt_version->name);
 		return -ENODEV;
 	}
-	sensor->id = id;
 
-	ret = vd55g1_read(sensor, VD55G1_REG_REVISION, &rev, NULL);
-	if (ret)
-		return ret;
-
-	if ((id == VD55G1_MODEL_ID_VD55G1 && rev != VD55G1_REVISION_CCB) &&
-	    (id == VD55G1_MODEL_ID_VD65G4 && rev != VD55G1_REVISION_BAYER)) {
-		dev_err(sensor->dev, "Unsupported sensor revision 0x%x for sensor %s\n",
-			(u16)rev, VD55G1_MODEL_ID_NAME(id));
-		return -ENODEV;
-	}
+	sensor->version = version;
 
 	return 0;
 }
@@ -2048,8 +2085,9 @@ static void vd55g1_remove(struct i2c_client *client)
 }
 
 static const struct of_device_id vd55g1_dt_ids[] = {
-	{ .compatible = "st,vd55g1", .data = (void *)VD55G1_MODEL_ID_VD55G1 },
-	{ .compatible = "st,vd65g4", .data = (void *)VD55G1_MODEL_ID_VD65G4 },
+	{ .compatible = "st,vd55g1", .data = (void *)&vd55g1_versions[0] },
+	{ .compatible = "st,vd55g4", .data = (void *)&vd55g1_versions[1] },
+	{ .compatible = "st,vd65g4", .data = (void *)&vd55g1_versions[2] },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, vd55g1_dt_ids);

-- 
2.43.0

[PATCH 5/5] media: dt-bindings: vd55g1: Add vd55g4 compatible

[Benjamin Mugnier]

Define it as a new monochrome variant of vd65g4.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml b/Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml
index 060ac6829b66..58b1f9e85a9d 100644
--- a/Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml
@@ -27,9 +27,10 @@ properties:
   compatible:
     enum:
       - st,vd55g1
+      - st,vd55g4
       - st,vd65g4
     description:
-      VD55G1 is the monochrome variant, while VD65G4 is the color one.
+      VD55G1 and VD55G4 are monochrome variants, while VD65G4 is a color one.
 
   reg:
     maxItems: 1

-- 
2.43.0

Re: [PATCH 5/5] media: dt-bindings: vd55g1: Add vd55g4 compatible

[Krzysztof Kozlowski]

On Tue, Apr 28, 2026 at 10:40:59AM +0200, Benjamin Mugnier wrote:
> Define it as a new monochrome variant of vd65g4.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Please organize the patch documenting the compatible (DT bindings)
before the patch using that compatible.
See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH 5/5] media: dt-bindings: vd55g1: Add vd55g4 compatible

[Benjamin Mugnier]

Hi Krzysztof,

Le 30/04/2026 à 11:37, Krzysztof Kozlowski a écrit :
> On Tue, Apr 28, 2026 at 10:40:59AM +0200, Benjamin Mugnier wrote:
>> Define it as a new monochrome variant of vd65g4.
>>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  Documentation/devicetree/bindings/media/i2c/st,vd55g1.yaml | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
> 
> Please organize the patch documenting the compatible (DT bindings)
> before the patch using that compatible.
> See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Thanks. Sakari if you merge this serie could you reorder the commits
please ?

If this serie requires more changes than the reordering I'll do it in V2.

> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> 
> Best regards,
> Krzysztof
> 

-- 
Regards,
Benjamin

Re: [PATCH 0/5] media: i2c: vd55g1: Add vd55g4 support and various fixes

[Benjamin Mugnier]

Hi,

Gentle ping so it doesn’t get lost in limbo :)

Le 28/04/2026 à 10:40, Benjamin Mugnier a écrit :
> The vd55g4 is the monochrome variant of the vd56g4. This series adds the
> necessary code in the driver to probe and stream from the sensor
> in the correct format, and a new compatible in device tree bindings.
> 
> This series also fixes some issues I encountered while developing.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
> Benjamin Mugnier (5):
>       media: i2c: vd55g1: Fix media bus code initialization
>       media: i2c: vd55g1: Remove spurious pad format update on init_state()
>       media: i2c: vd55g1: Fix manual digital gain on color variant
>       media: i2c: vd55g1: Add support for vd55g4
>       media: dt-bindings: vd55g1: Add vd55g4 compatible
> 
>  .../devicetree/bindings/media/i2c/st,vd55g1.yaml   |   3 +-
>  drivers/media/i2c/vd55g1.c                         | 141 ++++++++++++++-------
>  2 files changed, 99 insertions(+), 45 deletions(-)
> ---
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> change-id: 20260428-vd55g4_and_fixes-97dc23b6f266
> 
> Best regards,
> --  
> Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> 

-- 
Regards,
Benjamin

Re: [PATCH 1/5] media: i2c: vd55g1: Fix media bus code initialization

[Jacopo Mondi]

Hi Benjamin

On Tue, Apr 28, 2026 at 10:40:55AM +0200, Benjamin Mugnier wrote:
> In the driver initialization, the index of the default media bus code
> from the supported media bus code array is passed directly to the
> vd55g1_get_fmt_code() function instead of the proper media bus code.
>
> This works correctly as a proper media bus code is set after
> initialization but could not have been the case. This also resulted in
> mutliple "Unsupported mbus format" error messages.
>
> Retrieve the media bus code from the media bus code array, and pass this
> media bus code to vd55g1_get_fmt_code() instead of the code index.
>
> Rename VD55G1_MBUS_CODE_DEF to VD55G1_MBUS_CODE_IDX_DEF and
> VD55G1_MODE_DEF to VD55G1_MODE_IDX_DEF while at it to avoid future
> confusions. Display the guilty error code in warning message.
>
> Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")
>
You should cc stable for fixes

Cc: stable@vger.kernel.org


The CI should have flagged that, but for some reason it didn't run
properly on your series
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/pipelines/1655147

> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  drivers/media/i2c/vd55g1.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 78d18c028154..1e9db21322e3 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -114,9 +114,9 @@
>
>  #define VD55G1_WIDTH					804
>  #define VD55G1_HEIGHT					704
> -#define VD55G1_MODE_DEF					0
> +#define VD55G1_MODE_IDX_DEF				0
>  #define VD55G1_NB_GPIOS					4
> -#define VD55G1_MBUS_CODE_DEF				0
> +#define VD55G1_MBUS_CODE_IDX_DEF			0
>  #define VD55G1_DGAIN_DEF				256
>  #define VD55G1_AGAIN_DEF				19
>  #define VD55G1_EXPO_MAX_TERM				64
> @@ -634,7 +634,7 @@ static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)

Unrelated, but it seems you now have 2 codes for MONO. Does

	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
		return code;

need an update ?

>  				goto adapt_bayer_pattern;
>  		}
>  	}
> -	dev_warn(sensor->dev, "Unsupported mbus format\n");
> +	dev_warn(sensor->dev, "Unsupported mbus format: 0x%x\n", code);
>
>  	return code;
>
> @@ -1347,6 +1347,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  {
>  	struct vd55g1 *sensor = to_vd55g1(sd);
>  	struct v4l2_subdev_format fmt = { 0 };
> +	int code;
>  	struct v4l2_subdev_route routes[] = {
>  		{ .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE }
>  	};
> @@ -1361,9 +1362,13 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  	if (ret)
>  		return ret;
>
> -	vd55g1_update_pad_fmt(sensor, &vd55g1_supported_modes[VD55G1_MODE_DEF],
> -			      vd55g1_get_fmt_code(sensor, VD55G1_MBUS_CODE_DEF),
> -			      &fmt.format);
> +	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
> +		code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
> +	else
> +		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];

Being this a multi-dimensional array, I don't seem much value in
defining VD55G1_MBUS_CODE_IDX_DEF if this is the only place where it
is used. What's the meaning of VD55G1_MBUS_CODE_IDX_DEF for
vd55g1_mbus_formats_bayer ? Does it represent the bitwidth or does it
represent the bayer pattern ?

I would rather define a
VD55G1_DEF_MBUS_CODE_MONO       MEDIA_BUS_FMT_Y8_1X8
VD55G1_DEF_MBUS_CODE_BAYER      MEDIA_BUS_FMT_SRGGB8_1X8

Or maybe do

		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF]
                                                [VD55G1_MBUS_CODE_IDX_DEF];

if easier.

I understand it's a minor, so up to you.



> +	vd55g1_update_pad_fmt(sensor,
> +			      &vd55g1_supported_modes[VD55G1_MODE_IDX_DEF],
> +			      vd55g1_get_fmt_code(sensor, code), &fmt.format);
>
>  	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH 2/5] media: i2c: vd55g1: Remove spurious pad format update on init_state()

[Jacopo Mondi]

Hi Benjamin

On Tue, Apr 28, 2026 at 10:40:56AM +0200, Benjamin Mugnier wrote:
> vd55g1_update_pad_fmt() is called in vd55g1_init_state(). But
> vd55g1_set_pad_fmt(), called at the end of vd55g1_init_state(), also
> calls vd55g1_update_pad_fmt() itself.
>
> Enhance readability and clear confusion by only preparing the format in
> vd55g1_init_state() and let vd55g1_set_pad_fmt() update it instead,
> effectively calling it only 1 time instead of 2.
>
> Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")

Does this qualify as a fix ?

I think you could maybe squash it with the previous one if you want
also this change to be backported as part of a larger fix

>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
   j

> ---
>  drivers/media/i2c/vd55g1.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 1e9db21322e3..e44174056ace 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -1366,9 +1366,9 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  		code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
>  	else
>  		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
> -	vd55g1_update_pad_fmt(sensor,
> -			      &vd55g1_supported_modes[VD55G1_MODE_IDX_DEF],
> -			      vd55g1_get_fmt_code(sensor, code), &fmt.format);
> +	fmt.format.code = vd55g1_get_fmt_code(sensor, code);
> +	fmt.format.width = vd55g1_supported_modes[VD55G1_MODE_IDX_DEF].width;
> +	fmt.format.height = vd55g1_supported_modes[VD55G1_MODE_IDX_DEF].height;
>
>  	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH 3/5] media: i2c: vd55g1: Fix manual digital gain on color variant

[Jacopo Mondi]

Hi Benjamin

On Tue, Apr 28, 2026 at 10:40:57AM +0200, Benjamin Mugnier wrote:
> Apply digital gain to all channels, each channel representing a color.
>
> Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")

Also cc stable here

>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/i2c/vd55g1.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index e44174056ace..2c962fcb41d2 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -60,7 +60,10 @@
>  #define VD55G1_PATGEN_ENABLE				BIT(0)
>  #define VD55G1_REG_MANUAL_ANALOG_GAIN			CCI_REG8(0x0501)
>  #define VD55G1_REG_MANUAL_COARSE_EXPOSURE		CCI_REG16_LE(0x0502)
> -#define VD55G1_REG_MANUAL_DIGITAL_GAIN			CCI_REG16_LE(0x0504)
> +#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH0		CCI_REG16_LE(0x0504)
> +#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH1		CCI_REG16_LE(0x0506)
> +#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH2		CCI_REG16_LE(0x0508)
> +#define VD55G1_REG_MANUAL_DIGITAL_GAIN_CH3		CCI_REG16_LE(0x050a)
>  #define VD55G1_REG_APPLIED_COARSE_EXPOSURE		CCI_REG16_LE(0x00e8)
>  #define VD55G1_REG_APPLIED_ANALOG_GAIN			CCI_REG16_LE(0x00ea)
>  #define VD55G1_REG_APPLIED_DIGITAL_GAIN			CCI_REG16_LE(0x00ec)
> @@ -850,9 +853,16 @@ static int vd55g1_update_expo_cluster(struct vd55g1 *sensor, bool is_auto)
>  		vd55g1_write(sensor, VD55G1_REG_MANUAL_ANALOG_GAIN,
>  			     sensor->again_ctrl->val, &ret);
>
> -	if (!is_auto && sensor->dgain_ctrl->is_new)
> -		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN,
> +	if (!is_auto && sensor->dgain_ctrl->is_new) {
> +		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH0,
>  			     sensor->dgain_ctrl->val, &ret);
> +		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH1,
> +			     sensor->dgain_ctrl->val, &ret);
> +		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH2,
> +			     sensor->dgain_ctrl->val, &ret);
> +		vd55g1_write(sensor, VD55G1_REG_MANUAL_DIGITAL_GAIN_CH3,
> +			     sensor->dgain_ctrl->val, &ret);
> +	}
>
>  	return ret;
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH 4/5] media: i2c: vd55g1: Add support for vd55g4

[Jacopo Mondi]

Hi Benjamin

On Tue, Apr 28, 2026 at 10:40:58AM +0200, Benjamin Mugnier wrote:
> vd55g4 is the same device as vd65g4 but outputs in monochrome instead of
> RGB. Adapt the driver structure according to this new variant, and add
> its support.
>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  drivers/media/i2c/vd55g1.c | 110 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 74 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 2c962fcb41d2..9f62fc0428a1 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -29,11 +29,7 @@
>
>  /* Register Map */
>  #define VD55G1_REG_MODEL_ID				CCI_REG32_LE(0x0000)
> -#define VD55G1_MODEL_ID_VD55G1				0x53354731 /* Mono */
> -#define VD55G1_MODEL_ID_VD65G4				0x53354733 /* RGB */
> -#define VD55G1_REG_REVISION				CCI_REG16_LE(0x0004)
> -#define VD55G1_REVISION_CCB				0x2020
> -#define VD55G1_REVISION_BAYER				0x3030
> +#define VD55G1_REG_COLOR_VERSION			CCI_REG32_LE(0x0670)
>  #define VD55G1_REG_FWPATCH_REVISION			CCI_REG16_LE(0x0012)
>  #define VD55G1_REG_FWPATCH_START_ADDR			CCI_REG8(0x2000)
>  #define VD55G1_REG_SYSTEM_FSM				CCI_REG8(0x001c)
> @@ -138,8 +134,39 @@
>  #define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
>  #define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)
>
> -#define VD55G1_MODEL_ID_NAME(id) \
> -	((id) == VD55G1_MODEL_ID_VD55G1 ? "vd55g1" : "vd65g4")
> +enum vd55g1_model_id {
> +	VD55G1_MODEL_ID_2 = 0x53354731,
> +	VD55G1_MODEL_ID_3 = 0x53354733,
> +};
> +
> +enum vd55g1_color_version {
> +	VD55G1_COLOR_VERSION_MONO = 0x0,
> +	VD55G1_COLOR_VERSION_BAYER = 0x1,

nit: you don't need to initialize the enum members here

> +};
> +
> +struct vd55g1_version {
> +	char *name;
> +	enum vd55g1_model_id id;
> +	enum vd55g1_color_version color;
> +};
> +
> +static const struct vd55g1_version vd55g1_versions[] = {
> +	{
> +		.name  = "vd55g1",
> +		.id    = VD55G1_MODEL_ID_2,
> +		.color = VD55G1_COLOR_VERSION_MONO,
> +	},
> +	{
> +		.name  = "vd55g4",
> +		.id    = VD55G1_MODEL_ID_3,
> +		.color = VD55G1_COLOR_VERSION_MONO,
> +	},
> +	{
> +		.name  = "vd65g4",
> +		.id    = VD55G1_MODEL_ID_3,
> +		.color = VD55G1_COLOR_VERSION_BAYER,
> +	},
> +};
>
>  static const u8 vd55g1_patch_array[] = {
>  	0x44, 0x03, 0x09, 0x02, 0xe6, 0x01, 0x42, 0x00, 0xea, 0x01, 0x42, 0x00,
> @@ -535,7 +562,7 @@ struct vd55g1_vblank_limits {
>
>  struct vd55g1 {
>  	struct device *dev;
> -	unsigned int id;
> +	const struct vd55g1_version *version;
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	struct regulator_bulk_data supplies[ARRAY_SIZE(vd55g1_supply_name)];
> @@ -628,7 +655,7 @@ static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)
>  {
>  	unsigned int i, j;
>
> -	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
> +	if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
>  		return code;

As pointed out in the previous patch, you seem to have 2 mono formats.
Is this still ok ?

>
>  	for (i = 0; i < ARRAY_SIZE(vd55g1_mbus_formats_bayer); i++) {
> @@ -1183,8 +1210,8 @@ static int vd55g1_patch(struct vd55g1 *sensor)
>  	u64 patch;
>  	int ret = 0;
>
> -	/* vd55g1 needs a patch while vd65g4 does not */
> -	if (sensor->id == VD55G1_MODEL_ID_VD55G1) {
> +	/* Version 2 needs a patch while version 3 does not */
> +	if (sensor->version->id == VD55G1_MODEL_ID_2) {
>  		vd55g1_write_array(sensor, VD55G1_REG_FWPATCH_START_ADDR,
>  				   sizeof(vd55g1_patch_array),
>  				   vd55g1_patch_array, &ret);

You might want to consider renaming vd55g1_patch_array ?

> @@ -1256,7 +1283,7 @@ static int vd55g1_enum_mbus_code(struct v4l2_subdev *sd,
>  	struct vd55g1 *sensor = to_vd55g1(sd);
>  	u32 base_code;
>
> -	if (sensor->id == VD55G1_MODEL_ID_VD55G1) {
> +	if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER) {
>  		if (code->index >= ARRAY_SIZE(vd55g1_mbus_formats_mono))
>  			return -EINVAL;
>  		base_code = vd55g1_mbus_formats_mono[code->index];
> @@ -1372,7 +1399,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  	if (ret)
>  		return ret;
>
> -	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
> +	if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
>  		code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
>  	else
>  		code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
> @@ -1659,38 +1686,48 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
>  	return ret;
>  }
>
> +static const struct vd55g1_version *
> +	vd55g1_get_version(enum vd55g1_model_id id,
> +			   enum vd55g1_color_version color)

Should you indent one tab left ?

> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vd55g1_versions); i++) {

You can declare i inside the for loop

> +		if (vd55g1_versions[i].id == id &&
> +		    vd55g1_versions[i].color == color)
> +			return &vd55g1_versions[i];
> +	}
> +
> +	return NULL;
> +}
> +
>  static int vd55g1_detect(struct vd55g1 *sensor)
>  {
> -	unsigned int dt_id = (uintptr_t)device_get_match_data(sensor->dev);
> -	u64 rev, id;
> -	int ret;
> +	const struct vd55g1_version *dt_version =
> +		device_get_match_data(sensor->dev);
> +	const struct vd55g1_version *version;
> +	u64 color, id;
> +	int ret = 0;
>
> -	ret = vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, NULL);
> +	vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, &ret);
> +	vd55g1_read(sensor, VD55G1_REG_COLOR_VERSION, &color, &ret);
>  	if (ret)
>  		return ret;
>
> -	if (id != VD55G1_MODEL_ID_VD55G1 && id != VD55G1_MODEL_ID_VD65G4) {
> -		dev_warn(sensor->dev, "Unsupported sensor id 0x%x\n",
> -			 (u32)id);
> +	version = vd55g1_get_version(id, color);
> +	if (!version) {
> +		dev_warn(sensor->dev, "Unsupported sensor version, expected %s\n",
> +			 dt_version->name);
>  		return -ENODEV;
>  	}
> -	if (id != dt_id) {
> -		dev_err(sensor->dev, "Probed sensor %s and device tree definition (%s) mismatch",
> -			VD55G1_MODEL_ID_NAME(id), VD55G1_MODEL_ID_NAME(dt_id));
> +	if (version->id != dt_version->id ||
> +	    version->color != dt_version->color) {
> +		dev_err(sensor->dev, "Probed sensor version %s and device tree definition %s mismatch",
> +			version->name, dt_version->name);
>  		return -ENODEV;
>  	}
> -	sensor->id = id;
>
> -	ret = vd55g1_read(sensor, VD55G1_REG_REVISION, &rev, NULL);
> -	if (ret)
> -		return ret;
> -
> -	if ((id == VD55G1_MODEL_ID_VD55G1 && rev != VD55G1_REVISION_CCB) &&
> -	    (id == VD55G1_MODEL_ID_VD65G4 && rev != VD55G1_REVISION_BAYER)) {
> -		dev_err(sensor->dev, "Unsupported sensor revision 0x%x for sensor %s\n",
> -			(u16)rev, VD55G1_MODEL_ID_NAME(id));
> -		return -ENODEV;
> -	}
> +	sensor->version = version;
>
>  	return 0;
>  }
> @@ -2048,8 +2085,9 @@ static void vd55g1_remove(struct i2c_client *client)
>  }
>
>  static const struct of_device_id vd55g1_dt_ids[] = {
> -	{ .compatible = "st,vd55g1", .data = (void *)VD55G1_MODEL_ID_VD55G1 },
> -	{ .compatible = "st,vd65g4", .data = (void *)VD55G1_MODEL_ID_VD65G4 },
> +	{ .compatible = "st,vd55g1", .data = (void *)&vd55g1_versions[0] },
> +	{ .compatible = "st,vd55g4", .data = (void *)&vd55g1_versions[1] },
> +	{ .compatible = "st,vd65g4", .data = (void *)&vd55g1_versions[2] },
>  	{ /* sentinel */ }
>  };

All minors
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  MODULE_DEVICE_TABLE(of, vd55g1_dt_ids);
>
> --
> 2.43.0
>
>