[PATCH 00/13] media: i2c: Miscellaneous features and fixes for OV5647

[PATCH 00/13] media: i2c: Miscellaneous features and fixes for OV5647

[Jai Luthra]

This series adds support for some important features, like controls for
H/VFLIP, horizontal blanking, regulator controls etc. that are present
in the downstream raspberry pi kernel, to support their v1 camera
module.

Additionally, it also fixes some known issues with streaming lockups,
wrong pixel array size and compliance tests.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
Dave Stevenson (7):
      media: i2c: ov5647: Add support for regulator control.
      media: i2c: ov5647: Use v4l2_async_register_subdev_sensor for lens binding
      media: i2c: ov5647: Add control of V4L2_CID_HBLANK
      media: i2c: ov5647: Tidy up mode registers to make the order common
      media: i2c: ov5647: Separate out the common registers.
      media: i2c: ov5647: Use the same PLL config for full, 1080p, and binned modes
      media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

David Plowman (5):
      media: i2c: ov5647: Correct pixel array offset
      media: i2c: ov5647: Correct minimum VBLANK value
      media: i2c: ov5647: Fix v4l2-compliance failure subscribing to events
      media: i2c: ov5647: Sensor should report RAW color space
      media: i2c: ov5647: Support HFLIP and VFLIP

Laurent Pinchart (1):
      media: i2c: ov5647: Parse and register properties

 drivers/media/i2c/ov5647.c | 447 ++++++++++++++++++++++-----------------------
 1 file changed, 216 insertions(+), 231 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251024-b4-rpi-ov5647-05b600758762

Best regards,
-- 
Jai Luthra <jai.luthra@ideasonboard.com>

[PATCH 01/13] media: i2c: ov5647: Parse and register properties

[Jai Luthra]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Parse device properties and register controls for them using the V4L2
fwnode properties helpers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index e193fef4fcedf4661564c032cd7dbd80a9fd30a6..985a8e81529d2f88cb38ccb8c94f8605026a28a9 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -1284,10 +1284,11 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
 	.s_ctrl = ov5647_s_ctrl,
 };
 
-static int ov5647_init_controls(struct ov5647 *sensor)
+static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	int hblank, exposure_max, exposure_def;
+	struct v4l2_fwnode_device_properties props;
 
 	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
 
@@ -1338,6 +1339,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
 				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
 				     0, 0, ov5647_test_pattern_menu);
 
+	v4l2_fwnode_device_parse(dev, &props);
+
+	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
+					&props);
+
 	if (sensor->ctrls.error)
 		goto handler_free;
 
@@ -1420,7 +1426,7 @@ static int ov5647_probe(struct i2c_client *client)
 
 	sensor->mode = OV5647_DEFAULT_MODE;
 
-	ret = ov5647_init_controls(sensor);
+	ret = ov5647_init_controls(sensor, dev);
 	if (ret)
 		goto mutex_destroy;
 

-- 
2.51.0

Re: [PATCH 01/13] media: i2c: ov5647: Parse and register properties

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:12PM +0530, Jai Luthra wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Parse device properties and register controls for them using the V4L2
> fwnode properties helpers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fcedf4661564c032cd7dbd80a9fd30a6..985a8e81529d2f88cb38ccb8c94f8605026a28a9 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1284,10 +1284,11 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
>  	.s_ctrl = ov5647_s_ctrl,
>  };
>
> -static int ov5647_init_controls(struct ov5647 *sensor)
> +static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  	int hblank, exposure_max, exposure_def;
> +	struct v4l2_fwnode_device_properties props;

Since I have other comments, let me annoying and suggest moving this
up to maintain line length ordering in variables declaration.

>
>  	v4l2_ctrl_handler_init(&sensor->ctrls, 9);

Should we make this 11 ? With this change we can end up registering 2
additional controls.

>
> @@ -1338,6 +1339,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>  				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
>  				     0, 0, ov5647_test_pattern_menu);
>
> +	v4l2_fwnode_device_parse(dev, &props);
> +
> +	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> +					&props);
> +
>  	if (sensor->ctrls.error)
>  		goto handler_free;
>
> @@ -1420,7 +1426,7 @@ static int ov5647_probe(struct i2c_client *client)
>
>  	sensor->mode = OV5647_DEFAULT_MODE;
>
> -	ret = ov5647_init_controls(sensor);
> +	ret = ov5647_init_controls(sensor, dev);

You know, I think we have a bug that went unnoticed..

ov5647_init_controls() retrieves the i2c client with

	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);

but v4l2_set_subdevdata() is called by v4l2_i2c_subdev_init() which is
called after  ov5647_init_controls(). We don't hit a segfault because
client so far as only be used in the error path.

If you move init_controls() after v4l2_i2c_subdev_init() you can
access dev from the i2c_client instead of passing it to the function ?

Thanks
  j

>  	if (ret)
>  		goto mutex_destroy;
>
>
> --
> 2.51.0
>

[PATCH 02/13] media: i2c: ov5647: Correct pixel array offset

[Jai Luthra]

From: David Plowman <david.plowman@raspberrypi.com>

The top offset in the pixel array is actually 6 (see page 3-1 of the
OV5647 data sheet).

Fixes: 14f70a3232aa ("media: ov5647: Add support for get_selection()")
Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 985a8e81529d2f88cb38ccb8c94f8605026a28a9..4fed655f5a11c38e76d1ccc9ae9155cf945684ab 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -69,7 +69,7 @@
 #define OV5647_NATIVE_HEIGHT		1956U
 
 #define OV5647_PIXEL_ARRAY_LEFT		16U
-#define OV5647_PIXEL_ARRAY_TOP		16U
+#define OV5647_PIXEL_ARRAY_TOP		6U
 #define OV5647_PIXEL_ARRAY_WIDTH	2592U
 #define OV5647_PIXEL_ARRAY_HEIGHT	1944U
 

-- 
2.51.0

Re: [PATCH 02/13] media: i2c: ov5647: Correct pixel array offset

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:13PM +0530, Jai Luthra wrote:
> From: David Plowman <david.plowman@raspberrypi.com>
>
> The top offset in the pixel array is actually 6 (see page 3-1 of the
> OV5647 data sheet).
>
> Fixes: 14f70a3232aa ("media: ov5647: Add support for get_selection()")
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

The patch is correct and match the datasheet, but I wonder what the
implications of having a wrong top were..

I see the full 2592x1944 mode declaring 1944 lines but, as the top row
was set to 16, it means the mode should read 10 lines past the end of
the sensor's pixel array..

Anyway, on the patch
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/i2c/ov5647.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 985a8e81529d2f88cb38ccb8c94f8605026a28a9..4fed655f5a11c38e76d1ccc9ae9155cf945684ab 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -69,7 +69,7 @@
>  #define OV5647_NATIVE_HEIGHT		1956U
>
>  #define OV5647_PIXEL_ARRAY_LEFT		16U
> -#define OV5647_PIXEL_ARRAY_TOP		16U
> +#define OV5647_PIXEL_ARRAY_TOP		6U
>  #define OV5647_PIXEL_ARRAY_WIDTH	2592U
>  #define OV5647_PIXEL_ARRAY_HEIGHT	1944U
>
>
> --
> 2.51.0
>
>

Re: [PATCH 02/13] media: i2c: ov5647: Correct pixel array offset

[Dave Stevenson]

Hi Jacopo

On Sun, 2 Nov 2025 at 10:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Jai
>
> On Tue, Oct 28, 2025 at 12:57:13PM +0530, Jai Luthra wrote:
> > From: David Plowman <david.plowman@raspberrypi.com>
> >
> > The top offset in the pixel array is actually 6 (see page 3-1 of the
> > OV5647 data sheet).
> >
> > Fixes: 14f70a3232aa ("media: ov5647: Add support for get_selection()")
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>
> The patch is correct and match the datasheet, but I wonder what the
> implications of having a wrong top were..
>
> I see the full 2592x1944 mode declaring 1944 lines but, as the top row
> was set to 16, it means the mode should read 10 lines past the end of
> the sensor's pixel array..

It's not used in computing the register settings, only reported via
get_selection.

A user of the information from get_selection may get confused by
OV5647_PIXEL_ARRAY_TOP + OV5647_PIXEL_ARRAY_HEIGHT being greater than
OV5647_NATIVE_HEIGHT, but you'd still get images.

  Dave

> Anyway, on the patch
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>   j
>
> > ---
> >  drivers/media/i2c/ov5647.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 985a8e81529d2f88cb38ccb8c94f8605026a28a9..4fed655f5a11c38e76d1ccc9ae9155cf945684ab 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -69,7 +69,7 @@
> >  #define OV5647_NATIVE_HEIGHT         1956U
> >
> >  #define OV5647_PIXEL_ARRAY_LEFT              16U
> > -#define OV5647_PIXEL_ARRAY_TOP               16U
> > +#define OV5647_PIXEL_ARRAY_TOP               6U
> >  #define OV5647_PIXEL_ARRAY_WIDTH     2592U
> >  #define OV5647_PIXEL_ARRAY_HEIGHT    1944U
> >
> >
> > --
> > 2.51.0
> >
> >

[PATCH 03/13] media: i2c: ov5647: Correct minimum VBLANK value

[Jai Luthra]

From: David Plowman <david.plowman@raspberrypi.com>

Trial and error reveals that the minimum vblank value appears to be 24
(the OV5647 data sheet does not give any clues). This fixes streaming
lock-ups in full resolution mode.

Fixes: 2512c06441e3 ("media: ov5647: Support V4L2_CID_VBLANK control")
Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 4fed655f5a11c38e76d1ccc9ae9155cf945684ab..dfe36116e6d3936aa0568f172c79ad4dad21f8c2 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -73,7 +73,7 @@
 #define OV5647_PIXEL_ARRAY_WIDTH	2592U
 #define OV5647_PIXEL_ARRAY_HEIGHT	1944U
 
-#define OV5647_VBLANK_MIN		4
+#define OV5647_VBLANK_MIN		24
 #define OV5647_VTS_MAX			32767
 
 #define OV5647_EXPOSURE_MIN		4

-- 
2.51.0

Re: [PATCH 03/13] media: i2c: ov5647: Correct minimum VBLANK value

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:14PM +0530, Jai Luthra wrote:
> From: David Plowman <david.plowman@raspberrypi.com>
>
> Trial and error reveals that the minimum vblank value appears to be 24

I can only trust your trial and errors here!
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> (the OV5647 data sheet does not give any clues). This fixes streaming
> lock-ups in full resolution mode.
>
> Fixes: 2512c06441e3 ("media: ov5647: Support V4L2_CID_VBLANK control")
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 4fed655f5a11c38e76d1ccc9ae9155cf945684ab..dfe36116e6d3936aa0568f172c79ad4dad21f8c2 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -73,7 +73,7 @@
>  #define OV5647_PIXEL_ARRAY_WIDTH	2592U
>  #define OV5647_PIXEL_ARRAY_HEIGHT	1944U
>
> -#define OV5647_VBLANK_MIN		4
> +#define OV5647_VBLANK_MIN		24
>  #define OV5647_VTS_MAX			32767
>
>  #define OV5647_EXPOSURE_MIN		4
>
> --
> 2.51.0
>
>

[PATCH 04/13] media: i2c: ov5647: Fix v4l2-compliance failure subscribing to events

[Jai Luthra]

From: David Plowman <david.plowman@raspberrypi.com>

Fixes the following v4l2-compliance failure:

fail: v4l2-test-controls.cpp(871): subscribe event for control 'User
Controls' failed test

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index dfe36116e6d3936aa0568f172c79ad4dad21f8c2..8f11b5cbdc1658019e1340e641c7e6f398bff503 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -873,6 +873,8 @@ static const struct v4l2_subdev_core_ops ov5647_subdev_core_ops = {
 	.g_register		= ov5647_sensor_get_register,
 	.s_register		= ov5647_sensor_set_register,
 #endif
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
 static const struct v4l2_rect *

-- 
2.51.0

Re: [PATCH 04/13] media: i2c: ov5647: Fix v4l2-compliance failure subscribing to events

[kernel test robot]

Hi Jai,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 3a8660878839faadb4f1a6dd72c3179c1df56787]

url:    https://github.com/intel-lab-lkp/linux/commits/Jai-Luthra/media-i2c-ov5647-Parse-and-register-properties/20251028-153619
base:   3a8660878839faadb4f1a6dd72c3179c1df56787
patch link:    https://lore.kernel.org/r/20251028-b4-rpi-ov5647-v1-4-098413454f5e%40ideasonboard.com
patch subject: [PATCH 04/13] media: i2c: ov5647: Fix v4l2-compliance failure subscribing to events
config: sparc64-randconfig-r134-20251029 (https://download.01.org/0day-ci/archive/20251029/202510290816.8EQhDjD8-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project d1c086e82af239b245fe8d7832f2753436634990)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251029/202510290816.8EQhDjD8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510290816.8EQhDjD8-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/media/i2c/ov5647.c:870:10: sparse: sparse: Initializer entry defined twice
   drivers/media/i2c/ov5647.c:876:10: sparse:   also defined here

vim +870 drivers/media/i2c/ov5647.c

3c2472a3c54895 Ramiro Oliveira 2017-03-22  867  
c9a05cece64c60 Jacopo Mondi    2020-11-19  868  /* Subdev core operations registration */
3c2472a3c54895 Ramiro Oliveira 2017-03-22  869  static const struct v4l2_subdev_core_ops ov5647_subdev_core_ops = {
dc3373081396f5 Jacopo Mondi    2020-11-19 @870  	.subscribe_event	= v4l2_ctrl_subdev_subscribe_event,
dc3373081396f5 Jacopo Mondi    2020-11-19  871  	.unsubscribe_event	= v4l2_event_subdev_unsubscribe,
3c2472a3c54895 Ramiro Oliveira 2017-03-22  872  #ifdef CONFIG_VIDEO_ADV_DEBUG
3c2472a3c54895 Ramiro Oliveira 2017-03-22  873  	.g_register		= ov5647_sensor_get_register,
3c2472a3c54895 Ramiro Oliveira 2017-03-22  874  	.s_register		= ov5647_sensor_set_register,
3c2472a3c54895 Ramiro Oliveira 2017-03-22  875  #endif
d812c6225cf5be David Plowman   2025-10-28  876  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
d812c6225cf5be David Plowman   2025-10-28  877  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
3c2472a3c54895 Ramiro Oliveira 2017-03-22  878  };
3c2472a3c54895 Ramiro Oliveira 2017-03-22  879  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 04/13] media: i2c: ov5647: Fix v4l2-compliance failure subscribing to events

[Jai Luthra]

Quoting kernel test robot (2025-10-29 06:27:53)
> Hi Jai,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on 3a8660878839faadb4f1a6dd72c3179c1df56787]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Jai-Luthra/media-i2c-ov5647-Parse-and-register-properties/20251028-153619
> base:   3a8660878839faadb4f1a6dd72c3179c1df56787
> patch link:    https://lore.kernel.org/r/20251028-b4-rpi-ov5647-v1-4-098413454f5e%40ideasonboard.com
> patch subject: [PATCH 04/13] media: i2c: ov5647: Fix v4l2-compliance failure subscribing to events
> config: sparc64-randconfig-r134-20251029 (https://download.01.org/0day-ci/archive/20251029/202510290816.8EQhDjD8-lkp@intel.com/config)
> compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project d1c086e82af239b245fe8d7832f2753436634990)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251029/202510290816.8EQhDjD8-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202510290816.8EQhDjD8-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
> >> drivers/media/i2c/ov5647.c:870:10: sparse: sparse: Initializer entry defined twice
>    drivers/media/i2c/ov5647.c:876:10: sparse:   also defined here

My bad, this patch is already mainlined. I will drop it in next iteration.

> 
> vim +870 drivers/media/i2c/ov5647.c
> 
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  867  
> c9a05cece64c60 Jacopo Mondi    2020-11-19  868  /* Subdev core operations registration */
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  869  static const struct v4l2_subdev_core_ops ov5647_subdev_core_ops = {
> dc3373081396f5 Jacopo Mondi    2020-11-19 @870          .subscribe_event        = v4l2_ctrl_subdev_subscribe_event,
> dc3373081396f5 Jacopo Mondi    2020-11-19  871          .unsubscribe_event      = v4l2_event_subdev_unsubscribe,
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  872  #ifdef CONFIG_VIDEO_ADV_DEBUG
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  873          .g_register             = ov5647_sensor_get_register,
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  874          .s_register             = ov5647_sensor_set_register,
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  875  #endif
> d812c6225cf5be David Plowman   2025-10-28  876          .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> d812c6225cf5be David Plowman   2025-10-28  877          .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  878  };
> 3c2472a3c54895 Ramiro Oliveira 2017-03-22  879  
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Thanks,
    Jai

[PATCH 05/13] media: i2c: ov5647: Sensor should report RAW color space

[Jai Luthra]

From: David Plowman <david.plowman@raspberrypi.com>

