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

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

Hi Jacopo,

Le 26/06/2026 à 09:58, Jacopo Mondi a écrit :
> Hi Benjamin
> 
> On Thu, Jun 25, 2026 at 01:41:56PM +0200, Benjamin Mugnier wrote:
>> Hi Jacopo,
>>
>> Le 22/06/2026 à 12:16, Jacopo Mondi a écrit :
>>> 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
>>
>> Thank you, will do.
>>
>>>
>>>> +};
>>>> +
>>>> +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 ?
>>
>> It doesn't really patch an array, it writes an array of values into
> 
> My only point here was that it has 'vd55g1' in the name.

Sorry I thought you were talking about vd55g1_write_array() and not
vd55g1_patch_array for some reason.

My rule of thumb is that a static global variable must always be
prefixed by the driver name to avoid collisions. What rename are you
thinking about ?

> 
>> continuous i2c registers. Just like before converting to cci_write() I
>> used to have a vd55g1_write() function to write a register, I derived a
>> vd55g1_write_array() function. Here it happens to be a firmware patch,
>> but it could be anything you want ;)
>>
>>>
>>>> @@ -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 ?
>>>
>>
>> checkpatch.pl is fine with both. I can indent left, it looks cleaner.
>>
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(vd55g1_versions); i++) {
>>>
>>> You can declare i inside the for loop
>>>
>>
>> Yes, thank you.
>>
>>>> +		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
>>>>
>>>>
>>
>> --
>> Regards,
>> Benjamin
>>

-- 
Regards,
Benjamin