Re: [PATCH 01/11] media: i2c: ov5640: Set default WB gains

Re: [PATCH 01/11] media: i2c: ov5640: Set default WB gains

[Jacopo Mondi]

Hi Kieran

On Fri, May 01, 2026 at 04:39:03PM +0100, Kieran Bingham wrote:
> The default values for the Blue and Red balance should be
> the same as the Green balance (0x400/1024).
>
> Update the values to ensure no change is applied by default.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 85ecc23b3587..01ea6b2bfeeb 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3470,9 +3470,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  					   V4L2_CID_AUTO_WHITE_BALANCE,
>  					   0, 1, 1, 1);
>  	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE,
> -						0, 4095, 1, 0);
> +						0, 4095, 1, 1024);
>  	ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
> -					       0, 4095, 1, 0);
> +					       0, 4095, 1, 1024);

The register format is not documented, but 0x400 is indeed the
default. Looking at the control max value and the register default which
is reasonable to assume represents x1.0, I presume it is safe to
assume unsigned Q2.10 for the WB gains.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  	/* Auto/manual exposure */
>  	ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
>  						 V4L2_CID_EXPOSURE_AUTO,
>
> --
> 2.52.0
>
>

Re: [PATCH 02/11] media: i2c: ov5640: Set exposure minimum and defaults

[Jacopo Mondi]

Hi Kieran

On Fri, May 01, 2026 at 04:39:04PM +0100, Kieran Bingham wrote:
> A zero exposure would be a fully black image and not useful as a sensor.
> Increase the minimum and default control values to sane parameters.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 01ea6b2bfeeb..b5e529ea662b 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3479,7 +3479,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  						 V4L2_EXPOSURE_MANUAL, 0,
>  						 V4L2_EXPOSURE_AUTO);
>  	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> -					    0, 65535, 1, 0);
> +					    4, 65535, 1, 1000);

Where does 1000 come from ? The register default is 512, should we use
that value ?

Also, is the 4 lines step value correct ?

>  	/* Auto/manual gain */
>  	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
>  					     0, 1, 1, 1);
>
> --
> 2.52.0
>
>

Re: [PATCH 02/11] media: i2c: ov5640: Set exposure minimum and defaults

[Jacopo Mondi]

On Thu, May 14, 2026 at 09:58:43AM +0200, Jacopo Mondi wrote:
> Hi Kieran
>
> On Fri, May 01, 2026 at 04:39:04PM +0100, Kieran Bingham wrote:
> > A zero exposure would be a fully black image and not useful as a sensor.
> > Increase the minimum and default control values to sane parameters.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 01ea6b2bfeeb..b5e529ea662b 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -3479,7 +3479,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> >  						 V4L2_EXPOSURE_MANUAL, 0,
> >  						 V4L2_EXPOSURE_AUTO);
> >  	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > -					    0, 65535, 1, 0);
> > +					    4, 65535, 1, 1000);
>
> Where does 1000 come from ? The register default is 512, should we use
> that value ?
>
> Also, is the 4 lines step value correct ?

Sorry, this is the minimum of course. How did you pick 4 ?


>
> >  	/* Auto/manual gain */
> >  	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
> >  					     0, 1, 1, 1);
> >
> > --
> > 2.52.0
> >
> >

Re: [PATCH 03/11] media: i2c: ov5640: Fix minimum gain to 1.0x

[Jacopo Mondi]

On Fri, May 01, 2026 at 04:39:05PM +0100, Kieran Bingham wrote:
> Adjust the Analogue Gain control to only provide a minimum gain of 1.0,
> and ensure that this is the default value rather than '0' which is
> invalid.
>
> Requesting an Analogue gain of 0 would result in a black frame, and when
> the sensor is over exposed the Exposure should be reduced instead.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index b5e529ea662b..0dc4f4272d05 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3484,7 +3484,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
>  					     0, 1, 1, 1);
>  	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> -					0, 1023, 1, 0);
> +					16, 1023, 1, 16);

Again the register is not documented, but it is said that 0xff = x32
which suggests Q6.4 hence 16 should be x1.0

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
>  	ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
>  					      0, 255, 1, 64);
>
> --
> 2.52.0
>
>

Re: [PATCH 04/11] media: i2c: ov5640: fix error path in ov5640_set_mode

[Jacopo Mondi]

On Fri, May 01, 2026 at 04:39:06PM +0100, Kieran Bingham wrote:
> During ov5640_set_mode if the call to set the pclk fails, we should
> correctly restore the state of the auto exposure
> and auto gain if they were modified on entry into the function.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


> ---
>  drivers/media/i2c/ov5640.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 0dc4f4272d05..244c341d0e77 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -2349,7 +2349,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>  	else
>  		ret = ov5640_set_dvp_pclk(sensor);
>  	if (ret < 0)
> -		return 0;
> +		goto restore_auto_exp_gain;
>
>  	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
>  	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
>
> --
> 2.52.0
>
>

Re: [PATCH 05/11] media: i2c: ov5640: Remove unsupported bayer orders

[Jacopo Mondi]

Hi Kieran

On Fri, May 01, 2026 at 04:39:07PM +0100, Kieran Bingham wrote:
> The OV5640 only outputs SBGGR8. Remove the incorrectly advertised
> alternatives which allow a misconfigured pipeline to be established.

Do you have any idea why the datasheet mentions all the RGGB
permutations as valid outputs ?

>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 244c341d0e77..e1e253730206 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -309,27 +309,6 @@ static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
>  		.bpp		= 8,
>  		.ctrl00		= 0x00,
>  		.mux		= OV5640_FMT_MUX_RAW_DPC,
> -	}, {
> -		/* Raw bayer, GBGB... / RGRG... */
> -		.code		= MEDIA_BUS_FMT_SGBRG8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> -		.bpp		= 8,
> -		.ctrl00		= 0x01,
> -		.mux		= OV5640_FMT_MUX_RAW_DPC,
> -	}, {
> -		/* Raw bayer, GRGR... / BGBG... */
> -		.code		= MEDIA_BUS_FMT_SGRBG8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> -		.bpp		= 8,
> -		.ctrl00		= 0x02,
> -		.mux		= OV5640_FMT_MUX_RAW_DPC,
> -	}, {
> -		/* Raw bayer, RGRG... / GBGB... */
> -		.code		= MEDIA_BUS_FMT_SRGGB8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> -		.bpp		= 8,
> -		.ctrl00		= 0x03,
> -		.mux		= OV5640_FMT_MUX_RAW_DPC,
>  	},

Seems like you've missed the same entries in the ov5640_dvp_formats[]
table.

>  	{ /* sentinel */ }
>  };
>
> --
> 2.52.0
>
>

Re: [PATCH 06/11] media: i2c: ov5640: split out the LSC registers

[Jacopo Mondi]

Hi Kieran