As this sensor captures RAW bayer frames, the colorspace should be
V4L2_COLORSPACE_RAW instead of SRGB.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 8f11b5cbdc1658019e1340e641c7e6f398bff503..977b878b0d4b8cd5f39f510ebd8b33c9163f7da2 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -508,7 +508,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 	{
 		.format = {
 			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
-			.colorspace	= V4L2_COLORSPACE_SRGB,
+			.colorspace	= V4L2_COLORSPACE_RAW,
 			.field		= V4L2_FIELD_NONE,
 			.width		= 2592,
 			.height		= 1944
@@ -529,7 +529,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 	{
 		.format = {
 			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
-			.colorspace	= V4L2_COLORSPACE_SRGB,
+			.colorspace	= V4L2_COLORSPACE_RAW,
 			.field		= V4L2_FIELD_NONE,
 			.width		= 1920,
 			.height		= 1080
@@ -550,7 +550,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 	{
 		.format = {
 			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
-			.colorspace	= V4L2_COLORSPACE_SRGB,
+			.colorspace	= V4L2_COLORSPACE_RAW,
 			.field		= V4L2_FIELD_NONE,
 			.width		= 1296,
 			.height		= 972
@@ -571,7 +571,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 	{
 		.format = {
 			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
-			.colorspace	= V4L2_COLORSPACE_SRGB,
+			.colorspace	= V4L2_COLORSPACE_RAW,
 			.field		= V4L2_FIELD_NONE,
 			.width		= 640,
 			.height		= 480

-- 
2.51.0

Re: [PATCH 05/13] media: i2c: ov5647: Sensor should report RAW color space

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:16PM +0530, Jai Luthra wrote:
> From: David Plowman <david.plowman@raspberrypi.com>
>
> As this sensor captures RAW bayer frames, the colorspace should be
> V4L2_COLORSPACE_RAW instead of SRGB.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

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

Thanks
   j

> ---
>  drivers/media/i2c/ov5647.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 8f11b5cbdc1658019e1340e641c7e6f398bff503..977b878b0d4b8cd5f39f510ebd8b33c9163f7da2 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -508,7 +508,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  	{
>  		.format = {
>  			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
> -			.colorspace	= V4L2_COLORSPACE_SRGB,
> +			.colorspace	= V4L2_COLORSPACE_RAW,
>  			.field		= V4L2_FIELD_NONE,
>  			.width		= 2592,
>  			.height		= 1944
> @@ -529,7 +529,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  	{
>  		.format = {
>  			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
> -			.colorspace	= V4L2_COLORSPACE_SRGB,
> +			.colorspace	= V4L2_COLORSPACE_RAW,
>  			.field		= V4L2_FIELD_NONE,
>  			.width		= 1920,
>  			.height		= 1080
> @@ -550,7 +550,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  	{
>  		.format = {
>  			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
> -			.colorspace	= V4L2_COLORSPACE_SRGB,
> +			.colorspace	= V4L2_COLORSPACE_RAW,
>  			.field		= V4L2_FIELD_NONE,
>  			.width		= 1296,
>  			.height		= 972
> @@ -571,7 +571,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  	{
>  		.format = {
>  			.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
> -			.colorspace	= V4L2_COLORSPACE_SRGB,
> +			.colorspace	= V4L2_COLORSPACE_RAW,
>  			.field		= V4L2_FIELD_NONE,
>  			.width		= 640,
>  			.height		= 480
>
> --
> 2.51.0
>

[PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Jai Luthra]

From: David Plowman <david.plowman@raspberrypi.com>

Add missing controls for horizontal and vertical flipping.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -54,6 +54,8 @@
 #define OV5647_REG_GAIN_LO		0x350b
 #define OV5647_REG_VTS_HI		0x380e
 #define OV5647_REG_VTS_LO		0x380f
+#define OV5647_REG_VFLIP		0x3820
+#define OV5647_REG_HFLIP		0x3821
 #define OV5647_REG_FRAME_OFF_NUMBER	0x4202
 #define OV5647_REG_MIPI_CTRL00		0x4800
 #define OV5647_REG_MIPI_CTRL14		0x4814
@@ -109,6 +111,8 @@ struct ov5647 {
 	struct v4l2_ctrl		*hblank;
 	struct v4l2_ctrl		*vblank;
 	struct v4l2_ctrl		*exposure;
+	struct v4l2_ctrl		*hflip;
+	struct v4l2_ctrl		*vflip;
 };
 
 static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
@@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x3036, 0x69},
 	{0x303c, 0x11},
 	{0x3106, 0xf5},
-	{0x3821, 0x06},
+	{0x3821, 0x00},
 	{0x3820, 0x00},
 	{0x3827, 0xec},
 	{0x370c, 0x03},
@@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
 	{0x3036, 0x62},
 	{0x303c, 0x11},
 	{0x3106, 0xf5},
-	{0x3821, 0x06},
+	{0x3821, 0x00},
 	{0x3820, 0x00},
 	{0x3827, 0xec},
 	{0x370c, 0x03},
@@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 	{0x4800, 0x24},
 	{0x3503, 0x03},
 	{0x3820, 0x41},
-	{0x3821, 0x07},
+	{0x3821, 0x01},
 	{0x350a, 0x00},
 	{0x350b, 0x10},
 	{0x3500, 0x00},
@@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
 	{0x3035, 0x11},
 	{0x3036, 0x46},
 	{0x303c, 0x11},
-	{0x3821, 0x07},
+	{0x3821, 0x01},
 	{0x3820, 0x41},
 	{0x370c, 0x03},
 	{0x3612, 0x59},
@@ -935,6 +939,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
 	.s_stream =		ov5647_s_stream,
 };
 
+/*
+ * This function returns the mbus code for the current settings of the HFLIP
+ * and VFLIP controls.
+ */
+static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
+{
+	struct ov5647 *sensor = to_sensor(sd);
+	/* The control values are only 0 or 1. */
+	int index =  sensor->hflip->val | (sensor->vflip->val << 1);
+
+	static const u32 codes[4] = {
+		MEDIA_BUS_FMT_SGBRG10_1X10,
+		MEDIA_BUS_FMT_SBGGR10_1X10,
+		MEDIA_BUS_FMT_SRGGB10_1X10,
+		MEDIA_BUS_FMT_SGRBG10_1X10
+	};
+
+	return codes[index];
+}
+
 static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -942,7 +966,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
+	code->code = ov5647_get_mbus_code(sd);
 
 	return 0;
 }
@@ -953,7 +977,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
 {
 	const struct v4l2_mbus_framefmt *fmt;
 
-	if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
+	if (fse->code != ov5647_get_mbus_code(sd) ||
 	    fse->index >= ARRAY_SIZE(ov5647_modes))
 		return -EINVAL;
 
@@ -986,6 +1010,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
 	}
 
 	*fmt = *sensor_format;
+	/* The code we pass back must reflect the current h/vflips. */
+	fmt->code = ov5647_get_mbus_code(sd);
 	mutex_unlock(&sensor->lock);
 
 	return 0;
@@ -1033,6 +1059,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
 					 exposure_def);
 	}
 	*fmt = mode->format;
+	/* The code we pass back must reflect the current h/vflips. */
+	fmt->code = ov5647_get_mbus_code(sd);
 	mutex_unlock(&sensor->lock);
 
 	return 0;
@@ -1208,6 +1236,25 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
 	return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
 }
 
+static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
+{
+	int ret;
+	u8 reg_val;
+
+	/* Set or clear bit 1 and leave everything else alone. */
+	ret = ov5647_read(sd, reg, &reg_val);
+	if (ret == 0) {
+		if (ctrl_val)
+			reg_val |= 2;
+		else
+			reg_val &= ~2;
+
+		ret = ov5647_write(sd, reg, reg_val);
+	}
+
+	return ret;
+}
+
 static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov5647 *sensor = container_of(ctrl->handler,
@@ -1270,6 +1317,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 		/* Read-only, but we adjust it based on mode. */
 		break;
 
+	case V4L2_CID_HFLIP:
+		/* There's an in-built hflip in the sensor, so account for that here. */
+		ov5647_s_flip(sd, OV5647_REG_HFLIP, !ctrl->val);
+		break;
+	case V4L2_CID_VFLIP:
+		ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
+		break;
+
 	default:
 		dev_info(&client->dev,
 			 "Control (id:0x%x, val:0x%x) not supported\n",
@@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
 				     0, 0, ov5647_test_pattern_menu);
 
+	sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (sensor->hflip)
+		sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (sensor->vflip)
+		sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
 	v4l2_fwnode_device_parse(dev, &props);
 
 	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,

-- 
2.51.0

Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> From: David Plowman <david.plowman@raspberrypi.com>
>
> Add missing controls for horizontal and vertical flipping.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 71 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -54,6 +54,8 @@
>  #define OV5647_REG_GAIN_LO		0x350b
>  #define OV5647_REG_VTS_HI		0x380e
>  #define OV5647_REG_VTS_LO		0x380f
> +#define OV5647_REG_VFLIP		0x3820
> +#define OV5647_REG_HFLIP		0x3821
>  #define OV5647_REG_FRAME_OFF_NUMBER	0x4202
>  #define OV5647_REG_MIPI_CTRL00		0x4800
>  #define OV5647_REG_MIPI_CTRL14		0x4814
> @@ -109,6 +111,8 @@ struct ov5647 {
>  	struct v4l2_ctrl		*hblank;
>  	struct v4l2_ctrl		*vblank;
>  	struct v4l2_ctrl		*exposure;
> +	struct v4l2_ctrl		*hflip;
> +	struct v4l2_ctrl		*vflip;
>  };
>
>  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> @@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>  	{0x3036, 0x69},
>  	{0x303c, 0x11},
>  	{0x3106, 0xf5},
> -	{0x3821, 0x06},
> +	{0x3821, 0x00},
>  	{0x3820, 0x00},

That's interesting, as the datasheet says that by default

        3820 = 0x40
        3821 = 0x00

And
- BIT[2] = flip ISP
- BIT[1] = flip SNR

The implementation of ov5647_s_flip() toggles BIT(1) and ignores
BIT(2) while the modes definition have both (BIT(2) | BIT(1)) set

More interestingly the datasheet says:

In flip mode, the OV5647 does not need additional settings because the
ISP block will auto-detect whether the pixel is in the red line or
blue line and make the necessary adjustments

Might this suggest that if we flip using BIT(2) we don't need to
change the bayer pattern ordering ?

Now, I admit I'm not sure what are the ISP functions on the ov5647 and
this patch is super-duper-tested as it comes from the RPi BSP, so if
you don't have answers to the above questions, I'm fine with this
patch!

>  	{0x3827, 0xec},
>  	{0x370c, 0x03},
> @@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
>  	{0x3036, 0x62},
>  	{0x303c, 0x11},
>  	{0x3106, 0xf5},
> -	{0x3821, 0x06},
> +	{0x3821, 0x00},
>  	{0x3820, 0x00},
>  	{0x3827, 0xec},
>  	{0x370c, 0x03},
> @@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  	{0x4800, 0x24},
>  	{0x3503, 0x03},
>  	{0x3820, 0x41},
> -	{0x3821, 0x07},
> +	{0x3821, 0x01},
>  	{0x350a, 0x00},
>  	{0x350b, 0x10},
>  	{0x3500, 0x00},
> @@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
>  	{0x3035, 0x11},
>  	{0x3036, 0x46},
>  	{0x303c, 0x11},
> -	{0x3821, 0x07},
> +	{0x3821, 0x01},
>  	{0x3820, 0x41},
>  	{0x370c, 0x03},
>  	{0x3612, 0x59},
> @@ -935,6 +939,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
>  	.s_stream =		ov5647_s_stream,
>  };
>
> +/*
> + * This function returns the mbus code for the current settings of the HFLIP
> + * and VFLIP controls.
> + */
> +static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
> +{
> +	struct ov5647 *sensor = to_sensor(sd);
> +	/* The control values are only 0 or 1. */
> +	int index =  sensor->hflip->val | (sensor->vflip->val << 1);
> +
> +	static const u32 codes[4] = {
> +		MEDIA_BUS_FMT_SGBRG10_1X10,
> +		MEDIA_BUS_FMT_SBGGR10_1X10,
> +		MEDIA_BUS_FMT_SRGGB10_1X10,
> +		MEDIA_BUS_FMT_SGRBG10_1X10
> +	};
> +
> +	return codes[index];
> +}
> +
>  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -942,7 +966,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index > 0)
>  		return -EINVAL;
>
> -	code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +	code->code = ov5647_get_mbus_code(sd);
>
>  	return 0;
>  }
> @@ -953,7 +977,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
>  {
>  	const struct v4l2_mbus_framefmt *fmt;
>
> -	if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
> +	if (fse->code != ov5647_get_mbus_code(sd) ||
>  	    fse->index >= ARRAY_SIZE(ov5647_modes))
>  		return -EINVAL;
>
> @@ -986,6 +1010,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
>  	}
>
>  	*fmt = *sensor_format;
> +	/* The code we pass back must reflect the current h/vflips. */
> +	fmt->code = ov5647_get_mbus_code(sd);
>  	mutex_unlock(&sensor->lock);
>
>  	return 0;
> @@ -1033,6 +1059,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
>  					 exposure_def);
>  	}
>  	*fmt = mode->format;
> +	/* The code we pass back must reflect the current h/vflips. */
> +	fmt->code = ov5647_get_mbus_code(sd);
>  	mutex_unlock(&sensor->lock);
>
>  	return 0;
> @@ -1208,6 +1236,25 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
>  	return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
>  }
>
> +static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
> +{
> +	int ret;
> +	u8 reg_val;
> +
> +	/* Set or clear bit 1 and leave everything else alone. */
> +	ret = ov5647_read(sd, reg, &reg_val);
> +	if (ret == 0) {
> +		if (ctrl_val)
> +			reg_val |= 2;
> +		else
> +			reg_val &= ~2;
> +
> +		ret = ov5647_write(sd, reg, reg_val);
> +	}
> +
> +	return ret;
> +}
> +
>  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov5647 *sensor = container_of(ctrl->handler,
> @@ -1270,6 +1317,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  		/* Read-only, but we adjust it based on mode. */
>  		break;
>
> +	case V4L2_CID_HFLIP:
> +		/* There's an in-built hflip in the sensor, so account for that here. */
> +		ov5647_s_flip(sd, OV5647_REG_HFLIP, !ctrl->val);
> +		break;
> +	case V4L2_CID_VFLIP:
> +		ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
> +		break;

The modes definition used to set

        0x3820 = 0x00
        0x3821 = 0x06

Is this the built-in hflip ?

Or does it mean that setting the registers value to 1 'disabled' flips ?

> +
>  	default:
>  		dev_info(&client->dev,
>  			 "Control (id:0x%x, val:0x%x) not supported\n",
> @@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
>  				     0, 0, ov5647_test_pattern_menu);
>
> +	sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> +					  V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (sensor->hflip)
> +		sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;

I wonder if hflip is enabled by default we shouldn't register the
control with default value of 1 ?

> +
> +	sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> +					  V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (sensor->vflip)
> +		sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
>  	v4l2_fwnode_device_parse(dev, &props);
>
>  	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
>
> --
> 2.51.0
>

Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Jai Luthra]

Hi Jacopo,

Thanks a lot for the review.

Quoting Jacopo Mondi (2025-11-02 16:20:36)
> Hi Jai
> 
> On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> > From: David Plowman <david.plowman@raspberrypi.com>
> >
> > Add missing controls for horizontal and vertical flipping.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 71 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -54,6 +54,8 @@
> >  #define OV5647_REG_GAIN_LO           0x350b
> >  #define OV5647_REG_VTS_HI            0x380e
> >  #define OV5647_REG_VTS_LO            0x380f
> > +#define OV5647_REG_VFLIP             0x3820
> > +#define OV5647_REG_HFLIP             0x3821
> >  #define OV5647_REG_FRAME_OFF_NUMBER  0x4202
> >  #define OV5647_REG_MIPI_CTRL00               0x4800
> >  #define OV5647_REG_MIPI_CTRL14               0x4814
> > @@ -109,6 +111,8 @@ struct ov5647 {
> >       struct v4l2_ctrl                *hblank;
> >       struct v4l2_ctrl                *vblank;
> >       struct v4l2_ctrl                *exposure;
> > +     struct v4l2_ctrl                *hflip;
> > +     struct v4l2_ctrl                *vflip;
> >  };
> >
> >  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > @@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> >       {0x3036, 0x69},
> >       {0x303c, 0x11},
> >       {0x3106, 0xf5},
> > -     {0x3821, 0x06},
> > +     {0x3821, 0x00},
> >       {0x3820, 0x00},
> 
> That's interesting, as the datasheet says that by default
> 
>         3820 = 0x40
>         3821 = 0x00
> 
> And
> - BIT[2] = flip ISP
> - BIT[1] = flip SNR
> 
> The implementation of ov5647_s_flip() toggles BIT(1) and ignores
> BIT(2) while the modes definition have both (BIT(2) | BIT(1)) set
> 
> More interestingly the datasheet says:
> 
> In flip mode, the OV5647 does not need additional settings because the
> ISP block will auto-detect whether the pixel is in the red line or
> blue line and make the necessary adjustments
> 
> Might this suggest that if we flip using BIT(2) we don't need to
> change the bayer pattern ordering ?

Indeed! That was a great find, I am now able to set BIT(2) to get flips
without needing to modify the pixelarray layout.

Will do that in v2.

> 
> Now, I admit I'm not sure what are the ISP functions on the ov5647 and
> this patch is super-duper-tested as it comes from the RPi BSP, so if
> you don't have answers to the above questions, I'm fine with this
> patch!
> 
> >       {0x3827, 0xec},
> >       {0x370c, 0x03},
> > @@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> >       {0x3036, 0x62},
> >       {0x303c, 0x11},
> >       {0x3106, 0xf5},
> > -     {0x3821, 0x06},
> > +     {0x3821, 0x00},
> >       {0x3820, 0x00},
> >       {0x3827, 0xec},
> >       {0x370c, 0x03},
> > @@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> >       {0x4800, 0x24},
> >       {0x3503, 0x03},
> >       {0x3820, 0x41},
> > -     {0x3821, 0x07},
> > +     {0x3821, 0x01},
> >       {0x350a, 0x00},
> >       {0x350b, 0x10},
> >       {0x3500, 0x00},
> > @@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> >       {0x3035, 0x11},
> >       {0x3036, 0x46},
> >       {0x303c, 0x11},
> > -     {0x3821, 0x07},
> > +     {0x3821, 0x01},
> >       {0x3820, 0x41},
> >       {0x370c, 0x03},
> >       {0x3612, 0x59},
> > @@ -935,6 +939,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> >       .s_stream =             ov5647_s_stream,
> >  };
> >
> > +/*
> > + * This function returns the mbus code for the current settings of the HFLIP
> > + * and VFLIP controls.
> > + */
> > +static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
> > +{
> > +     struct ov5647 *sensor = to_sensor(sd);
> > +     /* The control values are only 0 or 1. */
> > +     int index =  sensor->hflip->val | (sensor->vflip->val << 1);
> > +
> > +     static const u32 codes[4] = {
> > +             MEDIA_BUS_FMT_SGBRG10_1X10,
> > +             MEDIA_BUS_FMT_SBGGR10_1X10,
> > +             MEDIA_BUS_FMT_SRGGB10_1X10,
> > +             MEDIA_BUS_FMT_SGRBG10_1X10
> > +     };
> > +
> > +     return codes[index];
> > +}
> > +
> >  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> >                                struct v4l2_subdev_state *sd_state,
> >                                struct v4l2_subdev_mbus_code_enum *code)
> > @@ -942,7 +966,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> >       if (code->index > 0)
> >               return -EINVAL;
> >
> > -     code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > +     code->code = ov5647_get_mbus_code(sd);
> >
> >       return 0;
> >  }
> > @@ -953,7 +977,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
> >  {
> >       const struct v4l2_mbus_framefmt *fmt;
> >
> > -     if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
> > +     if (fse->code != ov5647_get_mbus_code(sd) ||
> >           fse->index >= ARRAY_SIZE(ov5647_modes))
> >               return -EINVAL;
> >
> > @@ -986,6 +1010,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
> >       }
> >
> >       *fmt = *sensor_format;
> > +     /* The code we pass back must reflect the current h/vflips. */
> > +     fmt->code = ov5647_get_mbus_code(sd);
> >       mutex_unlock(&sensor->lock);
> >
> >       return 0;
> > @@ -1033,6 +1059,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> >                                        exposure_def);
> >       }
> >       *fmt = mode->format;
> > +     /* The code we pass back must reflect the current h/vflips. */
> > +     fmt->code = ov5647_get_mbus_code(sd);
> >       mutex_unlock(&sensor->lock);
> >
> >       return 0;
> > @@ -1208,6 +1236,25 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> >       return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> >  }
> >
> > +static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
> > +{
> > +     int ret;
> > +     u8 reg_val;
> > +
> > +     /* Set or clear bit 1 and leave everything else alone. */
> > +     ret = ov5647_read(sd, reg, &reg_val);
> > +     if (ret == 0) {
> > +             if (ctrl_val)
> > +                     reg_val |= 2;
> > +             else
> > +                     reg_val &= ~2;
> > +
> > +             ret = ov5647_write(sd, reg, reg_val);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >       struct ov5647 *sensor = container_of(ctrl->handler,
> > @@ -1270,6 +1317,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >               /* Read-only, but we adjust it based on mode. */
> >               break;
> >
> > +     case V4L2_CID_HFLIP:
> > +             /* There's an in-built hflip in the sensor, so account for that here. */
> > +             ov5647_s_flip(sd, OV5647_REG_HFLIP, !ctrl->val);
> > +             break;
> > +     case V4L2_CID_VFLIP:
> > +             ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
> > +             break;
> 
> The modes definition used to set
> 
>         0x3820 = 0x00
>         0x3821 = 0x06
> 
> Is this the built-in hflip ?
> 
> Or does it mean that setting the registers value to 1 'disabled' flips ?
> 

Yes, this particular sensor flips the image horizontally if 0x3820 = 0x00,
resulting in a mirrored image.

But now that userspace can control flips, I am a bit confused on how to
report this in the controls.

> > +
> >       default:
> >               dev_info(&client->dev,
> >                        "Control (id:0x%x, val:0x%x) not supported\n",
> > @@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> >                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> >                                    0, 0, ov5647_test_pattern_menu);
> >
> > +     sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > +                                       V4L2_CID_HFLIP, 0, 1, 1, 0);
> > +     if (sensor->hflip)
> > +             sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> 
> I wonder if hflip is enabled by default we shouldn't register the
> control with default value of 1 ?
> 

My preference is to let the driver toggle the value for HFLIP before
writing to the sensor, as it is done currently in this patch.

This makes this sensor behave like others, where you get a normal
non-mirrored image when you set HFLIP = 0, and mirrored if HFLIP = 1.

The alternative is to keep the driver transparent by reporting the register
values as-is in the control. In that case, we should make horizontal_flip =
1 the default value for the control. But I'm not aware if any userspace
applications will override the default value resulting in bad experience
for the users.

WDYT?

Thanks,
    Jai

> > +
> > +     sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > +                                       V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +     if (sensor->vflip)
> > +             sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +
> >       v4l2_fwnode_device_parse(dev, &props);
> >
> >       v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> >
> > --
> > 2.51.0
> >

Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Dave Stevenson]

Hi Jai & Jacopo.

On Wed, 12 Nov 2025 at 11:56, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thanks a lot for the review.
>
> Quoting Jacopo Mondi (2025-11-02 16:20:36)
> > Hi Jai
> >
> > On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> > > From: David Plowman <david.plowman@raspberrypi.com>
> > >
> > > Add missing controls for horizontal and vertical flipping.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 71 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -54,6 +54,8 @@
> > >  #define OV5647_REG_GAIN_LO           0x350b
> > >  #define OV5647_REG_VTS_HI            0x380e
> > >  #define OV5647_REG_VTS_LO            0x380f
> > > +#define OV5647_REG_VFLIP             0x3820
> > > +#define OV5647_REG_HFLIP             0x3821
> > >  #define OV5647_REG_FRAME_OFF_NUMBER  0x4202
> > >  #define OV5647_REG_MIPI_CTRL00               0x4800
> > >  #define OV5647_REG_MIPI_CTRL14               0x4814
> > > @@ -109,6 +111,8 @@ struct ov5647 {
> > >       struct v4l2_ctrl                *hblank;
> > >       struct v4l2_ctrl                *vblank;
> > >       struct v4l2_ctrl                *exposure;
> > > +     struct v4l2_ctrl                *hflip;
> > > +     struct v4l2_ctrl                *vflip;
> > >  };
> > >
> > >  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > > @@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> > >       {0x3036, 0x69},
> > >       {0x303c, 0x11},
> > >       {0x3106, 0xf5},
> > > -     {0x3821, 0x06},
> > > +     {0x3821, 0x00},
> > >       {0x3820, 0x00},
> >
> > That's interesting, as the datasheet says that by default
> >
> >         3820 = 0x40
> >         3821 = 0x00
> >
> > And
> > - BIT[2] = flip ISP
> > - BIT[1] = flip SNR
> >
> > The implementation of ov5647_s_flip() toggles BIT(1) and ignores
> > BIT(2) while the modes definition have both (BIT(2) | BIT(1)) set
> >
> > More interestingly the datasheet says:
> >
> > In flip mode, the OV5647 does not need additional settings because the
> > ISP block will auto-detect whether the pixel is in the red line or
> > blue line and make the necessary adjustments
> >
> > Might this suggest that if we flip using BIT(2) we don't need to
> > change the bayer pattern ordering ?
>
> Indeed! That was a great find, I am now able to set BIT(2) to get flips
> without needing to modify the pixelarray layout.
>
> Will do that in v2.

Are you going to update the rectangles reported by g_selection then?
The sensor can't magically change the filter colour of the pixels
being read out, therefore it must be reading different pixels.
Figure 3-1 sensor array region color filter layout lists 2592x1944
active pixels, and we're reading out all of those. "The backend
processor can use the boundary pixels for additional processing", but
how? There are no details.

Yes it's minor, but in using BIT(1) we know exactly what the sensor is
doing. Handling the change of Bayer order in the driver isn't that
involved, and is common to so many sensors that clients have to
support it anyway.

None of the ISP features of OV5647 are used by this driver. I suspect
the bits mentioned in the datasheet are probably a hang-over from
OV5640 which is effectively the same sensor array with built-in ISP
processing to produce YUYV images.

Just my two-cents.

> >
> > Now, I admit I'm not sure what are the ISP functions on the ov5647 and
> > this patch is super-duper-tested as it comes from the RPi BSP, so if
> > you don't have answers to the above questions, I'm fine with this
> > patch!
> >
> > >       {0x3827, 0xec},
> > >       {0x370c, 0x03},
> > > @@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> > >       {0x3036, 0x62},
> > >       {0x303c, 0x11},
> > >       {0x3106, 0xf5},
> > > -     {0x3821, 0x06},
> > > +     {0x3821, 0x00},
> > >       {0x3820, 0x00},
> > >       {0x3827, 0xec},
> > >       {0x370c, 0x03},
> > > @@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> > >       {0x4800, 0x24},
> > >       {0x3503, 0x03},
> > >       {0x3820, 0x41},
> > > -     {0x3821, 0x07},
> > > +     {0x3821, 0x01},
> > >       {0x350a, 0x00},
> > >       {0x350b, 0x10},
> > >       {0x3500, 0x00},
> > > @@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> > >       {0x3035, 0x11},
> > >       {0x3036, 0x46},
> > >       {0x303c, 0x11},
> > > -     {0x3821, 0x07},
> > > +     {0x3821, 0x01},
> > >       {0x3820, 0x41},
> > >       {0x370c, 0x03},
> > >       {0x3612, 0x59},
> > > @@ -935,6 +939,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> > >       .s_stream =             ov5647_s_stream,
> > >  };
> > >
> > > +/*
> > > + * This function returns the mbus code for the current settings of the HFLIP
> > > + * and VFLIP controls.
> > > + */
> > > +static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
> > > +{
> > > +     struct ov5647 *sensor = to_sensor(sd);
> > > +     /* The control values are only 0 or 1. */
> > > +     int index =  sensor->hflip->val | (sensor->vflip->val << 1);
> > > +
> > > +     static const u32 codes[4] = {
> > > +             MEDIA_BUS_FMT_SGBRG10_1X10,
> > > +             MEDIA_BUS_FMT_SBGGR10_1X10,
> > > +             MEDIA_BUS_FMT_SRGGB10_1X10,
> > > +             MEDIA_BUS_FMT_SGRBG10_1X10
> > > +     };
> > > +
> > > +     return codes[index];
> > > +}
> > > +
> > >  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> > >                                struct v4l2_subdev_state *sd_state,
> > >                                struct v4l2_subdev_mbus_code_enum *code)
> > > @@ -942,7 +966,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> > >       if (code->index > 0)
> > >               return -EINVAL;
> > >
> > > -     code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > > +     code->code = ov5647_get_mbus_code(sd);
> > >
> > >       return 0;
> > >  }
> > > @@ -953,7 +977,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
> > >  {
> > >       const struct v4l2_mbus_framefmt *fmt;
> > >
> > > -     if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
> > > +     if (fse->code != ov5647_get_mbus_code(sd) ||
> > >           fse->index >= ARRAY_SIZE(ov5647_modes))
> > >               return -EINVAL;
> > >
> > > @@ -986,6 +1010,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
> > >       }
> > >
> > >       *fmt = *sensor_format;
> > > +     /* The code we pass back must reflect the current h/vflips. */
> > > +     fmt->code = ov5647_get_mbus_code(sd);
> > >       mutex_unlock(&sensor->lock);
> > >
> > >       return 0;
> > > @@ -1033,6 +1059,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > >                                        exposure_def);
> > >       }
> > >       *fmt = mode->format;
> > > +     /* The code we pass back must reflect the current h/vflips. */
> > > +     fmt->code = ov5647_get_mbus_code(sd);
> > >       mutex_unlock(&sensor->lock);
> > >
> > >       return 0;
> > > @@ -1208,6 +1236,25 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> > >       return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> > >  }
> > >
> > > +static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
> > > +{
> > > +     int ret;
> > > +     u8 reg_val;
> > > +
> > > +     /* Set or clear bit 1 and leave everything else alone. */
> > > +     ret = ov5647_read(sd, reg, &reg_val);
> > > +     if (ret == 0) {
> > > +             if (ctrl_val)
> > > +                     reg_val |= 2;
> > > +             else
> > > +                     reg_val &= ~2;
> > > +
> > > +             ret = ov5647_write(sd, reg, reg_val);
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > >       struct ov5647 *sensor = container_of(ctrl->handler,
> > > @@ -1270,6 +1317,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > >               /* Read-only, but we adjust it based on mode. */
> > >               break;
> > >
> > > +     case V4L2_CID_HFLIP:
> > > +             /* There's an in-built hflip in the sensor, so account for that here. */
> > > +             ov5647_s_flip(sd, OV5647_REG_HFLIP, !ctrl->val);
> > > +             break;
> > > +     case V4L2_CID_VFLIP:
> > > +             ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
> > > +             break;
> >
> > The modes definition used to set
> >
> >         0x3820 = 0x00
> >         0x3821 = 0x06
> >
> > Is this the built-in hflip ?
> >
> > Or does it mean that setting the registers value to 1 'disabled' flips ?
> >
>
> Yes, this particular sensor flips the image horizontally if 0x3820 = 0x00,
> resulting in a mirrored image.
>
> But now that userspace can control flips, I am a bit confused on how to
> report this in the controls.
>
> > > +
> > >       default:
> > >               dev_info(&client->dev,
> > >                        "Control (id:0x%x, val:0x%x) not supported\n",
> > > @@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > >                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> > >                                    0, 0, ov5647_test_pattern_menu);
> > >
> > > +     sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > +                                       V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > +     if (sensor->hflip)
> > > +             sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >
> > I wonder if hflip is enabled by default we shouldn't register the
> > control with default value of 1 ?
> >
>
> My preference is to let the driver toggle the value for HFLIP before
> writing to the sensor, as it is done currently in this patch.
>
> This makes this sensor behave like others, where you get a normal
> non-mirrored image when you set HFLIP = 0, and mirrored if HFLIP = 1.
>
> The alternative is to keep the driver transparent by reporting the register
> values as-is in the control. In that case, we should make horizontal_flip =
> 1 the default value for the control. But I'm not aware if any userspace
> applications will override the default value resulting in bad experience
> for the users.
>
> WDYT?

My vote would be for the driver to invert the value before writing, as
this patch does.

Making userspace have to call QUERYCTRL / QUERY_EXT_CTRL to get the
default before it can then work out that it needs to invert any flip
settings is additional overhead for zero gain.

  Dave

> Thanks,
>     Jai
>
> > > +
> > > +     sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > +                                       V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > +     if (sensor->vflip)
> > > +             sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > +
> > >       v4l2_fwnode_device_parse(dev, &props);
> > >
> > >       v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> > >
> > > --
> > > 2.51.0
> > >

Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Kieran Bingham]

Quoting Dave Stevenson (2025-11-12 14:35:49)
> Hi Jai & Jacopo.
> 
> On Wed, 12 Nov 2025 at 11:56, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Thanks a lot for the review.
> >
> > Quoting Jacopo Mondi (2025-11-02 16:20:36)
> > > Hi Jai
> > >
> > > On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> > > > From: David Plowman <david.plowman@raspberrypi.com>
> > > >
> > > > Add missing controls for horizontal and vertical flipping.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 71 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -54,6 +54,8 @@
> > > >  #define OV5647_REG_GAIN_LO           0x350b
> > > >  #define OV5647_REG_VTS_HI            0x380e
> > > >  #define OV5647_REG_VTS_LO            0x380f
> > > > +#define OV5647_REG_VFLIP             0x3820
> > > > +#define OV5647_REG_HFLIP             0x3821
> > > >  #define OV5647_REG_FRAME_OFF_NUMBER  0x4202
> > > >  #define OV5647_REG_MIPI_CTRL00               0x4800
> > > >  #define OV5647_REG_MIPI_CTRL14               0x4814
> > > > @@ -109,6 +111,8 @@ struct ov5647 {
> > > >       struct v4l2_ctrl                *hblank;
> > > >       struct v4l2_ctrl                *vblank;
> > > >       struct v4l2_ctrl                *exposure;
> > > > +     struct v4l2_ctrl                *hflip;
> > > > +     struct v4l2_ctrl                *vflip;
> > > >  };
> > > >
> > > >  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > > > @@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> > > >       {0x3036, 0x69},
> > > >       {0x303c, 0x11},
> > > >       {0x3106, 0xf5},
> > > > -     {0x3821, 0x06},
> > > > +     {0x3821, 0x00},
> > > >       {0x3820, 0x00},
> > >
> > > That's interesting, as the datasheet says that by default
> > >
> > >         3820 = 0x40
> > >         3821 = 0x00
> > >
> > > And
> > > - BIT[2] = flip ISP
> > > - BIT[1] = flip SNR
> > >
> > > The implementation of ov5647_s_flip() toggles BIT(1) and ignores
> > > BIT(2) while the modes definition have both (BIT(2) | BIT(1)) set
> > >
> > > More interestingly the datasheet says:
> > >
> > > In flip mode, the OV5647 does not need additional settings because the
> > > ISP block will auto-detect whether the pixel is in the red line or
> > > blue line and make the necessary adjustments
> > >
> > > Might this suggest that if we flip using BIT(2) we don't need to
> > > change the bayer pattern ordering ?
> >
> > Indeed! That was a great find, I am now able to set BIT(2) to get flips
> > without needing to modify the pixelarray layout.
> >
> > Will do that in v2.
> 
> Are you going to update the rectangles reported by g_selection then?
> The sensor can't magically change the filter colour of the pixels
> being read out, therefore it must be reading different pixels.
> Figure 3-1 sensor array region color filter layout lists 2592x1944
> active pixels, and we're reading out all of those. "The backend
> processor can use the boundary pixels for additional processing", but
> how? There are no details.
> 
> Yes it's minor, but in using BIT(1) we know exactly what the sensor is
> doing. Handling the change of Bayer order in the driver isn't that
> involved, and is common to so many sensors that clients have to
> support it anyway.
> 
> None of the ISP features of OV5647 are used by this driver. I suspect
> the bits mentioned in the datasheet are probably a hang-over from
> OV5640 which is effectively the same sensor array with built-in ISP
> processing to produce YUYV images.
> 
> Just my two-cents.

I'm also on this side of the fence. Any magic that keeps the bayer order
- moves the picture, and unless we can report that accurately it's
undesireable.

It might be worth trying out the crop-detector in camshark to measure
the movements and relative positions of the full raw captures and crops
modes!

For instance on the IMX283 and IMX335 - in the product we worked on for
those sensors, the images are used for measurements - so even a single
pixel offset matters!

--
Kieran



> 
> > >
> > > Now, I admit I'm not sure what are the ISP functions on the ov5647 and
> > > this patch is super-duper-tested as it comes from the RPi BSP, so if
> > > you don't have answers to the above questions, I'm fine with this
> > > patch!
> > >
> > > >       {0x3827, 0xec},
> > > >       {0x370c, 0x03},
> > > > @@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> > > >       {0x3036, 0x62},
> > > >       {0x303c, 0x11},
> > > >       {0x3106, 0xf5},
> > > > -     {0x3821, 0x06},
> > > > +     {0x3821, 0x00},
> > > >       {0x3820, 0x00},
> > > >       {0x3827, 0xec},
> > > >       {0x370c, 0x03},
> > > > @@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> > > >       {0x4800, 0x24},
> > > >       {0x3503, 0x03},
> > > >       {0x3820, 0x41},
> > > > -     {0x3821, 0x07},
> > > > +     {0x3821, 0x01},
> > > >       {0x350a, 0x00},
> > > >       {0x350b, 0x10},
> > > >       {0x3500, 0x00},
> > > > @@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> > > >       {0x3035, 0x11},
> > > >       {0x3036, 0x46},
> > > >       {0x303c, 0x11},
> > > > -     {0x3821, 0x07},
> > > > +     {0x3821, 0x01},
> > > >       {0x3820, 0x41},
> > > >       {0x370c, 0x03},
> > > >       {0x3612, 0x59},
> > > > @@ -935,6 +939,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> > > >       .s_stream =             ov5647_s_stream,
> > > >  };
> > > >
> > > > +/*
> > > > + * This function returns the mbus code for the current settings of the HFLIP
> > > > + * and VFLIP controls.
> > > > + */
> > > > +static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
> > > > +{
> > > > +     struct ov5647 *sensor = to_sensor(sd);
> > > > +     /* The control values are only 0 or 1. */
> > > > +     int index =  sensor->hflip->val | (sensor->vflip->val << 1);
> > > > +
> > > > +     static const u32 codes[4] = {
> > > > +             MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > +             MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > +             MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > +             MEDIA_BUS_FMT_SGRBG10_1X10
> > > > +     };
> > > > +
> > > > +     return codes[index];
> > > > +}
> > > > +
> > > >  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> > > >                                struct v4l2_subdev_state *sd_state,
> > > >                                struct v4l2_subdev_mbus_code_enum *code)
> > > > @@ -942,7 +966,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> > > >       if (code->index > 0)
> > > >               return -EINVAL;
> > > >
> > > > -     code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > > > +     code->code = ov5647_get_mbus_code(sd);
> > > >
> > > >       return 0;
> > > >  }
> > > > @@ -953,7 +977,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
> > > >  {
> > > >       const struct v4l2_mbus_framefmt *fmt;
> > > >
> > > > -     if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
> > > > +     if (fse->code != ov5647_get_mbus_code(sd) ||
> > > >           fse->index >= ARRAY_SIZE(ov5647_modes))
> > > >               return -EINVAL;
> > > >
> > > > @@ -986,6 +1010,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
> > > >       }
> > > >
> > > >       *fmt = *sensor_format;
> > > > +     /* The code we pass back must reflect the current h/vflips. */
> > > > +     fmt->code = ov5647_get_mbus_code(sd);
> > > >       mutex_unlock(&sensor->lock);
> > > >
> > > >       return 0;
> > > > @@ -1033,6 +1059,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > > >                                        exposure_def);
> > > >       }
> > > >       *fmt = mode->format;
> > > > +     /* The code we pass back must reflect the current h/vflips. */
> > > > +     fmt->code = ov5647_get_mbus_code(sd);
> > > >       mutex_unlock(&sensor->lock);
> > > >
> > > >       return 0;
> > > > @@ -1208,6 +1236,25 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> > > >       return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> > > >  }
> > > >
> > > > +static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
> > > > +{
> > > > +     int ret;
> > > > +     u8 reg_val;
> > > > +
> > > > +     /* Set or clear bit 1 and leave everything else alone. */
> > > > +     ret = ov5647_read(sd, reg, &reg_val);
> > > > +     if (ret == 0) {
> > > > +             if (ctrl_val)
> > > > +                     reg_val |= 2;
> > > > +             else
> > > > +                     reg_val &= ~2;
> > > > +
> > > > +             ret = ov5647_write(sd, reg, reg_val);
> > > > +     }
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > >  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >  {
> > > >       struct ov5647 *sensor = container_of(ctrl->handler,
> > > > @@ -1270,6 +1317,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >               /* Read-only, but we adjust it based on mode. */
> > > >               break;
> > > >
> > > > +     case V4L2_CID_HFLIP:
> > > > +             /* There's an in-built hflip in the sensor, so account for that here. */
> > > > +             ov5647_s_flip(sd, OV5647_REG_HFLIP, !ctrl->val);
> > > > +             break;
> > > > +     case V4L2_CID_VFLIP:
> > > > +             ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
> > > > +             break;
> > >
> > > The modes definition used to set
> > >
> > >         0x3820 = 0x00
> > >         0x3821 = 0x06
> > >
> > > Is this the built-in hflip ?
> > >
> > > Or does it mean that setting the registers value to 1 'disabled' flips ?
> > >
> >
> > Yes, this particular sensor flips the image horizontally if 0x3820 = 0x00,
> > resulting in a mirrored image.
> >
> > But now that userspace can control flips, I am a bit confused on how to
> > report this in the controls.
> >
> > > > +
> > > >       default:
> > > >               dev_info(&client->dev,
> > > >                        "Control (id:0x%x, val:0x%x) not supported\n",
> > > > @@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > > >                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> > > >                                    0, 0, ov5647_test_pattern_menu);
> > > >
> > > > +     sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > > +                                       V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > > +     if (sensor->hflip)
> > > > +             sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > >
> > > I wonder if hflip is enabled by default we shouldn't register the
> > > control with default value of 1 ?
> > >
> >
> > My preference is to let the driver toggle the value for HFLIP before
> > writing to the sensor, as it is done currently in this patch.
> >
> > This makes this sensor behave like others, where you get a normal
> > non-mirrored image when you set HFLIP = 0, and mirrored if HFLIP = 1.
> >
> > The alternative is to keep the driver transparent by reporting the register
> > values as-is in the control. In that case, we should make horizontal_flip =
> > 1 the default value for the control. But I'm not aware if any userspace
> > applications will override the default value resulting in bad experience
> > for the users.
> >
> > WDYT?
> 
> My vote would be for the driver to invert the value before writing, as
> this patch does.
> 
> Making userspace have to call QUERYCTRL / QUERY_EXT_CTRL to get the
> default before it can then work out that it needs to invert any flip
> settings is additional overhead for zero gain.
> 
>   Dave
> 
> > Thanks,
> >     Jai
> >
> > > > +
> > > > +     sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > > +                                       V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > +     if (sensor->vflip)
> > > > +             sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > +
> > > >       v4l2_fwnode_device_parse(dev, &props);
> > > >
> > > >       v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> > > >
> > > > --
> > > > 2.51.0
> > > >

Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Jacopo Mondi]

Hi Jai, Dave

On Wed, Nov 12, 2025 at 02:35:49PM +0000, Dave Stevenson wrote:
> Hi Jai & Jacopo.
>
> On Wed, 12 Nov 2025 at 11:56, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Thanks a lot for the review.
> >
> > Quoting Jacopo Mondi (2025-11-02 16:20:36)
> > > Hi Jai
> > >
> > > On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> > > > From: David Plowman <david.plowman@raspberrypi.com>
> > > >
> > > > Add missing controls for horizontal and vertical flipping.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 71 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -54,6 +54,8 @@
> > > >  #define OV5647_REG_GAIN_LO           0x350b
> > > >  #define OV5647_REG_VTS_HI            0x380e
> > > >  #define OV5647_REG_VTS_LO            0x380f
> > > > +#define OV5647_REG_VFLIP             0x3820
> > > > +#define OV5647_REG_HFLIP             0x3821
> > > >  #define OV5647_REG_FRAME_OFF_NUMBER  0x4202
> > > >  #define OV5647_REG_MIPI_CTRL00               0x4800
> > > >  #define OV5647_REG_MIPI_CTRL14               0x4814
> > > > @@ -109,6 +111,8 @@ struct ov5647 {
> > > >       struct v4l2_ctrl                *hblank;
> > > >       struct v4l2_ctrl                *vblank;
> > > >       struct v4l2_ctrl                *exposure;
> > > > +     struct v4l2_ctrl                *hflip;
> > > > +     struct v4l2_ctrl                *vflip;
> > > >  };
> > > >
> > > >  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > > > @@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> > > >       {0x3036, 0x69},
> > > >       {0x303c, 0x11},
> > > >       {0x3106, 0xf5},
> > > > -     {0x3821, 0x06},
> > > > +     {0x3821, 0x00},
> > > >       {0x3820, 0x00},
> > >
> > > That's interesting, as the datasheet says that by default
> > >
> > >         3820 = 0x40
> > >         3821 = 0x00
> > >
> > > And
> > > - BIT[2] = flip ISP
> > > - BIT[1] = flip SNR
> > >
> > > The implementation of ov5647_s_flip() toggles BIT(1) and ignores
> > > BIT(2) while the modes definition have both (BIT(2) | BIT(1)) set
> > >
> > > More interestingly the datasheet says:
> > >
> > > In flip mode, the OV5647 does not need additional settings because the
> > > ISP block will auto-detect whether the pixel is in the red line or
> > > blue line and make the necessary adjustments
> > >
> > > Might this suggest that if we flip using BIT(2) we don't need to
> > > change the bayer pattern ordering ?
> >
> > Indeed! That was a great find, I am now able to set BIT(2) to get flips
> > without needing to modify the pixelarray layout.
> >
> > Will do that in v2.
>
> Are you going to update the rectangles reported by g_selection then?
> The sensor can't magically change the filter colour of the pixels
> being read out, therefore it must be reading different pixels.
> Figure 3-1 sensor array region color filter layout lists 2592x1944
> active pixels, and we're reading out all of those. "The backend
> processor can use the boundary pixels for additional processing", but
> how? There are no details.
>
> Yes it's minor, but in using BIT(1) we know exactly what the sensor is
> doing. Handling the change of Bayer order in the driver isn't that
> involved, and is common to so many sensors that clients have to
> support it anyway.
>
> None of the ISP features of OV5647 are used by this driver. I suspect
> the bits mentioned in the datasheet are probably a hang-over from
> OV5640 which is effectively the same sensor array with built-in ISP
> processing to produce YUYV images.
>
> Just my two-cents.

Agreed, we have everything in place to handle the bayer permutations
already, so no need to try to hide them

>
> > >
> > > Now, I admit I'm not sure what are the ISP functions on the ov5647 and
> > > this patch is super-duper-tested as it comes from the RPi BSP, so if
> > > you don't have answers to the above questions, I'm fine with this
> > > patch!
> > >
> > > >       {0x3827, 0xec},
> > > >       {0x370c, 0x03},
> > > > @@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> > > >       {0x3036, 0x62},
> > > >       {0x303c, 0x11},
> > > >       {0x3106, 0xf5},
> > > > -     {0x3821, 0x06},
> > > > +     {0x3821, 0x00},
> > > >       {0x3820, 0x00},
> > > >       {0x3827, 0xec},
> > > >       {0x370c, 0x03},
> > > > @@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> > > >       {0x4800, 0x24},
> > > >       {0x3503, 0x03},
> > > >       {0x3820, 0x41},
> > > > -     {0x3821, 0x07},
> > > > +     {0x3821, 0x01},
> > > >       {0x350a, 0x00},
> > > >       {0x350b, 0x10},
> > > >       {0x3500, 0x00},
> > > > @@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> > > >       {0x3035, 0x11},
> > > >       {0x3036, 0x46},
> > > >       {0x303c, 0x11},
> > > > -     {0x3821, 0x07},
> > > > +     {0x3821, 0x01},
> > > >       {0x3820, 0x41},
> > > >       {0x370c, 0x03},
> > > >       {0x3612, 0x59},
> > > > @@ -935,6 +939,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> > > >       .s_stream =             ov5647_s_stream,
> > > >  };
> > > >
> > > > +/*
> > > > + * This function returns the mbus code for the current settings of the HFLIP
> > > > + * and VFLIP controls.
> > > > + */
> > > > +static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
> > > > +{
> > > > +     struct ov5647 *sensor = to_sensor(sd);
> > > > +     /* The control values are only 0 or 1. */
> > > > +     int index =  sensor->hflip->val | (sensor->vflip->val << 1);
> > > > +
> > > > +     static const u32 codes[4] = {
> > > > +             MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > +             MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > +             MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > +             MEDIA_BUS_FMT_SGRBG10_1X10
> > > > +     };
> > > > +
> > > > +     return codes[index];
> > > > +}
> > > > +
> > > >  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> > > >                                struct v4l2_subdev_state *sd_state,
> > > >                                struct v4l2_subdev_mbus_code_enum *code)
> > > > @@ -942,7 +966,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> > > >       if (code->index > 0)
> > > >               return -EINVAL;
> > > >
> > > > -     code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > > > +     code->code = ov5647_get_mbus_code(sd);
> > > >
> > > >       return 0;
> > > >  }
> > > > @@ -953,7 +977,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
> > > >  {
> > > >       const struct v4l2_mbus_framefmt *fmt;
> > > >
> > > > -     if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
> > > > +     if (fse->code != ov5647_get_mbus_code(sd) ||
> > > >           fse->index >= ARRAY_SIZE(ov5647_modes))
> > > >               return -EINVAL;
> > > >
> > > > @@ -986,6 +1010,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
> > > >       }
> > > >
> > > >       *fmt = *sensor_format;
> > > > +     /* The code we pass back must reflect the current h/vflips. */
> > > > +     fmt->code = ov5647_get_mbus_code(sd);
> > > >       mutex_unlock(&sensor->lock);
> > > >
> > > >       return 0;
> > > > @@ -1033,6 +1059,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > > >                                        exposure_def);
> > > >       }
> > > >       *fmt = mode->format;
> > > > +     /* The code we pass back must reflect the current h/vflips. */
> > > > +     fmt->code = ov5647_get_mbus_code(sd);
> > > >       mutex_unlock(&sensor->lock);
> > > >
> > > >       return 0;
> > > > @@ -1208,6 +1236,25 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> > > >       return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> > > >  }
> > > >
> > > > +static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
> > > > +{
> > > > +     int ret;
> > > > +     u8 reg_val;
> > > > +
> > > > +     /* Set or clear bit 1 and leave everything else alone. */
> > > > +     ret = ov5647_read(sd, reg, &reg_val);
> > > > +     if (ret == 0) {
> > > > +             if (ctrl_val)
> > > > +                     reg_val |= 2;
> > > > +             else
> > > > +                     reg_val &= ~2;
> > > > +
> > > > +             ret = ov5647_write(sd, reg, reg_val);
> > > > +     }
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > >  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >  {
> > > >       struct ov5647 *sensor = container_of(ctrl->handler,
> > > > @@ -1270,6 +1317,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >               /* Read-only, but we adjust it based on mode. */
> > > >               break;
> > > >
> > > > +     case V4L2_CID_HFLIP:
> > > > +             /* There's an in-built hflip in the sensor, so account for that here. */
> > > > +             ov5647_s_flip(sd, OV5647_REG_HFLIP, !ctrl->val);
> > > > +             break;
> > > > +     case V4L2_CID_VFLIP:
> > > > +             ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
> > > > +             break;
> > >
> > > The modes definition used to set
> > >
> > >         0x3820 = 0x00
> > >         0x3821 = 0x06
> > >
> > > Is this the built-in hflip ?
> > >
> > > Or does it mean that setting the registers value to 1 'disabled' flips ?
> > >
> >
> > Yes, this particular sensor flips the image horizontally if 0x3820 = 0x00,
> > resulting in a mirrored image.
> >
> > But now that userspace can control flips, I am a bit confused on how to
> > report this in the controls.
> >
> > > > +
> > > >       default:
> > > >               dev_info(&client->dev,
> > > >                        "Control (id:0x%x, val:0x%x) not supported\n",
> > > > @@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > > >                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> > > >                                    0, 0, ov5647_test_pattern_menu);
> > > >
> > > > +     sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > > +                                       V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > > +     if (sensor->hflip)
> > > > +             sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > >
> > > I wonder if hflip is enabled by default we shouldn't register the
> > > control with default value of 1 ?
> > >
> >
> > My preference is to let the driver toggle the value for HFLIP before
> > writing to the sensor, as it is done currently in this patch.

As Jai reported
- writing 1 = disable flip
- writing 0 = enable flip

So if the sensor is initialized with 0x3821[1] = 1 it means flip is
disabled by default, so registering HFLIP as disabled is correct ?

> >
> > This makes this sensor behave like others, where you get a normal
> > non-mirrored image when you set HFLIP = 0, and mirrored if HFLIP = 1.
> >
> > The alternative is to keep the driver transparent by reporting the register
> > values as-is in the control. In that case, we should make horizontal_flip =
> > 1 the default value for the control. But I'm not aware if any userspace
> > applications will override the default value resulting in bad experience
> > for the users.
> >
> > WDYT?
>
> My vote would be for the driver to invert the value before writing, as
> this patch does.
>
> Making userspace have to call QUERYCTRL / QUERY_EXT_CTRL to get the
> default before it can then work out that it needs to invert any flip
> settings is additional overhead for zero gain.

I was just wondering what the default status is (regardless of the
actual register value). If hflip is disabled by default (confirmed by
the fact 0x3821[1]=1) , then the control should be initialized with 0
as default.

Thanks
  j

>
>   Dave
>
> > Thanks,
> >     Jai
> >
> > > > +
> > > > +     sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > > +                                       V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > +     if (sensor->vflip)
> > > > +             sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > +
> > > >       v4l2_fwnode_device_parse(dev, &props);
> > > >
> > > >       v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> > > >
> > > > --
> > > > 2.51.0
> > > >

[PATCH 07/13] media: i2c: ov5647: Add support for regulator control.

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The driver supported using GPIOs to control the shutdown line,
but no regulator control.

Add regulator hooks.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index a33e2d8edc114d302e830639cb7cb161f16a6208..598764638d518a28c8ac61ea590b996f09ecd45c 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/of_graph.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-ctrls.h>
@@ -83,6 +84,15 @@
 #define OV5647_EXPOSURE_DEFAULT		1000
 #define OV5647_EXPOSURE_MAX		65535
 
+/* regulator supplies */
+static const char * const ov5647_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dovdd",	/* Digital I/O power */
+	"dvdd",		/* Digital core power */
+};
+
+#define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
+
 struct regval_list {
 	u16 addr;
 	u8 data;
@@ -104,6 +114,7 @@ struct ov5647 {
 	struct mutex			lock;
 	struct clk			*xclk;
 	struct gpio_desc		*pwdn;
+	struct regulator_bulk_data supplies[OV5647_NUM_SUPPLIES];
 	bool				clock_ncont;
 	struct v4l2_ctrl_handler	ctrls;
 	const struct ov5647_mode	*mode;
@@ -781,6 +792,12 @@ static int ov5647_power_on(struct device *dev)
 
 	dev_dbg(dev, "OV5647 power on\n");
 
+	ret = regulator_bulk_enable(OV5647_NUM_SUPPLIES, sensor->supplies);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulators\n");
+		return ret;
+	}
+
 	if (sensor->pwdn) {
 		gpiod_set_value_cansleep(sensor->pwdn, 0);
 		msleep(PWDN_ACTIVE_DELAY_MS);
@@ -812,6 +829,7 @@ static int ov5647_power_on(struct device *dev)
 	clk_disable_unprepare(sensor->xclk);
 error_pwdn:
 	gpiod_set_value_cansleep(sensor->pwdn, 1);
+	regulator_bulk_disable(OV5647_NUM_SUPPLIES, sensor->supplies);
 
 	return ret;
 }
@@ -841,6 +859,7 @@ static int ov5647_power_off(struct device *dev)
 
 	clk_disable_unprepare(sensor->xclk);
 	gpiod_set_value_cansleep(sensor->pwdn, 1);
+	regulator_bulk_disable(OV5647_NUM_SUPPLIES, sensor->supplies);
 
 	return 0;
 }
@@ -1341,6 +1360,18 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
 	.s_ctrl = ov5647_s_ctrl,
 };
 
+static int ov5647_configure_regulators(struct device *dev,
+				       struct ov5647 *sensor)
+{
+	unsigned int i;
+
+	for (i = 0; i < OV5647_NUM_SUPPLIES; i++)
+		sensor->supplies[i].supply = ov5647_supply_names[i];
+
+	return devm_regulator_bulk_get(dev, OV5647_NUM_SUPPLIES,
+				       sensor->supplies);
+}
+
 static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
@@ -1489,6 +1520,12 @@ static int ov5647_probe(struct i2c_client *client)
 		return -EINVAL;
 	}
 
