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

[PATCH v2 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>
---
Changes in v2:
- Cc stable on "Fixes" patches
- Check mono code is correct in vd55g1_get_fmt_code()
- Remove VD55G1_MBUS_CODE_IDX_DEF
- Don't initialize vd55g1_color_version members
- Indent vd55g1_get_version() one tab left
- Declare vd55g1_get_version() iterator in for loop
- Reorder commits to put device tree patch before implementation patch
- Link to v1: https://patch.msgid.link/20260428-vd55g4_and_fixes-v1-0-4f745a83b87e@foss.st.com

To: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
To: Sylvain Petinot <sylvain.petinot@foss.st.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Hans Verkuil <hverkuil+cisco@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org

---
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: dt-bindings: vd55g1: Add vd55g4 compatible
      media: i2c: vd55g1: Add support for vd55g4

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

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

[PATCH v2 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.

Cc: stable@vger.kernel.org
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, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 78d18c028154..fceb437e19be 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -114,9 +114,8 @@
 
 #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_DGAIN_DEF				256
 #define VD55G1_AGAIN_DEF				19
 #define VD55G1_EXPO_MAX_TERM				64
@@ -634,7 +633,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 +1346,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 +1361,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[0];
+	else
+		code = vd55g1_mbus_formats_bayer[0][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 v2 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.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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 fceb437e19be..22464fe31562 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1365,9 +1365,9 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
 		code = vd55g1_mbus_formats_mono[0];
 	else
 		code = vd55g1_mbus_formats_bayer[0][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 v2 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.

Cc: stable@vger.kernel.org
Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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 22464fe31562..37d44abd1435 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)
@@ -849,9 +852,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 v2 4/5] media: dt-bindings: vd55g1: Add vd55g4 compatible

[Benjamin Mugnier]

Define it as a new monochrome variant of vd65g4.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
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

[PATCH v2 5/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.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 130 ++++++++++++++++++++++++++++++---------------
 1 file changed, 87 insertions(+), 43 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 37d44abd1435..c4142d771e7c 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)
@@ -137,8 +133,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,
+	VD55G1_COLOR_VERSION_BAYER,
+};
+
+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,
@@ -534,7 +561,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)];
@@ -625,20 +652,28 @@ static unsigned int vd55g1_get_fmt_data_type(u32 code)
 
 static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)
 {
+	u32 fallback_code;
 	unsigned int i, j;
 
-	if (sensor->id == VD55G1_MODEL_ID_VD55G1)
-		return code;
-
-	for (i = 0; i < ARRAY_SIZE(vd55g1_mbus_formats_bayer); i++) {
-		for (j = 0; j < ARRAY_SIZE(vd55g1_mbus_formats_bayer[i]); j++) {
-			if (vd55g1_mbus_formats_bayer[i][j] == code)
-				goto adapt_bayer_pattern;
+	if (sensor->version->color == VD55G1_COLOR_VERSION_MONO) {
+		fallback_code = vd55g1_mbus_formats_mono[0];
+		for (i = 0; i < ARRAY_SIZE(vd55g1_mbus_formats_mono); i++)
+			if (vd55g1_mbus_formats_mono[i] == code)
+				return code;
+	} else {
+		fallback_code = vd55g1_mbus_formats_bayer[0][0];
+		for (i = 0; i < ARRAY_SIZE(vd55g1_mbus_formats_bayer); i++) {
+			for (j = 0;
+			     j < ARRAY_SIZE(vd55g1_mbus_formats_bayer[i]);
+			     j++) {
+				if (vd55g1_mbus_formats_bayer[i][j] == code)
+					goto adapt_bayer_pattern;
+			}
 		}
 	}
 	dev_warn(sensor->dev, "Unsupported mbus format: 0x%x\n", code);
 
-	return code;
+	return fallback_code;
 
 adapt_bayer_pattern:
 	j = 0;
@@ -1182,8 +1217,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);
@@ -1255,7 +1290,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];
@@ -1371,7 +1406,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[0];
 	else
 		code = vd55g1_mbus_formats_bayer[0][0];
@@ -1658,38 +1693,46 @@ 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)
+{
+	for (unsigned int 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;
 }
@@ -2047,8 +2090,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Meaningful code and commit message mismatch: The commit message states 'Rename VD55G1_MBUS_CODE_DEF to VD55G1_MBUS_CODE_IDX_DEF', but the macro is deleted instead and hardcoded '0' indices are used.
- [Low] Type mismatch: The `code` variable is declared as a signed `int` but is used to store V4L2 media bus format codes, which are strictly unsigned 32-bit integers (`u32`).
--

commit 6cf5089e88c71a68129dc09d9aa82be4ff08d4b3
Author: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

media: i2c: vd55g1: Fix media bus code initialization

This commit passes the actual media bus code to vd55g1_get_fmt_code()
during driver initialization instead of the array index. It also renames
macro definitions to clarify their usage as indices.

> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 78d18c0281541..fceb437e19bec 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c

[ ... ]

> @@ -1347,6 +1346,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  {
>  	struct vd55g1 *sensor = to_vd55g1(sd);
>  	struct v4l2_subdev_format fmt = { 0 };
> +	int code;

[Severity: Low]
Does this type mismatch cause any issues? V4L2 media bus format codes are
strictly unsigned 32-bit integers (u32), but this is declared as a signed int.

>  	struct v4l2_subdev_route routes[] = {
>  		{ .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE }
>  	};
> @@ -1361,9 +1361,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[0];
> +	else
> +		code = vd55g1_mbus_formats_bayer[0][0];

[Severity: Medium]
The commit message states "Rename VD55G1_MBUS_CODE_DEF to
VD55G1_MBUS_CODE_IDX_DEF", but the macro was deleted entirely in this patch.

Should these hardcoded 0 indices use the promised VD55G1_MBUS_CODE_IDX_DEF
macro instead to avoid confusion?

> +	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);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-vd55g4_and_fixes-v2-0-ea0a7cf3cd66@foss.st.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The subdev state lock and the control handler lock are not shared, leading to lockdep failures and data races.
--

commit e1c6d43eeca25a31bf4c1014650f4e8b8276a063
Author: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

media: i2c: vd55g1: Remove spurious pad format update on init_state()

This patch prevents vd55g1_update_pad_fmt() from being called twice during
initialization by setting the format directly in vd55g1_init_state(). The
actual update is now deferred entirely to vd55g1_set_pad_fmt().

> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index fceb437e19bec..22464fe315624 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -1365,9 +1365,9 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  		code = vd55g1_mbus_formats_mono[0];
>  	else
>  		code = vd55g1_mbus_formats_bayer[0][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);
>  }

