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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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