+	ret = ov5647_configure_regulators(dev, sensor);
+	if (ret) {
+		dev_err(dev, "Failed to get power regulators\n");
+		return ret;
+	}
+
 	mutex_init(&sensor->lock);
 
 	sensor->mode = OV5647_DEFAULT_MODE;

-- 
2.51.0

Re: [PATCH 07/13] media: i2c: ov5647: Add support for regulator control.

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:18PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> The driver supported using GPIOs to control the shutdown line,
> but no regulator control.
>
> Add regulator hooks.

Do bindings need an update ?

>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index a33e2d8edc114d302e830639cb7cb161f16a6208..598764638d518a28c8ac61ea590b996f09ecd45c 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -20,6 +20,7 @@
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
>  #include <media/v4l2-ctrls.h>
> @@ -83,6 +84,15 @@
>  #define OV5647_EXPOSURE_DEFAULT		1000
>  #define OV5647_EXPOSURE_MAX		65535
>
> +/* regulator supplies */
> +static const char * const ov5647_supply_names[] = {
> +	"avdd",		/* Analog power */
> +	"dovdd",	/* Digital I/O power */
> +	"dvdd",		/* Digital core power */
> +};
> +
> +#define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> +
>  struct regval_list {
>  	u16 addr;
>  	u8 data;
> @@ -104,6 +114,7 @@ struct ov5647 {
>  	struct mutex			lock;
>  	struct clk			*xclk;
>  	struct gpio_desc		*pwdn;
> +	struct regulator_bulk_data supplies[OV5647_NUM_SUPPLIES];

nit: please align 'supplies' to other members

>  	bool				clock_ncont;
>  	struct v4l2_ctrl_handler	ctrls;
>  	const struct ov5647_mode	*mode;
> @@ -781,6 +792,12 @@ static int ov5647_power_on(struct device *dev)
>
>  	dev_dbg(dev, "OV5647 power on\n");
>
> +	ret = regulator_bulk_enable(OV5647_NUM_SUPPLIES, sensor->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulators\n");
> +		return ret;
> +	}
> +
>  	if (sensor->pwdn) {
>  		gpiod_set_value_cansleep(sensor->pwdn, 0);

Should we disable regulators if this fail ?

Also, gpiod_set_value_cansleep() supports optional gpios, so you might
want to remove if (sensor->pwdn), check the return value of this
function and jump to a new label

>  		msleep(PWDN_ACTIVE_DELAY_MS);
> @@ -812,6 +829,7 @@ static int ov5647_power_on(struct device *dev)
>  	clk_disable_unprepare(sensor->xclk);
>  error_pwdn:
>  	gpiod_set_value_cansleep(sensor->pwdn, 1);
> +	regulator_bulk_disable(OV5647_NUM_SUPPLIES, sensor->supplies);
>
>  	return ret;
>  }
> @@ -841,6 +859,7 @@ static int ov5647_power_off(struct device *dev)
>
>  	clk_disable_unprepare(sensor->xclk);
>  	gpiod_set_value_cansleep(sensor->pwdn, 1);
> +	regulator_bulk_disable(OV5647_NUM_SUPPLIES, sensor->supplies);
>
>  	return 0;
>  }
> @@ -1341,6 +1360,18 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
>  	.s_ctrl = ov5647_s_ctrl,
>  };
>
> +static int ov5647_configure_regulators(struct device *dev,
> +				       struct ov5647 *sensor)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < OV5647_NUM_SUPPLIES; i++)

