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

[Benjamin Mugnier]

Hi Jacopo,

Thank you for your review.

Le 22/06/2026 à 11:28, Jacopo Mondi a écrit :
> 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@xxxxxxxxxxxxxxx
> 

We talked about this very recently and somehow I still forgot.

> 
> 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@xxxxxxxxxxx>
>> ---
>>  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 ?>

Not in this patch because it does not add the new MONO sensor, but in
4/5 I separated the model ID from the color code. Example for the vd55g4 :

  .name  = "vd55g4",
  .id    = VD55G1_MODEL_ID_3,
  .color = VD55G1_COLOR_VERSION_MONO,

So the patch 4/5 updates the previous 'if' you mentioned to check the
color member instead of the model :

  if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)

Which IMO is a good way to handle this problematic. Tell me if you're
thinking about something else.

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

For vd55g1_mbus_formats_bayer, the first dimension of the array is the
bitwidth, and the second one is 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.

As you mentioned it's only used here. I won't mind removing
VD55G1_MBUS_CODE_IDX_DEF entirely and do :

  code = vd55g1_mbus_formats_bayer[0][0];

Does that sound okay ?

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

-- 
Regards,
Benjamin