[Severity: High]
This is a pre-existing issue, but does the initialization order in the driver
leave the control handler and subdev state locks unshared, potentially leading
to data races and lockdep warnings?

In vd55g1_subdev_init(), the lock assignment happens before the control handler
is initialized:

drivers/media/i2c/vd55g1.c:vd55g1_subdev_init() {
    ...
    sensor->sd.state_lock = sensor->ctrl_handler.lock;
    ret = v4l2_subdev_init_finalize(&sensor->sd);
    ...
}

Since sensor->ctrl_handler.lock is still NULL here, v4l2_subdev_init_finalize()
falls back to allocating an unshared lock for the active state.

The control handler lock is actually initialized later in vd55g1_init_ctrls():

drivers/media/i2c/vd55g1.c:vd55g1_init_ctrls() {
    ...
    v4l2_ctrl_handler_init(hdl, 16);
    ...
}

This means vd55g1_s_ctrl() runs with ctrl_handler.lock held, but expects the
active state lock to be held:

drivers/media/i2c/vd55g1.c:vd55g1_s_ctrl() {
    ...
    struct v4l2_subdev_state *state =
        v4l2_subdev_get_locked_active_state(&sensor->sd);
    ...
}

Because the locks are distinct, this triggers a lockdep assertion failure inside
v4l2_subdev_get_locked_active_state(). It also seems to expose a data race, as
vd55g1_s_ctrl() can read format and crop while vd55g1_set_pad_fmt() modifies
them concurrently using the active state lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-vd55g4_and_fixes-v2-0-ea0a7cf3cd66@foss.st.com?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unchecked dereference of `device_get_match_data()` result in `vd55g1_detect()` leads to a NULL pointer dereference.

Pre-existing issues:
- [High] The V4L2 subdev state lock and control handler lock are mismatched, causing lockless execution of `__v4l2_ctrl_modify_range()`.
--

commit 6446db5fc4015fc328aed2df077a1b5dfc46a590
Author: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

media: i2c: vd55g1: Add support for vd55g4

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.

> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 37d44abd1435c..c4142d771e7c8 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c

[ ... ]

> @@ -1658,38 +1693,46 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)

[ ... ]

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

[Severity: High]
Could dt_version be NULL here? If the device is instantiated via software
nodes or forced to bind via sysfs without matching OF compatible properties,
device_get_match_data() returns NULL.

If it returns NULL, this will cause a kernel oops when dereferencing
dt_version->name here, as well as dt_version->id and dt_version->color
below. The previous version of this code safely cast the result to an
integer instead of dereferencing it.

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

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a mismatched lock assignment
in vd55g1_subdev_init()?

It appears sensor->sd.state_lock is assigned to sensor->ctrl_handler.lock
before v4l2_ctrl_handler_init() is called, meaning the lock is still NULL:

vd55g1_subdev_init() {
    ...
    sensor->sd.state_lock = sensor->ctrl_handler.lock;
    ret = v4l2_subdev_init_finalize(&sensor->sd);
    ...
}

Since v4l2_ctrl_handler_init() allocates a different internal mutex later,
the V4L2 core allocates a separate mutex for the state. When V4L2 core
invokes vd55g1_set_pad_fmt(), it calls __v4l2_ctrl_modify_range() while
holding only the subdev state lock. Could this violate V4L2 locking
invariants and cause a data race if a concurrent user-space VIDIOC_S_CTRL
ioctl is issued?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-vd55g4_and_fixes-v2-0-ea0a7cf3cd66@foss.st.com?part=5