nit: you can now declared i inside the for loop

Thanks
  j

> +		sensor->supplies[i].supply = ov5647_supply_names[i];
> +
> +	return devm_regulator_bulk_get(dev, OV5647_NUM_SUPPLIES,
> +				       sensor->supplies);
> +}
> +
>  static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> @@ -1489,6 +1520,12 @@ static int ov5647_probe(struct i2c_client *client)
>  		return -EINVAL;
>  	}
>
> +	ret = ov5647_configure_regulators(dev, sensor);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power regulators\n");
> +		return ret;
> +	}
> +
>  	mutex_init(&sensor->lock);
>
>  	sensor->mode = OV5647_DEFAULT_MODE;
>
> --
> 2.51.0
>

Re: [PATCH 07/13] media: i2c: ov5647: Add support for regulator control.

[Stefan Wahren]

Am 28.10.25 um 08:27 schrieb Jai Luthra:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> The driver supported using GPIOs to control the shutdown line,
> but no regulator control.
>
> Add regulator hooks.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>   drivers/media/i2c/ov5647.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index a33e2d8edc114d302e830639cb7cb161f16a6208..598764638d518a28c8ac61ea590b996f09ecd45c 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -20,6 +20,7 @@
>   #include <linux/module.h>
>   #include <linux/of_graph.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   #include <linux/videodev2.h>
>   #include <media/v4l2-ctrls.h>
> @@ -83,6 +84,15 @@
>   #define OV5647_EXPOSURE_DEFAULT		1000
>   #define OV5647_EXPOSURE_MAX		65535
>   
> +/* regulator supplies */
> +static const char * const ov5647_supply_names[] = {
> +	"avdd",		/* Analog power */
> +	"dovdd",	/* Digital I/O power */
> +	"dvdd",		/* Digital core power */
> +};
> +
> +#define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> +
>   struct regval_list {
>   	u16 addr;
>   	u8 data;
> @@ -104,6 +114,7 @@ struct ov5647 {
>   	struct mutex			lock;
>   	struct clk			*xclk;
>   	struct gpio_desc		*pwdn;
> +	struct regulator_bulk_data supplies[OV5647_NUM_SUPPLIES];
>   	bool				clock_ncont;
>   	struct v4l2_ctrl_handler	ctrls;
>   	const struct ov5647_mode	*mode;
> @@ -781,6 +792,12 @@ static int ov5647_power_on(struct device *dev)
>   
>   	dev_dbg(dev, "OV5647 power on\n");
>   
> +	ret = regulator_bulk_enable(OV5647_NUM_SUPPLIES, sensor->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulators\n");
In case we want to log this, please add the return code as well
> +		return ret;
> +	}
> +
>   	if (sensor->pwdn) {
>   		gpiod_set_value_cansleep(sensor->pwdn, 0);
>   		msleep(PWDN_ACTIVE_DELAY_MS);
> @@ -812,6 +829,7 @@ static int ov5647_power_on(struct device *dev)
>   	clk_disable_unprepare(sensor->xclk);
>   error_pwdn:
>   	gpiod_set_value_cansleep(sensor->pwdn, 1);
> +	regulator_bulk_disable(OV5647_NUM_SUPPLIES, sensor->supplies);
>   
>   	return ret;
>   }
> @@ -841,6 +859,7 @@ static int ov5647_power_off(struct device *dev)
>   
>   	clk_disable_unprepare(sensor->xclk);
>   	gpiod_set_value_cansleep(sensor->pwdn, 1);
> +	regulator_bulk_disable(OV5647_NUM_SUPPLIES, sensor->supplies);
>   
>   	return 0;
>   }
> @@ -1341,6 +1360,18 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
>   	.s_ctrl = ov5647_s_ctrl,
>   };
>   
> +static int ov5647_configure_regulators(struct device *dev,
> +				       struct ov5647 *sensor)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < OV5647_NUM_SUPPLIES; i++)
> +		sensor->supplies[i].supply = ov5647_supply_names[i];
> +
> +	return devm_regulator_bulk_get(dev, OV5647_NUM_SUPPLIES,
> +				       sensor->supplies);
> +}
> +
>   static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>   {
>   	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> @@ -1489,6 +1520,12 @@ static int ov5647_probe(struct i2c_client *client)
>   		return -EINVAL;
>   	}
>   
> +	ret = ov5647_configure_regulators(dev, sensor);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power regulators\n");
Please use dev_err_probe here
> +		return ret;
> +	}
> +
>   	mutex_init(&sensor->lock);
>   
>   	sensor->mode = OV5647_DEFAULT_MODE;
>