On Fri, May 01, 2026 at 04:39:08PM +0100, Kieran Bingham wrote:
> Lens shading is a characteristic which is specific to the lens of a
> given module.
>
> Separate the Lens Shading Calibration registers from the init_setting
> to identify the registers which must be updated when changing a lens.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> ---
>
> The ordering of the table entries here are maintained and not resorted
> (i.e. that first line) to make it easy to confirm that no adjustment is
> made to the data here.
>
> I've also moved the LSC 'above' the init table as otherwise the diff
> becomes unreviewable - as it shows a different hunk being conceptually
> moved instead and hides the ability to see what is moving.
>
> Though that itself also confirms that the values don't change here, so
> I'm happy to move it down if requested.
> ---
>  drivers/media/i2c/ov5640.c | 51 +++++++++++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index e1e253730206..b4e1ec4364df 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -522,6 +522,31 @@ static const struct v4l2_mbus_framefmt ov5640_dvp_default_fmt = {
>  	.field = V4L2_FIELD_NONE,
>  };
>
> +static const struct reg_value ov5640_lsc[] = {
> +	/* Lens Shading Correction */
> +	{0x5800, 0x23, 0, 0}, {0x5801, 0x14, 0, 0},
> +	{0x5802, 0x0f, 0, 0}, {0x5803, 0x0f, 0, 0}, {0x5804, 0x12, 0, 0},
> +	{0x5805, 0x26, 0, 0}, {0x5806, 0x0c, 0, 0}, {0x5807, 0x08, 0, 0},
> +	{0x5808, 0x05, 0, 0}, {0x5809, 0x05, 0, 0}, {0x580a, 0x08, 0, 0},
> +	{0x580b, 0x0d, 0, 0}, {0x580c, 0x08, 0, 0}, {0x580d, 0x03, 0, 0},
> +	{0x580e, 0x00, 0, 0}, {0x580f, 0x00, 0, 0}, {0x5810, 0x03, 0, 0},
> +	{0x5811, 0x09, 0, 0}, {0x5812, 0x07, 0, 0}, {0x5813, 0x03, 0, 0},
> +	{0x5814, 0x00, 0, 0}, {0x5815, 0x01, 0, 0}, {0x5816, 0x03, 0, 0},
> +	{0x5817, 0x08, 0, 0}, {0x5818, 0x0d, 0, 0}, {0x5819, 0x08, 0, 0},
> +	{0x581a, 0x05, 0, 0}, {0x581b, 0x06, 0, 0}, {0x581c, 0x08, 0, 0},
> +	{0x581d, 0x0e, 0, 0}, {0x581e, 0x29, 0, 0}, {0x581f, 0x17, 0, 0},
> +	{0x5820, 0x11, 0, 0}, {0x5821, 0x11, 0, 0}, {0x5822, 0x15, 0, 0},
> +	{0x5823, 0x28, 0, 0}, {0x5824, 0x46, 0, 0}, {0x5825, 0x26, 0, 0},
> +	{0x5826, 0x08, 0, 0}, {0x5827, 0x26, 0, 0}, {0x5828, 0x64, 0, 0},
> +	{0x5829, 0x26, 0, 0}, {0x582a, 0x24, 0, 0}, {0x582b, 0x22, 0, 0},
> +	{0x582c, 0x24, 0, 0}, {0x582d, 0x24, 0, 0}, {0x582e, 0x06, 0, 0},
> +	{0x582f, 0x22, 0, 0}, {0x5830, 0x40, 0, 0}, {0x5831, 0x42, 0, 0},
> +	{0x5832, 0x24, 0, 0}, {0x5833, 0x26, 0, 0}, {0x5834, 0x24, 0, 0},
> +	{0x5835, 0x22, 0, 0}, {0x5836, 0x22, 0, 0}, {0x5837, 0x26, 0, 0},
> +	{0x5838, 0x44, 0, 0}, {0x5839, 0x24, 0, 0}, {0x583a, 0x26, 0, 0},
> +	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> +};
> +
>  static const struct reg_value ov5640_init_setting[] = {
>  	{0x3103, 0x11, 0, 0},
>  	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
> @@ -574,27 +599,8 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x548d, 0xcd, 0, 0}, {0x548e, 0xdd, 0, 0}, {0x548f, 0xea, 0, 0},
>  	{0x5490, 0x1d, 0, 0}, {0x5580, 0x02, 0, 0}, {0x5583, 0x40, 0, 0},
>  	{0x5584, 0x10, 0, 0}, {0x5589, 0x10, 0, 0}, {0x558a, 0x00, 0, 0},
> -	{0x558b, 0xf8, 0, 0}, {0x5800, 0x23, 0, 0}, {0x5801, 0x14, 0, 0},
> -	{0x5802, 0x0f, 0, 0}, {0x5803, 0x0f, 0, 0}, {0x5804, 0x12, 0, 0},
> -	{0x5805, 0x26, 0, 0}, {0x5806, 0x0c, 0, 0}, {0x5807, 0x08, 0, 0},
> -	{0x5808, 0x05, 0, 0}, {0x5809, 0x05, 0, 0}, {0x580a, 0x08, 0, 0},
> -	{0x580b, 0x0d, 0, 0}, {0x580c, 0x08, 0, 0}, {0x580d, 0x03, 0, 0},
> -	{0x580e, 0x00, 0, 0}, {0x580f, 0x00, 0, 0}, {0x5810, 0x03, 0, 0},
> -	{0x5811, 0x09, 0, 0}, {0x5812, 0x07, 0, 0}, {0x5813, 0x03, 0, 0},
> -	{0x5814, 0x00, 0, 0}, {0x5815, 0x01, 0, 0}, {0x5816, 0x03, 0, 0},
> -	{0x5817, 0x08, 0, 0}, {0x5818, 0x0d, 0, 0}, {0x5819, 0x08, 0, 0},
> -	{0x581a, 0x05, 0, 0}, {0x581b, 0x06, 0, 0}, {0x581c, 0x08, 0, 0},
> -	{0x581d, 0x0e, 0, 0}, {0x581e, 0x29, 0, 0}, {0x581f, 0x17, 0, 0},
> -	{0x5820, 0x11, 0, 0}, {0x5821, 0x11, 0, 0}, {0x5822, 0x15, 0, 0},
> -	{0x5823, 0x28, 0, 0}, {0x5824, 0x46, 0, 0}, {0x5825, 0x26, 0, 0},
> -	{0x5826, 0x08, 0, 0}, {0x5827, 0x26, 0, 0}, {0x5828, 0x64, 0, 0},
> -	{0x5829, 0x26, 0, 0}, {0x582a, 0x24, 0, 0}, {0x582b, 0x22, 0, 0},
> -	{0x582c, 0x24, 0, 0}, {0x582d, 0x24, 0, 0}, {0x582e, 0x06, 0, 0},
> -	{0x582f, 0x22, 0, 0}, {0x5830, 0x40, 0, 0}, {0x5831, 0x42, 0, 0},
> -	{0x5832, 0x24, 0, 0}, {0x5833, 0x26, 0, 0}, {0x5834, 0x24, 0, 0},
> -	{0x5835, 0x22, 0, 0}, {0x5836, 0x22, 0, 0}, {0x5837, 0x26, 0, 0},
> -	{0x5838, 0x44, 0, 0}, {0x5839, 0x24, 0, 0}, {0x583a, 0x26, 0, 0},
> -	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> +	{0x558b, 0xf8, 0, 0},
> +

