[PATCH 00/11] media: i2c: ov5640: Refactor ISP configuration

[PATCH 00/11] media: i2c: ov5640: Refactor ISP configuration

[Kieran Bingham]

The OV5640 has an internal ISP which has hard coded parameters for the
AWB, LSC, GMA and CMX encoded within the init register table.

This means that the original upstream submission contains module
specific tuning which do not match any subsequent users of the device
with the driver.

This series aims to commence the cleanup and abstraction of the
parameters of the OV5640 ISP to make them configurable and adaptable to
new modules.

Fortuantely a full register datasheet has been leaked publicly to enable
this work. I won't specify where this is here - but it can easily be
found online with your favourite search engines.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Kieran Bingham (11):
      media: i2c: ov5640: Set default WB gains
      media: i2c: ov5640: Set exposure minimum and defaults
      media: i2c: ov5640: Fix minimum gain to 1.0x
      media: i2c: ov5640: fix error path in ov5640_set_mode
      media: i2c: ov5640: Remove unsupported bayer orders
      media: i2c: ov5640: split out the LSC registers
      media: i2c: ov5640: Split out AWB registers
      media: i2c: ov5640: Document AWB control registers
      media: i2c: ov5640: Add ISP Control registers
      media: i2c: ov5640: Disable ISP for raw output
      media: i2c: ov5640: Split out format mux registers

 drivers/media/i2c/ov5640.c | 229 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 164 insertions(+), 65 deletions(-)
---
base-commit: 7666942083e1f5b82dab5fe8df17f2f932aeac7c
change-id: 20260501-ov5640_cleanup-a43fcfdb25fc

Best regards,
-- 
--
Kieran

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

[Kieran Bingham]

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);
 	/* Auto/manual exposure */
 	ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
 						 V4L2_CID_EXPOSURE_AUTO,

-- 
2.52.0

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

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

[Kieran Bingham]

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);
 	/* 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]

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

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

[Kieran Bingham]

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);
 
 	ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
 					      0, 255, 1, 64);

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

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

[Kieran Bingham]

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

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

[Kieran Bingham]

The OV5640 only outputs SBGGR8. Remove the incorrectly advertised
alternatives which allow a misconfigured pipeline to be established.

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,
 	},
 	{ /* sentinel */ }
 };

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

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

[Kieran Bingham]

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},
+
 	{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 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 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
> >
> >

[PATCH 07/11] media: i2c: ov5640: Split out AWB registers

[Kieran Bingham]

Move the AWB registers to their own lines and split out from
the bulk block independently as a first stage before documenting
to ensure no registers values get lost or reordered during
the updates.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/ov5640.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index b4e1ec4364df..4b6804fc47e1 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -573,17 +573,41 @@ 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}, {0x5180, 0xff, 0, 0},
-	{0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
-	{0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0, 0},
-	{0x5187, 0x09, 0, 0}, {0x5188, 0x09, 0, 0}, {0x5189, 0x88, 0, 0},
-	{0x518a, 0x54, 0, 0}, {0x518b, 0xee, 0, 0}, {0x518c, 0xb2, 0, 0},
-	{0x518d, 0x50, 0, 0}, {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}, {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},
+	{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},
+	{0x5186, 0x09, 0, 0},
+	{0x5187, 0x09, 0, 0},
+	{0x5188, 0x09, 0, 0},
+	{0x5189, 0x88, 0, 0},
+	{0x518a, 0x54, 0, 0},
+	{0x518b, 0xee, 0, 0},
+	{0x518c, 0xb2, 0, 0},
+	{0x518d, 0x50, 0, 0},
+	{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},
+	{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},
+
 	{0x5381, 0x1e, 0, 0}, {0x5382, 0x5b, 0, 0}, {0x5383, 0x08, 0, 0},
 	{0x5384, 0x0a, 0, 0}, {0x5385, 0x7e, 0, 0}, {0x5386, 0x88, 0, 0},
 	{0x5387, 0x7c, 0, 0}, {0x5388, 0x6c, 0, 0}, {0x5389, 0x10, 0, 0},

-- 
2.52.0

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

[Kieran Bingham]

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

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

[Kieran Bingham]

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)
+
 #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 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
>
>

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

[Kieran Bingham]

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},
 
 	/* 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;
+	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 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 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
> >
> >

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

[Kieran Bingham]

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.

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