[PATCH 08/13] media: i2c: ov5647: Use v4l2_async_register_subdev_sensor for lens binding

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

v4l2_async_register_subdev doesn't bind in lens or flash drivers,
but v4l2_async_register_subdev_sensor does.
Switch to using v4l2_async_register_subdev_sensor.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 598764638d518a28c8ac61ea590b996f09ecd45c..3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -1553,7 +1553,7 @@ static int ov5647_probe(struct i2c_client *client)
 	if (ret < 0)
 		goto power_off;
 
-	ret = v4l2_async_register_subdev(sd);
+	ret = v4l2_async_register_subdev_sensor(sd);
 	if (ret < 0)
 		goto power_off;
 

-- 
2.51.0

Re: [PATCH 08/13] media: i2c: ov5647: Use v4l2_async_register_subdev_sensor for lens binding

[Jacopo Mondi]

On Tue, Oct 28, 2025 at 12:57:19PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> v4l2_async_register_subdev doesn't bind in lens or flash drivers,
> but v4l2_async_register_subdev_sensor does.
> Switch to using v4l2_async_register_subdev_sensor.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 598764638d518a28c8ac61ea590b996f09ecd45c..3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1553,7 +1553,7 @@ static int ov5647_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		goto power_off;
>
> -	ret = v4l2_async_register_subdev(sd);
> +	ret = v4l2_async_register_subdev_sensor(sd);

We're really good at names :)

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

Thanks
  j

>  	if (ret < 0)
>  		goto power_off;
>
>
> --
> 2.51.0
>

[PATCH 09/13] media: i2c: ov5647: Add control of V4L2_CID_HBLANK

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The driver did expose V4L2_CID_HBLANK, but as a READ_ONLY control.

The sensor only uses the HTS register to control the line length,
so convert this control to read/write, with the appropriate ranges.
Adopt the old fixed values as the minimum values permitted in each
mode to avoid issues of it not streaming.

This should allow exposure times up to ~3 seconds (up from ~1sec).

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2..59c21b91d09d79f073a54871221f197a0bcf3aa2 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -53,6 +53,8 @@
 #define OV5647_REG_AEC_AGC		0x3503
 #define OV5647_REG_GAIN_HI		0x350a
 #define OV5647_REG_GAIN_LO		0x350b
+#define OV5647_REG_HTS_HI		0x380c
+#define OV5647_REG_HTS_LO		0x380d
 #define OV5647_REG_VTS_HI		0x380e
 #define OV5647_REG_VTS_LO		0x380f
 #define OV5647_REG_VFLIP		0x3820
@@ -79,6 +81,8 @@
 #define OV5647_VBLANK_MIN		24
 #define OV5647_VTS_MAX			32767
 
+#define OV5647_HTS_MAX			0x1fff
+
 #define OV5647_EXPOSURE_MIN		4
 #define OV5647_EXPOSURE_STEP		1
 #define OV5647_EXPOSURE_DEFAULT		1000
@@ -187,8 +191,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x3a19, 0xf8},
 	{0x3c01, 0x80},
 	{0x3b07, 0x0c},
-	{0x380c, 0x0b},
-	{0x380d, 0x1c},
 	{0x3814, 0x11},
 	{0x3815, 0x11},
 	{0x3708, 0x64},
@@ -276,8 +278,6 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
 	{0x3a19, 0xf8},
 	{0x3c01, 0x80},
 	{0x3b07, 0x0c},
-	{0x380c, 0x09},
-	{0x380d, 0x70},
 	{0x3814, 0x11},
 	{0x3815, 0x11},
 	{0x3708, 0x64},
@@ -375,8 +375,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 	{0x3809, 0x10},
 	{0x380a, 0x03},
 	{0x380b, 0xcc},
-	{0x380c, 0x07},
-	{0x380d, 0x68},
 	{0x3811, 0x0c},
 	{0x3813, 0x06},
 	{0x3814, 0x31},
@@ -450,8 +448,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
 	{0x3a19, 0xf8},
 	{0x3c01, 0x80},
 	{0x3b07, 0x0c},
-	{0x380c, 0x07},
-	{0x380d, 0x3c},
 	{0x3814, 0x35},
 	{0x3815, 0x35},
 	{0x3708, 0x64},
@@ -1061,7 +1057,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
 					 mode->pixel_rate, 1, mode->pixel_rate);
 
 		hblank = mode->hts - mode->format.width;
-		__v4l2_ctrl_modify_range(sensor->hblank, hblank, hblank, 1,
+		__v4l2_ctrl_modify_range(sensor->hblank, hblank,
+					 OV5647_HTS_MAX - mode->format.width, 1,
 					 hblank);
 
 		vblank = mode->vts - mode->format.height;
@@ -1325,6 +1322,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
 				     sensor->mode->format.height + ctrl->val);
 		break;
+	case V4L2_CID_HBLANK:
+		ret = ov5647_write16(sd, OV5647_REG_HTS_HI,
+				     sensor->mode->format.width + ctrl->val);
+		break;
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
 				   ov5647_test_pattern_val[ctrl->val]);
@@ -1332,7 +1333,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	/* Read-only, but we adjust it based on mode. */
 	case V4L2_CID_PIXEL_RATE:
-	case V4L2_CID_HBLANK:
 		/* Read-only, but we adjust it based on mode. */
 		break;
 
@@ -1409,10 +1409,11 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 					       sensor->mode->pixel_rate, 1,
 					       sensor->mode->pixel_rate);
 
-	/* By default, HBLANK is read only, but it does change per mode. */
 	hblank = sensor->mode->hts - sensor->mode->format.width;
 	sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
-					   V4L2_CID_HBLANK, hblank, hblank, 1,
+					   V4L2_CID_HBLANK, hblank,
+					   OV5647_HTS_MAX -
+					   sensor->mode->format.width, 1,
 					   hblank);
 
 	sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
@@ -1446,7 +1447,6 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 		goto handler_free;
 
 	sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 	sensor->sd.ctrl_handler = &sensor->ctrls;
 
 	return 0;

-- 
2.51.0

