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

[Sakari Ailus]

Hi Benjamin,

On Tue, Jun 10, 2025 at 11:31:11AM +0200, Benjamin Mugnier wrote:
> On 6/10/25 11:04, Sakari Ailus wrote:
> > 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@xxxxxxxxxxx>
> >> ---
> >>  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.
> > 
> 
> You guessed correctly, local variables are here to avoid overflowing the
> 80 characters per line.
> If I put VD55G1_DEFAULT_MODE directly then checkpatch will fail. As I
> understand this is a hard requirement ?

It's not a hard requirement in media-ci, no. In some cases it's justified
to have longer lines than that (see V4L2 IOCTL definitions, for instance),
I don't mind if it happens above. You could also make the function name
shorter, it's pretty long. Up to you.

> 
> I could do something like :
> 
> const struct vd55g1_mode *mode =
> 	&vd55g1_supported_modes[VD55G1_MODE_DEF];
> const struct vd55g1_fmt_desc *mbus_code =
> 	&vd55g1_mbus_codes[VD55G1_MBUS_CODE_DEF];
> [...]
> vd55g1_update_img_pad_format(sensor, mode, mbus_code->code,
> 			     &fmt.format);
> 
> Which IMO improves readability, what do you think ?

I'd just do this without local variables.

-- 
Regards,

Sakari Ailus