[PATCH 0/4] media: i2c: vd55g1: Miscellaneous fixes

[PATCH 0/4] media: i2c: vd55g1: Miscellaneous fixes

[Benjamin Mugnier]

This series provides small fixes and style improvements to the vd55g1
driver.
Nothing fancy really, just to keep everything up to date.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
Benjamin Mugnier (4):
      media: i2c: vd55g1: Fix RATE macros not being expressed in bps
      media: i2c: vd55g1: Fix return code in vd55g1_enable_streams error path
      media: i2c: vd55g1: Setup sensor external clock before patching
      media: i2c: vd55g1: Use first index of mbus codes array as default

 drivers/media/i2c/vd55g1.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
---
base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae
change-id: 20250609-fix_vd55g1-83e924648d85

Best regards,
-- 
Benjamin Mugnier <benjamin.mugnier@foss.st.com>

[PATCH 1/4] media: i2c: vd55g1: Fix RATE macros not being expressed in bps

[Benjamin Mugnier]

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 25e2fc88a0367bf6a28bb22d209323ace99299f2..78dd22d9cab03edf4ff3e5a301f8d045e930c997 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -129,8 +129,8 @@
 #define VD55G1_FWPATCH_REVISION_MINOR			9
 #define VD55G1_XCLK_FREQ_MIN				(6 * HZ_PER_MHZ)
 #define VD55G1_XCLK_FREQ_MAX				(27 * HZ_PER_MHZ)