Re: [PATCH 09/13] media: i2c: ov5647: Add control of V4L2_CID_HBLANK

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:20PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> The driver did expose V4L2_CID_HBLANK, but as a READ_ONLY control.
>
> The sensor only uses the HTS register to control the line length,
> so convert this control to read/write, with the appropriate ranges.
> Adopt the old fixed values as the minimum values permitted in each
> mode to avoid issues of it not streaming.
>
> This should allow exposure times up to ~3 seconds (up from ~1sec).
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2..59c21b91d09d79f073a54871221f197a0bcf3aa2 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -53,6 +53,8 @@
>  #define OV5647_REG_AEC_AGC		0x3503
>  #define OV5647_REG_GAIN_HI		0x350a
>  #define OV5647_REG_GAIN_LO		0x350b
> +#define OV5647_REG_HTS_HI		0x380c
> +#define OV5647_REG_HTS_LO		0x380d
>  #define OV5647_REG_VTS_HI		0x380e
>  #define OV5647_REG_VTS_LO		0x380f
>  #define OV5647_REG_VFLIP		0x3820
> @@ -79,6 +81,8 @@
>  #define OV5647_VBLANK_MIN		24
>  #define OV5647_VTS_MAX			32767
>
> +#define OV5647_HTS_MAX			0x1fff
> +
>  #define OV5647_EXPOSURE_MIN		4
>  #define OV5647_EXPOSURE_STEP		1
>  #define OV5647_EXPOSURE_DEFAULT		1000
> @@ -187,8 +191,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>  	{0x3a19, 0xf8},
>  	{0x3c01, 0x80},
>  	{0x3b07, 0x0c},
> -	{0x380c, 0x0b},
> -	{0x380d, 0x1c},
>  	{0x3814, 0x11},
>  	{0x3815, 0x11},
>  	{0x3708, 0x64},
> @@ -276,8 +278,6 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
>  	{0x3a19, 0xf8},
>  	{0x3c01, 0x80},
>  	{0x3b07, 0x0c},
> -	{0x380c, 0x09},
> -	{0x380d, 0x70},
>  	{0x3814, 0x11},
>  	{0x3815, 0x11},
>  	{0x3708, 0x64},
> @@ -375,8 +375,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  	{0x3809, 0x10},
>  	{0x380a, 0x03},
>  	{0x380b, 0xcc},
> -	{0x380c, 0x07},
> -	{0x380d, 0x68},
>  	{0x3811, 0x0c},
>  	{0x3813, 0x06},
>  	{0x3814, 0x31},
> @@ -450,8 +448,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
>  	{0x3a19, 0xf8},
>  	{0x3c01, 0x80},
>  	{0x3b07, 0x0c},
> -	{0x380c, 0x07},
> -	{0x380d, 0x3c},
>  	{0x3814, 0x35},
>  	{0x3815, 0x35},
>  	{0x3708, 0x64},
> @@ -1061,7 +1057,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
>  					 mode->pixel_rate, 1, mode->pixel_rate);
>
>  		hblank = mode->hts - mode->format.width;
> -		__v4l2_ctrl_modify_range(sensor->hblank, hblank, hblank, 1,
> +		__v4l2_ctrl_modify_range(sensor->hblank, hblank,
> +					 OV5647_HTS_MAX - mode->format.width, 1,

Is '1' really the min ? Who knows the datasheet doesn't report that :(

>  					 hblank);
>
>  		vblank = mode->vts - mode->format.height;
> @@ -1325,6 +1322,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
>  				     sensor->mode->format.height + ctrl->val);
>  		break;
> +	case V4L2_CID_HBLANK:
> +		ret = ov5647_write16(sd, OV5647_REG_HTS_HI,
> +				     sensor->mode->format.width + ctrl->val);

Why are we writing HTS_HI only ? The max control value is set to
0x1fff, this spans two registers..

> +		break;
>  	case V4L2_CID_TEST_PATTERN:
>  		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
>  				   ov5647_test_pattern_val[ctrl->val]);
> @@ -1332,7 +1333,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>
>  	/* Read-only, but we adjust it based on mode. */
>  	case V4L2_CID_PIXEL_RATE:
> -	case V4L2_CID_HBLANK:
>  		/* Read-only, but we adjust it based on mode. */

We really like this comment, at the point of repeating it twice...

Speaking of which... if you set the ctrl_handler to NULL when
registering a ro control

	sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, NULL,
					       V4L2_CID_PIXEL_RATE,
					       sensor->mode->pixel_rate,
					       sensor->mode->pixel_rate, 1,
					       sensor->mode->pixel_rate);

you can remove the above 4 lines.

(the background is that for RO controls the control_handler has to be
set to NULL, to avoid having to handle them in the s_ctrl handler)

Maybe add a patch to this series ?

>  		break;
>
> @@ -1409,10 +1409,11 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  					       sensor->mode->pixel_rate, 1,
>  					       sensor->mode->pixel_rate);
>
> -	/* By default, HBLANK is read only, but it does change per mode. */
>  	hblank = sensor->mode->hts - sensor->mode->format.width;
>  	sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> -					   V4L2_CID_HBLANK, hblank, hblank, 1,
> +					   V4L2_CID_HBLANK, hblank,
> +					   OV5647_HTS_MAX -
> +					   sensor->mode->format.width, 1,
>  					   hblank);
>
>  	sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> @@ -1446,7 +1447,6 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  		goto handler_free;
>
>  	sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -	sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  	sensor->sd.ctrl_handler = &sensor->ctrls;
>
>  	return 0;
>
> --
> 2.51.0
>

Re: [PATCH 09/13] media: i2c: ov5647: Add control of V4L2_CID_HBLANK

[Jai Luthra]

Hi Jacopo,

Thanks for the review.

Quoting Jacopo Mondi (2025-11-02 16:37:55)
> Hi Jai
> 
> On Tue, Oct 28, 2025 at 12:57:20PM +0530, Jai Luthra wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > The driver did expose V4L2_CID_HBLANK, but as a READ_ONLY control.
> >
> > The sensor only uses the HTS register to control the line length,
> > so convert this control to read/write, with the appropriate ranges.
> > Adopt the old fixed values as the minimum values permitted in each
> > mode to avoid issues of it not streaming.
> >
> > This should allow exposure times up to ~3 seconds (up from ~1sec).
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 3aad3dc9b5cd0c24c07a37e2567e3c61c52e4fc2..59c21b91d09d79f073a54871221f197a0bcf3aa2 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -53,6 +53,8 @@
> >  #define OV5647_REG_AEC_AGC           0x3503
> >  #define OV5647_REG_GAIN_HI           0x350a
> >  #define OV5647_REG_GAIN_LO           0x350b
> > +#define OV5647_REG_HTS_HI            0x380c
> > +#define OV5647_REG_HTS_LO            0x380d
> >  #define OV5647_REG_VTS_HI            0x380e
> >  #define OV5647_REG_VTS_LO            0x380f
> >  #define OV5647_REG_VFLIP             0x3820
> > @@ -79,6 +81,8 @@
> >  #define OV5647_VBLANK_MIN            24
> >  #define OV5647_VTS_MAX                       32767
> >
> > +#define OV5647_HTS_MAX                       0x1fff
> > +
> >  #define OV5647_EXPOSURE_MIN          4
> >  #define OV5647_EXPOSURE_STEP         1
> >  #define OV5647_EXPOSURE_DEFAULT              1000
> > @@ -187,8 +191,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> >       {0x3a19, 0xf8},
> >       {0x3c01, 0x80},
> >       {0x3b07, 0x0c},
> > -     {0x380c, 0x0b},
> > -     {0x380d, 0x1c},
> >       {0x3814, 0x11},
> >       {0x3815, 0x11},
> >       {0x3708, 0x64},
> > @@ -276,8 +278,6 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> >       {0x3a19, 0xf8},
> >       {0x3c01, 0x80},
> >       {0x3b07, 0x0c},
> > -     {0x380c, 0x09},
> > -     {0x380d, 0x70},
> >       {0x3814, 0x11},
> >       {0x3815, 0x11},
> >       {0x3708, 0x64},
> > @@ -375,8 +375,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> >       {0x3809, 0x10},
> >       {0x380a, 0x03},
> >       {0x380b, 0xcc},
> > -     {0x380c, 0x07},
> > -     {0x380d, 0x68},
> >       {0x3811, 0x0c},
> >       {0x3813, 0x06},
> >       {0x3814, 0x31},
> > @@ -450,8 +448,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> >       {0x3a19, 0xf8},
> >       {0x3c01, 0x80},
> >       {0x3b07, 0x0c},
> > -     {0x380c, 0x07},
> > -     {0x380d, 0x3c},
> >       {0x3814, 0x35},
> >       {0x3815, 0x35},
> >       {0x3708, 0x64},
> > @@ -1061,7 +1057,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> >                                        mode->pixel_rate, 1, mode->pixel_rate);
> >
> >               hblank = mode->hts - mode->format.width;
> > -             __v4l2_ctrl_modify_range(sensor->hblank, hblank, hblank, 1,
> > +             __v4l2_ctrl_modify_range(sensor->hblank, hblank,
> > +                                      OV5647_HTS_MAX - mode->format.width, 1,
> 
> Is '1' really the min ? Who knows the datasheet doesn't report that :(
> 

I think 1 is set as the step value here, keeping the minimum equal to
default HTS - width (to be safe, as I presume the HTS values for a mode
come from the sensor manufacturer)

> >                                        hblank);
> >
> >               vblank = mode->vts - mode->format.height;
> > @@ -1325,6 +1322,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >               ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> >                                    sensor->mode->format.height + ctrl->val);
> >               break;
> > +     case V4L2_CID_HBLANK:
> > +             ret = ov5647_write16(sd, OV5647_REG_HTS_HI,
> > +                                  sensor->mode->format.width + ctrl->val);
> 
> Why are we writing HTS_HI only ? The max control value is set to
> 0x1fff, this spans two registers..
> 

IIUC ov5647_write16 sends two bytes of data over I2C, where it auto
increments the address when writing the second byte.

> > +             break;
> >       case V4L2_CID_TEST_PATTERN:
> >               ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> >                                  ov5647_test_pattern_val[ctrl->val]);
> > @@ -1332,7 +1333,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >
> >       /* Read-only, but we adjust it based on mode. */
> >       case V4L2_CID_PIXEL_RATE:
> > -     case V4L2_CID_HBLANK:
> >               /* Read-only, but we adjust it based on mode. */
> 
> We really like this comment, at the point of repeating it twice...
> 

Oops, you're right, I'll drop this in the cleanup commit for PIXEL_RATE
control.

> Speaking of which... if you set the ctrl_handler to NULL when
> registering a ro control
> 
>         sensor->pixel_rate = v4l2_ctrl_new_std(&sensor->ctrls, NULL,
>                                                V4L2_CID_PIXEL_RATE,
>                                                sensor->mode->pixel_rate,
>                                                sensor->mode->pixel_rate, 1,
>                                                sensor->mode->pixel_rate);
> 
> you can remove the above 4 lines.
> 
> (the background is that for RO controls the control_handler has to be
> set to NULL, to avoid having to handle them in the s_ctrl handler)
> 
> Maybe add a patch to this series ?

Nice, will do!

> 
> >               break;
> >
> > @@ -1409,10 +1409,11 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> >                                              sensor->mode->pixel_rate, 1,
> >                                              sensor->mode->pixel_rate);
> >
> > -     /* By default, HBLANK is read only, but it does change per mode. */
> >       hblank = sensor->mode->hts - sensor->mode->format.width;
> >       sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > -                                        V4L2_CID_HBLANK, hblank, hblank, 1,
> > +                                        V4L2_CID_HBLANK, hblank,
> > +                                        OV5647_HTS_MAX -
> > +                                        sensor->mode->format.width, 1,
> >                                          hblank);
> >
> >       sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > @@ -1446,7 +1447,6 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> >               goto handler_free;
> >
> >       sensor->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > -     sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >       sensor->sd.ctrl_handler = &sensor->ctrls;
> >
> >       return 0;
> >
> > --
> > 2.51.0
> >

Thanks,
Jai

[PATCH 10/13] media: i2c: ov5647: Tidy up mode registers to make the order common

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

To make comparisons of the mode registers easier, put the registers
for the binned and VGA modes in the same order as the others.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 59c21b91d09d79f073a54871221f197a0bcf3aa2..2c9f50fd20d99f2adce2a1fbe4289cf7aeea2ba4 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -343,6 +343,8 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 	{0x3036, 0x62},
 	{0x303c, 0x11},
 	{0x3106, 0xf5},
+	{0x3821, 0x01},
+	{0x3820, 0x41},
 	{0x3827, 0xec},
 	{0x370c, 0x03},
 	{0x3612, 0x59},
@@ -415,8 +417,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 	{0x4837, 0x16},
 	{0x4800, 0x24},
 	{0x3503, 0x03},
-	{0x3820, 0x41},
-	{0x3821, 0x01},
 	{0x350a, 0x00},
 	{0x350b, 0x10},
 	{0x3500, 0x00},
@@ -429,20 +429,27 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 static struct regval_list ov5647_640x480_10bpp[] = {
 	{0x0100, 0x00},
 	{0x0103, 0x01},
-	{0x3035, 0x11},
+	{0x3034, 0x1a},
+	{0x3035, 0x21},
 	{0x3036, 0x46},
 	{0x303c, 0x11},
+	{0x3106, 0xf5},
 	{0x3821, 0x01},
 	{0x3820, 0x41},
+	{0x3827, 0xec},
 	{0x370c, 0x03},
 	{0x3612, 0x59},
 	{0x3618, 0x00},
 	{0x5000, 0x06},
 	{0x5003, 0x08},
 	{0x5a00, 0x08},
-	{0x3000, 0xff},
-	{0x3001, 0xff},
-	{0x3002, 0xff},
+	{0x3000, 0x00},
+	{0x3001, 0x00},
+	{0x3002, 0x00},
+	{0x3016, 0x08},
+	{0x3017, 0xe0},
+	{0x3018, 0x44},
+	{0x301c, 0xf8},
 	{0x301d, 0xf0},
 	{0x3a18, 0x00},
 	{0x3a19, 0xf8},
@@ -468,6 +475,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
 	{0x3632, 0xe2},
 	{0x3633, 0x23},
 	{0x3634, 0x44},
+	{0x3636, 0x06},
 	{0x3620, 0x64},
 	{0x3621, 0xe0},
 	{0x3600, 0x37},
@@ -496,19 +504,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
 	{0x4001, 0x02},
 	{0x4004, 0x02},
 	{0x4000, 0x09},
-	{0x3000, 0x00},
-	{0x3001, 0x00},
-	{0x3002, 0x00},
-	{0x3017, 0xe0},
-	{0x301c, 0xfc},
-	{0x3636, 0x06},
-	{0x3016, 0x08},
-	{0x3827, 0xec},
-	{0x3018, 0x44},
-	{0x3035, 0x21},
-	{0x3106, 0xf5},
-	{0x3034, 0x1a},
-	{0x301c, 0xf8},
 	{0x4800, 0x34},
 	{0x3503, 0x03},
 	{0x0100, 0x01},

-- 
2.51.0

Re: [PATCH 10/13] media: i2c: ov5647: Tidy up mode registers to make the order common

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:21PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> To make comparisons of the mode registers easier, put the registers
> for the binned and VGA modes in the same order as the others.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 59c21b91d09d79f073a54871221f197a0bcf3aa2..2c9f50fd20d99f2adce2a1fbe4289cf7aeea2ba4 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -343,6 +343,8 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  	{0x3036, 0x62},
>  	{0x303c, 0x11},
>  	{0x3106, 0xf5},
> +	{0x3821, 0x01},
> +	{0x3820, 0x41},
>  	{0x3827, 0xec},
>  	{0x370c, 0x03},
>  	{0x3612, 0x59},
> @@ -415,8 +417,6 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  	{0x4837, 0x16},
>  	{0x4800, 0x24},
>  	{0x3503, 0x03},
> -	{0x3820, 0x41},
> -	{0x3821, 0x01},
>  	{0x350a, 0x00},
>  	{0x350b, 0x10},
>  	{0x3500, 0x00},
> @@ -429,20 +429,27 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  static struct regval_list ov5647_640x480_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
> -	{0x3035, 0x11},
> +	{0x3034, 0x1a},
> +	{0x3035, 0x21},

Why has the register value changed ?

>  	{0x3036, 0x46},
>  	{0x303c, 0x11},
> +	{0x3106, 0xf5},
>  	{0x3821, 0x01},
>  	{0x3820, 0x41},
> +	{0x3827, 0xec},
>  	{0x370c, 0x03},
>  	{0x3612, 0x59},
>  	{0x3618, 0x00},
>  	{0x5000, 0x06},
>  	{0x5003, 0x08},
>  	{0x5a00, 0x08},
> -	{0x3000, 0xff},
> -	{0x3001, 0xff},
> -	{0x3002, 0xff},
> +	{0x3000, 0x00},
> +	{0x3001, 0x00},
> +	{0x3002, 0x00},

Uh these ones changed as well, is it intentional ?

> +	{0x3016, 0x08},
> +	{0x3017, 0xe0},
> +	{0x3018, 0x44},
> +	{0x301c, 0xf8},
>  	{0x301d, 0xf0},
>  	{0x3a18, 0x00},
>  	{0x3a19, 0xf8},
> @@ -468,6 +475,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
>  	{0x3632, 0xe2},
>  	{0x3633, 0x23},
>  	{0x3634, 0x44},
> +	{0x3636, 0x06},
>  	{0x3620, 0x64},
>  	{0x3621, 0xe0},
>  	{0x3600, 0x37},
> @@ -496,19 +504,6 @@ static struct regval_list ov5647_640x480_10bpp[] = {
>  	{0x4001, 0x02},
>  	{0x4004, 0x02},
>  	{0x4000, 0x09},
> -	{0x3000, 0x00},
> -	{0x3001, 0x00},
> -	{0x3002, 0x00},

Ah, that's why!

> -	{0x3017, 0xe0},
> -	{0x301c, 0xfc},
> -	{0x3636, 0x06},
> -	{0x3016, 0x08},
> -	{0x3827, 0xec},
> -	{0x3018, 0x44},
> -	{0x3035, 0x21},
> -	{0x3106, 0xf5},
> -	{0x3034, 0x1a},
> -	{0x301c, 0xf8},

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

Thanks
  j

>  	{0x4800, 0x34},
>  	{0x3503, 0x03},
>  	{0x0100, 0x01},
>
> --
> 2.51.0
>

[PATCH 11/13] media: i2c: ov5647: Separate out the common registers.

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

There are many registers in common between all the modes.
Pull those out into one common table.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 243 ++++++++++-----------------------------------
 1 file changed, 50 insertions(+), 193 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 2c9f50fd20d99f2adce2a1fbe4289cf7aeea2ba4..a0aaf83759e089aea727c44b614c6a9f3237586c 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -161,22 +161,16 @@ static const struct regval_list sensor_oe_enable_regs[] = {
 	{0x3002, 0xe4},
 };
 
-static struct regval_list ov5647_2592x1944_10bpp[] = {
+static struct regval_list ov5647_common_regs[] = {
 	{0x0100, 0x00},
 	{0x0103, 0x01},
 	{0x3034, 0x1a},
 	{0x3035, 0x21},
-	{0x3036, 0x69},
 	{0x303c, 0x11},
 	{0x3106, 0xf5},
-	{0x3821, 0x00},
-	{0x3820, 0x00},
 	{0x3827, 0xec},
 	{0x370c, 0x03},
-	{0x3612, 0x5b},
-	{0x3618, 0x04},
 	{0x5000, 0x06},
-	{0x5002, 0x41},
 	{0x5003, 0x08},
 	{0x5a00, 0x08},
 	{0x3000, 0x00},
@@ -191,24 +185,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x3a19, 0xf8},
 	{0x3c01, 0x80},
 	{0x3b07, 0x0c},
-	{0x3814, 0x11},
-	{0x3815, 0x11},
-	{0x3708, 0x64},
-	{0x3709, 0x12},
-	{0x3808, 0x0a},
-	{0x3809, 0x20},
-	{0x380a, 0x07},
-	{0x380b, 0x98},
-	{0x3800, 0x00},
-	{0x3801, 0x00},
-	{0x3802, 0x00},
-	{0x3803, 0x00},
-	{0x3804, 0x0a},
-	{0x3805, 0x3f},
-	{0x3806, 0x07},
-	{0x3807, 0xa3},
-	{0x3811, 0x10},
-	{0x3813, 0x06},
 	{0x3630, 0x2e},
 	{0x3632, 0xe2},
 	{0x3633, 0x23},
@@ -228,11 +204,6 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x3f06, 0x10},
 	{0x3f01, 0x0a},
 	{0x3a08, 0x01},
-	{0x3a09, 0x28},
-	{0x3a0a, 0x00},
-	{0x3a0b, 0xf6},
-	{0x3a0d, 0x08},
-	{0x3a0e, 0x06},
 	{0x3a0f, 0x58},
 	{0x3a10, 0x50},
 	{0x3a1b, 0x58},
@@ -240,52 +211,57 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x3a11, 0x60},
 	{0x3a1f, 0x28},
 	{0x4001, 0x02},
-	{0x4004, 0x04},
 	{0x4000, 0x09},
+	{0x3503, 0x03},
+};
+
+static struct regval_list ov5647_2592x1944_10bpp[] = {
+	{0x3036, 0x69},
+	{0x3821, 0x00},
+	{0x3820, 0x00},
+	{0x3612, 0x5b},
+	{0x3618, 0x04},
+	{0x5002, 0x41},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3708, 0x64},
+	{0x3709, 0x12},
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x0a},
+	{0x3805, 0x3f},
+	{0x3806, 0x07},
+	{0x3807, 0xa3},
+	{0x3808, 0x0a},
+	{0x3809, 0x20},
+	{0x380a, 0x07},
+	{0x380b, 0x98},
+	{0x3811, 0x10},
+	{0x3813, 0x06},
+	{0x3a09, 0x28},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0xf6},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x4004, 0x04},
 	{0x4837, 0x19},
 	{0x4800, 0x24},