I'm wondering if the empty line here is intentional.
Regardless:
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
>  	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
>  	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> @@ -2396,6 +2402,9 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
>  	ov5640_load_regs(sensor, ov5640_init_setting,
>  			 ARRAY_SIZE(ov5640_init_setting));
>
> +	/* Load the Lens Shading Correction Table */
> +	ov5640_load_regs(sensor, ov5640_lsc, ARRAY_SIZE(ov5640_lsc));
> +
>  	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
>  			     (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
>  			     ilog2(OV5640_SCLK_ROOT_DIV));
>
> --
> 2.52.0
>
>

Re: [PATCH 08/11] media: i2c: ov5640: Document AWB control registers

[Jacopo Mondi]

On Fri, May 01, 2026 at 04:39:10PM +0100, Kieran Bingham wrote:
> Identify and map the registers that are controlling the AWB and
> document their current impact inline in the register set.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 61 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 4b6804fc47e1..34fe7f51e17b 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -112,6 +112,34 @@
>  #define OV5640_REG_PCLK_PERIOD		0x4837
>  #define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
>  #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
> +
> +#define OV5640_REG_AWB_CONTROL_00	0x5180 /* AWB B block */
> +#define OV5640_REG_AWB_CONTROL_01	0x5181 /* AWB Step and Slope control */
> +#define OV5640_REG_AWB_CONTROL_02	0x5182 /* 7:4 Max local counter 3:0 mas fast counter */

s/mas/max ?

> +#define OV5640_REG_AWB_CONTROL_03	0x5183 /* AWB Simple/Advanced control */
> +#define OV5640_REG_AWB_CONTROL_04	0x5184 /* Count and G enable */
> +#define OV5640_REG_AWB_CONTROL_05	0x5185 /* Stable Range Thresholds */
> +
> +#define OV5640_REG_AWB_CONTROL_17	0x5191 /* AWB Top limit */
> +#define OV5640_REG_AWB_CONTROL_18	0x5192 /* AWB Bottom limit */
> +#define OV5640_REG_AWB_CONTROL_19	0x5193 /* Red limit */
> +#define OV5640_REG_AWB_CONTROL_20	0x5194 /* Green limit */
> +#define OV5640_REG_AWB_CONTROL_21	0x5195 /* Blue limit */
> +
> +#define OV5640_REG_AWB_CONTROL_22	0x5196 /* AWB Freeze and Simple Selection */
> +#define OV5640_AWB_FREEZE		BIT(5) /* AWB freeze */
> +#define OV5640_AWB_SIMPLE_SELECT_MASK	GENMASK(3, 2)
> +#define OV5640_AWB_SIMPLE_AFTER_AWB_0	0 /* AWB simple from after AWB gain */
> +#define OV5640_AWB_SIMPLE_AFTER_GMA_0	1 /* AWB simple from after RAW GMA */
> +#define OV5640_AWB_SIMPLE_AFTER_GMA_1	2 /* AWB simple from after RAW GMA */

That's weird, but that's what the datasheet describes, yes

> +#define OV5640_AWB_SIMPLE_AFTER_AWB_1	3 /* AWB simple from after AWB gain */
> +#define OV5640_AWB_FAST_ENABLE		BIT(1) /* AWB fast enable */
> +#define OV5640_AWB_BIAS_STAT		BIT(0)
> +
> +#define OV5640_REG_AWB_CONTROL_23	0x5197 /* Local Limit */
> +
> +#define OV5640_REG_AWB_CONTROL_30	0x519e /* Local limit and Stable Select */
> +
>  #define OV5640_REG_SDE_CTRL0		0x5580
>  #define OV5640_REG_SDE_CTRL1		0x5581
>  #define OV5640_REG_SDE_CTRL3		0x5583
> @@ -576,12 +604,14 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0},
>
>  	/* AWB Control */
> -	{0x5180, 0xff, 0, 0},
> -	{0x5181, 0xf2, 0, 0},
> -	{0x5182, 0x00, 0, 0},
> -	{0x5183, 0x14, 0, 0},
> -	{0x5184, 0x25, 0, 0},
> -	{0x5185, 0x24, 0, 0},
> +	{OV5640_REG_AWB_CONTROL_00, 0xff, 0, 0}, /* AWB B Block */
> +	{OV5640_REG_AWB_CONTROL_01, 0xf2, 0, 0}, /* Step and Slope  - one zone, 0 slope, step fast=step local = 3 */
> +	{OV5640_REG_AWB_CONTROL_02, 0x00, 0, 0}, /* Local/Fast counters @ 0 */
> +	{OV5640_REG_AWB_CONTROL_03, 0x14, 0, 0}, /* Advanced AWB: AWB SIMF, AWB Win = 1 */
> +	{OV5640_REG_AWB_CONTROL_04, 0x25, 0, 0}, /* G-Enable, Count-limit=1, count threshold=1 */
> +	{OV5640_REG_AWB_CONTROL_05, 0x24, 0, 0}, /* Stable Ranges: Threshold for [7:4] unstable to stable [3:0] stable to unstable */
> +
> +	/* AWB Advanced Control - Undocumented */
>  	{0x5186, 0x09, 0, 0},
>  	{0x5187, 0x09, 0, 0},
>  	{0x5188, 0x09, 0, 0},
> @@ -593,20 +623,23 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x518e, 0x34, 0, 0},
>  	{0x518f, 0x6b, 0, 0},
>  	{0x5190, 0x46, 0, 0},
> -	{0x5191, 0xf8, 0, 0},
> -	{0x5192, 0x04, 0, 0},
> -	{0x5193, 0x70, 0, 0},
> -	{0x5194, 0xf0, 0, 0},
> -	{0x5195, 0xf0, 0, 0},
> -	{0x5196, 0x03, 0, 0},
> -	{0x5197, 0x01, 0, 0},
> +
> +	{OV5640_REG_AWB_CONTROL_17, 0xf8, 0, 0}, /* AWB Top limit (Default 0xff)*/
> +	{OV5640_REG_AWB_CONTROL_18, 0x04, 0, 0}, /* AWB Bottom limit (Default 0x00) */
> +	{OV5640_REG_AWB_CONTROL_19, 0x70, 0, 0}, /* Red limit (Default 0xf0) */
> +	{OV5640_REG_AWB_CONTROL_20, 0xf0, 0, 0}, /* Green Limit (Default 0xf0) */
> +	{OV5640_REG_AWB_CONTROL_21, 0xf0, 0, 0}, /* Blue limit (Default 0xf0) */
> +	{OV5640_REG_AWB_CONTROL_22, 0x03, 0, 0}, /* AWB after AWB gain; Fast enable; Bias stat; */
> +	{OV5640_REG_AWB_CONTROL_23, 0x01, 0, 0}, /* Local limit (Default 0x02) */
> +
> +	/* Debug mode - Undocumented */
>  	{0x5198, 0x04, 0, 0},
>  	{0x5199, 0x6c, 0, 0},
>  	{0x519a, 0x04, 0, 0},
>  	{0x519b, 0x00, 0, 0},
>  	{0x519c, 0x09, 0, 0},
>  	{0x519d, 0x2b, 0, 0},
> -	{0x519e, 0x38, 0, 0},
> +	{OV5640_REG_AWB_CONTROL_30, 0x38, 0, 0}, /* [7:4] Debug = 3; [3] Local Limit Select = 1; [2] Simple stable select=0; [1:0] Debug=0 */

