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

[Jacopo Mondi]

Hi Benjamin

On Thu, Jun 25, 2026 at 01:41:48PM +0200, Benjamin Mugnier wrote:
> 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

Not in this patch ofc

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

Yes, I was thinking about the unconditional return of 'code'.

You know have two mono codes, shouldn't you do the same as you do with
the bayer ones ?

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

It does, thanks

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