-	{0x3503, 0x03},
 	{0x0100, 0x01},
 };
 
 static struct regval_list ov5647_1080p30_10bpp[] = {
-	{0x0100, 0x00},
-	{0x0103, 0x01},
-	{0x3034, 0x1a},
-	{0x3035, 0x21},
 	{0x3036, 0x62},
-	{0x303c, 0x11},
-	{0x3106, 0xf5},
 	{0x3821, 0x00},
 	{0x3820, 0x00},
-	{0x3827, 0xec},
-	{0x370c, 0x03},
 	{0x3612, 0x5b},
 	{0x3618, 0x04},
-	{0x5000, 0x06},
 	{0x5002, 0x41},
-	{0x5003, 0x08},
-	{0x5a00, 0x08},
-	{0x3000, 0x00},
-	{0x3001, 0x00},
-	{0x3002, 0x00},
-	{0x3016, 0x08},
-	{0x3017, 0xe0},
-	{0x3018, 0x44},
-	{0x301c, 0xf8},
-	{0x301d, 0xf0},
-	{0x3a18, 0x00},
-	{0x3a19, 0xf8},
-	{0x3c01, 0x80},
-	{0x3b07, 0x0c},
 	{0x3814, 0x11},
 	{0x3815, 0x11},
 	{0x3708, 0x64},
 	{0x3709, 0x12},
-	{0x3808, 0x07},
-	{0x3809, 0x80},
-	{0x380a, 0x04},
-	{0x380b, 0x38},
 	{0x3800, 0x01},
 	{0x3801, 0x5c},
 	{0x3802, 0x01},
@@ -294,77 +270,30 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
 	{0x3805, 0xe3},
 	{0x3806, 0x05},
 	{0x3807, 0xf1},
+	{0x3808, 0x07},
+	{0x3809, 0x80},
+	{0x380a, 0x04},
+	{0x380b, 0x38},
 	{0x3811, 0x04},
 	{0x3813, 0x02},
-	{0x3630, 0x2e},
-	{0x3632, 0xe2},
-	{0x3633, 0x23},
-	{0x3634, 0x44},
-	{0x3636, 0x06},
-	{0x3620, 0x64},
-	{0x3621, 0xe0},
-	{0x3600, 0x37},
-	{0x3704, 0xa0},
-	{0x3703, 0x5a},
-	{0x3715, 0x78},
-	{0x3717, 0x01},
-	{0x3731, 0x02},
-	{0x370b, 0x60},
-	{0x3705, 0x1a},
-	{0x3f05, 0x02},
-	{0x3f06, 0x10},
-	{0x3f01, 0x0a},
-	{0x3a08, 0x01},
 	{0x3a09, 0x4b},
 	{0x3a0a, 0x01},
 	{0x3a0b, 0x13},
 	{0x3a0d, 0x04},
 	{0x3a0e, 0x03},
-	{0x3a0f, 0x58},
-	{0x3a10, 0x50},
-	{0x3a1b, 0x58},
-	{0x3a1e, 0x50},
-	{0x3a11, 0x60},
-	{0x3a1f, 0x28},
-	{0x4001, 0x02},
 	{0x4004, 0x04},
-	{0x4000, 0x09},
 	{0x4837, 0x19},
 	{0x4800, 0x34},
-	{0x3503, 0x03},
 	{0x0100, 0x01},
 };
 
 static struct regval_list ov5647_2x2binned_10bpp[] = {
-	{0x0100, 0x00},
-	{0x0103, 0x01},
-	{0x3034, 0x1a},
-	{0x3035, 0x21},
 	{0x3036, 0x62},
-	{0x303c, 0x11},
-	{0x3106, 0xf5},
 	{0x3821, 0x01},
 	{0x3820, 0x41},
-	{0x3827, 0xec},
-	{0x370c, 0x03},
 	{0x3612, 0x59},
 	{0x3618, 0x00},
-	{0x5000, 0x06},
 	{0x5002, 0x41},
-	{0x5003, 0x08},
-	{0x5a00, 0x08},
-	{0x3000, 0x00},
-	{0x3001, 0x00},
-	{0x3002, 0x00},
-	{0x3016, 0x08},
-	{0x3017, 0xe0},
-	{0x3018, 0x44},
-	{0x301c, 0xf8},
-	{0x301d, 0xf0},
-	{0x3a18, 0x00},
-	{0x3a19, 0xf8},
-	{0x3c01, 0x80},
-	{0x3b07, 0x0c},
 	{0x3800, 0x00},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
@@ -381,42 +310,14 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 	{0x3813, 0x06},
 	{0x3814, 0x31},
 	{0x3815, 0x31},
-	{0x3630, 0x2e},
-	{0x3632, 0xe2},
-	{0x3633, 0x23},
-	{0x3634, 0x44},
-	{0x3636, 0x06},
-	{0x3620, 0x64},
-	{0x3621, 0xe0},
-	{0x3600, 0x37},
-	{0x3704, 0xa0},
-	{0x3703, 0x5a},
-	{0x3715, 0x78},
-	{0x3717, 0x01},
-	{0x3731, 0x02},
-	{0x370b, 0x60},
-	{0x3705, 0x1a},
-	{0x3f05, 0x02},
-	{0x3f06, 0x10},
-	{0x3f01, 0x0a},
-	{0x3a08, 0x01},
 	{0x3a09, 0x28},
 	{0x3a0a, 0x00},
 	{0x3a0b, 0xf6},
 	{0x3a0d, 0x08},
 	{0x3a0e, 0x06},
-	{0x3a0f, 0x58},
-	{0x3a10, 0x50},
-	{0x3a1b, 0x58},
-	{0x3a1e, 0x50},
-	{0x3a11, 0x60},
-	{0x3a1f, 0x28},
-	{0x4001, 0x02},
 	{0x4004, 0x04},
-	{0x4000, 0x09},
 	{0x4837, 0x16},
 	{0x4800, 0x24},
-	{0x3503, 0x03},
 	{0x350a, 0x00},
 	{0x350b, 0x10},
 	{0x3500, 0x00},
@@ -427,42 +328,15 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 };
 
 static struct regval_list ov5647_640x480_10bpp[] = {
-	{0x0100, 0x00},
-	{0x0103, 0x01},
-	{0x3034, 0x1a},
-	{0x3035, 0x21},
 	{0x3036, 0x46},
-	{0x303c, 0x11},
-	{0x3106, 0xf5},
 	{0x3821, 0x01},
 	{0x3820, 0x41},
-	{0x3827, 0xec},
-	{0x370c, 0x03},
 	{0x3612, 0x59},
 	{0x3618, 0x00},
-	{0x5000, 0x06},
-	{0x5003, 0x08},
-	{0x5a00, 0x08},
-	{0x3000, 0x00},
-	{0x3001, 0x00},
-	{0x3002, 0x00},
-	{0x3016, 0x08},
-	{0x3017, 0xe0},
-	{0x3018, 0x44},
-	{0x301c, 0xf8},
-	{0x301d, 0xf0},
-	{0x3a18, 0x00},
-	{0x3a19, 0xf8},
-	{0x3c01, 0x80},
-	{0x3b07, 0x0c},
 	{0x3814, 0x35},
 	{0x3815, 0x35},
 	{0x3708, 0x64},
 	{0x3709, 0x52},
-	{0x3808, 0x02},
-	{0x3809, 0x80},
-	{0x380a, 0x01},
-	{0x380b, 0xe0},
 	{0x3800, 0x00},
 	{0x3801, 0x10},
 	{0x3802, 0x00},
@@ -471,41 +345,17 @@ static struct regval_list ov5647_640x480_10bpp[] = {
 	{0x3805, 0x2f},
 	{0x3806, 0x07},
 	{0x3807, 0x9f},
-	{0x3630, 0x2e},
-	{0x3632, 0xe2},
-	{0x3633, 0x23},
-	{0x3634, 0x44},
-	{0x3636, 0x06},
-	{0x3620, 0x64},
-	{0x3621, 0xe0},
-	{0x3600, 0x37},
-	{0x3704, 0xa0},
-	{0x3703, 0x5a},
-	{0x3715, 0x78},
-	{0x3717, 0x01},
-	{0x3731, 0x02},
-	{0x370b, 0x60},
-	{0x3705, 0x1a},
-	{0x3f05, 0x02},
-	{0x3f06, 0x10},
-	{0x3f01, 0x0a},
-	{0x3a08, 0x01},
+	{0x3808, 0x02},
+	{0x3809, 0x80},
+	{0x380a, 0x01},
+	{0x380b, 0xe0},
 	{0x3a09, 0x2e},
 	{0x3a0a, 0x00},
 	{0x3a0b, 0xfb},
 	{0x3a0d, 0x02},
 	{0x3a0e, 0x01},
-	{0x3a0f, 0x58},
-	{0x3a10, 0x50},
-	{0x3a1b, 0x58},
-	{0x3a1e, 0x50},
-	{0x3a11, 0x60},
-	{0x3a1f, 0x28},
-	{0x4001, 0x02},
 	{0x4004, 0x02},
-	{0x4000, 0x09},
 	{0x4800, 0x34},
-	{0x3503, 0x03},
 	{0x0100, 0x01},
 };
 
@@ -701,6 +551,13 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
 	if (ret < 0)
 		return ret;
 
+	ret = ov5647_write_array(sd, ov5647_common_regs,
+				 ARRAY_SIZE(ov5647_common_regs));
+	if (ret < 0) {
+		dev_err(&client->dev, "write sensor common regs error\n");
+		return ret;
+	}
+
 	ret = ov5647_write_array(sd, sensor->mode->reg_list,
 				 sensor->mode->num_regs);
 	if (ret < 0) {

-- 
2.51.0

[PATCH 12/13] media: i2c: ov5647: Use the same PLL config for full, 1080p, and binned modes

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

In order to simplify the driver slightly, use the same PLL
configuration, and hence pixel rate and link frequency (to be
added) for the full, 1080p, and binned modes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index a0aaf83759e089aea727c44b614c6a9f3237586c..be0b96c4372ae0c6d8fc57280b195d6069dd7019 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -252,7 +252,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 };
 
 static struct regval_list ov5647_1080p30_10bpp[] = {
-	{0x3036, 0x62},
+	{0x3036, 0x69},
 	{0x3821, 0x00},
 	{0x3820, 0x00},
 	{0x3612, 0x5b},
@@ -288,7 +288,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
 };
 
 static struct regval_list ov5647_2x2binned_10bpp[] = {
-	{0x3036, 0x62},
+	{0x3036, 0x69},
 	{0x3821, 0x01},
 	{0x3820, 0x41},
 	{0x3612, 0x59},
@@ -396,7 +396,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 			.width		= 1928,
 			.height		= 1080,
 		},
-		.pixel_rate	= 81666700,
+		.pixel_rate	= 87500000,
 		.hts		= 2416,
 		.vts		= 0x450,
 		.reg_list	= ov5647_1080p30_10bpp,
@@ -417,7 +417,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 			.width		= 2592,
 			.height		= 1944,
 		},
-		.pixel_rate	= 81666700,
+		.pixel_rate	= 87500000,
 		.hts		= 1896,
 		.vts		= 0x59b,
 		.reg_list	= ov5647_2x2binned_10bpp,

-- 
2.51.0

Re: [PATCH 12/13] media: i2c: ov5647: Use the same PLL config for full, 1080p, and binned modes

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:23PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> In order to simplify the driver slightly, use the same PLL
> configuration, and hence pixel rate and link frequency (to be
> added) for the full, 1080p, and binned modes.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index a0aaf83759e089aea727c44b614c6a9f3237586c..be0b96c4372ae0c6d8fc57280b195d6069dd7019 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -252,7 +252,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>  };
>
>  static struct regval_list ov5647_1080p30_10bpp[] = {
> -	{0x3036, 0x62},
> +	{0x3036, 0x69},
>  	{0x3821, 0x00},
>  	{0x3820, 0x00},
>  	{0x3612, 0x5b},
> @@ -288,7 +288,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
>  };
>
>  static struct regval_list ov5647_2x2binned_10bpp[] = {
> -	{0x3036, 0x62},
> +	{0x3036, 0x69},
>  	{0x3821, 0x01},
>  	{0x3820, 0x41},
>  	{0x3612, 0x59},
> @@ -396,7 +396,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  			.width		= 1928,
>  			.height		= 1080,
>  		},
> -		.pixel_rate	= 81666700,
> +		.pixel_rate	= 87500000,

I get a value of 87499965, so I guess this is correct approximation

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

Thanks
  j

>  		.hts		= 2416,
>  		.vts		= 0x450,
>  		.reg_list	= ov5647_1080p30_10bpp,
> @@ -417,7 +417,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  			.width		= 2592,
>  			.height		= 1944,
>  		},
> -		.pixel_rate	= 81666700,
> +		.pixel_rate	= 87500000,
>  		.hts		= 1896,
>  		.vts		= 0x59b,
>  		.reg_list	= ov5647_2x2binned_10bpp,
>
> --
> 2.51.0
>

[PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The link frequency can vary between modes, so add it as a
control.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
 
 #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
 
+#define FREQ_INDEX_FULL		0
+#define FREQ_INDEX_VGA		1
+static const s64 ov5647_link_freqs[] = {
+	[FREQ_INDEX_FULL]	= 218500000,
+	[FREQ_INDEX_VGA]	= 208333000,
+};
+
 struct regval_list {
 	u16 addr;
 	u8 data;
@@ -106,6 +113,7 @@ struct ov5647_mode {
 	struct v4l2_mbus_framefmt	format;
 	struct v4l2_rect		crop;
 	u64				pixel_rate;
+	unsigned int			link_freq_index;
 	int				hts;
 	int				vts;
 	const struct regval_list	*reg_list;
@@ -128,6 +136,7 @@ struct ov5647 {
 	struct v4l2_ctrl		*exposure;
 	struct v4l2_ctrl		*hflip;
 	struct v4l2_ctrl		*vflip;
+	struct v4l2_ctrl		*link_freq;
 };
 
 static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
@@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 			.height		= 1944
 		},
 		.pixel_rate	= 87500000,
+		.link_freq_index = FREQ_INDEX_FULL,
 		.hts		= 2844,
 		.vts		= 0x7b0,
 		.reg_list	= ov5647_2592x1944_10bpp,
@@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 			.height		= 1080,
 		},
 		.pixel_rate	= 87500000,
+		.link_freq_index = FREQ_INDEX_FULL,
 		.hts		= 2416,
 		.vts		= 0x450,
 		.reg_list	= ov5647_1080p30_10bpp,
@@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 			.height		= 1944,
 		},
 		.pixel_rate	= 87500000,
+		.link_freq_index = FREQ_INDEX_FULL,
 		.hts		= 1896,
 		.vts		= 0x59b,
 		.reg_list	= ov5647_2x2binned_10bpp,
@@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
 			.height		= 1920,
 		},
 		.pixel_rate	= 55000000,
+		.link_freq_index = FREQ_INDEX_VGA,
 		.hts		= 1852,
 		.vts		= 0x1f8,
 		.reg_list	= ov5647_640x480_10bpp,
@@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
 					 sensor->exposure->minimum,
 					 exposure_max, sensor->exposure->step,
 					 exposure_def);
+
+		__v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
 	}
 	*fmt = mode->format;
 	/* The code we pass back must reflect the current h/vflips. */
@@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 	int hblank, exposure_max, exposure_def;
 	struct v4l2_fwnode_device_properties props;
 
-	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
+	v4l2_ctrl_handler_init(&sensor->ctrls, 10);
 
 	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
@@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
 	if (sensor->vflip)
 		sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
+	sensor->link_freq =
+		v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
+				       V4L2_CID_LINK_FREQ,
+				       ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
+				       ov5647_link_freqs);
+	if (sensor->link_freq)
+		sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	v4l2_fwnode_device_parse(dev, &props);
 
 	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,

-- 
2.51.0

