Re: [PATCH 4/4] media: i2c: vd55g1: Use first index of mbus codes array as default

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Benjamin,

On Mon, Jun 09, 2025 at 03:46:24PM +0200, Benjamin Mugnier wrote:
> Factorize code and prevent future erros in case of media bus codes
> change.
> Rename VD55G1_DEFAULT_MODE to VD55G1_MODE_DEF to mimic other macros
> while at it.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  drivers/media/i2c/vd55g1.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index dec6e3e231d54a742bdd08ff2a506c152bb89429..177caa5470cfcf49e0ae2fb568d7872a5608a11f 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -111,9 +111,9 @@
>  
>  #define VD55G1_WIDTH					804
>  #define VD55G1_HEIGHT					704
> -#define VD55G1_DEFAULT_MODE				0
> +#define VD55G1_MODE_DEF					0
>  #define VD55G1_NB_GPIOS					4
> -#define VD55G1_MEDIA_BUS_FMT_DEF			MEDIA_BUS_FMT_Y8_1X8
> +#define VD55G1_MBUS_CODE_DEF				0
>  #define VD55G1_DGAIN_DEF				256
>  #define VD55G1_AGAIN_DEF				19
>  #define VD55G1_EXPO_MAX_TERM				64
> @@ -1260,7 +1260,8 @@ static int vd55g1_set_pad_fmt(struct v4l2_subdev *sd,
>  static int vd55g1_init_state(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> -	unsigned int def_mode = VD55G1_DEFAULT_MODE;
> +	unsigned int def_mode = VD55G1_MODE_DEF;
> +	unsigned int def_mbus_code = VD55G1_MBUS_CODE_DEF;

Why the local variables?

>  	struct vd55g1 *sensor = to_vd55g1(sd);
>  	struct v4l2_subdev_format fmt = { 0 };
>  	struct v4l2_subdev_route routes[] = {
> @@ -1278,7 +1279,8 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  		return ret;
>  
>  	vd55g1_update_img_pad_format(sensor, &vd55g1_supported_modes[def_mode],
> -				     VD55G1_MEDIA_BUS_FMT_DEF, &fmt.format);
> +				     vd55g1_mbus_codes[def_mbus_code].code,
> +				     &fmt.format);

I'd remove def_mode, too, and just use VD55G1_DEFAULT_MODE. The 80
characters per line is preferred but I think in this case using local
variables just to use them once doens't make the code easier to read.

>  
>  	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
>  }
> 

-- 
Regards,

Sakari Ailus