I'm happy to go over line limit for better documentation.

I anticipate someone might ask to place the comment in the line above
to reduce the line length, but I'm not going to be that person :)

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
>  	{0x5381, 0x1e, 0, 0}, {0x5382, 0x5b, 0, 0}, {0x5383, 0x08, 0, 0},
>  	{0x5384, 0x0a, 0, 0}, {0x5385, 0x7e, 0, 0}, {0x5386, 0x88, 0, 0},
>
> --
> 2.52.0
>
>

Re: [PATCH 08/11] media: i2c: ov5640: Document AWB control registers

[Jacopo Mondi]

Ah sorry,
  I would squash this with the previous one. If you prefer to keep
  them separate, add my tag to 07 as well.

On Fri, May 01, 2026 at 04:39:10PM +0100, Kieran Bingham wrote:
> Identify and map the registers that are controlling the AWB and
> document their current impact inline in the register set.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 61 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 4b6804fc47e1..34fe7f51e17b 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -112,6 +112,34 @@
>  #define OV5640_REG_PCLK_PERIOD		0x4837
>  #define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
>  #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
> +
> +#define OV5640_REG_AWB_CONTROL_00	0x5180 /* AWB B block */
> +#define OV5640_REG_AWB_CONTROL_01	0x5181 /* AWB Step and Slope control */
> +#define OV5640_REG_AWB_CONTROL_02	0x5182 /* 7:4 Max local counter 3:0 mas fast counter */
> +#define OV5640_REG_AWB_CONTROL_03	0x5183 /* AWB Simple/Advanced control */
> +#define OV5640_REG_AWB_CONTROL_04	0x5184 /* Count and G enable */
> +#define OV5640_REG_AWB_CONTROL_05	0x5185 /* Stable Range Thresholds */
> +
> +#define OV5640_REG_AWB_CONTROL_17	0x5191 /* AWB Top limit */
> +#define OV5640_REG_AWB_CONTROL_18	0x5192 /* AWB Bottom limit */
> +#define OV5640_REG_AWB_CONTROL_19	0x5193 /* Red limit */
> +#define OV5640_REG_AWB_CONTROL_20	0x5194 /* Green limit */
> +#define OV5640_REG_AWB_CONTROL_21	0x5195 /* Blue limit */
> +
> +#define OV5640_REG_AWB_CONTROL_22	0x5196 /* AWB Freeze and Simple Selection */
> +#define OV5640_AWB_FREEZE		BIT(5) /* AWB freeze */
> +#define OV5640_AWB_SIMPLE_SELECT_MASK	GENMASK(3, 2)
> +#define OV5640_AWB_SIMPLE_AFTER_AWB_0	0 /* AWB simple from after AWB gain */
> +#define OV5640_AWB_SIMPLE_AFTER_GMA_0	1 /* AWB simple from after RAW GMA */
> +#define OV5640_AWB_SIMPLE_AFTER_GMA_1	2 /* AWB simple from after RAW GMA */
> +#define OV5640_AWB_SIMPLE_AFTER_AWB_1	3 /* AWB simple from after AWB gain */
> +#define OV5640_AWB_FAST_ENABLE		BIT(1) /* AWB fast enable */
> +#define OV5640_AWB_BIAS_STAT		BIT(0)
> +
> +#define OV5640_REG_AWB_CONTROL_23	0x5197 /* Local Limit */
> +
> +#define OV5640_REG_AWB_CONTROL_30	0x519e /* Local limit and Stable Select */
> +
>  #define OV5640_REG_SDE_CTRL0		0x5580
>  #define OV5640_REG_SDE_CTRL1		0x5581
>  #define OV5640_REG_SDE_CTRL3		0x5583
> @@ -576,12 +604,14 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0},
>
>  	/* AWB Control */
> -	{0x5180, 0xff, 0, 0},
> -	{0x5181, 0xf2, 0, 0},
> -	{0x5182, 0x00, 0, 0},
> -	{0x5183, 0x14, 0, 0},
> -	{0x5184, 0x25, 0, 0},
> -	{0x5185, 0x24, 0, 0},
> +	{OV5640_REG_AWB_CONTROL_00, 0xff, 0, 0}, /* AWB B Block */
> +	{OV5640_REG_AWB_CONTROL_01, 0xf2, 0, 0}, /* Step and Slope  - one zone, 0 slope, step fast=step local = 3 */
> +	{OV5640_REG_AWB_CONTROL_02, 0x00, 0, 0}, /* Local/Fast counters @ 0 */
> +	{OV5640_REG_AWB_CONTROL_03, 0x14, 0, 0}, /* Advanced AWB: AWB SIMF, AWB Win = 1 */
> +	{OV5640_REG_AWB_CONTROL_04, 0x25, 0, 0}, /* G-Enable, Count-limit=1, count threshold=1 */
> +	{OV5640_REG_AWB_CONTROL_05, 0x24, 0, 0}, /* Stable Ranges: Threshold for [7:4] unstable to stable [3:0] stable to unstable */
> +
> +	/* AWB Advanced Control - Undocumented */
>  	{0x5186, 0x09, 0, 0},
>  	{0x5187, 0x09, 0, 0},
>  	{0x5188, 0x09, 0, 0},
> @@ -593,20 +623,23 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x518e, 0x34, 0, 0},
>  	{0x518f, 0x6b, 0, 0},
>  	{0x5190, 0x46, 0, 0},
> -	{0x5191, 0xf8, 0, 0},
> -	{0x5192, 0x04, 0, 0},
> -	{0x5193, 0x70, 0, 0},
> -	{0x5194, 0xf0, 0, 0},
> -	{0x5195, 0xf0, 0, 0},
> -	{0x5196, 0x03, 0, 0},
> -	{0x5197, 0x01, 0, 0},
> +
> +	{OV5640_REG_AWB_CONTROL_17, 0xf8, 0, 0}, /* AWB Top limit (Default 0xff)*/
> +	{OV5640_REG_AWB_CONTROL_18, 0x04, 0, 0}, /* AWB Bottom limit (Default 0x00) */
> +	{OV5640_REG_AWB_CONTROL_19, 0x70, 0, 0}, /* Red limit (Default 0xf0) */
> +	{OV5640_REG_AWB_CONTROL_20, 0xf0, 0, 0}, /* Green Limit (Default 0xf0) */
> +	{OV5640_REG_AWB_CONTROL_21, 0xf0, 0, 0}, /* Blue limit (Default 0xf0) */
> +	{OV5640_REG_AWB_CONTROL_22, 0x03, 0, 0}, /* AWB after AWB gain; Fast enable; Bias stat; */
> +	{OV5640_REG_AWB_CONTROL_23, 0x01, 0, 0}, /* Local limit (Default 0x02) */
> +
> +	/* Debug mode - Undocumented */
>  	{0x5198, 0x04, 0, 0},
>  	{0x5199, 0x6c, 0, 0},
>  	{0x519a, 0x04, 0, 0},
>  	{0x519b, 0x00, 0, 0},
>  	{0x519c, 0x09, 0, 0},
>  	{0x519d, 0x2b, 0, 0},
> -	{0x519e, 0x38, 0, 0},
> +	{OV5640_REG_AWB_CONTROL_30, 0x38, 0, 0}, /* [7:4] Debug = 3; [3] Local Limit Select = 1; [2] Simple stable select=0; [1:0] Debug=0 */
>
>  	{0x5381, 0x1e, 0, 0}, {0x5382, 0x5b, 0, 0}, {0x5383, 0x08, 0, 0},
>  	{0x5384, 0x0a, 0, 0}, {0x5385, 0x7e, 0, 0}, {0x5386, 0x88, 0, 0},
>
> --
> 2.52.0
>
>