-#define VD55G1_MIPI_RATE_MIN				(250 * HZ_PER_MHZ)
-#define VD55G1_MIPI_RATE_MAX				(1200 * HZ_PER_MHZ)
+#define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
+#define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)
 
 static const u8 patch_array[] = {
 	0x44, 0x03, 0x09, 0x02, 0xe6, 0x01, 0x42, 0x00, 0xea, 0x01, 0x42, 0x00,

-- 
2.25.1

Re: [PATCH 1/4] media: i2c: vd55g1: Fix RATE macros not being expressed in bps

[Sakari Ailus]

On Mon, Jun 09, 2025 at 03:46:21PM +0200, Benjamin Mugnier wrote:
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  drivers/media/i2c/vd55g1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 25e2fc88a0367bf6a28bb22d209323ace99299f2..78dd22d9cab03edf4ff3e5a301f8d045e930c997 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -129,8 +129,8 @@
>  #define VD55G1_FWPATCH_REVISION_MINOR			9
>  #define VD55G1_XCLK_FREQ_MIN				(6 * HZ_PER_MHZ)
>  #define VD55G1_XCLK_FREQ_MAX				(27 * HZ_PER_MHZ)
> -#define VD55G1_MIPI_RATE_MIN				(250 * HZ_PER_MHZ)
> -#define VD55G1_MIPI_RATE_MAX				(1200 * HZ_PER_MHZ)
> +#define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
> +#define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)

As the meaning of Hz is just /s, I don't think the use of HZ_PER_MHZ was
wrong in any way above.

-- 
Sakari Ailus

Re: [PATCH 1/4] media: i2c: vd55g1: Fix RATE macros not being expressed in bps

[Benjamin Mugnier]

On 6/10/25 11:01, Sakari Ailus wrote:
> On Mon, Jun 09, 2025 at 03:46:21PM +0200, Benjamin Mugnier wrote:
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  drivers/media/i2c/vd55g1.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
>> index 25e2fc88a0367bf6a28bb22d209323ace99299f2..78dd22d9cab03edf4ff3e5a301f8d045e930c997 100644
>> --- a/drivers/media/i2c/vd55g1.c
>> +++ b/drivers/media/i2c/vd55g1.c
>> @@ -129,8 +129,8 @@
>>  #define VD55G1_FWPATCH_REVISION_MINOR			9
>>  #define VD55G1_XCLK_FREQ_MIN				(6 * HZ_PER_MHZ)
>>  #define VD55G1_XCLK_FREQ_MAX				(27 * HZ_PER_MHZ)
>> -#define VD55G1_MIPI_RATE_MIN				(250 * HZ_PER_MHZ)
>> -#define VD55G1_MIPI_RATE_MAX				(1200 * HZ_PER_MHZ)
>> +#define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
>> +#define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)
> 
> As the meaning of Hz is just /s, I don't think the use of HZ_PER_MHZ was
> wrong in any way above.
> 

Should I just drop this patch then ?

-- 
Regards,
Benjamin

Re: [PATCH 1/4] media: i2c: vd55g1: Fix RATE macros not being expressed in bps

[Sakari Ailus]

On Tue, Jun 10, 2025 at 11:31:09AM +0200, Benjamin Mugnier wrote:
> On 6/10/25 11:01, Sakari Ailus wrote:
> > On Mon, Jun 09, 2025 at 03:46:21PM +0200, Benjamin Mugnier wrote:
> >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >> ---
> >>  drivers/media/i2c/vd55g1.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> >> index 25e2fc88a0367bf6a28bb22d209323ace99299f2..78dd22d9cab03edf4ff3e5a301f8d045e930c997 100644
> >> --- a/drivers/media/i2c/vd55g1.c
> >> +++ b/drivers/media/i2c/vd55g1.c
> >> @@ -129,8 +129,8 @@
> >>  #define VD55G1_FWPATCH_REVISION_MINOR			9
> >>  #define VD55G1_XCLK_FREQ_MIN				(6 * HZ_PER_MHZ)
> >>  #define VD55G1_XCLK_FREQ_MAX				(27 * HZ_PER_MHZ)
> >> -#define VD55G1_MIPI_RATE_MIN				(250 * HZ_PER_MHZ)
> >> -#define VD55G1_MIPI_RATE_MAX				(1200 * HZ_PER_MHZ)
> >> +#define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
> >> +#define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)
> > 
> > As the meaning of Hz is just /s, I don't think the use of HZ_PER_MHZ was
> > wrong in any way above.
> > 
> 
> Should I just drop this patch then ?

Up to you.

-- 
Sakari Ailus

Re: [PATCH 1/4] media: i2c: vd55g1: Fix RATE macros not being expressed in bps

[Benjamin Mugnier]

On 6/12/25 08:24, Sakari Ailus wrote:
> On Tue, Jun 10, 2025 at 11:31:09AM +0200, Benjamin Mugnier wrote:
>> On 6/10/25 11:01, Sakari Ailus wrote:
>>> On Mon, Jun 09, 2025 at 03:46:21PM +0200, Benjamin Mugnier wrote:
>>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>> ---
>>>>  drivers/media/i2c/vd55g1.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
>>>> index 25e2fc88a0367bf6a28bb22d209323ace99299f2..78dd22d9cab03edf4ff3e5a301f8d045e930c997 100644
>>>> --- a/drivers/media/i2c/vd55g1.c
>>>> +++ b/drivers/media/i2c/vd55g1.c
>>>> @@ -129,8 +129,8 @@
>>>>  #define VD55G1_FWPATCH_REVISION_MINOR			9
>>>>  #define VD55G1_XCLK_FREQ_MIN				(6 * HZ_PER_MHZ)
>>>>  #define VD55G1_XCLK_FREQ_MAX				(27 * HZ_PER_MHZ)
>>>> -#define VD55G1_MIPI_RATE_MIN				(250 * HZ_PER_MHZ)
>>>> -#define VD55G1_MIPI_RATE_MAX				(1200 * HZ_PER_MHZ)
>>>> +#define VD55G1_MIPI_RATE_MIN				(250 * MEGA)
>>>> +#define VD55G1_MIPI_RATE_MAX				(1200 * MEGA)
>>>
>>> As the meaning of Hz is just /s, I don't think the use of HZ_PER_MHZ was
>>> wrong in any way above.
>>>
>>
>> Should I just drop this patch then ?
> 
> Up to you.
> 

Then I'd like to keep it please :)

-- 
Regards,
Benjamin

[PATCH 2/4] media: i2c: vd55g1: Fix return code in vd55g1_enable_streams error path

[Benjamin Mugnier]

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 78dd22d9cab03edf4ff3e5a301f8d045e930c997..336dc3c85ac9e695f22aa524e0df6138dc76e45c 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1084,7 +1084,7 @@ static int vd55g1_enable_streams(struct v4l2_subdev *sd,
 
 err_rpm_put:
 	pm_runtime_put(sensor->dev);
-	return 0;
+	return -EINVAL;
 }
 
 static int vd55g1_disable_streams(struct v4l2_subdev *sd,

-- 
2.25.1

[PATCH 3/4] media: i2c: vd55g1: Setup sensor external clock before patching

[Benjamin Mugnier]

Proper clock configuration is required to advance through FSM states.
Prior than this having a different clock value than default sensor's
value was used (12 MHz) could prevent the sensor from booting.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 drivers/media/i2c/vd55g1.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 336dc3c85ac9e695f22aa524e0df6138dc76e45c..dec6e3e231d54a742bdd08ff2a506c152bb89429 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1038,8 +1038,6 @@ static int vd55g1_enable_streams(struct v4l2_subdev *sd,
 	if (ret < 0)
 		return ret;
 
-	vd55g1_write(sensor, VD55G1_REG_EXT_CLOCK, sensor->xclk_freq, &ret);
-
 	/* Configure output */
 	vd55g1_write(sensor, VD55G1_REG_MIPI_DATA_RATE,
 		     sensor->mipi_rate, &ret);
@@ -1613,6 +1611,9 @@ static int vd55g1_power_on(struct device *dev)
 		goto disable_clock;
 	}
 
+	/* Setup clock now to advance through system FSM states */
+	vd55g1_write(sensor, VD55G1_REG_EXT_CLOCK, sensor->xclk_freq, &ret);
+
 	ret = vd55g1_patch(sensor);
 	if (ret) {
 		dev_err(dev, "Sensor patch failed %d\n", ret);

-- 
2.25.1

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

[Benjamin Mugnier]

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;
 	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);
 
 	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
 }

-- 
2.25.1

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

[Sakari Ailus]

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

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

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

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

Re: [PATCH 0/4] media: i2c: vd55g1: Miscellaneous fixes

[Sakari Ailus]

Hi Benjamin,

Thanks for the set.

On Mon, Jun 09, 2025 at 03:46:20PM +0200, Benjamin Mugnier wrote:
> This series provides small fixes and style improvements to the vd55g1
> driver.
> Nothing fancy really, just to keep everything up to date.

On all (or at least more than one) patches:

- please add a proper commit message beyond the subject line and

- properly wrap the commit paragraphs (up to 75 characters per line, e.g.
  the above paragraph would fit on two lines).

-- 
Regards,

Sakari Ailus

Re: [PATCH 0/4] media: i2c: vd55g1: Miscellaneous fixes

[Benjamin Mugnier]

Hi Sakari,

Thank you for your review.

On 6/10/25 11:00, Sakari Ailus wrote:
> Hi Benjamin,
> 
> Thanks for the set.
> 
> On Mon, Jun 09, 2025 at 03:46:20PM +0200, Benjamin Mugnier wrote:
>> This series provides small fixes and style improvements to the vd55g1
>> driver.
>> Nothing fancy really, just to keep everything up to date.
> 
> On all (or at least more than one) patches:
> 
> - please add a proper commit message beyond the subject line and
> 
> - properly wrap the commit paragraphs (up to 75 characters per line, e.g.
>   the above paragraph would fit on two lines).
> 

Yes, commit descriptions have been added in v2. Media-ci rightfully
yelled at me for that ;)

