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

[Benjamin Mugnier]

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 ?

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 ?

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

-- 
Regards,
Benjamin