Re: [PATCH 09/11] media: i2c: ov5640: Add ISP Control registers

[Jacopo Mondi]

Hi Kieran

On Fri, May 01, 2026 at 04:39:11PM +0100, Kieran Bingham wrote:
> Define the bits for the ISP control register to be able to use
> and explain component enablement.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 34fe7f51e17b..fd369a13463e 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -110,6 +110,21 @@
>  #define OV5640_REG_MIPI_CTRL00		0x4800
>  #define OV5640_REG_DEBUG_MODE		0x4814
>  #define OV5640_REG_PCLK_PERIOD		0x4837
> +
> +#define OV5640_REG_ISP_CTRL00		0x5000
> +#define OV5640_ISP_00_LENC_ENABLE	BIT(7)
> +#define OV5640_ISP_00_GMA_ENABLE	BIT(5)
> +#define OV5640_ISP_00_BPC_ENABLE	BIT(2)
> +#define OV5640_ISP_00_WPC_ENABLE	BIT(1)
> +#define OV5640_ISP_00_CIP_ENABLE	BIT(0)
> +
> +#define OV5640_REG_ISP_CTRL01		0x5001
> +#define OV5640_ISP_01_SDE_ENABLE	BIT(7)
> +#define OV5640_ISP_01_SCL_ENABLE	BIT(5)
> +#define OV5640_ISP_01_UVA_ENABLE	BIT(2)
> +#define OV5640_ISP_01_CMX_ENABLE	BIT(1)
> +#define OV5640_ISP_01_AWB_ENABLE	BIT(0)
> +

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  #define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
>  #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
>
> @@ -601,7 +616,10 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
>  	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>  	{0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
> -	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0},
> +
> +	/* ISP Control */
> +	{OV5640_REG_ISP_CTRL00, 0xa7, 0, 0},
> +	{OV5640_REG_ISP_CTRL01, 0xa3, 0, 0},
>
>  	/* AWB Control */
>  	{OV5640_REG_AWB_CONTROL_00, 0xff, 0, 0}, /* AWB B Block */
>
> --
> 2.52.0
>
>

Re: [PATCH 10/11] media: i2c: ov5640: Disable ISP for raw output

[Jacopo Mondi]

Hi Kieran