Re: [PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

[Jacopo Mondi]

Hi Jai

On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> The link frequency can vary between modes, so add it as a
> control.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
>
>  #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
>
> +#define FREQ_INDEX_FULL		0
> +#define FREQ_INDEX_VGA		1
> +static const s64 ov5647_link_freqs[] = {
> +	[FREQ_INDEX_FULL]	= 218500000,

The full mode pixel rate is set to 87500000, which considering CSI-2
DDR mode and the 2 lanes in use give me a link freq of 21875000.

Do you know where 218500000 comes from ? (it might be perfectly legit,
I'm not questioning that).

> +	[FREQ_INDEX_VGA]	= 208333000,
> +};
> +
>  struct regval_list {
>  	u16 addr;
>  	u8 data;
> @@ -106,6 +113,7 @@ struct ov5647_mode {
>  	struct v4l2_mbus_framefmt	format;
>  	struct v4l2_rect		crop;
>  	u64				pixel_rate;
> +	unsigned int			link_freq_index;
>  	int				hts;
>  	int				vts;
>  	const struct regval_list	*reg_list;
> @@ -128,6 +136,7 @@ struct ov5647 {
>  	struct v4l2_ctrl		*exposure;
>  	struct v4l2_ctrl		*hflip;
>  	struct v4l2_ctrl		*vflip;
> +	struct v4l2_ctrl		*link_freq;
>  };
>
>  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  			.height		= 1944
>  		},
>  		.pixel_rate	= 87500000,
> +		.link_freq_index = FREQ_INDEX_FULL,
>  		.hts		= 2844,
>  		.vts		= 0x7b0,
>  		.reg_list	= ov5647_2592x1944_10bpp,
> @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  			.height		= 1080,
>  		},
>  		.pixel_rate	= 87500000,
> +		.link_freq_index = FREQ_INDEX_FULL,
>  		.hts		= 2416,
>  		.vts		= 0x450,
>  		.reg_list	= ov5647_1080p30_10bpp,
> @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  			.height		= 1944,
>  		},
>  		.pixel_rate	= 87500000,
> +		.link_freq_index = FREQ_INDEX_FULL,
>  		.hts		= 1896,
>  		.vts		= 0x59b,
>  		.reg_list	= ov5647_2x2binned_10bpp,
> @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
>  			.height		= 1920,
>  		},
>  		.pixel_rate	= 55000000,
> +		.link_freq_index = FREQ_INDEX_VGA,
>  		.hts		= 1852,
>  		.vts		= 0x1f8,
>  		.reg_list	= ov5647_640x480_10bpp,
> @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
>  					 sensor->exposure->minimum,
>  					 exposure_max, sensor->exposure->step,
>  					 exposure_def);
> +
> +		__v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);

Doesn't this cause an error in s_ctrl where the control is not handled
?

>  	}
>  	*fmt = mode->format;
>  	/* The code we pass back must reflect the current h/vflips. */
> @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  	int hblank, exposure_max, exposure_def;
>  	struct v4l2_fwnode_device_properties props;
>
> -	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> +	v4l2_ctrl_handler_init(&sensor->ctrls, 10);
>
>  	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
>  			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
>  	if (sensor->vflip)
>  		sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> +	sensor->link_freq =
> +		v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,

As suggested for PIXEL_RATE, if you make the control read-only you
should set the control ops to NULL.
> +				       V4L2_CID_LINK_FREQ,
> +				       ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> +				       ov5647_link_freqs);
> +	if (sensor->link_freq)
> +		sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;

You know, I thought link_freq was set as READ_ONLY by the framework,
but it's actuallt PIXEL_RATE (you can remove setting the flags
in the driver if you send a patch to remove the control ops when
registering PIXEL_RATE).

Thanks
  j

> +
>  	v4l2_fwnode_device_parse(dev, &props);
>
>  	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
>
> --
> 2.51.0
>

Re: [PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

[Jai Luthra]

Hi Jacopo,

Quoting Jacopo Mondi (2025-11-02 16:59:02)
> Hi Jai
> 
> On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > The link frequency can vary between modes, so add it as a
> > control.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> >
> >  #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> >
> > +#define FREQ_INDEX_FULL              0
> > +#define FREQ_INDEX_VGA               1
> > +static const s64 ov5647_link_freqs[] = {
> > +     [FREQ_INDEX_FULL]       = 218500000,
> 
> The full mode pixel rate is set to 87500000, which considering CSI-2
> DDR mode and the 2 lanes in use give me a link freq of 21875000.

Indeed, I get the same value, will update.

> 
> Do you know where 218500000 comes from ? (it might be perfectly legit,
> I'm not questioning that).
> 

> > +     [FREQ_INDEX_VGA]        = 208333000,

This value should be 137500000 if we do the same calculation using the
pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
binning + 2x2 subsampling, which is quite a bit different than other modes.

https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
that the pixel rate value calculated from the bus link frequency does not
necessarily have to match the PIXEL_RATE control value (which is for the
sensor's internal readout of pixels including blanking).

Ultimately, these values are coming from the BSP where the CFE driver is
using the link frequency control to configure the DPHY-RX rate, so I think
it would be wiser to not reduce the VGA link frequency value, which may
cause issues with DPHY-RX latching. We can always fix it later if needed.

> > +};
> > +
> >  struct regval_list {
> >       u16 addr;
> >       u8 data;
> > @@ -106,6 +113,7 @@ struct ov5647_mode {
> >       struct v4l2_mbus_framefmt       format;
> >       struct v4l2_rect                crop;
> >       u64                             pixel_rate;
> > +     unsigned int                    link_freq_index;
> >       int                             hts;
> >       int                             vts;
> >       const struct regval_list        *reg_list;
> > @@ -128,6 +136,7 @@ struct ov5647 {
> >       struct v4l2_ctrl                *exposure;
> >       struct v4l2_ctrl                *hflip;
> >       struct v4l2_ctrl                *vflip;
> > +     struct v4l2_ctrl                *link_freq;
> >  };
> >
> >  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> >                       .height         = 1944
> >               },
> >               .pixel_rate     = 87500000,
> > +             .link_freq_index = FREQ_INDEX_FULL,
> >               .hts            = 2844,
> >               .vts            = 0x7b0,
> >               .reg_list       = ov5647_2592x1944_10bpp,
> > @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> >                       .height         = 1080,
> >               },
> >               .pixel_rate     = 87500000,
> > +             .link_freq_index = FREQ_INDEX_FULL,
> >               .hts            = 2416,
> >               .vts            = 0x450,
> >               .reg_list       = ov5647_1080p30_10bpp,
> > @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> >                       .height         = 1944,
> >               },
> >               .pixel_rate     = 87500000,
> > +             .link_freq_index = FREQ_INDEX_FULL,
> >               .hts            = 1896,
> >               .vts            = 0x59b,
> >               .reg_list       = ov5647_2x2binned_10bpp,
> > @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> >                       .height         = 1920,
> >               },
> >               .pixel_rate     = 55000000,
> > +             .link_freq_index = FREQ_INDEX_VGA,
> >               .hts            = 1852,
> >               .vts            = 0x1f8,
> >               .reg_list       = ov5647_640x480_10bpp,
> > @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> >                                        sensor->exposure->minimum,
> >                                        exposure_max, sensor->exposure->step,
> >                                        exposure_def);
> > +
> > +             __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
> 
> Doesn't this cause an error in s_ctrl where the control is not handled
> ?

The framework returns -EACCESS for read-only controls in validate_ctrls()

> 
> >       }
> >       *fmt = mode->format;
> >       /* The code we pass back must reflect the current h/vflips. */
> > @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> >       int hblank, exposure_max, exposure_def;
> >       struct v4l2_fwnode_device_properties props;
> >
> > -     v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > +     v4l2_ctrl_handler_init(&sensor->ctrls, 10);
> >
> >       v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> >                         V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> >       if (sensor->vflip)
> >               sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >
> > +     sensor->link_freq =
> > +             v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
> 
> As suggested for PIXEL_RATE, if you make the control read-only you
> should set the control ops to NULL.

Will do in v2.

> > +                                    V4L2_CID_LINK_FREQ,
> > +                                    ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> > +                                    ov5647_link_freqs);
> > +     if (sensor->link_freq)
> > +             sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
> You know, I thought link_freq was set as READ_ONLY by the framework,
> but it's actuallt PIXEL_RATE (you can remove setting the flags
> in the driver if you send a patch to remove the control ops when
> registering PIXEL_RATE).

Will do.

> 
> Thanks
>   j
> 
> > +
> >       v4l2_fwnode_device_parse(dev, &props);
> >
> >       v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> >
> > --
> > 2.51.0
> >

Thanks,
Jai

Re: [PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

[Dave Stevenson]

Hi Jacopo & Jai

On Tue, 18 Nov 2025 at 11:28, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-11-02 16:59:02)
> > Hi Jai
> >
> > On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >
> > > The link frequency can vary between modes, so add it as a
> > > control.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> > >
> > >  #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> > >
> > > +#define FREQ_INDEX_FULL              0
> > > +#define FREQ_INDEX_VGA               1
> > > +static const s64 ov5647_link_freqs[] = {
> > > +     [FREQ_INDEX_FULL]       = 218500000,
> >
> > The full mode pixel rate is set to 87500000, which considering CSI-2
> > DDR mode and the 2 lanes in use give me a link freq of 21875000.
>
> Indeed, I get the same value, will update.

Agreed. I obviously lost a digit.

> >
> > Do you know where 218500000 comes from ? (it might be perfectly legit,
> > I'm not questioning that).
> >
>
> > > +     [FREQ_INDEX_VGA]        = 208333000,
>
> This value should be 137500000 if we do the same calculation using the
> pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
> binning + 2x2 subsampling, which is quite a bit different than other modes.
>
> https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
> that the pixel rate value calculated from the bus link frequency does not
> necessarily have to match the PIXEL_RATE control value (which is for the
> sensor's internal readout of pixels including blanking).

Indeed you should never assume that pixel rate and link frequency are
directly linked. So many sensors have separate PLLs for the pixel
array vs the MIPI block.

Having said that, OV5647 appears to use the same PLL for pixel clock
and MIPI, although it does have a separate PLLADCLK which is
presumably for the ADC.

> Ultimately, these values are coming from the BSP where the CFE driver is
> using the link frequency control to configure the DPHY-RX rate, so I think
> it would be wiser to not reduce the VGA link frequency value, which may
> cause issues with DPHY-RX latching. We can always fix it later if needed.

It's been a long time since I looked at these settings, but I do have
a spreadsheet from Omnivision that gives clock frequencies based on
register values.
In my experience the link frequency isn't critical to be exactly right
as it typically only sets up timeout ranges in the PHY. Even if the
value is significantly out it will generally work just fine.

VGA and full modes differ in register 0x3036 (SC_CMMN_PLL_MULTIPLIER)
which alters all the timings.

Running the numbers again, I get the VGA link frequency to be
145.8333MHz, but also the pixel rate to be 58.333MPix/s vs 55 in the
driver. I don't recall the VGA mode being 6% out on frame rate and
exposure setup, so I can't quite square that with reality. I'll try to
find 10 minutes to confirm, unless either of you happen to have one
set up and could validate the frame times.

  Dave

> > > +};
> > > +
> > >  struct regval_list {
> > >       u16 addr;
> > >       u8 data;
> > > @@ -106,6 +113,7 @@ struct ov5647_mode {
> > >       struct v4l2_mbus_framefmt       format;
> > >       struct v4l2_rect                crop;
> > >       u64                             pixel_rate;
> > > +     unsigned int                    link_freq_index;
> > >       int                             hts;
> > >       int                             vts;
> > >       const struct regval_list        *reg_list;
> > > @@ -128,6 +136,7 @@ struct ov5647 {
> > >       struct v4l2_ctrl                *exposure;
> > >       struct v4l2_ctrl                *hflip;
> > >       struct v4l2_ctrl                *vflip;
> > > +     struct v4l2_ctrl                *link_freq;
> > >  };
> > >
> > >  static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > > @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > >                       .height         = 1944
> > >               },
> > >               .pixel_rate     = 87500000,
> > > +             .link_freq_index = FREQ_INDEX_FULL,
> > >               .hts            = 2844,
> > >               .vts            = 0x7b0,
> > >               .reg_list       = ov5647_2592x1944_10bpp,
> > > @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > >                       .height         = 1080,
> > >               },
> > >               .pixel_rate     = 87500000,
> > > +             .link_freq_index = FREQ_INDEX_FULL,
> > >               .hts            = 2416,
> > >               .vts            = 0x450,
> > >               .reg_list       = ov5647_1080p30_10bpp,
> > > @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > >                       .height         = 1944,
> > >               },
> > >               .pixel_rate     = 87500000,
> > > +             .link_freq_index = FREQ_INDEX_FULL,
> > >               .hts            = 1896,
> > >               .vts            = 0x59b,
> > >               .reg_list       = ov5647_2x2binned_10bpp,
> > > @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > >                       .height         = 1920,
> > >               },
> > >               .pixel_rate     = 55000000,
> > > +             .link_freq_index = FREQ_INDEX_VGA,
> > >               .hts            = 1852,
> > >               .vts            = 0x1f8,
> > >               .reg_list       = ov5647_640x480_10bpp,
> > > @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > >                                        sensor->exposure->minimum,
> > >                                        exposure_max, sensor->exposure->step,
> > >                                        exposure_def);
> > > +
> > > +             __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
> >
> > Doesn't this cause an error in s_ctrl where the control is not handled
> > ?
>
> The framework returns -EACCESS for read-only controls in validate_ctrls()
>
> >
> > >       }
> > >       *fmt = mode->format;
> > >       /* The code we pass back must reflect the current h/vflips. */
> > > @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > >       int hblank, exposure_max, exposure_def;
> > >       struct v4l2_fwnode_device_properties props;
> > >
> > > -     v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > > +     v4l2_ctrl_handler_init(&sensor->ctrls, 10);
> > >
> > >       v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > >                         V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > > @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > >       if (sensor->vflip)
> > >               sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > >
> > > +     sensor->link_freq =
> > > +             v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
> >
> > As suggested for PIXEL_RATE, if you make the control read-only you
> > should set the control ops to NULL.
>
> Will do in v2.
>
> > > +                                    V4L2_CID_LINK_FREQ,
> > > +                                    ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> > > +                                    ov5647_link_freqs);
> > > +     if (sensor->link_freq)
> > > +             sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > You know, I thought link_freq was set as READ_ONLY by the framework,
> > but it's actuallt PIXEL_RATE (you can remove setting the flags
> > in the driver if you send a patch to remove the control ops when
> > registering PIXEL_RATE).
>
> Will do.
>
> >
> > Thanks
> >   j
> >
> > > +
> > >       v4l2_fwnode_device_parse(dev, &props);
> > >
> > >       v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> > >
> > > --
> > > 2.51.0
> > >
>
> Thanks,
> Jai

Re: [PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

[Jai Luthra]

Hi Dave,

Quoting Dave Stevenson (2025-11-18 18:17:25)
> Hi Jacopo & Jai
> 
> On Tue, 18 Nov 2025 at 11:28, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Quoting Jacopo Mondi (2025-11-02 16:59:02)
> > > Hi Jai
> > >
> > > On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > >
> > > > The link frequency can vary between modes, so add it as a
> > > > control.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> > > >
> > > >  #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> > > >
> > > > +#define FREQ_INDEX_FULL              0
> > > > +#define FREQ_INDEX_VGA               1
> > > > +static const s64 ov5647_link_freqs[] = {
> > > > +     [FREQ_INDEX_FULL]       = 218500000,
> > >
> > > The full mode pixel rate is set to 87500000, which considering CSI-2
> > > DDR mode and the 2 lanes in use give me a link freq of 21875000.
> >
> > Indeed, I get the same value, will update.
> 
> Agreed. I obviously lost a digit.
> 
> > >
> > > Do you know where 218500000 comes from ? (it might be perfectly legit,
> > > I'm not questioning that).
> > >
> >
> > > > +     [FREQ_INDEX_VGA]        = 208333000,
> >
> > This value should be 137500000 if we do the same calculation using the
> > pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
> > binning + 2x2 subsampling, which is quite a bit different than other modes.
> >
> > https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
> > that the pixel rate value calculated from the bus link frequency does not
> > necessarily have to match the PIXEL_RATE control value (which is for the
> > sensor's internal readout of pixels including blanking).
> 
> Indeed you should never assume that pixel rate and link frequency are
> directly linked. So many sensors have separate PLLs for the pixel
> array vs the MIPI block.
> 
> Having said that, OV5647 appears to use the same PLL for pixel clock
> and MIPI, although it does have a separate PLLADCLK which is
> presumably for the ADC.

Ah that's really helpful information, thanks.

> 
> > Ultimately, these values are coming from the BSP where the CFE driver is
> > using the link frequency control to configure the DPHY-RX rate, so I think
> > it would be wiser to not reduce the VGA link frequency value, which may
> > cause issues with DPHY-RX latching. We can always fix it later if needed.
> 
> It's been a long time since I looked at these settings, but I do have
> a spreadsheet from Omnivision that gives clock frequencies based on
> register values.
> In my experience the link frequency isn't critical to be exactly right
> as it typically only sets up timeout ranges in the PHY. Even if the
> value is significantly out it will generally work just fine.
> 
> VGA and full modes differ in register 0x3036 (SC_CMMN_PLL_MULTIPLIER)
> which alters all the timings.
> 
> Running the numbers again, I get the VGA link frequency to be
> 145.8333MHz, but also the pixel rate to be 58.333MPix/s vs 55 in the
> driver. I don't recall the VGA mode being 6% out on frame rate and
> exposure setup, so I can't quite square that with reality. I'll try to
> find 10 minutes to confirm, unless either of you happen to have one
> set up and could validate the frame times.
> 

Running 640x480 capture with default hblank/vblank I get frames at
62.49fps.

58333000/(1852*504) = 62.4946

So 58.333MPix/s seems to be the correct value instead of 55MPix/s.

I had already sent a v2, so I'll wait till next -rc1 is tagged for any
more comments, and will update these values in v3.

Thanks,
Jai

>   Dave
> 

[snip]