I don't see any commit paragraphs being above 75 characters, my vim
seems to be properly configured.
Do you mean the commit header too perhaps ? For example "media: i2c:
vd55g1: Miscellaneous fixes".

-- 
Regards,
Benjamin

Re: [PATCH 0/4] media: i2c: vd55g1: Miscellaneous fixes

[Sakari Ailus]

Hi Benjamin,

On Tue, Jun 10, 2025 at 11:31:05AM +0200, Benjamin Mugnier wrote:
> Hi Sakari,
> 
> Thank you for your review.
> 
> On 6/10/25 11:00, Sakari Ailus wrote:
> > Hi Benjamin,
> > 
> > Thanks for the set.
> > 
> > On Mon, Jun 09, 2025 at 03:46:20PM +0200, Benjamin Mugnier wrote:
> >> This series provides small fixes and style improvements to the vd55g1
> >> driver.
> >> Nothing fancy really, just to keep everything up to date.
> > 
> > On all (or at least more than one) patches:
> > 
> > - please add a proper commit message beyond the subject line and
> > 
> > - properly wrap the commit paragraphs (up to 75 characters per line, e.g.
> >   the above paragraph would fit on two lines).
> > 
> 
> Yes, commit descriptions have been added in v2. Media-ci rightfully
> yelled at me for that ;)
> 
> I don't see any commit paragraphs being above 75 characters, my vim
> seems to be properly configured.

It's not over 75 characters per line but some lines are shorter than they
could be.

> Do you mean the commit header too perhaps ? For example "media: i2c:
> vd55g1: Miscellaneous fixes".

-- 
Regards,

Sakari Ailus