On Fri, May 01, 2026 at 04:39:12PM +0100, Kieran Bingham wrote:
> The OV5640 has ISP operations that can run even when outputting RAW
> bayer data.  These include the Lens Shading Correction, Gamma and Auto
> white balance which need to be disabled when performing module
> calibration using RAW data.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index fd369a13463e..f63d81640f54 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -280,28 +280,28 @@ static const struct ov5640_pixfmt ov5640_dvp_formats[] = {
>  	}, {
>  		/* Raw, BGBG... / GRGR... */
>  		.code		= MEDIA_BUS_FMT_SBGGR8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.colorspace	= V4L2_COLORSPACE_RAW,
>  		.bpp		= 8,
>  		.ctrl00		= 0x00,
>  		.mux		= OV5640_FMT_MUX_RAW_DPC,
>  	}, {
>  		/* Raw bayer, GBGB... / RGRG... */
>  		.code		= MEDIA_BUS_FMT_SGBRG8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.colorspace	= V4L2_COLORSPACE_RAW,
>  		.bpp		= 8,
>  		.ctrl00		= 0x01,
>  		.mux		= OV5640_FMT_MUX_RAW_DPC,
>  	}, {
>  		/* Raw bayer, GRGR... / BGBG... */
>  		.code		= MEDIA_BUS_FMT_SGRBG8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.colorspace	= V4L2_COLORSPACE_RAW,
>  		.bpp		= 8,
>  		.ctrl00		= 0x02,
>  		.mux		= OV5640_FMT_MUX_RAW_DPC,
>  	}, {
>  		/* Raw bayer, RGRG... / GBGB... */
>  		.code		= MEDIA_BUS_FMT_SRGGB8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.colorspace	= V4L2_COLORSPACE_RAW,
>  		.bpp		= 8,
>  		.ctrl00		= 0x03,
>  		.mux		= OV5640_FMT_MUX_RAW_DPC,
> @@ -348,7 +348,7 @@ static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
>  	}, {
>  		/* Raw, BGBG... / GRGR... */
>  		.code		= MEDIA_BUS_FMT_SBGGR8_1X8,
> -		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.colorspace	= V4L2_COLORSPACE_RAW,
>  		.bpp		= 8,
>  		.ctrl00		= 0x00,
>  		.mux		= OV5640_FMT_MUX_RAW_DPC,
> @@ -473,6 +473,7 @@ struct ov5640_dev {
>
>  	struct v4l2_mbus_framefmt fmt;
>  	bool pending_fmt_change;
> +	bool is_raw;
>
>  	const struct ov5640_mode_info *current_mode;
>  	const struct ov5640_mode_info *last_mode;
> @@ -618,8 +619,13 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
>
>  	/* ISP Control */
> -	{OV5640_REG_ISP_CTRL00, 0xa7, 0, 0},
> -	{OV5640_REG_ISP_CTRL01, 0xa3, 0, 0},
> +	{OV5640_REG_ISP_CTRL00, OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> +				OV5640_ISP_00_BPC_ENABLE | OV5640_ISP_00_WPC_ENABLE |
> +				OV5640_ISP_00_CIP_ENABLE, 0, 0},
> +
> +	/* OV5640_ISP_01_UVA_ENABLE is not enabled */
> +	{OV5640_REG_ISP_CTRL01, OV5640_ISP_01_SDE_ENABLE | OV5640_ISP_01_SCL_ENABLE |
> +				OV5640_ISP_01_CMX_ENABLE | OV5640_ISP_01_AWB_ENABLE, 0, 0},

Should we avoid writing these registers at all in the init sequence ?

>
>  	/* AWB Control */
>  	{OV5640_REG_AWB_CONTROL_00, 0xff, 0, 0}, /* AWB B Block */
> @@ -3116,6 +3122,31 @@ static int ov5640_set_framefmt(struct ov5640_dev *sensor,
>  	if (ret)
>  		return ret;
>
> +	/*
> +	 * Disable all ISP image processing (Lens Shading, Gamma, AWB...) for
> +	 * RAW modes to facilitate module tuning.
> +	 */
> +	sensor->is_raw = pixfmt->colorspace == V4L2_COLORSPACE_RAW;

Unless it is used later on, the 'is_raw' flag can be kept a local
variable.

> +	if (sensor->is_raw) {
> +		ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL00, 0);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL00,
> +				       OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> +				       OV5640_ISP_00_BPC_ENABLE | OV5640_ISP_00_WPC_ENABLE |
> +				       OV5640_ISP_00_CIP_ENABLE);
> +		if (ret)
> +			return ret;
> +
> +		/* OV5640_ISP_01_UVA_ENABLE is not enabled */
> +		ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL01,
> +				       OV5640_ISP_01_SDE_ENABLE | OV5640_ISP_01_SCL_ENABLE |
> +				       OV5640_ISP_01_CMX_ENABLE | OV5640_ISP_01_AWB_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * TIMING TC REG21:
>  	 * - [5]:	JPEG enable
>
> --
> 2.52.0
>
>

Re: [PATCH 11/11] media: i2c: ov5640: Split out format mux registers

[Jacopo Mondi]

On Fri, May 01, 2026 at 04:39:13PM +0100, Kieran Bingham wrote:
> The init_setting table configures format control and mux to default for
> YUV420 output.  These are later determined by the users format
> selection, and so are candidates for removal from the init table.

Can we remove them then :) ?

>
> Split the additions out from the register block and document them.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index f63d81640f54..477f6a3ddbf6 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -615,8 +615,13 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
>  	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
>  	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
> -	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> -	{0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
> +	{0x302e, 0x08, 0, 0},
> +
> +	/* Format control, Mux control */
> +	{0x4300, 0x3f, 0, 0}, /* YUV420 YYYY... / UYVY... */
> +	{0x501f, 0x00, 0, 0}, /* ISP YUV422 */
> +
> +	{0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
>
>  	/* ISP Control */
>  	{OV5640_REG_ISP_CTRL00, OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
>
> --
> 2.52.0
>
>

Re: [PATCH 05/11] media: i2c: ov5640: Remove unsupported bayer orders

[Kieran Bingham]

Quoting Jacopo Mondi (2026-05-14 09:10:48)
> Hi Kieran
> 
> On Fri, May 01, 2026 at 04:39:07PM +0100, Kieran Bingham wrote:
> > The OV5640 only outputs SBGGR8. Remove the incorrectly advertised
> > alternatives which allow a misconfigured pipeline to be established.
> 
> Do you have any idea why the datasheet mentions all the RGGB
> permutations as valid outputs ?

I would anticipate it's because the internal ISP component can support them.

But now I think more I have to admit, I have no idea if other modules
might have different physical orderings.

But certainly as far as I can tell any given module can only have a
single 'correct' value here.

So we'll have to try to work out how to confirm/verify that perhaps ?

> 
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 21 ---------------------
> >  1 file changed, 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 244c341d0e77..e1e253730206 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -309,27 +309,6 @@ static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
> >               .bpp            = 8,
> >               .ctrl00         = 0x00,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> > -     }, {
> > -             /* Raw bayer, GBGB... / RGRG... */
> > -             .code           = MEDIA_BUS_FMT_SGBRG8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > -             .bpp            = 8,
> > -             .ctrl00         = 0x01,
> > -             .mux            = OV5640_FMT_MUX_RAW_DPC,
> > -     }, {
> > -             /* Raw bayer, GRGR... / BGBG... */
> > -             .code           = MEDIA_BUS_FMT_SGRBG8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > -             .bpp            = 8,
> > -             .ctrl00         = 0x02,
> > -             .mux            = OV5640_FMT_MUX_RAW_DPC,
> > -     }, {
> > -             /* Raw bayer, RGRG... / GBGB... */
> > -             .code           = MEDIA_BUS_FMT_SRGGB8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > -             .bpp            = 8,
> > -             .ctrl00         = 0x03,
> > -             .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       },
> 
> Seems like you've missed the same entries in the ov5640_dvp_formats[]
> table.

That was intentional so far as I can only test the CSI variant. But
perhaps for the same reasons mentioned above, whatever happens on one
will be the same on the other.

I hope posting this will find some wider testers 'perhaps' if anyone
cares.

--
Kieran.

> 
> >       { /* sentinel */ }
> >  };
> >
> > --
> > 2.52.0
> >
> >

Re: [PATCH 06/11] media: i2c: ov5640: split out the LSC registers

[Kieran Bingham]

Quoting Jacopo Mondi (2026-05-14 09:23:50)
> Hi Kieran
> 
> On Fri, May 01, 2026 at 04:39:08PM +0100, Kieran Bingham wrote:
> > Lens shading is a characteristic which is specific to the lens of a
> > given module.
> >
> > Separate the Lens Shading Calibration registers from the init_setting
> > to identify the registers which must be updated when changing a lens.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > ---
> >
> > The ordering of the table entries here are maintained and not resorted
> > (i.e. that first line) to make it easy to confirm that no adjustment is
> > made to the data here.
> >
> > I've also moved the LSC 'above' the init table as otherwise the diff
> > becomes unreviewable - as it shows a different hunk being conceptually
> > moved instead and hides the ability to see what is moving.
> >
> > Though that itself also confirms that the values don't change here, so
> > I'm happy to move it down if requested.
> > ---
> >  drivers/media/i2c/ov5640.c | 51 +++++++++++++++++++++++++++-------------------
> >  1 file changed, 30 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index e1e253730206..b4e1ec4364df 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -522,6 +522,31 @@ static const struct v4l2_mbus_framefmt ov5640_dvp_default_fmt = {
> >       .field = V4L2_FIELD_NONE,
> >  };
> >
> > +static const struct reg_value ov5640_lsc[] = {
> > +     /* Lens Shading Correction */
> > +     {0x5800, 0x23, 0, 0}, {0x5801, 0x14, 0, 0},
> > +     {0x5802, 0x0f, 0, 0}, {0x5803, 0x0f, 0, 0}, {0x5804, 0x12, 0, 0},
> > +     {0x5805, 0x26, 0, 0}, {0x5806, 0x0c, 0, 0}, {0x5807, 0x08, 0, 0},
> > +     {0x5808, 0x05, 0, 0}, {0x5809, 0x05, 0, 0}, {0x580a, 0x08, 0, 0},
> > +     {0x580b, 0x0d, 0, 0}, {0x580c, 0x08, 0, 0}, {0x580d, 0x03, 0, 0},
> > +     {0x580e, 0x00, 0, 0}, {0x580f, 0x00, 0, 0}, {0x5810, 0x03, 0, 0},
> > +     {0x5811, 0x09, 0, 0}, {0x5812, 0x07, 0, 0}, {0x5813, 0x03, 0, 0},
> > +     {0x5814, 0x00, 0, 0}, {0x5815, 0x01, 0, 0}, {0x5816, 0x03, 0, 0},
> > +     {0x5817, 0x08, 0, 0}, {0x5818, 0x0d, 0, 0}, {0x5819, 0x08, 0, 0},
> > +     {0x581a, 0x05, 0, 0}, {0x581b, 0x06, 0, 0}, {0x581c, 0x08, 0, 0},
> > +     {0x581d, 0x0e, 0, 0}, {0x581e, 0x29, 0, 0}, {0x581f, 0x17, 0, 0},
> > +     {0x5820, 0x11, 0, 0}, {0x5821, 0x11, 0, 0}, {0x5822, 0x15, 0, 0},
> > +     {0x5823, 0x28, 0, 0}, {0x5824, 0x46, 0, 0}, {0x5825, 0x26, 0, 0},
> > +     {0x5826, 0x08, 0, 0}, {0x5827, 0x26, 0, 0}, {0x5828, 0x64, 0, 0},
> > +     {0x5829, 0x26, 0, 0}, {0x582a, 0x24, 0, 0}, {0x582b, 0x22, 0, 0},
> > +     {0x582c, 0x24, 0, 0}, {0x582d, 0x24, 0, 0}, {0x582e, 0x06, 0, 0},
> > +     {0x582f, 0x22, 0, 0}, {0x5830, 0x40, 0, 0}, {0x5831, 0x42, 0, 0},
> > +     {0x5832, 0x24, 0, 0}, {0x5833, 0x26, 0, 0}, {0x5834, 0x24, 0, 0},
> > +     {0x5835, 0x22, 0, 0}, {0x5836, 0x22, 0, 0}, {0x5837, 0x26, 0, 0},
> > +     {0x5838, 0x44, 0, 0}, {0x5839, 0x24, 0, 0}, {0x583a, 0x26, 0, 0},
> > +     {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> > +};
> > +
> >  static const struct reg_value ov5640_init_setting[] = {
> >       {0x3103, 0x11, 0, 0},
> >       {0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
> > @@ -574,27 +599,8 @@ static const struct reg_value ov5640_init_setting[] = {
> >       {0x548d, 0xcd, 0, 0}, {0x548e, 0xdd, 0, 0}, {0x548f, 0xea, 0, 0},
> >       {0x5490, 0x1d, 0, 0}, {0x5580, 0x02, 0, 0}, {0x5583, 0x40, 0, 0},
> >       {0x5584, 0x10, 0, 0}, {0x5589, 0x10, 0, 0}, {0x558a, 0x00, 0, 0},
> > -     {0x558b, 0xf8, 0, 0}, {0x5800, 0x23, 0, 0}, {0x5801, 0x14, 0, 0},
> > -     {0x5802, 0x0f, 0, 0}, {0x5803, 0x0f, 0, 0}, {0x5804, 0x12, 0, 0},
> > -     {0x5805, 0x26, 0, 0}, {0x5806, 0x0c, 0, 0}, {0x5807, 0x08, 0, 0},
> > -     {0x5808, 0x05, 0, 0}, {0x5809, 0x05, 0, 0}, {0x580a, 0x08, 0, 0},
> > -     {0x580b, 0x0d, 0, 0}, {0x580c, 0x08, 0, 0}, {0x580d, 0x03, 0, 0},
> > -     {0x580e, 0x00, 0, 0}, {0x580f, 0x00, 0, 0}, {0x5810, 0x03, 0, 0},
> > -     {0x5811, 0x09, 0, 0}, {0x5812, 0x07, 0, 0}, {0x5813, 0x03, 0, 0},
> > -     {0x5814, 0x00, 0, 0}, {0x5815, 0x01, 0, 0}, {0x5816, 0x03, 0, 0},
> > -     {0x5817, 0x08, 0, 0}, {0x5818, 0x0d, 0, 0}, {0x5819, 0x08, 0, 0},
> > -     {0x581a, 0x05, 0, 0}, {0x581b, 0x06, 0, 0}, {0x581c, 0x08, 0, 0},
> > -     {0x581d, 0x0e, 0, 0}, {0x581e, 0x29, 0, 0}, {0x581f, 0x17, 0, 0},
> > -     {0x5820, 0x11, 0, 0}, {0x5821, 0x11, 0, 0}, {0x5822, 0x15, 0, 0},
> > -     {0x5823, 0x28, 0, 0}, {0x5824, 0x46, 0, 0}, {0x5825, 0x26, 0, 0},
> > -     {0x5826, 0x08, 0, 0}, {0x5827, 0x26, 0, 0}, {0x5828, 0x64, 0, 0},
> > -     {0x5829, 0x26, 0, 0}, {0x582a, 0x24, 0, 0}, {0x582b, 0x22, 0, 0},
> > -     {0x582c, 0x24, 0, 0}, {0x582d, 0x24, 0, 0}, {0x582e, 0x06, 0, 0},
> > -     {0x582f, 0x22, 0, 0}, {0x5830, 0x40, 0, 0}, {0x5831, 0x42, 0, 0},
> > -     {0x5832, 0x24, 0, 0}, {0x5833, 0x26, 0, 0}, {0x5834, 0x24, 0, 0},
> > -     {0x5835, 0x22, 0, 0}, {0x5836, 0x22, 0, 0}, {0x5837, 0x26, 0, 0},
> > -     {0x5838, 0x44, 0, 0}, {0x5839, 0x24, 0, 0}, {0x583a, 0x26, 0, 0},
> > -     {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> > +     {0x558b, 0xf8, 0, 0},
> > +
> 
> I'm wondering if the empty line here is intentional.

Yes, to start separating the functional blocks. I think we can work a
lot more to break out this whole register table with the full datasheet
being 'public'.


> Regardless:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> >       {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
> >       {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> >       {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> > @@ -2396,6 +2402,9 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
> >       ov5640_load_regs(sensor, ov5640_init_setting,
> >                        ARRAY_SIZE(ov5640_init_setting));
> >
> > +     /* Load the Lens Shading Correction Table */
> > +     ov5640_load_regs(sensor, ov5640_lsc, ARRAY_SIZE(ov5640_lsc));
> > +
> >       ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
> >                            (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
> >                            ilog2(OV5640_SCLK_ROOT_DIV));
> >
> > --
> > 2.52.0
> >
> >

Re: [PATCH 08/11] media: i2c: ov5640: Document AWB control registers

[Kieran Bingham]

Quoting Jacopo Mondi (2026-05-14 09:34:40)
> Ah sorry,
>   I would squash this with the previous one. If you prefer to keep
>   them separate, add my tag to 07 as well.

Ack, I tried to keep things separated so it was clear I wasn't modifying
values while splitting, and then changing the readability. I didn't want
any unintended changes to be disguised.

Happy to squash now it's had some eyes over though.

--
Kieran

Re: [PATCH 10/11] media: i2c: ov5640: Disable ISP for raw output

[Kieran Bingham]

Quoting Jacopo Mondi (2026-05-14 09:39:31)
> Hi Kieran
> 
> On Fri, May 01, 2026 at 04:39:12PM +0100, Kieran Bingham wrote:
> > The OV5640 has ISP operations that can run even when outputting RAW
> > bayer data.  These include the Lens Shading Correction, Gamma and Auto
> > white balance which need to be disabled when performing module
> > calibration using RAW data.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index fd369a13463e..f63d81640f54 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -280,28 +280,28 @@ static const struct ov5640_pixfmt ov5640_dvp_formats[] = {
> >       }, {
> >               /* Raw, BGBG... / GRGR... */
> >               .code           = MEDIA_BUS_FMT_SBGGR8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x00,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       }, {
> >               /* Raw bayer, GBGB... / RGRG... */
> >               .code           = MEDIA_BUS_FMT_SGBRG8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x01,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       }, {
> >               /* Raw bayer, GRGR... / BGBG... */
> >               .code           = MEDIA_BUS_FMT_SGRBG8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x02,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       }, {
> >               /* Raw bayer, RGRG... / GBGB... */
> >               .code           = MEDIA_BUS_FMT_SRGGB8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x03,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> > @@ -348,7 +348,7 @@ static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
> >       }, {
> >               /* Raw, BGBG... / GRGR... */
> >               .code           = MEDIA_BUS_FMT_SBGGR8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x00,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> > @@ -473,6 +473,7 @@ struct ov5640_dev {
> >
> >       struct v4l2_mbus_framefmt fmt;
> >       bool pending_fmt_change;
> > +     bool is_raw;
> >
> >       const struct ov5640_mode_info *current_mode;
> >       const struct ov5640_mode_info *last_mode;
> > @@ -618,8 +619,13 @@ static const struct reg_value ov5640_init_setting[] = {
> >       {0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
> >
> >       /* ISP Control */
> > -     {OV5640_REG_ISP_CTRL00, 0xa7, 0, 0},
> > -     {OV5640_REG_ISP_CTRL01, 0xa3, 0, 0},
> > +     {OV5640_REG_ISP_CTRL00, OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> > +                             OV5640_ISP_00_BPC_ENABLE | OV5640_ISP_00_WPC_ENABLE |
> > +                             OV5640_ISP_00_CIP_ENABLE, 0, 0},
> > +
> > +     /* OV5640_ISP_01_UVA_ENABLE is not enabled */
> > +     {OV5640_REG_ISP_CTRL01, OV5640_ISP_01_SDE_ENABLE | OV5640_ISP_01_SCL_ENABLE |
> > +                             OV5640_ISP_01_CMX_ENABLE | OV5640_ISP_01_AWB_ENABLE, 0, 0},
> 
> Should we avoid writing these registers at all in the init sequence ?

Oh probably. I haven't tested that yet, but indeed.

> >
> >       /* AWB Control */
> >       {OV5640_REG_AWB_CONTROL_00, 0xff, 0, 0}, /* AWB B Block */
> > @@ -3116,6 +3122,31 @@ static int ov5640_set_framefmt(struct ov5640_dev *sensor,
> >       if (ret)
> >               return ret;
> >
> > +     /*
> > +      * Disable all ISP image processing (Lens Shading, Gamma, AWB...) for
> > +      * RAW modes to facilitate module tuning.
> > +      */
> > +     sensor->is_raw = pixfmt->colorspace == V4L2_COLORSPACE_RAW;
> 
> Unless it is used later on, the 'is_raw' flag can be kept a local
> variable.

Haha yes, no idea why I put it in the main dev structure.

Will move. Thanks.

> 
> > +     if (sensor->is_raw) {
> > +             ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL00, 0);
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL00,
> > +                                    OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> > +                                    OV5640_ISP_00_BPC_ENABLE | OV5640_ISP_00_WPC_ENABLE |
> > +                                    OV5640_ISP_00_CIP_ENABLE);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             /* OV5640_ISP_01_UVA_ENABLE is not enabled */
> > +             ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL01,
> > +                                    OV5640_ISP_01_SDE_ENABLE | OV5640_ISP_01_SCL_ENABLE |
> > +                                    OV5640_ISP_01_CMX_ENABLE | OV5640_ISP_01_AWB_ENABLE);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       /*
> >        * TIMING TC REG21:
> >        * - [5]:       JPEG enable
> >
> > --
> > 2.52.0
> >
> >

Re: [PATCH 11/11] media: i2c: ov5640: Split out format mux registers

[Kieran Bingham]

Quoting Jacopo Mondi (2026-05-14 09:41:02)
> 
> On Fri, May 01, 2026 at 04:39:13PM +0100, Kieran Bingham wrote:
> > The init_setting table configures format control and mux to default for
> > YUV420 output.  These are later determined by the users format
> > selection, and so are candidates for removal from the init table.
> 
> Can we remove them then :) ?

Maybe indeed! I'll double check this when I next spin the series.
--
Kieran


> 
> >
> > Split the additions out from the register block and document them.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index f63d81640f54..477f6a3ddbf6 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -615,8 +615,13 @@ static const struct reg_value ov5640_init_setting[] = {
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> >       {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
> >       {0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
> > -     {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > -     {0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
> > +     {0x302e, 0x08, 0, 0},
> > +
> > +     /* Format control, Mux control */
> > +     {0x4300, 0x3f, 0, 0}, /* YUV420 YYYY... / UYVY... */
> > +     {0x501f, 0x00, 0, 0}, /* ISP YUV422 */
> > +
> > +     {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
> >
> >       /* ISP Control */
> >       {OV5640_REG_ISP_CTRL00, OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> >
> > --
> > 2.52.0
> >
> >

Re: [PATCH 05/11] media: i2c: ov5640: Remove unsupported bayer orders

[Jacopo Mondi]

Hi Kieran

On Thu, May 14, 2026 at 09:43:42AM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2026-05-14 09:10:48)
> > Hi Kieran
> >
> > On Fri, May 01, 2026 at 04:39:07PM +0100, Kieran Bingham wrote:
> > > The OV5640 only outputs SBGGR8. Remove the incorrectly advertised
> > > alternatives which allow a misconfigured pipeline to be established.
> >
> > Do you have any idea why the datasheet mentions all the RGGB
> > permutations as valid outputs ?
>
> I would anticipate it's because the internal ISP component can support them.
>

Note that an analogue crop rectangle not aligned with the Bayer macro-pixel
could change the pattern, but the sensor's ISP shouldn't need to be
informed afaiu

> But now I think more I have to admit, I have no idea if other modules
> might have different physical orderings.
>
> But certainly as far as I can tell any given module can only have a
> single 'correct' value here.

analogue crop alignment apart :)

>
> So we'll have to try to work out how to confirm/verify that perhaps ?
>
> >
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/ov5640.c | 21 ---------------------
> > >  1 file changed, 21 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 244c341d0e77..e1e253730206 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -309,27 +309,6 @@ static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
> > >               .bpp            = 8,
> > >               .ctrl00         = 0x00,
> > >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> > > -     }, {
> > > -             /* Raw bayer, GBGB... / RGRG... */
> > > -             .code           = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > > -             .bpp            = 8,
> > > -             .ctrl00         = 0x01,
> > > -             .mux            = OV5640_FMT_MUX_RAW_DPC,
> > > -     }, {
> > > -             /* Raw bayer, GRGR... / BGBG... */
> > > -             .code           = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > > -             .bpp            = 8,
> > > -             .ctrl00         = 0x02,
> > > -             .mux            = OV5640_FMT_MUX_RAW_DPC,
> > > -     }, {
> > > -             /* Raw bayer, RGRG... / GBGB... */
> > > -             .code           = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > > -             .bpp            = 8,
> > > -             .ctrl00         = 0x03,
> > > -             .mux            = OV5640_FMT_MUX_RAW_DPC,
> > >       },
> >
> > Seems like you've missed the same entries in the ov5640_dvp_formats[]
> > table.
>
> That was intentional so far as I can only test the CSI variant. But
> perhaps for the same reasons mentioned above, whatever happens on one
> will be the same on the other.
>
> I hope posting this will find some wider testers 'perhaps' if anyone
> cares.
>
> --
> Kieran.
>
> >
> > >       { /* sentinel */ }
> > >  };
> > >
> > > --
> > > 2.52.0
> > >
> > >