[PATCH 00/25] media: i2c: ov01a10: Add crop, ov01a1b and ov01a1s support

[PATCH 00/25] media: i2c: ov01a10: Add crop, ov01a1b and ov01a1s support

[Hans de Goede]

Hi All,

This is a series with various ov01a10 driver improvements
1. A set of bugfixes
2. Add cropping support / allow arbitrary sizes
3. OV01A1B monochrome/IR model support
4. OV01A1S RGB-IR model support

This has been tested on:
1. A Dell XPS 13 9320 Raptor Lake with OV1A10 color + OV1A1B IR sensor
2. A Dell Latitude 9420 Tiger Lake with OV01A1S RGB-IR

Testing has been done both with:
1. libcamera (qcam + softISP patches for RGB-IR); and
2. Intel's proprietary userspace stack with out of tree psys driver (*)

Regards,

Hans


*) The out of tree ov01a1s driver has a fixed resolution of 1296x798
   (with the height not being a multiple of bayer-pattern-size <sigh>)
   the closest the mainline driver can get after this series is 1288x800.
   This requires some changes to the xml files describing the ov01a1s
   graph in ipu6-camera-hal.


Hans de Goede (25):
  media: i2c: ov01a10: Fix the horizontal flip control
  media: i2c: ov01a10: Fix reported pixel-rate value
  media: i2c: ov01a10: Fix gain range
  media: i2c: ov01a10: Add missing v4l2_subdev_cleanup() calls
  media: i2c: ov01a10: Fix passing stream instead of pad to
    v4l2_subdev_state_get_format()
  media: i2c: ov01a10: Fix test-pattern disabling
  media: i2c: ov01a10: Change default vblank value to a vblank resulting
    in 30 fps
  media: i2c: ov01a10: Convert to new CCI register access helpers
  media: i2c: ov01a10: Remove overly verbose probe() error reporting
  media: i2c: ov01a10: Store dev pointer in struct ov01a10
  media: i2c: ov01a10: Add ov01a10_check_hwcfg() function
  media: i2c: ov01a10: Add power on/off sequencing support
  media: i2c: ov01a10: Don't update pixel_rate and link_freq from
    set_fmt
  media: i2c: ov01a10: Move setting of ctrl->flags to after checking
    ctrl_hdlr->error
  media: i2c: ov01a10: Use native and default for pixel-array size names
  media: i2c: ov01a10: Add cropping support / allow arbitrary sizes
  media: i2c: ov01a10: Remove struct ov01a10_reg_list
  media: i2c: ov01a10: Replace exposure->min/step with direct define use
  media: i2c: ov01a10: Only set register 0x0305 once
  media: i2c: ov01a10: Remove values set by controls from
    global_setting[]
  media: i2c: ov01a10: Add ov01a10_sensor_cfg struct
  media: i2c: ov01a10: Optimize setting h/vflip values
  media: i2c: ov01a10: Add ov01a1b support
  media: i2c: ov01a10: Add ov01a1s support
  media: i2c: ov01a10: Register tweaks for ov01a1s model

 drivers/media/i2c/Kconfig   |   1 +
 drivers/media/i2c/ov01a10.c | 959 ++++++++++++++++++++++--------------
 2 files changed, 592 insertions(+), 368 deletions(-)

-- 
2.51.0

[PATCH 01/25] media: i2c: ov01a10: Fix the horizontal flip control

[Hans de Goede]

During sensor calibration I noticed that with the hflip control set
to false/disabled the image was mirrored.

So it seems that the horizontal flip control is inverted and needs to
be set to 1 to not flip (just like the similar problem recently fixed
on the ov08x40 sensor).

Invert the hflip control to fix the sensor mirroring by default.

As the comment above the newly added OV01A10_MEDIA_BUS_FMT define explains
the control being inverted also means that the native Bayer-order of
the sensor actually is GBRG not BGGR, but so as to not break userspace
the Bayer-order is kept at BGGR.

Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 141cb6f75b55..e5df01f97978 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -75,6 +75,15 @@
 #define OV01A10_REG_X_WIN		0x3811
 #define OV01A10_REG_Y_WIN		0x3813
 
+/*
+ * The native ov01a10 bayer-pattern is GBRG, but there was a driver bug enabling
+ * hflip/mirroring by default resulting in BGGR. Because of this bug Intel's
+ * proprietary IPU6 userspace stack expects BGGR. So we report BGGR to not break
+ * userspace and fix things up by shifting the crop window-x coordinate by 1
+ * when hflip is *disabled*.
+ */
+#define OV01A10_MEDIA_BUS_FMT		MEDIA_BUS_FMT_SBGGR10_1X10
+
 struct ov01a10_reg {
 	u16 address;
 	u8 val;
@@ -185,14 +194,14 @@ static const struct ov01a10_reg sensor_1280x800_setting[] = {
 	{0x380e, 0x03},
 	{0x380f, 0x80},
 	{0x3810, 0x00},
-	{0x3811, 0x08},
+	{0x3811, 0x09},
 	{0x3812, 0x00},
 	{0x3813, 0x08},
 	{0x3814, 0x01},
 	{0x3815, 0x01},
 	{0x3816, 0x01},
 	{0x3817, 0x01},
-	{0x3820, 0xa0},
+	{0x3820, 0xa8},
 	{0x3822, 0x13},
 	{0x3832, 0x28},
 	{0x3833, 0x10},
@@ -411,7 +420,7 @@ static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
 	int ret;
 	u32 val, offset;
 
-	offset = hflip ? 0x9 : 0x8;
+	offset = hflip ? 0x8 : 0x9;
 	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_X_WIN, 1, offset);
 	if (ret)
 		return ret;
@@ -420,8 +429,8 @@ static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
 	if (ret)
 		return ret;
 
-	val = hflip ? val | FIELD_PREP(OV01A10_HFLIP_MASK, 0x1) :
-		val & ~OV01A10_HFLIP_MASK;
+	val = hflip ? val & ~OV01A10_HFLIP_MASK :
+		      val | FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
 
 	return ov01a10_write_reg(ov01a10, OV01A10_REG_FORMAT1, 1, val);
 }
@@ -610,7 +619,7 @@ static void ov01a10_update_pad_format(const struct ov01a10_mode *mode,
 {
 	fmt->width = mode->width;
 	fmt->height = mode->height;
-	fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
+	fmt->code = OV01A10_MEDIA_BUS_FMT;
 	fmt->field = V4L2_FIELD_NONE;
 	fmt->colorspace = V4L2_COLORSPACE_RAW;
 }
@@ -751,7 +760,7 @@ static int ov01a10_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
+	code->code = OV01A10_MEDIA_BUS_FMT;
 
 	return 0;
 }
@@ -761,7 +770,7 @@ static int ov01a10_enum_frame_size(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
 	if (fse->index >= ARRAY_SIZE(supported_modes) ||
-	    fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
+	    fse->code != OV01A10_MEDIA_BUS_FMT)
 		return -EINVAL;
 
 	fse->min_width = supported_modes[fse->index].width;
-- 
2.51.0

[PATCH 02/25] media: i2c: ov01a10: Fix reported pixel-rate value

[Hans de Goede]

CSI lanes are double-clocked so with a single lane at 400MHZ the resulting
pixel-rate for 10-bits pixels is 400 MHz * 2 / 10 = 80 MHz, not 40 MHz.

This also matches with the observed frame-rate of 60 fps with the default
vblank setting: 80000000 / (1488 * 896) = 60.

Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index e5df01f97978..0b1a1ecfffd0 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -16,7 +16,7 @@
 #include <media/v4l2-fwnode.h>
 
 #define OV01A10_LINK_FREQ_400MHZ	400000000ULL
-#define OV01A10_SCLK			40000000LL
+#define OV01A10_SCLK			80000000LL
 #define OV01A10_DATA_LANES		1
 
 #define OV01A10_REG_CHIP_ID		0x300a
-- 
2.51.0

[PATCH 03/25] media: i2c: ov01a10: Fix gain range

[Hans de Goede]

A maximum gain of 0xffff / 65525 seems unlikely and testing indeed shows
that the gain control wraps-around at 4096, so set the maximum gain to
0xfff / 4095.

The minimum gain of 0x100 is correct. Setting bits 8-11 to 0x0 results
in the same gain values as setting these bits to 0x1, with bits 0-7
still increasing the gain when going from 0x000 - 0x0ff in the exact
same range as when going from 0x100 - 0x1ff.

Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 0b1a1ecfffd0..95d6a0f046a0 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -48,7 +48,7 @@
 /* analog gain controls */
 #define OV01A10_REG_ANALOG_GAIN		0x3508
 #define OV01A10_ANAL_GAIN_MIN		0x100
-#define OV01A10_ANAL_GAIN_MAX		0xffff
+#define OV01A10_ANAL_GAIN_MAX		0xfff
 #define OV01A10_ANAL_GAIN_STEP		1
 
 /* digital gain controls */
-- 
2.51.0

[PATCH 04/25] media: i2c: ov01a10: Add missing v4l2_subdev_cleanup() calls

[Hans de Goede]

Add missing v4l2_subdev_cleanup() calls to cleanup after
v4l2_subdev_init_finalize().

Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 95d6a0f046a0..f92867f542f0 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -864,6 +864,7 @@ static void ov01a10_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sd->entity);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 
@@ -934,6 +935,7 @@ static int ov01a10_probe(struct i2c_client *client)
 err_pm_disable:
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(&client->dev);
+	v4l2_subdev_cleanup(&ov01a10->sd);
 
 err_media_entity_cleanup:
 	media_entity_cleanup(&ov01a10->sd.entity);
-- 
2.51.0

[PATCH 05/25] media: i2c: ov01a10: Fix passing stream instead of pad to v4l2_subdev_state_get_format()

[Hans de Goede]

The 2 argument version of v4l2_subdev_state_get_format() takes the pad
as second argument, not the stream.

Fixes: bc0e8d91feec ("media: v4l: subdev: Switch to stream-aware state functions")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Note the problem pre-exists the Fixes: commit but backporting further
will require manual backports and seems unnecessary.
---
 drivers/media/i2c/ov01a10.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index f92867f542f0..0405ec7c75fd 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -731,7 +731,7 @@ static int ov01a10_set_format(struct v4l2_subdev *sd,
 					 h_blank);
 	}
 
-	format = v4l2_subdev_state_get_format(sd_state, fmt->stream);
+	format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
 	*format = fmt->format;
 
 	return 0;
-- 
2.51.0

[PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Hans de Goede]

When the test-pattern control gets set to 0 (Disabled) 0 should be written
to the test-pattern register, rather then doing nothing.

Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 0405ec7c75fd..733e5bf0180c 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -406,10 +406,8 @@ static int ov01a10_update_digital_gain(struct ov01a10 *ov01a10, u32 d_gain)
 
 static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
 {
-	if (!pattern)
-		return 0;
-
-	pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
+	if (pattern)
+		pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
 
 	return ov01a10_write_reg(ov01a10, OV01A10_REG_TEST_PATTERN, 1, pattern);
 }
-- 
2.51.0

[PATCH 07/25] media: i2c: ov01a10: Change default vblank value to a vblank resulting in 30 fps

[Hans de Goede]

The ov01a10 is quite a small sensor, which does not capture a lot of
light, increase the default vblank so that the sensor runs at 30 fps
by default, doubling the default exposure.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 733e5bf0180c..0ae23d435a65 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -34,7 +34,7 @@
 
 /* vertical and horizontal timings */
 #define OV01A10_REG_VTS			0x380e
-#define OV01A10_VTS_DEF			0x0380
+#define OV01A10_VTS_DEF			0x0700
 #define OV01A10_VTS_MIN			0x0380
 #define OV01A10_VTS_MAX			0xffff
 #define OV01A10_HTS_DEF			1488
@@ -191,8 +191,8 @@ static const struct ov01a10_reg sensor_1280x800_setting[] = {
 	{0x380b, 0x20},
 	{0x380c, 0x02},
 	{0x380d, 0xe8},
-	{0x380e, 0x03},
-	{0x380f, 0x80},
+	{0x380e, 0x07},
+	{0x380f, 0x00},
 	{0x3810, 0x00},
 	{0x3811, 0x09},
 	{0x3812, 0x00},
-- 
2.51.0

[PATCH 08/25] media: i2c: ov01a10: Convert to new CCI register access helpers

[Hans de Goede]

Use the new comon CCI register access helpers to replace the private
register access helpers in the ov01a10 driver.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/Kconfig   |   1 +
 drivers/media/i2c/ov01a10.c | 219 ++++++++++--------------------------
 2 files changed, 59 insertions(+), 161 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 6fed163803d3..a14332a700be 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -364,6 +364,7 @@ config VIDEO_OG0VE1B
 
 config VIDEO_OV01A10
 	tristate "OmniVision OV01A10 sensor support"
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor driver for the OmniVision
 	  OV01A10 camera.
diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 0ae23d435a65..d17a04f4ca5b 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -10,7 +10,9 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 
+#include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -19,10 +21,10 @@
 #define OV01A10_SCLK			80000000LL
 #define OV01A10_DATA_LANES		1
 
-#define OV01A10_REG_CHIP_ID		0x300a
+#define OV01A10_REG_CHIP_ID		CCI_REG24(0x300a)
 #define OV01A10_CHIP_ID			0x560141
 
-#define OV01A10_REG_MODE_SELECT		0x0100
+#define OV01A10_REG_MODE_SELECT		CCI_REG8(0x0100)
 #define OV01A10_MODE_STANDBY		0x00
 #define OV01A10_MODE_STREAMING		0x01
 
@@ -33,47 +35,47 @@
 #define OV01A10_ACITVE_HEIGHT		800
 
 /* vertical and horizontal timings */
-#define OV01A10_REG_VTS			0x380e
+#define OV01A10_REG_VTS			CCI_REG16(0x380e)
 #define OV01A10_VTS_DEF			0x0700
 #define OV01A10_VTS_MIN			0x0380
 #define OV01A10_VTS_MAX			0xffff
 #define OV01A10_HTS_DEF			1488
 
 /* exposure controls */
-#define OV01A10_REG_EXPOSURE		0x3501
+#define OV01A10_REG_EXPOSURE		CCI_REG16(0x3501)
 #define OV01A10_EXPOSURE_MIN		4
 #define OV01A10_EXPOSURE_MAX_MARGIN	8
 #define OV01A10_EXPOSURE_STEP		1
 
 /* analog gain controls */
-#define OV01A10_REG_ANALOG_GAIN		0x3508
+#define OV01A10_REG_ANALOG_GAIN		CCI_REG16(0x3508)
 #define OV01A10_ANAL_GAIN_MIN		0x100
 #define OV01A10_ANAL_GAIN_MAX		0xfff
 #define OV01A10_ANAL_GAIN_STEP		1
 
 /* digital gain controls */
-#define OV01A10_REG_DIGITAL_GAIN_B	0x350a
-#define OV01A10_REG_DIGITAL_GAIN_GB	0x3510
-#define OV01A10_REG_DIGITAL_GAIN_GR	0x3513
-#define OV01A10_REG_DIGITAL_GAIN_R	0x3516
+#define OV01A10_REG_DIGITAL_GAIN_B	CCI_REG24(0x350a)
+#define OV01A10_REG_DIGITAL_GAIN_GB	CCI_REG24(0x3510)
+#define OV01A10_REG_DIGITAL_GAIN_GR	CCI_REG24(0x3513)
+#define OV01A10_REG_DIGITAL_GAIN_R	CCI_REG24(0x3516)
 #define OV01A10_DGTL_GAIN_MIN		0
 #define OV01A10_DGTL_GAIN_MAX		0x3ffff
 #define OV01A10_DGTL_GAIN_STEP		1
 #define OV01A10_DGTL_GAIN_DEFAULT	1024
 
 /* test pattern control */
-#define OV01A10_REG_TEST_PATTERN	0x4503
+#define OV01A10_REG_TEST_PATTERN	CCI_REG8(0x4503)
 #define OV01A10_TEST_PATTERN_ENABLE	BIT(7)
 #define OV01A10_LINK_FREQ_400MHZ_INDEX	0
 
 /* flip and mirror control */
-#define OV01A10_REG_FORMAT1		0x3820
+#define OV01A10_REG_FORMAT1		CCI_REG8(0x3820)
 #define OV01A10_VFLIP_MASK		BIT(4)
 #define OV01A10_HFLIP_MASK		BIT(3)
 
 /* window offset */
-#define OV01A10_REG_X_WIN		0x3811
-#define OV01A10_REG_Y_WIN		0x3813
+#define OV01A10_REG_X_WIN		CCI_REG16(0x3810)
+#define OV01A10_REG_Y_WIN		CCI_REG16(0x3812)
 
 /*
  * The native ov01a10 bayer-pattern is GBRG, but there was a driver bug enabling
@@ -84,14 +86,9 @@
  */
 #define OV01A10_MEDIA_BUS_FMT		MEDIA_BUS_FMT_SBGGR10_1X10
 
-struct ov01a10_reg {
-	u16 address;
-	u8 val;
-};
-
 struct ov01a10_reg_list {
 	u32 num_of_regs;
-	const struct ov01a10_reg *regs;
+	const struct reg_sequence *regs;
 };
 
 struct ov01a10_link_freq_config {
@@ -109,7 +106,7 @@ struct ov01a10_mode {
 	const struct ov01a10_reg_list reg_list;
 };
 
-static const struct ov01a10_reg mipi_data_rate_720mbps[] = {
+static const struct reg_sequence mipi_data_rate_720mbps[] = {
 	{0x0103, 0x01},
 	{0x0302, 0x00},
 	{0x0303, 0x06},
@@ -125,7 +122,7 @@ static const struct ov01a10_reg mipi_data_rate_720mbps[] = {
 	{0x0325, 0x68},
 };
 
-static const struct ov01a10_reg sensor_1280x800_setting[] = {
+static const struct reg_sequence sensor_1280x800_setting[] = {
 	{0x3002, 0xa1},
 	{0x301e, 0xf0},
 	{0x3022, 0x01},
@@ -283,6 +280,7 @@ static const struct ov01a10_mode supported_modes[] = {
 };
 
 struct ov01a10 {
+	struct regmap *regmap;
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -302,104 +300,15 @@ static inline struct ov01a10 *to_ov01a10(struct v4l2_subdev *subdev)
 	return container_of(subdev, struct ov01a10, sd);
 }
 
-static int ov01a10_read_reg(struct ov01a10 *ov01a10, u16 reg, u16 len, u32 *val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
-	struct i2c_msg msgs[2];
-	u8 addr_buf[2];
-	u8 data_buf[4] = {0};
-	int ret = 0;
-
-	if (len > sizeof(data_buf))
-		return -EINVAL;
-
-	put_unaligned_be16(reg, addr_buf);
-	msgs[0].addr = client->addr;
-	msgs[0].flags = 0;
-	msgs[0].len = sizeof(addr_buf);
-	msgs[0].buf = addr_buf;
-	msgs[1].addr = client->addr;
-	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = len;
-	msgs[1].buf = &data_buf[sizeof(data_buf) - len];
-
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
-
-	if (ret != ARRAY_SIZE(msgs))
-		return ret < 0 ? ret : -EIO;
-
-	*val = get_unaligned_be32(data_buf);
-
-	return 0;
-}
-
-static int ov01a10_write_reg(struct ov01a10 *ov01a10, u16 reg, u16 len, u32 val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
-	u8 buf[6];
-	int ret = 0;
-
-	if (len > 4)
-		return -EINVAL;
-
-	put_unaligned_be16(reg, buf);
-	put_unaligned_be32(val << 8 * (4 - len), buf + 2);
-
-	ret = i2c_master_send(client, buf, len + 2);
-	if (ret != len + 2)
-		return ret < 0 ? ret : -EIO;
-
-	return 0;
-}
-
-static int ov01a10_write_reg_list(struct ov01a10 *ov01a10,
-				  const struct ov01a10_reg_list *r_list)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
-	unsigned int i;
-	int ret = 0;
-
-	for (i = 0; i < r_list->num_of_regs; i++) {
-		ret = ov01a10_write_reg(ov01a10, r_list->regs[i].address, 1,
-					r_list->regs[i].val);
-		if (ret) {
-			dev_err_ratelimited(&client->dev,
-					    "write reg 0x%4.4x err = %d\n",
-					    r_list->regs[i].address, ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static int ov01a10_update_digital_gain(struct ov01a10 *ov01a10, u32 d_gain)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
 	u32 real = d_gain << 6;
 	int ret = 0;
 
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_DIGITAL_GAIN_B, 3, real);
-	if (ret) {
-		dev_err(&client->dev, "failed to set DIGITAL_GAIN_B\n");
-		return ret;
-	}
-
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_DIGITAL_GAIN_GB, 3, real);
-	if (ret) {
-		dev_err(&client->dev, "failed to set DIGITAL_GAIN_GB\n");
-		return ret;
-	}
-
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_DIGITAL_GAIN_GR, 3, real);
-	if (ret) {
-		dev_err(&client->dev, "failed to set DIGITAL_GAIN_GR\n");
-		return ret;
-	}
-
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_DIGITAL_GAIN_R, 3, real);
-	if (ret)
-		dev_err(&client->dev, "failed to set DIGITAL_GAIN_R\n");
+	cci_write(ov01a10->regmap, OV01A10_REG_DIGITAL_GAIN_B, real, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_DIGITAL_GAIN_GB, real, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_DIGITAL_GAIN_GR, real, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_DIGITAL_GAIN_R, real, &ret);
 
 	return ret;
 }
@@ -409,48 +318,39 @@ static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
 	if (pattern)
 		pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
 
-	return ov01a10_write_reg(ov01a10, OV01A10_REG_TEST_PATTERN, 1, pattern);
+	return cci_write(ov01a10->regmap, OV01A10_REG_TEST_PATTERN, pattern,
+			 NULL);
 }
 
 /* for vflip and hflip, use 0x9 as window offset to keep the bayer */
 static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
 {
-	int ret;
 	u32 val, offset;
+	int ret = 0;
 
 	offset = hflip ? 0x8 : 0x9;
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_X_WIN, 1, offset);
-	if (ret)
-		return ret;
+	val = hflip ? 0 : FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
 
-	ret = ov01a10_read_reg(ov01a10, OV01A10_REG_FORMAT1, 1, &val);
-	if (ret)
-		return ret;
+	cci_write(ov01a10->regmap, OV01A10_REG_X_WIN, offset, &ret);
+	cci_update_bits(ov01a10->regmap, OV01A10_REG_FORMAT1,
+			OV01A10_HFLIP_MASK, val, &ret);
 
-	val = hflip ? val & ~OV01A10_HFLIP_MASK :
-		      val | FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
-
-	return ov01a10_write_reg(ov01a10, OV01A10_REG_FORMAT1, 1, val);
+	return ret;
 }
 
 static int ov01a10_set_vflip(struct ov01a10 *ov01a10, u32 vflip)
 {
-	int ret;
 	u32 val, offset;
+	int ret = 0;
 
 	offset = vflip ? 0x9 : 0x8;
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_Y_WIN, 1, offset);
-	if (ret)
-		return ret;
+	val = vflip ? FIELD_PREP(OV01A10_VFLIP_MASK, 0x1) : 0;
 
-	ret = ov01a10_read_reg(ov01a10, OV01A10_REG_FORMAT1, 1, &val);
-	if (ret)
-		return ret;
+	cci_write(ov01a10->regmap, OV01A10_REG_Y_WIN, offset, &ret);
+	cci_update_bits(ov01a10->regmap, OV01A10_REG_FORMAT1,
+			OV01A10_VFLIP_MASK, val, &ret);
 
-	val = vflip ? val | FIELD_PREP(OV01A10_VFLIP_MASK, 0x1) :
-		val & ~OV01A10_VFLIP_MASK;
-
-	return ov01a10_write_reg(ov01a10, OV01A10_REG_FORMAT1, 1, val);
+	return ret;
 }
 
 static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
@@ -475,8 +375,8 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_ANALOGUE_GAIN:
-		ret = ov01a10_write_reg(ov01a10, OV01A10_REG_ANALOG_GAIN, 2,
-					ctrl->val);
+		ret = cci_write(ov01a10->regmap, OV01A10_REG_ANALOG_GAIN,
+				ctrl->val, NULL);
 		break;
 
 	case V4L2_CID_DIGITAL_GAIN:
@@ -484,13 +384,13 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 
 	case V4L2_CID_EXPOSURE:
-		ret = ov01a10_write_reg(ov01a10, OV01A10_REG_EXPOSURE, 2,
-					ctrl->val);
+		ret = cci_write(ov01a10->regmap, OV01A10_REG_EXPOSURE,
+				ctrl->val, NULL);
 		break;
 
 	case V4L2_CID_VBLANK:
-		ret = ov01a10_write_reg(ov01a10, OV01A10_REG_VTS, 2,
-					ov01a10->cur_mode->height + ctrl->val);
+		ret = cci_write(ov01a10->regmap, OV01A10_REG_VTS,
+				ov01a10->cur_mode->height + ctrl->val, NULL);
 		break;
 
 	case V4L2_CID_TEST_PATTERN:
@@ -631,14 +531,16 @@ static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 
 	link_freq_index = ov01a10->cur_mode->link_freq_index;
 	reg_list = &link_freq_configs[link_freq_index].reg_list;
-	ret = ov01a10_write_reg_list(ov01a10, reg_list);
+	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
+				     reg_list->num_of_regs);
 	if (ret) {
 		dev_err(&client->dev, "failed to set plls\n");
 		return ret;
 	}
 
 	reg_list = &ov01a10->cur_mode->reg_list;
-	ret = ov01a10_write_reg_list(ov01a10, reg_list);
+	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
+				     reg_list->num_of_regs);
 	if (ret) {
 		dev_err(&client->dev, "failed to set mode\n");
 		return ret;
@@ -648,23 +550,14 @@ static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 	if (ret)
 		return ret;
 
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_MODE_SELECT, 1,
-				OV01A10_MODE_STREAMING);
-	if (ret)
-		dev_err(&client->dev, "failed to start streaming\n");
-
-	return ret;
+	return cci_write(ov01a10->regmap, OV01A10_REG_MODE_SELECT,
+			 OV01A10_MODE_STREAMING, NULL);
 }
 
 static void ov01a10_stop_streaming(struct ov01a10 *ov01a10)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
-	int ret = 0;
-
-	ret = ov01a10_write_reg(ov01a10, OV01A10_REG_MODE_SELECT, 1,
-				OV01A10_MODE_STANDBY);
-	if (ret)
-		dev_err(&client->dev, "failed to stop streaming\n");
+	cci_write(ov01a10->regmap, OV01A10_REG_MODE_SELECT,
+		  OV01A10_MODE_STANDBY, NULL);
 }
 
 static int ov01a10_set_stream(struct v4l2_subdev *sd, int enable)
@@ -842,14 +735,14 @@ static int ov01a10_identify_module(struct ov01a10 *ov01a10)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
 	int ret;
-	u32 val;
+	u64 val;
 
-	ret = ov01a10_read_reg(ov01a10, OV01A10_REG_CHIP_ID, 3, &val);
+	ret = cci_read(ov01a10->regmap, OV01A10_REG_CHIP_ID, &val, NULL);
 	if (ret)
 		return ret;
 
 	if (val != OV01A10_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
 			OV01A10_CHIP_ID, val);
 		return -EIO;
 	}
@@ -880,6 +773,10 @@ static int ov01a10_probe(struct i2c_client *client)
 	if (!ov01a10)
 		return -ENOMEM;
 
+	ov01a10->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(ov01a10->regmap))
+		return PTR_ERR(ov01a10->regmap);
+
 	v4l2_i2c_subdev_init(&ov01a10->sd, client, &ov01a10_subdev_ops);
 	ov01a10->sd.internal_ops = &ov01a10_internal_ops;
 
-- 
2.51.0

[PATCH 09/25] media: i2c: ov01a10: Remove overly verbose probe() error reporting

[Hans de Goede]

Many of the functions called from ov01a10_probe() are expected to never
fail and they should all already log some message if they fail. Remove
the unnecessarily verbose dev_err[_probe]() calls from the error-exit
paths in probe().

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index d17a04f4ca5b..52e1f3e0e4e5 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -782,16 +782,13 @@ static int ov01a10_probe(struct i2c_client *client)
 
 	ret = ov01a10_identify_module(ov01a10);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "failed to find sensor\n");
+		return ret;
 
 	ov01a10->cur_mode = &supported_modes[0];
 
 	ret = ov01a10_init_controls(ov01a10);
-	if (ret) {
-		dev_err(dev, "failed to init controls: %d\n", ret);
+	if (ret)
 		return ret;
-	}
 
 	ov01a10->sd.state_lock = ov01a10->ctrl_handler.lock;
 	ov01a10->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -800,16 +797,12 @@ static int ov01a10_probe(struct i2c_client *client)
 	ov01a10->pad.flags = MEDIA_PAD_FL_SOURCE;
 
 	ret = media_entity_pads_init(&ov01a10->sd.entity, 1, &ov01a10->pad);
-	if (ret) {
-		dev_err(dev, "Failed to init entity pads: %d\n", ret);
+	if (ret)
 		goto err_handler_free;
-	}
 
 	ret = v4l2_subdev_init_finalize(&ov01a10->sd);
-	if (ret) {
-		dev_err(dev, "Failed to allocate subdev state: %d\n", ret);
+	if (ret)
 		goto err_media_entity_cleanup;
-	}
 
 	/*
 	 * Device is already turned on by i2c-core with ACPI domain PM.
@@ -820,10 +813,8 @@ static int ov01a10_probe(struct i2c_client *client)
 	pm_runtime_idle(dev);
 
 	ret = v4l2_async_register_subdev_sensor(&ov01a10->sd);
-	if (ret < 0) {
-		dev_err(dev, "Failed to register subdev: %d\n", ret);
+	if (ret)
 		goto err_pm_disable;
-	}
 
 	return 0;
 
-- 
2.51.0

[PATCH 10/25] media: i2c: ov01a10: Store dev pointer in struct ov01a10

[Hans de Goede]

Now that the cci_* register access helpers are used we no longer need
the i2c_client in various functions.

Some code is still getting the client just to be able to get to the device
pointer. Directly store a struct device *dev pointing to &client->dev
inside struct ov01a10 to make the code simpler.

This also fixes a mismatch of using dev vs &client->dev in the
runtime_pm_*() calls in probe().

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 52e1f3e0e4e5..de293e2431e9 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -280,6 +280,7 @@ static const struct ov01a10_mode supported_modes[] = {
 };
 
 struct ov01a10 {
+	struct device *dev;
 	struct regmap *regmap;
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -357,7 +358,6 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov01a10 *ov01a10 = container_of(ctrl->handler,
 					       struct ov01a10, ctrl_handler);
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
 	s64 exposure_max;
 	int ret = 0;
 
@@ -370,7 +370,7 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 					 exposure_max);
 	}
 
-	if (!pm_runtime_get_if_in_use(&client->dev))
+	if (!pm_runtime_get_if_in_use(ov01a10->dev))
 		return 0;
 
 	switch (ctrl->id) {
@@ -410,7 +410,7 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_put(ov01a10->dev);
 
 	return ret;
 }
@@ -421,7 +421,6 @@ static const struct v4l2_ctrl_ops ov01a10_ctrl_ops = {
 
 static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
 	struct v4l2_fwnode_device_properties props;
 	u32 vblank_min, vblank_max, vblank_default;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
@@ -430,7 +429,7 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	int ret = 0;
 	int size;
 
-	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	ret = v4l2_fwnode_device_parse(ov01a10->dev, &props);
 	if (ret)
 		return ret;
 
@@ -524,7 +523,6 @@ static void ov01a10_update_pad_format(const struct ov01a10_mode *mode,
 
 static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
 	const struct ov01a10_reg_list *reg_list;
 	int link_freq_index;
 	int ret = 0;
@@ -534,7 +532,7 @@ static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
 				     reg_list->num_of_regs);
 	if (ret) {
-		dev_err(&client->dev, "failed to set plls\n");
+		dev_err(ov01a10->dev, "failed to set plls\n");
 		return ret;
 	}
 
@@ -542,7 +540,7 @@ static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
 				     reg_list->num_of_regs);
 	if (ret) {
-		dev_err(&client->dev, "failed to set mode\n");
+		dev_err(ov01a10->dev, "failed to set mode\n");
 		return ret;
 	}
 
@@ -563,25 +561,24 @@ static void ov01a10_stop_streaming(struct ov01a10 *ov01a10)
 static int ov01a10_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct ov01a10 *ov01a10 = to_ov01a10(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct v4l2_subdev_state *state;
 	int ret = 0;
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
 
 	if (enable) {
-		ret = pm_runtime_resume_and_get(&client->dev);
+		ret = pm_runtime_resume_and_get(ov01a10->dev);
 		if (ret < 0)
 			goto unlock;
 
 		ret = ov01a10_start_streaming(ov01a10);
 		if (ret) {
-			pm_runtime_put(&client->dev);
+			pm_runtime_put(ov01a10->dev);
 			goto unlock;
 		}
 	} else {
 		ov01a10_stop_streaming(ov01a10);
-		pm_runtime_put(&client->dev);
+		pm_runtime_put(ov01a10->dev);
 	}
 
 unlock:
@@ -733,7 +730,6 @@ static const struct media_entity_operations ov01a10_subdev_entity_ops = {
 
 static int ov01a10_identify_module(struct ov01a10 *ov01a10)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov01a10->sd);
 	int ret;
 	u64 val;
 
@@ -742,7 +738,7 @@ static int ov01a10_identify_module(struct ov01a10 *ov01a10)
 		return ret;
 
 	if (val != OV01A10_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
+		dev_err(ov01a10->dev, "chip id mismatch: %x!=%llx\n",
 			OV01A10_CHIP_ID, val);
 		return -EIO;
 	}
@@ -765,14 +761,15 @@ static void ov01a10_remove(struct i2c_client *client)
 
 static int ov01a10_probe(struct i2c_client *client)
 {
-	struct device *dev = &client->dev;
 	struct ov01a10 *ov01a10;
 	int ret = 0;
 
-	ov01a10 = devm_kzalloc(dev, sizeof(*ov01a10), GFP_KERNEL);
+	ov01a10 = devm_kzalloc(&client->dev, sizeof(*ov01a10), GFP_KERNEL);
 	if (!ov01a10)
 		return -ENOMEM;
 
+	ov01a10->dev = &client->dev;
+
 	ov01a10->regmap = devm_cci_regmap_init_i2c(client, 16);
 	if (IS_ERR(ov01a10->regmap))
 		return PTR_ERR(ov01a10->regmap);
@@ -809,8 +806,8 @@ static int ov01a10_probe(struct i2c_client *client)
 	 * Enable runtime PM and turn off the device.
 	 */
 	pm_runtime_set_active(&client->dev);
-	pm_runtime_enable(dev);
-	pm_runtime_idle(dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_idle(&client->dev);
 
 	ret = v4l2_async_register_subdev_sensor(&ov01a10->sd);
 	if (ret)
@@ -819,7 +816,7 @@ static int ov01a10_probe(struct i2c_client *client)
 	return 0;
 
 err_pm_disable:
-	pm_runtime_disable(dev);
+	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	v4l2_subdev_cleanup(&ov01a10->sd);
 
-- 
2.51.0

[PATCH 11/25] media: i2c: ov01a10: Add ov01a10_check_hwcfg() function

[Hans de Goede]

Add a function to check that the number of mipi-lanes and there frequency
are what the driver expects.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 58 ++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index de293e2431e9..666c75b19dd9 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -294,6 +294,7 @@ struct ov01a10 {
 	struct v4l2_ctrl *exposure;
 
 	const struct ov01a10_mode *cur_mode;
+	u32 link_freq_index;
 };
 
 static inline struct ov01a10 *to_ov01a10(struct v4l2_subdev *subdev)
@@ -427,7 +428,6 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	const struct ov01a10_mode *cur_mode;
 	s64 exposure_max, h_blank;
 	int ret = 0;
-	int size;
 
 	ret = v4l2_fwnode_device_parse(ov01a10->dev, &props);
 	if (ret)
@@ -439,12 +439,11 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 		return ret;
 
 	cur_mode = ov01a10->cur_mode;
-	size = ARRAY_SIZE(link_freq_menu_items);
 
 	ov01a10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 						    &ov01a10_ctrl_ops,
 						    V4L2_CID_LINK_FREQ,
-						    size - 1, 0,
+						    ov01a10->link_freq_index, 0,
 						    link_freq_menu_items);
 	if (ov01a10->link_freq)
 		ov01a10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
@@ -746,6 +745,53 @@ static int ov01a10_identify_module(struct ov01a10 *ov01a10)
 	return 0;
 }
 
+static int ov01a10_check_hwcfg(struct ov01a10 *ov01a10)
+{
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+	struct fwnode_handle *ep, *fwnode = dev_fwnode(ov01a10->dev);
+	unsigned long link_freq_bitmap;
+	int ret;
+
+	/*
+	 * Sometimes the fwnode graph is initialized by the bridge driver,
+	 * wait for this.
+	 */
+	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
+	if (!ep)
+		return dev_err_probe(ov01a10->dev, -EPROBE_DEFER,
+				     "waiting for fwnode graph endpoint\n");
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret)
+		return dev_err_probe(ov01a10->dev, ret, "parsing endpoint\n");
+
+	ret = v4l2_link_freq_to_bitmap(ov01a10->dev,
+				       bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       link_freq_menu_items,
+				       ARRAY_SIZE(link_freq_menu_items),
+				       &link_freq_bitmap);
+	if (ret)
+		goto check_hwcfg_error;
+
+	/* v4l2_link_freq_to_bitmap() guarantees at least 1 bit is set */
+	ov01a10->link_freq_index = ffs(link_freq_bitmap) - 1;
+
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV01A10_DATA_LANES) {
+		ret = dev_err_probe(ov01a10->dev, -EINVAL,
+				    "number of CSI2 data lanes %u is not supported\n",
+				    bus_cfg.bus.mipi_csi2.num_data_lanes);
+		goto check_hwcfg_error;
+	}
+
+check_hwcfg_error:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+	return ret;
+}
+
 static void ov01a10_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
@@ -762,7 +808,7 @@ static void ov01a10_remove(struct i2c_client *client)
 static int ov01a10_probe(struct i2c_client *client)
 {
 	struct ov01a10 *ov01a10;
-	int ret = 0;
+	int ret;
 
 	ov01a10 = devm_kzalloc(&client->dev, sizeof(*ov01a10), GFP_KERNEL);
 	if (!ov01a10)
@@ -777,6 +823,10 @@ static int ov01a10_probe(struct i2c_client *client)
 	v4l2_i2c_subdev_init(&ov01a10->sd, client, &ov01a10_subdev_ops);
 	ov01a10->sd.internal_ops = &ov01a10_internal_ops;
 
+	ret = ov01a10_check_hwcfg(ov01a10);
+	if (ret)
+		return ret;
+
 	ret = ov01a10_identify_module(ov01a10);
 	if (ret)
 		return ret;
-- 
2.51.0

[PATCH 12/25] media: i2c: ov01a10: Add power on/off sequencing support

[Hans de Goede]

So far the ov01a10 driver has only been used on laptops with an IVSC chip
where the IVSC chip controls the power on/off sequencing of the sensor.

But there are also designs with an ov01a10 sensor where the kernel needs
to directly take care of the power-sequencing, controlling clks, regulators
and GPIOs. Add support for these designs.

The 2 ms minimum reset assertion time is taken from other Omnivision sensor
drivers like the ov5675. The 20 ms delay after reset de-assert comes from
the out of tree ov01a1s driver.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 126 +++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 666c75b19dd9..8fe0b0d4f9e6 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -7,10 +7,14 @@
 
 #include <linux/acpi.h>
 #include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
@@ -20,6 +24,7 @@
 #define OV01A10_LINK_FREQ_400MHZ	400000000ULL
 #define OV01A10_SCLK			80000000LL
 #define OV01A10_DATA_LANES		1
+#define OV01A10_MCLK			19200000
 
 #define OV01A10_REG_CHIP_ID		CCI_REG24(0x300a)
 #define OV01A10_CHIP_ID			0x560141
@@ -279,6 +284,12 @@ static const struct ov01a10_mode supported_modes[] = {
 	},
 };
 
+static const char * const ov01a10_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 struct ov01a10 {
 	struct device *dev;
 	struct regmap *regmap;
@@ -295,6 +306,11 @@ struct ov01a10 {
 
 	const struct ov01a10_mode *cur_mode;
 	u32 link_freq_index;
+
+	struct clk *clk;
+	struct gpio_desc *reset;
+	struct gpio_desc *powerdown;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ov01a10_supply_names)];
 };
 
 static inline struct ov01a10 *to_ov01a10(struct v4l2_subdev *subdev)
@@ -727,6 +743,92 @@ static const struct media_entity_operations ov01a10_subdev_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
+static int ov01a10_get_pm_resources(struct ov01a10 *ov01a10)
+{
+	unsigned long freq;
+	int i, ret;
+
+	ov01a10->clk = devm_v4l2_sensor_clk_get(ov01a10->dev, NULL);
+	if (IS_ERR(ov01a10->clk))
+		return dev_err_probe(ov01a10->dev, PTR_ERR(ov01a10->clk),
+				     "getting clock\n");
+
+	freq = clk_get_rate(ov01a10->clk);
+	if (freq != OV01A10_MCLK)
+		return dev_err_probe(ov01a10->dev, -EINVAL,
+				     "external clock %lu is not supported",
+				     freq);
+
+	ov01a10->reset = devm_gpiod_get_optional(ov01a10->dev, "reset",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(ov01a10->reset))
+		return dev_err_probe(ov01a10->dev, PTR_ERR(ov01a10->reset),
+				     "getting reset gpio\n");
+
+	ov01a10->powerdown = devm_gpiod_get_optional(ov01a10->dev, "powerdown",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(ov01a10->powerdown))
+		return dev_err_probe(ov01a10->dev, PTR_ERR(ov01a10->powerdown),
+				     "getting powerdown gpio\n");
+
+	for (i = 0; i < ARRAY_SIZE(ov01a10_supply_names); i++)
+		ov01a10->supplies[i].supply = ov01a10_supply_names[i];
+
+	ret = devm_regulator_bulk_get(ov01a10->dev,
+				      ARRAY_SIZE(ov01a10_supply_names),
+				      ov01a10->supplies);
+	if (ret)
+		return dev_err_probe(ov01a10->dev, ret, "getting regulators\n");
+
+	return 0;
+}
+
+static int ov01a10_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+	int ret;
+
+	ret = clk_prepare_enable(ov01a10->clk);
+	if (ret) {
+		dev_err(dev, "Error enabling clk: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov01a10_supply_names),
+				    ov01a10->supplies);
+	if (ret) {
+		dev_err(dev, "Error enabling regulators: %d\n", ret);
+		clk_disable_unprepare(ov01a10->clk);
+		return ret;
+	}
+
+	if (ov01a10->reset || ov01a10->powerdown) {
+		/* Assert reset/powerdown for at least 2ms on back to back off-on */
+		fsleep(2000);
+		gpiod_set_value_cansleep(ov01a10->powerdown, 0);
+		gpiod_set_value_cansleep(ov01a10->reset, 0);
+		fsleep(20000);
+	}
+
+	return 0;
+}
+
+static int ov01a10_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+
+	gpiod_set_value_cansleep(ov01a10->reset, 1);
+	gpiod_set_value_cansleep(ov01a10->powerdown, 1);
+
+	regulator_bulk_disable(ARRAY_SIZE(ov01a10_supply_names),
+			       ov01a10->supplies);
+
+	clk_disable_unprepare(ov01a10->clk);
+	return 0;
+}
+
 static int ov01a10_identify_module(struct ov01a10 *ov01a10)
 {
 	int ret;
@@ -802,7 +904,10 @@ static void ov01a10_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 
 	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev)) {
+		ov01a10_power_off(&client->dev);
+		pm_runtime_set_suspended(&client->dev);
+	}
 }
 
 static int ov01a10_probe(struct i2c_client *client)
@@ -827,15 +932,23 @@ static int ov01a10_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	ret = ov01a10_identify_module(ov01a10);
+	ret = ov01a10_get_pm_resources(ov01a10);
 	if (ret)
 		return ret;
 
+	ret = ov01a10_power_on(&client->dev);
+	if (ret)
+		return ret;
+
+	ret = ov01a10_identify_module(ov01a10);
+	if (ret)
+		goto err_power_off;
+
 	ov01a10->cur_mode = &supported_modes[0];
 
 	ret = ov01a10_init_controls(ov01a10);
 	if (ret)
-		return ret;
+		goto err_power_off;
 
 	ov01a10->sd.state_lock = ov01a10->ctrl_handler.lock;
 	ov01a10->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -876,9 +989,15 @@ static int ov01a10_probe(struct i2c_client *client)
 err_handler_free:
 	v4l2_ctrl_handler_free(ov01a10->sd.ctrl_handler);
 
+err_power_off:
+	ov01a10_power_off(&client->dev);
+
 	return ret;
 }
 
+static DEFINE_RUNTIME_DEV_PM_OPS(ov01a10_pm_ops, ov01a10_power_off,
+				 ov01a10_power_on, NULL);
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov01a10_acpi_ids[] = {
 	{ "OVTI01A0" },
@@ -891,6 +1010,7 @@ MODULE_DEVICE_TABLE(acpi, ov01a10_acpi_ids);
 static struct i2c_driver ov01a10_i2c_driver = {
 	.driver = {
 		.name = "ov01a10",
+		.pm = pm_sleep_ptr(&ov01a10_pm_ops),
 		.acpi_match_table = ACPI_PTR(ov01a10_acpi_ids),
 	},
 	.probe = ov01a10_probe,
-- 
2.51.0

[PATCH 13/25] media: i2c: ov01a10: Don't update pixel_rate and link_freq from set_fmt

[Hans de Goede]

The pixel_rate and link_freq never change, stop updating them on every
set_fmt.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 8fe0b0d4f9e6..21fca0d13c87 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -620,8 +620,6 @@ static int ov01a10_set_format(struct v4l2_subdev *sd,
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		ov01a10->cur_mode = mode;
-		__v4l2_ctrl_s_ctrl(ov01a10->link_freq, mode->link_freq_index);
-		__v4l2_ctrl_s_ctrl_int64(ov01a10->pixel_rate, OV01A10_SCLK);
 
 		vblank_def = mode->vts_def - mode->height;
 		__v4l2_ctrl_modify_range(ov01a10->vblank,
-- 
2.51.0

[PATCH 14/25] media: i2c: ov01a10: Move setting of ctrl->flags to after checking ctrl_hdlr->error

[Hans de Goede]

Instead of checking successful creation of the link_freq and vblank
controls, set their flags after checking ctrl_hdlr->error where it
is guaranteed that the controls will exist.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 21fca0d13c87..d0153e606c9a 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -461,8 +461,6 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 						    V4L2_CID_LINK_FREQ,
 						    ov01a10->link_freq_index, 0,
 						    link_freq_menu_items);
-	if (ov01a10->link_freq)
-		ov01a10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	ov01a10->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
 						V4L2_CID_PIXEL_RATE, 0,
@@ -479,8 +477,6 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	ov01a10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
 					    V4L2_CID_HBLANK, h_blank, h_blank,
 					    1, h_blank);
-	if (ov01a10->hblank)
-		ov01a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
 			  OV01A10_ANAL_GAIN_MIN, OV01A10_ANAL_GAIN_MAX,
@@ -517,6 +513,9 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 		goto fail;
 	}
 
+	ov01a10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	ov01a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	ov01a10->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.51.0

[PATCH 15/25] media: i2c: ov01a10: Use native and default for pixel-array size names

[Hans de Goede]

According to the OV01A10 product-brief PDF the OV01A10 has an active pixel
array size of 1296x816. In otherwords the native and active sizes are
the same.

Replace the (misspelled) ACTIVE defines for the default resolution of
1280x800 with DEFAULT to avoid giving the impression that the active pixel
array size is only 1280x800.

And replace PIXEL_ARRAY with NATIVE to make clear this is the native pixel
array size / to match the V4L2_SEL_TGT_NATIVE_SIZE naming.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index d0153e606c9a..f3bcb61c88dd 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -34,10 +34,10 @@
 #define OV01A10_MODE_STREAMING		0x01
 
 /* pixel array */
-#define OV01A10_PIXEL_ARRAY_WIDTH	1296
-#define OV01A10_PIXEL_ARRAY_HEIGHT	816
-#define OV01A10_ACITVE_WIDTH		1280
-#define OV01A10_ACITVE_HEIGHT		800
+#define OV01A10_NATIVE_WIDTH		1296
+#define OV01A10_NATIVE_HEIGHT		816
+#define OV01A10_DEFAULT_WIDTH		1280
+#define OV01A10_DEFAULT_HEIGHT		800
 
 /* vertical and horizontal timings */
 #define OV01A10_REG_VTS			CCI_REG16(0x380e)
@@ -271,8 +271,8 @@ static const struct ov01a10_link_freq_config link_freq_configs[] = {
 
 static const struct ov01a10_mode supported_modes[] = {
 	{
-		.width = OV01A10_ACITVE_WIDTH,
-		.height = OV01A10_ACITVE_HEIGHT,
+		.width = OV01A10_DEFAULT_WIDTH,
+		.height = OV01A10_DEFAULT_HEIGHT,
 		.hts = OV01A10_HTS_DEF,
 		.vts_def = OV01A10_VTS_DEF,
 		.vts_min = OV01A10_VTS_MIN,
@@ -643,8 +643,8 @@ static int ov01a10_init_state(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 		.format = {
-			.width = OV01A10_ACITVE_WIDTH,
-			.height = OV01A10_ACITVE_HEIGHT,
+			.width = OV01A10_DEFAULT_WIDTH,
+			.height = OV01A10_DEFAULT_HEIGHT,
 		},
 	};
 
@@ -693,17 +693,17 @@ static int ov01a10_get_selection(struct v4l2_subdev *sd,
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 		sel->r.top = 0;
 		sel->r.left = 0;
-		sel->r.width = OV01A10_PIXEL_ARRAY_WIDTH;
-		sel->r.height = OV01A10_PIXEL_ARRAY_HEIGHT;
+		sel->r.width = OV01A10_NATIVE_WIDTH;
+		sel->r.height = OV01A10_NATIVE_HEIGHT;
 		return 0;
 	case V4L2_SEL_TGT_CROP:
 	case V4L2_SEL_TGT_CROP_DEFAULT:
-		sel->r.top = (OV01A10_PIXEL_ARRAY_HEIGHT -
-			      OV01A10_ACITVE_HEIGHT) / 2;
-		sel->r.left = (OV01A10_PIXEL_ARRAY_WIDTH -
-			       OV01A10_ACITVE_WIDTH) / 2;
-		sel->r.width = OV01A10_ACITVE_WIDTH;
-		sel->r.height = OV01A10_ACITVE_HEIGHT;
+		sel->r.top = (OV01A10_NATIVE_HEIGHT -
+			      OV01A10_DEFAULT_HEIGHT) / 2;
+		sel->r.left = (OV01A10_NATIVE_WIDTH -
+			       OV01A10_DEFAULT_WIDTH) / 2;
+		sel->r.width = OV01A10_DEFAULT_WIDTH;
+		sel->r.height = OV01A10_DEFAULT_HEIGHT;
 		return 0;
 	}
 
-- 
2.51.0

[PATCH 16/25] media: i2c: ov01a10: Add cropping support / allow arbitrary sizes

[Hans de Goede]

Remove the fixed mode list and add cropping support. The main reason for
doing this is to allow libcamera to select 1292x812 instead of 1280x800
so that after the extra border which the CPU debayer code needs libcamera
can output 1280x720 instead of 1276x720.

This in turn allows google-meet to use 720p instead of it falling back
to a pretty bad 360p.

This has been tested on a Dell XPS 9320, with both libcamera as well as
with Intel's out-of-tree psys driver + proprietary userspace stack.

Libcamera asks for 1292x812 where as the Intel stack asks for 1280x800
and neither stack explicitly sets the crop-window. Hence the need for
ov01a10_set_format() to adjust the crop-window if necessary.

Note the differentiating between pattern_size and border_size is done in
preparation for adding support for the monochrome OV01A1B model where
coordinates still need to be aligned to a multiple of 2, but there will
be no need for a border (border_size=0).

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2337593
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 344 ++++++++++++++++++++++--------------
 1 file changed, 210 insertions(+), 134 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index f3bcb61c88dd..e8ccb295fdc9 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -40,7 +40,6 @@
 #define OV01A10_DEFAULT_HEIGHT		800
 
 /* vertical and horizontal timings */
-#define OV01A10_REG_VTS			CCI_REG16(0x380e)
 #define OV01A10_VTS_DEF			0x0700
 #define OV01A10_VTS_MIN			0x0380
 #define OV01A10_VTS_MAX			0xffff
@@ -68,19 +67,26 @@
 #define OV01A10_DGTL_GAIN_STEP		1
 #define OV01A10_DGTL_GAIN_DEFAULT	1024
 
-/* test pattern control */
-#define OV01A10_REG_TEST_PATTERN	CCI_REG8(0x4503)
-#define OV01A10_TEST_PATTERN_ENABLE	BIT(7)
-#define OV01A10_LINK_FREQ_400MHZ_INDEX	0
+/* timing control */
+#define OV01A10_REG_X_ADDR_START	CCI_REG16(0x3800)
+#define OV01A10_REG_Y_ADDR_START	CCI_REG16(0x3802)
+#define OV01A10_REG_X_ADDR_END		CCI_REG16(0x3804)
+#define OV01A10_REG_Y_ADDR_END		CCI_REG16(0x3806)
+#define OV01A10_REG_X_OUTPUT_SIZE	CCI_REG16(0x3808)
+#define OV01A10_REG_Y_OUTPUT_SIZE	CCI_REG16(0x380a)
+#define OV01A10_REG_HTS			CCI_REG16(0x380c) /* in units of 2 pixels */
+#define OV01A10_REG_VTS			CCI_REG16(0x380e)
+#define OV01A10_REG_X_WIN		CCI_REG16(0x3810)
+#define OV01A10_REG_Y_WIN		CCI_REG16(0x3812)
 
 /* flip and mirror control */
 #define OV01A10_REG_FORMAT1		CCI_REG8(0x3820)
 #define OV01A10_VFLIP_MASK		BIT(4)
 #define OV01A10_HFLIP_MASK		BIT(3)
 
-/* window offset */
-#define OV01A10_REG_X_WIN		CCI_REG16(0x3810)
-#define OV01A10_REG_Y_WIN		CCI_REG16(0x3812)
+/* test pattern control */
+#define OV01A10_REG_TEST_PATTERN	CCI_REG8(0x4503)
+#define OV01A10_TEST_PATTERN_ENABLE	BIT(7)
 
 /*
  * The native ov01a10 bayer-pattern is GBRG, but there was a driver bug enabling
@@ -90,6 +96,7 @@
  * when hflip is *disabled*.
  */
 #define OV01A10_MEDIA_BUS_FMT		MEDIA_BUS_FMT_SBGGR10_1X10
+#define OV01A10_BAYER_PATTERN_SIZE	2 /* 2x2 */
 
 struct ov01a10_reg_list {
 	u32 num_of_regs;
@@ -100,17 +107,6 @@ struct ov01a10_link_freq_config {
 	const struct ov01a10_reg_list reg_list;
 };
 
-struct ov01a10_mode {
-	u32 width;
-	u32 height;
-	u32 hts;
-	u32 vts_def;
-	u32 vts_min;
-	u32 link_freq_index;
-
-	const struct ov01a10_reg_list reg_list;
-};
-
 static const struct reg_sequence mipi_data_rate_720mbps[] = {
 	{0x0103, 0x01},
 	{0x0302, 0x00},
@@ -127,7 +123,7 @@ static const struct reg_sequence mipi_data_rate_720mbps[] = {
 	{0x0325, 0x68},
 };
 
-static const struct reg_sequence sensor_1280x800_setting[] = {
+static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x3002, 0xa1},
 	{0x301e, 0xf0},
 	{0x3022, 0x01},
@@ -179,26 +175,6 @@ static const struct reg_sequence sensor_1280x800_setting[] = {
 	{0x37e4, 0x04},
 	{0x37e5, 0x03},
 	{0x37e6, 0x04},
-	{0x3800, 0x00},
-	{0x3801, 0x00},
-	{0x3802, 0x00},
-	{0x3803, 0x00},
-	{0x3804, 0x05},
-	{0x3805, 0x0f},
-	{0x3806, 0x03},
-	{0x3807, 0x2f},
-	{0x3808, 0x05},
-	{0x3809, 0x00},
-	{0x380a, 0x03},
-	{0x380b, 0x20},
-	{0x380c, 0x02},
-	{0x380d, 0xe8},
-	{0x380e, 0x07},
-	{0x380f, 0x00},
-	{0x3810, 0x00},
-	{0x3811, 0x09},
-	{0x3812, 0x00},
-	{0x3813, 0x08},
 	{0x3814, 0x01},
 	{0x3815, 0x01},
 	{0x3816, 0x01},
@@ -261,7 +237,7 @@ static const s64 link_freq_menu_items[] = {
 };
 
 static const struct ov01a10_link_freq_config link_freq_configs[] = {
-	[OV01A10_LINK_FREQ_400MHZ_INDEX] = {
+	{
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mipi_data_rate_720mbps),
 			.regs = mipi_data_rate_720mbps,
@@ -269,19 +245,11 @@ static const struct ov01a10_link_freq_config link_freq_configs[] = {
 	},
 };
 
-static const struct ov01a10_mode supported_modes[] = {
-	{
-		.width = OV01A10_DEFAULT_WIDTH,
-		.height = OV01A10_DEFAULT_HEIGHT,
-		.hts = OV01A10_HTS_DEF,
-		.vts_def = OV01A10_VTS_DEF,
-		.vts_min = OV01A10_VTS_MIN,
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(sensor_1280x800_setting),
-			.regs = sensor_1280x800_setting,
-		},
-		.link_freq_index = OV01A10_LINK_FREQ_400MHZ_INDEX,
-	},
+static const struct v4l2_rect ov01a10_default_crop = {
+	.left = (OV01A10_NATIVE_WIDTH - OV01A10_DEFAULT_WIDTH) / 2,
+	.top = (OV01A10_NATIVE_HEIGHT - OV01A10_DEFAULT_HEIGHT) / 2,
+	.width = OV01A10_DEFAULT_WIDTH,
+	.height = OV01A10_DEFAULT_HEIGHT,
 };
 
 static const char * const ov01a10_supply_names[] = {
@@ -304,7 +272,6 @@ struct ov01a10 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
 
-	const struct ov01a10_mode *cur_mode;
 	u32 link_freq_index;
 
 	struct clk *clk;
@@ -318,6 +285,22 @@ static inline struct ov01a10 *to_ov01a10(struct v4l2_subdev *subdev)
 	return container_of(subdev, struct ov01a10, sd);
 }
 
+static struct v4l2_mbus_framefmt *ov01a10_get_active_format(struct ov01a10 *ov01a10)
+{
+	struct v4l2_subdev_state *active_state =
+		v4l2_subdev_get_locked_active_state(&ov01a10->sd);
+
+	return v4l2_subdev_state_get_format(active_state, 0);
+}
+
+static struct v4l2_rect *ov01a10_get_active_crop(struct ov01a10 *ov01a10)
+{
+	struct v4l2_subdev_state *active_state =
+		v4l2_subdev_get_locked_active_state(&ov01a10->sd);
+
+	return v4l2_subdev_state_get_crop(active_state, 0);
+}
+
 static int ov01a10_update_digital_gain(struct ov01a10 *ov01a10, u32 d_gain)
 {
 	u32 real = d_gain << 6;
@@ -340,13 +323,16 @@ static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
 			 NULL);
 }
 
-/* for vflip and hflip, use 0x9 as window offset to keep the bayer */
 static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
 {
+	struct v4l2_rect *crop = ov01a10_get_active_crop(ov01a10);
 	u32 val, offset;
 	int ret = 0;
 
-	offset = hflip ? 0x8 : 0x9;
+	offset = crop->left;
+	if (!hflip)
+		offset++;
+
 	val = hflip ? 0 : FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
 
 	cci_write(ov01a10->regmap, OV01A10_REG_X_WIN, offset, &ret);
@@ -358,10 +344,14 @@ static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
 
 static int ov01a10_set_vflip(struct ov01a10 *ov01a10, u32 vflip)
 {
+	struct v4l2_rect *crop = ov01a10_get_active_crop(ov01a10);
 	u32 val, offset;
 	int ret = 0;
 
-	offset = vflip ? 0x9 : 0x8;
+	offset = crop->top;
+	if (vflip)
+		offset++;
+
 	val = vflip ? FIELD_PREP(OV01A10_VFLIP_MASK, 0x1) : 0;
 
 	cci_write(ov01a10->regmap, OV01A10_REG_Y_WIN, offset, &ret);
@@ -375,12 +365,13 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov01a10 *ov01a10 = container_of(ctrl->handler,
 					       struct ov01a10, ctrl_handler);
+	struct v4l2_mbus_framefmt *fmt = ov01a10_get_active_format(ov01a10);
 	s64 exposure_max;
 	int ret = 0;
 
 	if (ctrl->id == V4L2_CID_VBLANK) {
-		exposure_max = ov01a10->cur_mode->height + ctrl->val -
-			OV01A10_EXPOSURE_MAX_MARGIN;
+		exposure_max = fmt->height + ctrl->val -
+			       OV01A10_EXPOSURE_MAX_MARGIN;
 		__v4l2_ctrl_modify_range(ov01a10->exposure,
 					 ov01a10->exposure->minimum,
 					 exposure_max, ov01a10->exposure->step,
@@ -407,7 +398,7 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	case V4L2_CID_VBLANK:
 		ret = cci_write(ov01a10->regmap, OV01A10_REG_VTS,
-				ov01a10->cur_mode->height + ctrl->val, NULL);
+				fmt->height + ctrl->val, NULL);
 		break;
 
 	case V4L2_CID_TEST_PATTERN:
@@ -441,7 +432,6 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	struct v4l2_fwnode_device_properties props;
 	u32 vblank_min, vblank_max, vblank_default;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
-	const struct ov01a10_mode *cur_mode;
 	s64 exposure_max, h_blank;
 	int ret = 0;
 
@@ -454,8 +444,6 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	if (ret)
 		return ret;
 
-	cur_mode = ov01a10->cur_mode;
-
 	ov01a10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 						    &ov01a10_ctrl_ops,
 						    V4L2_CID_LINK_FREQ,
@@ -466,14 +454,14 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 						V4L2_CID_PIXEL_RATE, 0,
 						OV01A10_SCLK, 1, OV01A10_SCLK);
 
-	vblank_min = cur_mode->vts_min - cur_mode->height;
-	vblank_max = OV01A10_VTS_MAX - cur_mode->height;
-	vblank_default = cur_mode->vts_def - cur_mode->height;
+	vblank_min = OV01A10_VTS_MIN - OV01A10_DEFAULT_HEIGHT;
+	vblank_max = OV01A10_VTS_MAX - OV01A10_DEFAULT_HEIGHT;
+	vblank_default = OV01A10_VTS_DEF - OV01A10_DEFAULT_HEIGHT;
 	ov01a10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
 					    V4L2_CID_VBLANK, vblank_min,
 					    vblank_max, 1, vblank_default);
 
-	h_blank = cur_mode->hts - cur_mode->width;
+	h_blank = OV01A10_HTS_DEF - OV01A10_DEFAULT_WIDTH;
 	ov01a10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
 					    V4L2_CID_HBLANK, h_blank, h_blank,
 					    1, h_blank);
@@ -485,7 +473,7 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 			  OV01A10_DGTL_GAIN_MIN, OV01A10_DGTL_GAIN_MAX,
 			  OV01A10_DGTL_GAIN_STEP, OV01A10_DGTL_GAIN_DEFAULT);
 
-	exposure_max = cur_mode->vts_def - OV01A10_EXPOSURE_MAX_MARGIN;
+	exposure_max = OV01A10_VTS_DEF - OV01A10_EXPOSURE_MAX_MARGIN;
 	ov01a10->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
 					      V4L2_CID_EXPOSURE,
 					      OV01A10_EXPOSURE_MIN,
@@ -525,24 +513,48 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	return ret;
 }
 
-static void ov01a10_update_pad_format(const struct ov01a10_mode *mode,
-				      struct v4l2_mbus_framefmt *fmt)
+static void ov01a10_fill_format(struct v4l2_mbus_framefmt *fmt,
+				unsigned int width, unsigned int height)
 {
-	fmt->width = mode->width;
-	fmt->height = mode->height;
+	memset(fmt, 0, sizeof(*fmt));
+	fmt->width = width;
+	fmt->height = height;
 	fmt->code = OV01A10_MEDIA_BUS_FMT;
 	fmt->field = V4L2_FIELD_NONE;
 	fmt->colorspace = V4L2_COLORSPACE_RAW;
 }
 
+static int ov01a10_set_mode(struct ov01a10 *ov01a10)
+{
+	struct v4l2_mbus_framefmt *fmt = ov01a10_get_active_format(ov01a10);
+	int ret = 0;
+
+	cci_write(ov01a10->regmap, OV01A10_REG_X_ADDR_START, 0, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_Y_ADDR_START, 0, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_X_ADDR_END,
+		  OV01A10_NATIVE_WIDTH - 1, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_Y_ADDR_END,
+		  OV01A10_NATIVE_HEIGHT - 1, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_X_OUTPUT_SIZE,
+		  fmt->width, &ret);
+	cci_write(ov01a10->regmap, OV01A10_REG_Y_OUTPUT_SIZE,
+		  fmt->height, &ret);
+	/* HTS register is in units of 2 pixels */
+	cci_write(ov01a10->regmap, OV01A10_REG_HTS,
+		  OV01A10_HTS_DEF / 2, &ret);
+	/* OV01A10_REG_VTS is set by vblank control */
+	/* OV01A10_REG_X_WIN is set by hlip control */
+	/* OV01A10_REG_Y_WIN is set by vflip control */
+
+	return ret;
+}
+
 static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 {
 	const struct ov01a10_reg_list *reg_list;
-	int link_freq_index;
-	int ret = 0;
+	int ret;
 
-	link_freq_index = ov01a10->cur_mode->link_freq_index;
-	reg_list = &link_freq_configs[link_freq_index].reg_list;
+	reg_list = &link_freq_configs[ov01a10->link_freq_index].reg_list;
 	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
 				     reg_list->num_of_regs);
 	if (ret) {
@@ -550,9 +562,14 @@ static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 		return ret;
 	}
 
-	reg_list = &ov01a10->cur_mode->reg_list;
-	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
-				     reg_list->num_of_regs);
+	ret = regmap_multi_reg_write(ov01a10->regmap, ov01a10_global_setting,
+				     ARRAY_SIZE(ov01a10_global_setting));
+	if (ret) {
+		dev_err(ov01a10->dev, "failed to initialize sensor\n");
+		return ret;
+	}
+
+	ret = ov01a10_set_mode(ov01a10);
 	if (ret) {
 		dev_err(ov01a10->dev, "failed to set mode\n");
 		return ret;
@@ -601,54 +618,64 @@ static int ov01a10_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static void ov01a10_update_blank_ctrls(struct ov01a10 *ov01a10,
+				       unsigned int width, unsigned int height)
+{
+	s32 hblank, vblank_def;
+
+	vblank_def = OV01A10_VTS_DEF - height;
+	__v4l2_ctrl_modify_range(ov01a10->vblank,
+				 OV01A10_VTS_MIN - height,
+				 OV01A10_VTS_MAX - height, 1,
+				 vblank_def);
+	__v4l2_ctrl_s_ctrl(ov01a10->vblank, vblank_def);
+
+	hblank = OV01A10_HTS_DEF - width;
+	__v4l2_ctrl_modify_range(ov01a10->hblank, hblank, hblank, 1, hblank);
+}
+
 static int ov01a10_set_format(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *sd_state,
 			      struct v4l2_subdev_format *fmt)
 {
+	struct v4l2_rect *crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
+	const int pattern_size = OV01A10_BAYER_PATTERN_SIZE;
+	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
 	struct ov01a10 *ov01a10 = to_ov01a10(sd);
-	const struct ov01a10_mode *mode;
-	struct v4l2_mbus_framefmt *format;
-	s32 vblank_def, h_blank;
+	unsigned int width, height;
 
-	mode = v4l2_find_nearest_size(supported_modes,
-				      ARRAY_SIZE(supported_modes), width,
-				      height, fmt->format.width,
-				      fmt->format.height);
+	width = clamp_val(ALIGN(fmt->format.width, pattern_size),
+			  pattern_size,
+			  OV01A10_NATIVE_WIDTH - 2 * border_size);
+	height = clamp_val(ALIGN(fmt->format.height, pattern_size),
+			   pattern_size,
+			   OV01A10_NATIVE_HEIGHT - 2 * border_size);
 
-	ov01a10_update_pad_format(mode, &fmt->format);
-
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		ov01a10->cur_mode = mode;
-
-		vblank_def = mode->vts_def - mode->height;
-		__v4l2_ctrl_modify_range(ov01a10->vblank,
-					 mode->vts_min - mode->height,
-					 OV01A10_VTS_MAX - mode->height, 1,
-					 vblank_def);
-		__v4l2_ctrl_s_ctrl(ov01a10->vblank, vblank_def);
-		h_blank = mode->hts - mode->width;
-		__v4l2_ctrl_modify_range(ov01a10->hblank, h_blank, h_blank, 1,
-					 h_blank);
+	/* Center image for userspace which does not set the crop first */
+	if (width != crop->width || height != crop->height) {
+		crop->left = ALIGN((OV01A10_NATIVE_WIDTH - width) / 2,
+				   pattern_size);
+		crop->top = ALIGN((OV01A10_NATIVE_HEIGHT - height) / 2,
+				  pattern_size);
+		crop->width = width;
+		crop->height = height;
 	}
 
-	format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
-	*format = fmt->format;
+	ov01a10_fill_format(&fmt->format, width, height);
+	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		ov01a10_update_blank_ctrls(ov01a10, width, height);
 
 	return 0;
 }
 
 static int ov01a10_init_state(struct v4l2_subdev *sd,
-			      struct v4l2_subdev_state *state)
+			      struct v4l2_subdev_state *sd_state)
 {
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_TRY,
-		.format = {
-			.width = OV01A10_DEFAULT_WIDTH,
-			.height = OV01A10_DEFAULT_HEIGHT,
-		},
-	};
-
-	ov01a10_set_format(sd, state, &fmt);
+	*v4l2_subdev_state_get_crop(sd_state, 0) = ov01a10_default_crop;
+	ov01a10_fill_format(v4l2_subdev_state_get_format(sd_state, 0),
+			    OV01A10_DEFAULT_WIDTH, OV01A10_DEFAULT_HEIGHT);
 
 	return 0;
 }
@@ -669,14 +696,16 @@ static int ov01a10_enum_frame_size(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->index >= ARRAY_SIZE(supported_modes) ||
-	    fse->code != OV01A10_MEDIA_BUS_FMT)
+	const int pattern_size = OV01A10_BAYER_PATTERN_SIZE;
+	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
+
+	if (fse->index)
 		return -EINVAL;
 
-	fse->min_width = supported_modes[fse->index].width;
-	fse->max_width = fse->min_width;
-	fse->min_height = supported_modes[fse->index].height;
-	fse->max_height = fse->min_height;
+	fse->min_width = pattern_size;
+	fse->max_width = OV01A10_NATIVE_WIDTH - 2 * border_size;
+	fse->min_height = pattern_size;
+	fse->max_height = OV01A10_NATIVE_HEIGHT - 2 * border_size;
 
 	return 0;
 }
@@ -685,31 +714,79 @@ static int ov01a10_get_selection(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_selection *sel)
 {
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
+	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
 
 	switch (sel->target) {
-	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
+		return 0;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r = ov01a10_default_crop;
+		return 0;
 	case V4L2_SEL_TGT_CROP_BOUNDS:
-		sel->r.top = 0;
+		/* Keep a border for hvflip shift to preserve bayer-pattern */
+		sel->r.left = border_size;
+		sel->r.top = border_size;
+		sel->r.width = OV01A10_NATIVE_WIDTH - 2 * border_size;
+		sel->r.height = OV01A10_NATIVE_HEIGHT - 2 * border_size;
+		return 0;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
 		sel->r.left = 0;
+		sel->r.top = 0;
 		sel->r.width = OV01A10_NATIVE_WIDTH;
 		sel->r.height = OV01A10_NATIVE_HEIGHT;
 		return 0;
-	case V4L2_SEL_TGT_CROP:
-	case V4L2_SEL_TGT_CROP_DEFAULT:
-		sel->r.top = (OV01A10_NATIVE_HEIGHT -
-			      OV01A10_DEFAULT_HEIGHT) / 2;
-		sel->r.left = (OV01A10_NATIVE_WIDTH -
-			       OV01A10_DEFAULT_WIDTH) / 2;
-		sel->r.width = OV01A10_DEFAULT_WIDTH;
-		sel->r.height = OV01A10_DEFAULT_HEIGHT;
-		return 0;
 	}
 
 	return -EINVAL;
 }
 
+static int ov01a10_set_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_selection *sel)
+{
+	const int pattern_size = OV01A10_BAYER_PATTERN_SIZE;
+	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
+	struct v4l2_rect rect;
+
+	if (sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	/*
+	 * Clamp the boundaries of the crop rectangle to the size of the sensor
+	 * pixel array. Align to pattern-size to ensure pattern isn't disrupted.
+	 */
+	rect.left = clamp_val(ALIGN(sel->r.left, pattern_size), border_size,
+			      OV01A10_NATIVE_WIDTH - 2 * border_size);
+	rect.top = clamp_val(ALIGN(sel->r.top, pattern_size), border_size,
+			     OV01A10_NATIVE_HEIGHT - 2 * border_size);
+	rect.width = clamp_val(ALIGN(sel->r.width, pattern_size), pattern_size,
+			       OV01A10_NATIVE_WIDTH - rect.left - border_size);
+	rect.height = clamp_val(ALIGN(sel->r.height, pattern_size), pattern_size,
+				OV01A10_NATIVE_HEIGHT - rect.top - border_size);
+
+	crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
+
+	/* Reset the output size if the crop rectangle size has changed */
+	if (rect.width != crop->width || rect.height != crop->height) {
+		format = v4l2_subdev_state_get_format(sd_state, sel->pad);
+		format->width = rect.width;
+		format->height = rect.height;
+
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			ov01a10_update_blank_ctrls(ov01a10, rect.width,
+						   rect.height);
+	}
+
+	*crop = rect;
+	sel->r = rect;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_core_ops ov01a10_core_ops = {
 	.log_status = v4l2_ctrl_subdev_log_status,
 };
@@ -722,6 +799,7 @@ static const struct v4l2_subdev_pad_ops ov01a10_pad_ops = {
 	.set_fmt = ov01a10_set_format,
 	.get_fmt = v4l2_subdev_get_fmt,
 	.get_selection = ov01a10_get_selection,
+	.set_selection = ov01a10_set_selection,
 	.enum_mbus_code = ov01a10_enum_mbus_code,
 	.enum_frame_size = ov01a10_enum_frame_size,
 };
@@ -941,8 +1019,6 @@ static int ov01a10_probe(struct i2c_client *client)
 	if (ret)
 		goto err_power_off;
 
-	ov01a10->cur_mode = &supported_modes[0];
-
 	ret = ov01a10_init_controls(ov01a10);
 	if (ret)
 		goto err_power_off;
-- 
2.51.0

[PATCH 17/25] media: i2c: ov01a10: Remove struct ov01a10_reg_list

[Hans de Goede]

After the conversion to the CCI register access helpers, struct
ov01a10_reg_list is only used inside struct ov01a10_link_freq_config.

Simplify things by embedding the ov01a10_reg_list members directly into
struct ov01a10_link_freq_config.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index e8ccb295fdc9..805ed42d86f9 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -98,13 +98,9 @@
 #define OV01A10_MEDIA_BUS_FMT		MEDIA_BUS_FMT_SBGGR10_1X10
 #define OV01A10_BAYER_PATTERN_SIZE	2 /* 2x2 */
 
-struct ov01a10_reg_list {
-	u32 num_of_regs;
-	const struct reg_sequence *regs;
-};
-
 struct ov01a10_link_freq_config {
-	const struct ov01a10_reg_list reg_list;
+	const struct reg_sequence *regs;
+	int regs_len;
 };
 
 static const struct reg_sequence mipi_data_rate_720mbps[] = {
@@ -238,10 +234,8 @@ static const s64 link_freq_menu_items[] = {
 
 static const struct ov01a10_link_freq_config link_freq_configs[] = {
 	{
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(mipi_data_rate_720mbps),
-			.regs = mipi_data_rate_720mbps,
-		}
+		.regs = mipi_data_rate_720mbps,
+		.regs_len = ARRAY_SIZE(mipi_data_rate_720mbps),
 	},
 };
 
@@ -551,12 +545,12 @@ static int ov01a10_set_mode(struct ov01a10 *ov01a10)
 
 static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 {
-	const struct ov01a10_reg_list *reg_list;
+	const struct ov01a10_link_freq_config *freq_cfg;
 	int ret;
 
-	reg_list = &link_freq_configs[ov01a10->link_freq_index].reg_list;
-	ret = regmap_multi_reg_write(ov01a10->regmap, reg_list->regs,
-				     reg_list->num_of_regs);
+	freq_cfg = &link_freq_configs[ov01a10->link_freq_index];
+	ret = regmap_multi_reg_write(ov01a10->regmap, freq_cfg->regs,
+				     freq_cfg->regs_len);
 	if (ret) {
 		dev_err(ov01a10->dev, "failed to set plls\n");
 		return ret;
-- 
2.51.0

[PATCH 18/25] media: i2c: ov01a10: Replace exposure->min/step with direct define use

[Hans de Goede]

The exposure minimum and step are constant use the defines for this
instead of retrieving these from the exposure-control.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 805ed42d86f9..c8b870cf6318 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -367,9 +367,8 @@ static int ov01a10_set_ctrl(struct v4l2_ctrl *ctrl)
 		exposure_max = fmt->height + ctrl->val -
 			       OV01A10_EXPOSURE_MAX_MARGIN;
 		__v4l2_ctrl_modify_range(ov01a10->exposure,
-					 ov01a10->exposure->minimum,
-					 exposure_max, ov01a10->exposure->step,
-					 exposure_max);
+					 OV01A10_EXPOSURE_MIN, exposure_max,
+					 OV01A10_EXPOSURE_STEP, exposure_max);
 	}
 
 	if (!pm_runtime_get_if_in_use(ov01a10->dev))
-- 
2.51.0

[PATCH 19/25] media: i2c: ov01a10: Only set register 0x0305 once

[Hans de Goede]

Stop setting register 0x0305 to one value from mipi_data_rate_720mbps
only to override it with a different value from sensor_1280x800_setting.

Instead directly set it to 0xf4.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index c8b870cf6318..77c82201815b 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -108,7 +108,7 @@ static const struct reg_sequence mipi_data_rate_720mbps[] = {
 	{0x0302, 0x00},
 	{0x0303, 0x06},
 	{0x0304, 0x01},
-	{0x0305, 0xe0},
+	{0x0305, 0xf4},
 	{0x0306, 0x00},
 	{0x0308, 0x01},
 	{0x0309, 0x00},
@@ -216,7 +216,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x5200, 0x18},
 	{0x5004, 0x00},
 	{0x5080, 0x40},
-	{0x0305, 0xf4},
 	{0x0325, 0xc2},
 };
 
-- 
2.51.0

[PATCH 20/25] media: i2c: ov01a10: Remove values set by controls from global_setting[]

[Hans de Goede]

Registers 0x3501 (exposure), 0x3508 (analogue-gain) and 0x4503 (test-
pattern) are already set through __v4l2_ctrl_handler_setup() drop them
from ov01a10_global_setting[].

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 77c82201815b..17d8778561d4 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -123,11 +123,7 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x3002, 0xa1},
 	{0x301e, 0xf0},
 	{0x3022, 0x01},
-	{0x3501, 0x03},
-	{0x3502, 0x78},
 	{0x3504, 0x0c},
-	{0x3508, 0x01},
-	{0x3509, 0x00},
 	{0x3601, 0xc0},
 	{0x3603, 0x71},
 	{0x3610, 0x68},
@@ -197,7 +193,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x4300, 0xff},
 	{0x4301, 0x00},
 	{0x4302, 0x0f},
-	{0x4503, 0x00},
 	{0x4601, 0x50},
 	{0x4800, 0x64},
 	{0x481f, 0x34},
-- 
2.51.0

[PATCH 21/25] media: i2c: ov01a10: Add ov01a10_sensor_cfg struct

[Hans de Goede]

Add a struct with some sensor variant (ov01a10 / ov01a1b / ov01a1s)
specific settings.

This is a preparation patch for adding support for the ov01a1s sensor
which uses the same sensor with a different (RGBI) color-filter.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 87 +++++++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 27 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 17d8778561d4..51c9c015765f 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -88,16 +88,6 @@
 #define OV01A10_REG_TEST_PATTERN	CCI_REG8(0x4503)
 #define OV01A10_TEST_PATTERN_ENABLE	BIT(7)
 
-/*
- * The native ov01a10 bayer-pattern is GBRG, but there was a driver bug enabling
- * hflip/mirroring by default resulting in BGGR. Because of this bug Intel's
- * proprietary IPU6 userspace stack expects BGGR. So we report BGGR to not break
- * userspace and fix things up by shifting the crop window-x coordinate by 1
- * when hflip is *disabled*.
- */
-#define OV01A10_MEDIA_BUS_FMT		MEDIA_BUS_FMT_SBGGR10_1X10
-#define OV01A10_BAYER_PATTERN_SIZE	2 /* 2x2 */
-
 struct ov01a10_link_freq_config {
 	const struct reg_sequence *regs;
 	int regs_len;
@@ -246,9 +236,19 @@ static const char * const ov01a10_supply_names[] = {
 	"dvdd",		/* Digital core power */
 };
 
+struct ov01a10_sensor_cfg {
+	const char *model;
+	u32 bus_fmt;
+	int pattern_size;
+	int border_size;
+	bool invert_hflip_shift;
+	bool invert_vflip_shift;
+};
+
 struct ov01a10 {
 	struct device *dev;
 	struct regmap *regmap;
+	const struct ov01a10_sensor_cfg *cfg;
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -311,14 +311,15 @@ static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
 			 NULL);
 }
 
-static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
+static int ov01a10_set_hflip(struct ov01a10 *ov01a10, bool hflip)
 {
 	struct v4l2_rect *crop = ov01a10_get_active_crop(ov01a10);
+	const struct ov01a10_sensor_cfg *cfg = ov01a10->cfg;
 	u32 val, offset;
 	int ret = 0;
 
 	offset = crop->left;
-	if (!hflip)
+	if ((hflip ^ cfg->invert_hflip_shift) && cfg->border_size)
 		offset++;
 
 	val = hflip ? 0 : FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
@@ -330,14 +331,15 @@ static int ov01a10_set_hflip(struct ov01a10 *ov01a10, u32 hflip)
 	return ret;
 }
 
-static int ov01a10_set_vflip(struct ov01a10 *ov01a10, u32 vflip)
+static int ov01a10_set_vflip(struct ov01a10 *ov01a10, bool vflip)
 {
 	struct v4l2_rect *crop = ov01a10_get_active_crop(ov01a10);
+	const struct ov01a10_sensor_cfg *cfg = ov01a10->cfg;
 	u32 val, offset;
 	int ret = 0;
 
 	offset = crop->top;
-	if (vflip)
+	if ((vflip ^ cfg->invert_vflip_shift) && cfg->border_size)
 		offset++;
 
 	val = vflip ? FIELD_PREP(OV01A10_VFLIP_MASK, 0x1) : 0;
@@ -500,13 +502,14 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 	return ret;
 }
 
-static void ov01a10_fill_format(struct v4l2_mbus_framefmt *fmt,
+static void ov01a10_fill_format(struct ov01a10 *ov01a10,
+				struct v4l2_mbus_framefmt *fmt,
 				unsigned int width, unsigned int height)
 {
 	memset(fmt, 0, sizeof(*fmt));
 	fmt->width = width;
 	fmt->height = height;
-	fmt->code = OV01A10_MEDIA_BUS_FMT;
+	fmt->code = ov01a10->cfg->bus_fmt;
 	fmt->field = V4L2_FIELD_NONE;
 	fmt->colorspace = V4L2_COLORSPACE_RAW;
 }
@@ -626,9 +629,9 @@ static int ov01a10_set_format(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_format *fmt)
 {
 	struct v4l2_rect *crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
-	const int pattern_size = OV01A10_BAYER_PATTERN_SIZE;
-	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
 	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+	const int pattern_size = ov01a10->cfg->pattern_size;
+	const int border_size = ov01a10->cfg->border_size;
 	unsigned int width, height;
 
 	width = clamp_val(ALIGN(fmt->format.width, pattern_size),
@@ -648,7 +651,7 @@ static int ov01a10_set_format(struct v4l2_subdev *sd,
 		crop->height = height;
 	}
 
-	ov01a10_fill_format(&fmt->format, width, height);
+	ov01a10_fill_format(ov01a10, &fmt->format, width, height);
 	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
@@ -660,8 +663,10 @@ static int ov01a10_set_format(struct v4l2_subdev *sd,
 static int ov01a10_init_state(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *sd_state)
 {
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+
 	*v4l2_subdev_state_get_crop(sd_state, 0) = ov01a10_default_crop;
-	ov01a10_fill_format(v4l2_subdev_state_get_format(sd_state, 0),
+	ov01a10_fill_format(ov01a10, v4l2_subdev_state_get_format(sd_state, 0),
 			    OV01A10_DEFAULT_WIDTH, OV01A10_DEFAULT_HEIGHT);
 
 	return 0;
@@ -671,10 +676,12 @@ static int ov01a10_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_mbus_code_enum *code)
 {
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = OV01A10_MEDIA_BUS_FMT;
+	code->code = ov01a10->cfg->bus_fmt;
 
 	return 0;
 }
@@ -683,8 +690,9 @@ static int ov01a10_enum_frame_size(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
-	const int pattern_size = OV01A10_BAYER_PATTERN_SIZE;
-	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+	const int pattern_size = ov01a10->cfg->pattern_size;
+	const int border_size = ov01a10->cfg->border_size;
 
 	if (fse->index)
 		return -EINVAL;
@@ -701,7 +709,8 @@ static int ov01a10_get_selection(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_selection *sel)
 {
-	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
+	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+	const int border_size = ov01a10->cfg->border_size;
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
@@ -732,9 +741,9 @@ static int ov01a10_set_selection(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_selection *sel)
 {
-	const int pattern_size = OV01A10_BAYER_PATTERN_SIZE;
-	const int border_size = OV01A10_BAYER_PATTERN_SIZE;
 	struct ov01a10 *ov01a10 = to_ov01a10(sd);
+	const int pattern_size = ov01a10->cfg->pattern_size;
+	const int border_size = ov01a10->cfg->border_size;
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
 	struct v4l2_rect rect;
@@ -974,20 +983,28 @@ static void ov01a10_remove(struct i2c_client *client)
 
 static int ov01a10_probe(struct i2c_client *client)
 {
+	const struct ov01a10_sensor_cfg *cfg;
 	struct ov01a10 *ov01a10;
 	int ret;
 
+	cfg = device_get_match_data(&client->dev);
+	if (!cfg)
+		return -EINVAL;
+
 	ov01a10 = devm_kzalloc(&client->dev, sizeof(*ov01a10), GFP_KERNEL);
 	if (!ov01a10)
 		return -ENOMEM;
 
 	ov01a10->dev = &client->dev;
+	ov01a10->cfg = cfg;
 
 	ov01a10->regmap = devm_cci_regmap_init_i2c(client, 16);
 	if (IS_ERR(ov01a10->regmap))
 		return PTR_ERR(ov01a10->regmap);
 
 	v4l2_i2c_subdev_init(&ov01a10->sd, client, &ov01a10_subdev_ops);
+	/* Override driver->name with actual sensor model */
+	v4l2_i2c_subdev_set_name(&ov01a10->sd, client, cfg->model, NULL);
 	ov01a10->sd.internal_ops = &ov01a10_internal_ops;
 
 	ret = ov01a10_check_hwcfg(ov01a10);
@@ -1059,8 +1076,24 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ov01a10_pm_ops, ov01a10_power_off,
 				 ov01a10_power_on, NULL);
 
 #ifdef CONFIG_ACPI
+/*
+ * The native ov01a10 bayer-pattern is GBRG, but there was a driver bug enabling
+ * hflip/mirroring by default resulting in BGGR. Because of this bug Intel's
+ * proprietary IPU6 userspace stack expects BGGR. So we report BGGR to not break
+ * userspace and fix things up by shifting the crop window-x coordinate by 1
+ * when hflip is *disabled*.
+ */
+static const struct ov01a10_sensor_cfg ov01a10_cfg = {
+	.model = "ov01a10",
+	.bus_fmt = MEDIA_BUS_FMT_SBGGR10_1X10,
+	.pattern_size = 2, /* 2x2 */
+	.border_size = 2,
+	.invert_hflip_shift = true,
+	.invert_vflip_shift = false,
+};
+
 static const struct acpi_device_id ov01a10_acpi_ids[] = {
-	{ "OVTI01A0" },
+	{ "OVTI01A0", (uintptr_t)&ov01a10_cfg },
 	{ }
 };
 
-- 
2.51.0

[PATCH 22/25] media: i2c: ov01a10: Optimize setting h/vflip values

[Hans de Goede]

Since ov01a10_global_setting[] sets the initial format1 register value,
there is no need to do a read-write-modify when setting the flip controls.

Only write format1 when setting the flip-controls and remove the now
unnecessary format1 register init from ov01a10_global_setting[].

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 41 +++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 51c9c015765f..40756e4d6301 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -161,7 +161,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x3815, 0x01},
 	{0x3816, 0x01},
 	{0x3817, 0x01},
-	{0x3820, 0xa8},
 	{0x3822, 0x13},
 	{0x3832, 0x28},
 	{0x3833, 0x10},
@@ -241,6 +240,7 @@ struct ov01a10_sensor_cfg {
 	u32 bus_fmt;
 	int pattern_size;
 	int border_size;
+	u8 format1_base_val;
 	bool invert_hflip_shift;
 	bool invert_vflip_shift;
 };
@@ -259,6 +259,8 @@ struct ov01a10 {
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *vflip;
 
 	u32 link_freq_index;
 
@@ -311,22 +313,33 @@ static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
 			 NULL);
 }
 
+static void ov01a10_set_format1(struct ov01a10 *ov01a10, int *ret)
+{
+	u8 val = ov01a10->cfg->format1_base_val;
+
+	/* hflip register bit is inverted */
+	if (!ov01a10->hflip->val)
+		val |= FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
+
+	if (ov01a10->vflip->val)
+		val |= FIELD_PREP(OV01A10_VFLIP_MASK, 0x1);
+
+	cci_write(ov01a10->regmap, OV01A10_REG_FORMAT1, val, ret);
+}
+
 static int ov01a10_set_hflip(struct ov01a10 *ov01a10, bool hflip)
 {
 	struct v4l2_rect *crop = ov01a10_get_active_crop(ov01a10);
 	const struct ov01a10_sensor_cfg *cfg = ov01a10->cfg;
-	u32 val, offset;
+	u32 offset;
 	int ret = 0;
 
 	offset = crop->left;
 	if ((hflip ^ cfg->invert_hflip_shift) && cfg->border_size)
 		offset++;
 
-	val = hflip ? 0 : FIELD_PREP(OV01A10_HFLIP_MASK, 0x1);
-
 	cci_write(ov01a10->regmap, OV01A10_REG_X_WIN, offset, &ret);
-	cci_update_bits(ov01a10->regmap, OV01A10_REG_FORMAT1,
-			OV01A10_HFLIP_MASK, val, &ret);
+	ov01a10_set_format1(ov01a10, &ret);
 
 	return ret;
 }
@@ -335,18 +348,15 @@ static int ov01a10_set_vflip(struct ov01a10 *ov01a10, bool vflip)
 {
 	struct v4l2_rect *crop = ov01a10_get_active_crop(ov01a10);
 	const struct ov01a10_sensor_cfg *cfg = ov01a10->cfg;
-	u32 val, offset;
+	u32 offset;
 	int ret = 0;
 
 	offset = crop->top;
 	if ((vflip ^ cfg->invert_vflip_shift) && cfg->border_size)
 		offset++;
 
-	val = vflip ? FIELD_PREP(OV01A10_VFLIP_MASK, 0x1) : 0;
-
 	cci_write(ov01a10->regmap, OV01A10_REG_Y_WIN, offset, &ret);
-	cci_update_bits(ov01a10->regmap, OV01A10_REG_FORMAT1,
-			OV01A10_VFLIP_MASK, val, &ret);
+	ov01a10_set_format1(ov01a10, &ret);
 
 	return ret;
 }
@@ -475,10 +485,10 @@ static int ov01a10_init_controls(struct ov01a10 *ov01a10)
 				     ARRAY_SIZE(ov01a10_test_pattern_menu) - 1,
 				     0, 0, ov01a10_test_pattern_menu);
 
-	v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops, V4L2_CID_HFLIP,
-			  0, 1, 1, 0);
-	v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops, V4L2_CID_VFLIP,
-			  0, 1, 1, 0);
+	ov01a10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
+					   V4L2_CID_HFLIP, 0, 1, 1, 0);
+	ov01a10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
+					   V4L2_CID_VFLIP, 0, 1, 1, 0);
 
 	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov01a10_ctrl_ops,
 					      &props);
@@ -1088,6 +1098,7 @@ static const struct ov01a10_sensor_cfg ov01a10_cfg = {
 	.bus_fmt = MEDIA_BUS_FMT_SBGGR10_1X10,
 	.pattern_size = 2, /* 2x2 */
 	.border_size = 2,
+	.format1_base_val = 0xa0,
 	.invert_hflip_shift = true,
 	.invert_vflip_shift = false,
 };
-- 
2.51.0

[PATCH 23/25] media: i2c: ov01a10: Add ov01a1b support

[Hans de Goede]

Add support for the ov01a1b model which is the exact same sensor as
the ov01a10 without a color-filter.

Note since there is no color-filter there is also no need to shift
the crop-window when flipping, so the crop window set by userspace may
cover the full sensor (border_size=0).

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 40756e4d6301..6ddfb8f79b55 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -1103,8 +1103,17 @@ static const struct ov01a10_sensor_cfg ov01a10_cfg = {
 	.invert_vflip_shift = false,
 };
 
+static const struct ov01a10_sensor_cfg ov01a1b_cfg = {
+	.model = "ov01a1b",
+	.bus_fmt = MEDIA_BUS_FMT_Y10_1X10,
+	.pattern_size = 2, /* Keep coordinates aligned to a multiple of 2 */
+	.border_size = 0,
+	.format1_base_val = 0xa0,
+};
+
 static const struct acpi_device_id ov01a10_acpi_ids[] = {
 	{ "OVTI01A0", (uintptr_t)&ov01a10_cfg },
+	{ "OVTI01AB", (uintptr_t)&ov01a1b_cfg },
 	{ }
 };
 
-- 
2.51.0

[PATCH 24/25] media: i2c: ov01a10: Add ov01a1s support

[Hans de Goede]

The ov01a1s sensor is the exact same sensor as the ov01a10 with a different
(RGBI) color-filter.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Note I'm not sure if we should apply this one as is and fixup
the reported mbus-format + add a bayer-order v4l2-control later
or if we delay merging until we can do it correct right away.
---
 drivers/media/i2c/ov01a10.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 6ddfb8f79b55..058ff311a289 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -1111,9 +1111,31 @@ static const struct ov01a10_sensor_cfg ov01a1b_cfg = {
 	.format1_base_val = 0xa0,
 };
 
+/*
+ * The native ov01a1s bayer_pattern is GRGB-IGIG-GBGR-IGIG but Intel's
+ * proprietary IPU6 userspace stack expects IGIG-GBGR-IGIG-GRGB. So we
+ * generate this by shifting the crop window-y coordinate by 1 when
+ * vflip is *disabled*.
+ */
+static const struct ov01a10_sensor_cfg ov01a1s_cfg = {
+	.model = "ov01a1s",
+	/*
+	 * FIXME this obviously is wrong, this needs to be changed to
+	 * MEDIA_BUS_FMT_RAW10_1x10 + reporting the proper bayer-order through
+	 * a v4l2-control once the sensor internal pads series has landed.
+	 */
+	.bus_fmt = MEDIA_BUS_FMT_SGRBG10_1X10, /* really IGIG-GBGR-IGIG-GRGB */
+	.pattern_size = 4, /* 4x4 */
+	.border_size = 4,
+	.format1_base_val = 0x80,
+	.invert_hflip_shift = false,
+	.invert_vflip_shift = true,
+};
+
 static const struct acpi_device_id ov01a10_acpi_ids[] = {
 	{ "OVTI01A0", (uintptr_t)&ov01a10_cfg },
 	{ "OVTI01AB", (uintptr_t)&ov01a1b_cfg },
+	{ "OVTI01AS", (uintptr_t)&ov01a1s_cfg },
 	{ }
 };
 
-- 
2.51.0

[PATCH 25/25] media: i2c: ov01a10: Register tweaks for ov01a1s model

[Hans de Goede]

The out of tree drivers from:
https://github.com/intel/ipu6-drivers/tree/master/drivers/media/i2c

Have a separate driver for the ov01a1s. This driver is 95% the same as
the ov01a10 driver (same sensor, different color-filters) but there are
a couple of registers for which the out of tree ov01a1s driver uses
different values.

Add a per model reg_sequence to struct ov01a10_sensor_cfg and apply
the register tweaks from the out of tree driver to the ov01a1s model.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/ov01a10.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 058ff311a289..08cab1e4be1d 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -161,7 +161,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x3815, 0x01},
 	{0x3816, 0x01},
 	{0x3817, 0x01},
-	{0x3822, 0x13},
 	{0x3832, 0x28},
 	{0x3833, 0x10},
 	{0x3b00, 0x00},
@@ -186,7 +185,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x4800, 0x64},
 	{0x481f, 0x34},
 	{0x4825, 0x33},
-	{0x4837, 0x11},
 	{0x4881, 0x40},
 	{0x4883, 0x01},
 	{0x4890, 0x00},
@@ -203,6 +201,17 @@ static const struct reg_sequence ov01a10_global_setting[] = {
 	{0x0325, 0xc2},
 };
 
+static const struct reg_sequence ov01a10_regs[] = {
+	{0x3822, 0x13},
+	{0x4837, 0x11},
+};
+
+static const struct reg_sequence ov01a1s_regs[] = {
+	{0x3822, 0x03},
+	{0x4837, 0x14},
+	{0x373d, 0x24},
+};
+
 static const char * const ov01a10_test_pattern_menu[] = {
 	"Disabled",
 	"Color Bar",
@@ -237,6 +246,8 @@ static const char * const ov01a10_supply_names[] = {
 
 struct ov01a10_sensor_cfg {
 	const char *model;
+	const struct reg_sequence *regs;
+	int regs_len;
 	u32 bus_fmt;
 	int pattern_size;
 	int border_size;
@@ -551,6 +562,7 @@ static int ov01a10_set_mode(struct ov01a10 *ov01a10)
 
 static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 {
+	const struct ov01a10_sensor_cfg *cfg = ov01a10->cfg;
 	const struct ov01a10_link_freq_config *freq_cfg;
 	int ret;
 
@@ -569,6 +581,12 @@ static int ov01a10_start_streaming(struct ov01a10 *ov01a10)
 		return ret;
 	}
 
+	ret = regmap_multi_reg_write(ov01a10->regmap, cfg->regs, cfg->regs_len);
+	if (ret) {
+		dev_err(ov01a10->dev, "failed to write sensor regs\n");
+		return ret;
+	}
+
 	ret = ov01a10_set_mode(ov01a10);
 	if (ret) {
 		dev_err(ov01a10->dev, "failed to set mode\n");
@@ -1095,6 +1113,8 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ov01a10_pm_ops, ov01a10_power_off,
  */
 static const struct ov01a10_sensor_cfg ov01a10_cfg = {
 	.model = "ov01a10",
+	.regs = ov01a10_regs,
+	.regs_len = ARRAY_SIZE(ov01a10_regs),
 	.bus_fmt = MEDIA_BUS_FMT_SBGGR10_1X10,
 	.pattern_size = 2, /* 2x2 */
 	.border_size = 2,
@@ -1105,6 +1125,8 @@ static const struct ov01a10_sensor_cfg ov01a10_cfg = {
 
 static const struct ov01a10_sensor_cfg ov01a1b_cfg = {
 	.model = "ov01a1b",
+	.regs = ov01a10_regs,
+	.regs_len = ARRAY_SIZE(ov01a10_regs),
 	.bus_fmt = MEDIA_BUS_FMT_Y10_1X10,
 	.pattern_size = 2, /* Keep coordinates aligned to a multiple of 2 */
 	.border_size = 0,
@@ -1119,6 +1141,8 @@ static const struct ov01a10_sensor_cfg ov01a1b_cfg = {
  */
 static const struct ov01a10_sensor_cfg ov01a1s_cfg = {
 	.model = "ov01a1s",
+	.regs = ov01a1s_regs,
+	.regs_len = ARRAY_SIZE(ov01a1s_regs),
 	/*
 	 * FIXME this obviously is wrong, this needs to be changed to
 	 * MEDIA_BUS_FMT_RAW10_1x10 + reporting the proper bayer-order through
-- 
2.51.0

Re: [PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Bingbu Cao]

Hans,

Thank you for the fix. 
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 10/15/25 1:40 AM, Hans de Goede wrote:
> When the test-pattern control gets set to 0 (Disabled) 0 should be written
> to the test-pattern register, rather then doing nothing.
> 
> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/ov01a10.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> index 0405ec7c75fd..733e5bf0180c 100644
> --- a/drivers/media/i2c/ov01a10.c
> +++ b/drivers/media/i2c/ov01a10.c
> @@ -406,10 +406,8 @@ static int ov01a10_update_digital_gain(struct ov01a10 *ov01a10, u32 d_gain)
>  
>  static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
>  {
> -	if (!pattern)
> -		return 0;
> -
> -	pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
> +	if (pattern)
> +		pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
>  
>  	return ov01a10_write_reg(ov01a10, OV01A10_REG_TEST_PATTERN, 1, pattern);
>  }
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 04/25] media: i2c: ov01a10: Add missing v4l2_subdev_cleanup() calls

[Bingbu Cao]

Hans,

Thank for the patch.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 10/15/25 1:40 AM, Hans de Goede wrote:
> Add missing v4l2_subdev_cleanup() calls to cleanup after
> v4l2_subdev_init_finalize().
> 
> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/ov01a10.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> index 95d6a0f046a0..f92867f542f0 100644
> --- a/drivers/media/i2c/ov01a10.c
> +++ b/drivers/media/i2c/ov01a10.c
> @@ -864,6 +864,7 @@ static void ov01a10_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&sd->entity);
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  
> @@ -934,6 +935,7 @@ static int ov01a10_probe(struct i2c_client *client)
>  err_pm_disable:
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(&client->dev);
> +	v4l2_subdev_cleanup(&ov01a10->sd);
>  
>  err_media_entity_cleanup:
>  	media_entity_cleanup(&ov01a10->sd.entity);
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 04/25] media: i2c: ov01a10: Add missing v4l2_subdev_cleanup() calls

[Bingbu Cao]

Hans,

Thank you for the fix. 
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 10/15/25 1:40 AM, Hans de Goede wrote:
> Add missing v4l2_subdev_cleanup() calls to cleanup after
> v4l2_subdev_init_finalize().
> 
> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/ov01a10.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> index 95d6a0f046a0..f92867f542f0 100644
> --- a/drivers/media/i2c/ov01a10.c
> +++ b/drivers/media/i2c/ov01a10.c
> @@ -864,6 +864,7 @@ static void ov01a10_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&sd->entity);
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  
> @@ -934,6 +935,7 @@ static int ov01a10_probe(struct i2c_client *client)
>  err_pm_disable:
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(&client->dev);
> +	v4l2_subdev_cleanup(&ov01a10->sd);
>  
>  err_media_entity_cleanup:
>  	media_entity_cleanup(&ov01a10->sd.entity);
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 25/25] media: i2c: ov01a10: Register tweaks for ov01a1s model

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.18-rc1 next-20251014]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-i2c-ov01a10-Fix-the-horizontal-flip-control/20251015-014349
base:   linus/master
patch link:    https://lore.kernel.org/r/20251014174033.20534-26-hansg%40kernel.org
patch subject: [PATCH 25/25] media: i2c: ov01a10: Register tweaks for ov01a1s model
config: um-randconfig-001-20251015 (https://download.01.org/0day-ci/archive/20251015/202510151822.TJUD8C7K-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 39f292ffa13d7ca0d1edff27ac8fd55024bb4d19)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251015/202510151822.TJUD8C7K-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/202510151822.TJUD8C7K-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/media/i2c/ov01a10.c:13:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:27:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:12:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:1209:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
    1209 |         return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
         |                                                   ~~~~~~~~~~ ^
>> drivers/media/i2c/ov01a10.c:204:34: warning: unused variable 'ov01a10_regs' [-Wunused-const-variable]
     204 | static const struct reg_sequence ov01a10_regs[] = {
         |                                  ^~~~~~~~~~~~
>> drivers/media/i2c/ov01a10.c:209:34: warning: unused variable 'ov01a1s_regs' [-Wunused-const-variable]
     209 | static const struct reg_sequence ov01a1s_regs[] = {
         |                                  ^~~~~~~~~~~~
   3 warnings generated.


vim +/ov01a10_regs +204 drivers/media/i2c/ov01a10.c

   203	
 > 204	static const struct reg_sequence ov01a10_regs[] = {
   205		{0x3822, 0x13},
   206		{0x4837, 0x11},
   207	};
   208	
 > 209	static const struct reg_sequence ov01a1s_regs[] = {
   210		{0x3822, 0x03},
   211		{0x4837, 0x14},
   212		{0x373d, 0x24},
   213	};
   214	

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

Re: [PATCH 01/25] media: i2c: ov01a10: Fix the horizontal flip control

[Mehdi Djait]

Hello Hans,

Thank you for the patches.

On Tue, Oct 14, 2025 at 07:40:09PM +0200, Hans de Goede wrote:
> During sensor calibration I noticed that with the hflip control set
> to false/disabled the image was mirrored.
> 
> So it seems that the horizontal flip control is inverted and needs to
> be set to 1 to not flip (just like the similar problem recently fixed
> on the ov08x40 sensor).
> 
> Invert the hflip control to fix the sensor mirroring by default.
> 
> As the comment above the newly added OV01A10_MEDIA_BUS_FMT define explains
> the control being inverted also means that the native Bayer-order of
> the sensor actually is GBRG not BGGR, but so as to not break userspace
> the Bayer-order is kept at BGGR.
> 
> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 02/25] media: i2c: ov01a10: Fix reported pixel-rate value

[Mehdi Djait]

Hello Hans,

Thank you for the patches.

On Tue, Oct 14, 2025 at 07:40:10PM +0200, Hans de Goede wrote:
> CSI lanes are double-clocked so with a single lane at 400MHZ the resulting
> pixel-rate for 10-bits pixels is 400 MHz * 2 / 10 = 80 MHz, not 40 MHz.
> 
> This also matches with the observed frame-rate of 60 fps with the default
> vblank setting: 80000000 / (1488 * 896) = 60.
> 
> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

Re: [PATCH 03/25] media: i2c: ov01a10: Fix gain range

[Mehdi Djait]

Hello Hans,

Thank you for the patches,


On Tue, Oct 14, 2025 at 07:40:11PM +0200, Hans de Goede wrote:
> A maximum gain of 0xffff / 65525 seems unlikely and testing indeed shows
> that the gain control wraps-around at 4096, so set the maximum gain to
> 0xfff / 4095.
> 
> The minimum gain of 0x100 is correct. Setting bits 8-11 to 0x0 results
> in the same gain values as setting these bits to 0x1, with bits 0-7
> still increasing the gain when going from 0x000 - 0x0ff in the exact
> same range as when going from 0x100 - 0x1ff.
> 

Two things:

1 - You could mention in the commit msg that this is about the analog gain.

2 - You can add a patch for the max digital gain: which is defined as
    follows:

#define OV01A10_DGTL_GAIN_MAX		0x3ffff

My testing shows that the digital_gain ctrl wraps-around at
16383 = 0x3fff

with that:

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 04/25] media: i2c: ov01a10: Add missing v4l2_subdev_cleanup() calls

[Mehdi Djait]

Hello Hans,

Thank you for the patches.

On Tue, Oct 14, 2025 at 07:40:12PM +0200, Hans de Goede wrote:
> Add missing v4l2_subdev_cleanup() calls to cleanup after
> v4l2_subdev_init_finalize().
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 05/25] media: i2c: ov01a10: Fix passing stream instead of pad to v4l2_subdev_state_get_format()

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:13PM +0200, Hans de Goede wrote:
> The 2 argument version of v4l2_subdev_state_get_format() takes the pad
> as second argument, not the stream.
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Fixes: bc0e8d91feec ("media: v4l: subdev: Switch to stream-aware state functions")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:14PM +0200, Hans de Goede wrote:
> When the test-pattern control gets set to 0 (Disabled) 0 should be written
> to the test-pattern register, rather then doing nothing.
> 

A small question: Do you see any difference between test_pattern 1 and
test_pattern 2 or I did not look hard enough at the screen ?

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Fixes: 0827b58dabff ("media: i2c: add ov01a10 image sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Hans de Goede]

Hi Mehdi,

Thank you for all the reviews and testing.

On 28-Oct-25 1:08 PM, Mehdi Djait wrote:
> Hi Hans,
> 
> Thank you for the patches!
> 
> On Tue, Oct 14, 2025 at 07:40:14PM +0200, Hans de Goede wrote:
>> When the test-pattern control gets set to 0 (Disabled) 0 should be written
>> to the test-pattern register, rather then doing nothing.
>>
> 
> A small question: Do you see any difference between test_pattern 1 and
> test_pattern 2 or I did not look hard enough at the screen ?

IIRC the one has the colors fading (a bit) from left to right and
the other from top to bottom ?

Regards,

Hans

Re: [PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Mehdi Djait]

Hi Hans,

On Tue, Oct 28, 2025 at 03:38:28PM +0100, Hans de Goede wrote:
> Hi Mehdi,
> 
> Thank you for all the reviews and testing.
> 
> On 28-Oct-25 1:08 PM, Mehdi Djait wrote:
> > Hi Hans,
> > 
> > Thank you for the patches!
> > 
> > On Tue, Oct 14, 2025 at 07:40:14PM +0200, Hans de Goede wrote:
> >> When the test-pattern control gets set to 0 (Disabled) 0 should be written
> >> to the test-pattern register, rather then doing nothing.
> >>
> > 
> > A small question: Do you see any difference between test_pattern 1 and
> > test_pattern 2 or I did not look hard enough at the screen ?
> 
> IIRC the one has the colors fading (a bit) from left to right and
> the other from top to bottom ?

I see:
1 and 2 are the same ?!
3 fading from left -> right
4 fading from top  ->  bottom

--
Kind Regards
Mehdi Djait

Re: [PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Hans de Goede]

Hi Medhi,

On 28-Oct-25 4:38 PM, Mehdi Djait wrote:
> Hi Hans,
> 
> On Tue, Oct 28, 2025 at 03:38:28PM +0100, Hans de Goede wrote:
>> Hi Mehdi,
>>
>> Thank you for all the reviews and testing.
>>
>> On 28-Oct-25 1:08 PM, Mehdi Djait wrote:
>>> Hi Hans,
>>>
>>> Thank you for the patches!
>>>
>>> On Tue, Oct 14, 2025 at 07:40:14PM +0200, Hans de Goede wrote:
>>>> When the test-pattern control gets set to 0 (Disabled) 0 should be written
>>>> to the test-pattern register, rather then doing nothing.
>>>>
>>>
>>> A small question: Do you see any difference between test_pattern 1 and
>>> test_pattern 2 or I did not look hard enough at the screen ?
>>
>> IIRC the one has the colors fading (a bit) from left to right and
>> the other from top to bottom ?
> 
> I see:
> 1 and 2 are the same ?!
> 3 fading from left -> right
> 4 fading from top  ->  bottom

That might very well be correct. Unfortunately I no longer
have access to the Dell XPS 13 9320 I tested this on, so I cannot
confirm.

I think I should squash the following fix into this one:

diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
index 6a1ab5fa70a2..1fe643cb1e6b 100644
--- a/drivers/media/i2c/ov01a10.c
+++ b/drivers/media/i2c/ov01a10.c
@@ -215,9 +215,8 @@ static const struct reg_sequence ov01a1s_regs[] = {
 static const char * const ov01a10_test_pattern_menu[] = {
 	"Disabled",
 	"Color Bar",
+	"Left-Right Darker Color Bar",
 	"Top-Bottom Darker Color Bar",
-	"Right-Left Darker Color Bar",
-	"Color Bar type 4",
 };
 
 static const s64 link_freq_menu_items[] = {
@@ -318,7 +317,7 @@ static int ov01a10_update_digital_gain(struct ov01a10 *ov01a10, u32 d_gain)
 static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
 {
 	if (pattern)
-		pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
+		pattern |= OV01A10_TEST_PATTERN_ENABLE;
 
 	return cci_write(ov01a10->regmap, OV01A10_REG_TEST_PATTERN, pattern,
 			 NULL);

This skips setting 0 as the pattern, since 0 is the same as 1,
removes the weird "Color Bar type 4" from the menu and fixes
the order of the 2 fading controls.

Can you give this a test ?

Regards,

Hans

Re: [PATCH 07/25] media: i2c: ov01a10: Change default vblank value to a vblank resulting in 30 fps

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:15PM +0200, Hans de Goede wrote:
> The ov01a10 is quite a small sensor, which does not capture a lot of
> light, increase the default vblank so that the sensor runs at 30 fps
> by default, doubling the default exposure.
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 08/25] media: i2c: ov01a10: Convert to new CCI register access helpers

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:16PM +0200, Hans de Goede wrote:
> Use the new comon CCI register access helpers to replace the private
> register access helpers in the ov01a10 driver.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>

[..]

> +#define OV01A10_REG_DIGITAL_GAIN_B	CCI_REG24(0x350a)
> +#define OV01A10_REG_DIGITAL_GAIN_GB	CCI_REG24(0x3510)
> +#define OV01A10_REG_DIGITAL_GAIN_GR	CCI_REG24(0x3513)
> +#define OV01A10_REG_DIGITAL_GAIN_R	CCI_REG24(0x3516)

As mentioned in [PATCH 03/25] the digital gain loops after 0x3fff:
Should the digital gain registers be CCI_REG16() ?

--
Kind Regards
Mehdi Djait

Re: [PATCH 09/25] media: i2c: ov01a10: Remove overly verbose probe() error reporting

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:17PM +0200, Hans de Goede wrote:
> Many of the functions called from ov01a10_probe() are expected to never
> fail and they should all already log some message if they fail. Remove
> the unnecessarily verbose dev_err[_probe]() calls from the error-exit
> paths in probe().
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 10/25] media: i2c: ov01a10: Store dev pointer in struct ov01a10

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:18PM +0200, Hans de Goede wrote:
> Now that the cci_* register access helpers are used we no longer need
> the i2c_client in various functions.
> 
> Some code is still getting the client just to be able to get to the device
> pointer. Directly store a struct device *dev pointing to &client->dev
> inside struct ov01a10 to make the code simpler.
> 
> This also fixes a mismatch of using dev vs &client->dev in the
> runtime_pm_*() calls in probe().
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 11/25] media: i2c: ov01a10: Add ov01a10_check_hwcfg() function

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:19PM +0200, Hans de Goede wrote:
> Add a function to check that the number of mipi-lanes and there frequency
> are what the driver expects.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>

[..]

> +static int ov01a10_check_hwcfg(struct ov01a10 *ov01a10)
> +{
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	struct fwnode_handle *ep, *fwnode = dev_fwnode(ov01a10->dev);
> +	unsigned long link_freq_bitmap;
> +	int ret;
> +
> +	/*
> +	 * Sometimes the fwnode graph is initialized by the bridge driver,
> +	 * wait for this.
> +	 */
> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> +	if (!ep)
> +		return dev_err_probe(ov01a10->dev, -EPROBE_DEFER,
> +				     "waiting for fwnode graph endpoint\n");
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return dev_err_probe(ov01a10->dev, ret, "parsing endpoint\n");
> +
> +	ret = v4l2_link_freq_to_bitmap(ov01a10->dev,
> +				       bus_cfg.link_frequencies,
> +				       bus_cfg.nr_of_link_frequencies,
> +				       link_freq_menu_items,
> +				       ARRAY_SIZE(link_freq_menu_items),
> +				       &link_freq_bitmap);
> +	if (ret)
> +		goto check_hwcfg_error;
> +
> +	/* v4l2_link_freq_to_bitmap() guarantees at least 1 bit is set */
> +	ov01a10->link_freq_index = ffs(link_freq_bitmap) - 1;
> +
> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV01A10_DATA_LANES) {
> +		ret = dev_err_probe(ov01a10->dev, -EINVAL,
> +				    "number of CSI2 data lanes %u is not supported\n",
> +				    bus_cfg.bus.mipi_csi2.num_data_lanes);
> +		goto check_hwcfg_error;

You don't need this goto at the end.

> +	}
> +
> +check_hwcfg_error:
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +	return ret;
> +}

--
Kind Regards
Mehdi Djait

Re: [PATCH 12/25] media: i2c: ov01a10: Add power on/off sequencing support

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:20PM +0200, Hans de Goede wrote:
> So far the ov01a10 driver has only been used on laptops with an IVSC chip
> where the IVSC chip controls the power on/off sequencing of the sensor.
> 
> But there are also designs with an ov01a10 sensor where the kernel needs
> to directly take care of the power-sequencing, controlling clks, regulators
> and GPIOs. Add support for these designs.
> 
> The 2 ms minimum reset assertion time is taken from other Omnivision sensor
> drivers like the ov5675. The 20 ms delay after reset de-assert comes from
> the out of tree ov01a1s driver.
> 

Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 13/25] media: i2c: ov01a10: Don't update pixel_rate and link_freq from set_fmt

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:21PM +0200, Hans de Goede wrote:
> The pixel_rate and link_freq never change, stop updating them on every
> set_fmt.
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com> 

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 14/25] media: i2c: ov01a10: Move setting of ctrl->flags to after checking ctrl_hdlr->error

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:22PM +0200, Hans de Goede wrote:
> Instead of checking successful creation of the link_freq and vblank
> controls, set their flags after checking ctrl_hdlr->error where it
> is guaranteed that the controls will exist.
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 15/25] media: i2c: ov01a10: Use native and default for pixel-array size names

[Mehdi Djait]

Hi Hans,

On Tue, Oct 14, 2025 at 07:40:23PM +0200, Hans de Goede wrote:
> According to the OV01A10 product-brief PDF the OV01A10 has an active pixel
> array size of 1296x816. In otherwords the native and active sizes are
> the same.

Isn't that an error in the product-brief ?

I understood the following:

The native pixel array size is 1296x816
The active pixel array size is 1280x800 = these are the active pixels that can
be output.

1296x816 is not lised under the supported image sizes, so it
is not the active pixel array size.

I think in most sensors the active pixel array size is smaller than the
native one.

Or am I confused here ?

> 
> Replace the (misspelled) ACTIVE defines for the default resolution of
> 1280x800 with DEFAULT to avoid giving the impression that the active pixel
> array size is only 1280x800.
> 
> And replace PIXEL_ARRAY with NATIVE to make clear this is the native pixel
> array size / to match the V4L2_SEL_TGT_NATIVE_SIZE naming.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/ov01a10.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> index d0153e606c9a..f3bcb61c88dd 100644
> --- a/drivers/media/i2c/ov01a10.c
> +++ b/drivers/media/i2c/ov01a10.c
> @@ -34,10 +34,10 @@
>  #define OV01A10_MODE_STREAMING		0x01
>  
>  /* pixel array */
> -#define OV01A10_PIXEL_ARRAY_WIDTH	1296
> -#define OV01A10_PIXEL_ARRAY_HEIGHT	816
> -#define OV01A10_ACITVE_WIDTH		1280
> -#define OV01A10_ACITVE_HEIGHT		800
> +#define OV01A10_NATIVE_WIDTH		1296
> +#define OV01A10_NATIVE_HEIGHT		816
> +#define OV01A10_DEFAULT_WIDTH		1280
> +#define OV01A10_DEFAULT_HEIGHT		800

--
Kind Regards
Mehdi Djait

Re: [PATCH 17/25] media: i2c: ov01a10: Remove struct ov01a10_reg_list

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:25PM +0200, Hans de Goede wrote:
> After the conversion to the CCI register access helpers, struct
> ov01a10_reg_list is only used inside struct ov01a10_link_freq_config.
> 
> Simplify things by embedding the ov01a10_reg_list members directly into
> struct ov01a10_link_freq_config.
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com> 

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 18/25] media: i2c: ov01a10: Replace exposure->min/step with direct define use

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:26PM +0200, Hans de Goede wrote:
> The exposure minimum and step are constant use the defines for this
> instead of retrieving these from the exposure-control.
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com> 

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 19/25] media: i2c: ov01a10: Only set register 0x0305 once

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:27PM +0200, Hans de Goede wrote:
> Stop setting register 0x0305 to one value from mipi_data_rate_720mbps
> only to override it with a different value from sensor_1280x800_setting.
> 
> Instead directly set it to 0xf4.
>

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com> 

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 00/25] media: i2c: ov01a10: Add crop, ov01a1b and ov01a1s support

[Hans de Goede]

Hi all,

On 14-Oct-25 7:40 PM, Hans de Goede wrote:
> Hi All,
> 
> This is a series with various ov01a10 driver improvements
> 1. A set of bugfixes
> 2. Add cropping support / allow arbitrary sizes
> 3. OV01A1B monochrome/IR model support
> 4. OV01A1S RGB-IR model support
> 
> This has been tested on:
> 1. A Dell XPS 13 9320 Raptor Lake with OV1A10 color + OV1A1B IR sensor
> 2. A Dell Latitude 9420 Tiger Lake with OV01A1S RGB-IR
> 
> Testing has been done both with:
> 1. libcamera (qcam + softISP patches for RGB-IR); and
> 2. Intel's proprietary userspace stack with out of tree psys driver (*)
> 
> Regards,
> 
> Hans
> 
> 
> *) The out of tree ov01a1s driver has a fixed resolution of 1296x798
>    (with the height not being a multiple of bayer-pattern-size <sigh>)
>    the closest the mainline driver can get after this series is 1288x800.
>    This requires some changes to the xml files describing the ov01a1s
>    graph in ipu6-camera-hal.

I've submitted pull-requests for the out of tree ov01a1s driver resp.
ipu6-camera-hal to match the 1288x800 output size the mainline driver
will support on the ov01a1s and to update the xml sensor / graph
configs for this new size:

https://github.com/intel/ipu6-drivers/pull/395
https://github.com/intel/ipu6-camera-hal/pull/160

Regards,

Hans

Re: [PATCH 11/25] media: i2c: ov01a10: Add ov01a10_check_hwcfg() function

[Hans de Goede]

Hi Mehdi,

On 28-Oct-25 6:29 PM, Mehdi Djait wrote:
> Hi Hans,
> 
> Thank you for the patches!
> 
> On Tue, Oct 14, 2025 at 07:40:19PM +0200, Hans de Goede wrote:
>> Add a function to check that the number of mipi-lanes and there frequency
>> are what the driver expects.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
> 
> [..]
> 
>> +static int ov01a10_check_hwcfg(struct ov01a10 *ov01a10)
>> +{
>> +	struct v4l2_fwnode_endpoint bus_cfg = {
>> +		.bus_type = V4L2_MBUS_CSI2_DPHY
>> +	};
>> +	struct fwnode_handle *ep, *fwnode = dev_fwnode(ov01a10->dev);
>> +	unsigned long link_freq_bitmap;
>> +	int ret;
>> +
>> +	/*
>> +	 * Sometimes the fwnode graph is initialized by the bridge driver,
>> +	 * wait for this.
>> +	 */
>> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
>> +	if (!ep)
>> +		return dev_err_probe(ov01a10->dev, -EPROBE_DEFER,
>> +				     "waiting for fwnode graph endpoint\n");
>> +
>> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
>> +	fwnode_handle_put(ep);
>> +	if (ret)
>> +		return dev_err_probe(ov01a10->dev, ret, "parsing endpoint\n");
>> +
>> +	ret = v4l2_link_freq_to_bitmap(ov01a10->dev,
>> +				       bus_cfg.link_frequencies,
>> +				       bus_cfg.nr_of_link_frequencies,
>> +				       link_freq_menu_items,
>> +				       ARRAY_SIZE(link_freq_menu_items),
>> +				       &link_freq_bitmap);
>> +	if (ret)
>> +		goto check_hwcfg_error;
>> +
>> +	/* v4l2_link_freq_to_bitmap() guarantees at least 1 bit is set */
>> +	ov01a10->link_freq_index = ffs(link_freq_bitmap) - 1;
>> +
>> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV01A10_DATA_LANES) {
>> +		ret = dev_err_probe(ov01a10->dev, -EINVAL,
>> +				    "number of CSI2 data lanes %u is not supported\n",
>> +				    bus_cfg.bus.mipi_csi2.num_data_lanes);
>> +		goto check_hwcfg_error;
> 
> You don't need this goto at the end.

Thank you for the review. I prefer to leave this goto in place even
though this is the last error check so that if extra checks are added
later on after this block the error handling is still correct.

But if you feel strongly about removing the goto I can remove it for
v2 of this series.

Please let me know how you want to proceed with this.

Regards,

Hans




> 
>> +	}
>> +
>> +check_hwcfg_error:
>> +	v4l2_fwnode_endpoint_free(&bus_cfg);
>> +	return ret;
>> +}
> 
> --
> Kind Regards
> Mehdi Djait

Re: [PATCH 15/25] media: i2c: ov01a10: Use native and default for pixel-array size names

[Hans de Goede]

Hi Mehdi,

On 28-Oct-25 8:01 PM, Mehdi Djait wrote:
> Hi Hans,
> 
> On Tue, Oct 14, 2025 at 07:40:23PM +0200, Hans de Goede wrote:
>> According to the OV01A10 product-brief PDF the OV01A10 has an active pixel
>> array size of 1296x816. In otherwords the native and active sizes are
>> the same.
> 
> Isn't that an error in the product-brief ?
> 
> I understood the following:
> 
> The native pixel array size is 1296x816
> The active pixel array size is 1280x800 = these are the active pixels that can
> be output.
> 
> 1296x816 is not lised under the supported image sizes, so it
> is not the active pixel array size.

The list of supported image sizes typically is just plain non-sense.

As the later patch adding arbitrary output sizes shows there really
is nothing special about the listed supported image sizes.

Typically the vendor will provide a set of register values
for the supported image sizes, which is why we see various sensor
drivers with a fixed list of modes/output-sizes with then a long
register-list per mode.

That does not mean those are the only modes the sensor can actually
do since most sensors can do cropping at at least a 2x2 pixel precision
making any mode possible (although cropping away a lot will significantly
change the field-of-view).

Sometimes there may be some rows of dark pixels which are covered
with something which does not allow light to pass through for blacklevel
measurement but that does not appear to be the case here.

If you run the libcamera+softISP stack anmd specifically qcam with this
series then you will get 1292x816 as resolution and there are no black
borders or artifacts at the borders so it can really do 1296x816.

Also see the out of tree ov01a1s driver:

https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ov01a1s.c

which also has an output width of 1296 pixels.

> I think in most sensors the active pixel array size is smaller than the
> native one.

Sometimes it is because there are e.g. a few rows of pixels
for blacklevel measuring. But often the entire array can be used.

Even if there are extra pixels for blacklevel measurement then
the actual active area typically is still bigger then a standard
resolution like e.hg. 1280x800 as many hardware ISPs typically
also need some extra pixels at the border for Bayer pattern
interpolation / demosaicing.

Regards,

Hans



>>
>> Replace the (misspelled) ACTIVE defines for the default resolution of
>> 1280x800 with DEFAULT to avoid giving the impression that the active pixel
>> array size is only 1280x800.
>>
>> And replace PIXEL_ARRAY with NATIVE to make clear this is the native pixel
>> array size / to match the V4L2_SEL_TGT_NATIVE_SIZE naming.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/media/i2c/ov01a10.c | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
>> index d0153e606c9a..f3bcb61c88dd 100644
>> --- a/drivers/media/i2c/ov01a10.c
>> +++ b/drivers/media/i2c/ov01a10.c
>> @@ -34,10 +34,10 @@
>>  #define OV01A10_MODE_STREAMING		0x01
>>  
>>  /* pixel array */
>> -#define OV01A10_PIXEL_ARRAY_WIDTH	1296
>> -#define OV01A10_PIXEL_ARRAY_HEIGHT	816
>> -#define OV01A10_ACITVE_WIDTH		1280
>> -#define OV01A10_ACITVE_HEIGHT		800
>> +#define OV01A10_NATIVE_WIDTH		1296
>> +#define OV01A10_NATIVE_HEIGHT		816
>> +#define OV01A10_DEFAULT_WIDTH		1280
>> +#define OV01A10_DEFAULT_HEIGHT		800
> 
> --
> Kind Regards
> Mehdi Djait

Re: [PATCH 11/25] media: i2c: ov01a10: Add ov01a10_check_hwcfg() function

[Mehdi Djait]

Hi Hans,

On Tue, Oct 28, 2025 at 09:09:16PM +0100, Hans de Goede wrote:
> Hi Mehdi,
> 
> On 28-Oct-25 6:29 PM, Mehdi Djait wrote:
> > Hi Hans,
> > 
> > Thank you for the patches!
> > 
> > On Tue, Oct 14, 2025 at 07:40:19PM +0200, Hans de Goede wrote:
> >> Add a function to check that the number of mipi-lanes and there frequency
> >> are what the driver expects.
> >>
> >> Signed-off-by: Hans de Goede <hansg@kernel.org>
> > 
> > [..]
> > 
> >> +static int ov01a10_check_hwcfg(struct ov01a10 *ov01a10)
> >> +{
> >> +	struct v4l2_fwnode_endpoint bus_cfg = {
> >> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> >> +	};
> >> +	struct fwnode_handle *ep, *fwnode = dev_fwnode(ov01a10->dev);
> >> +	unsigned long link_freq_bitmap;
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * Sometimes the fwnode graph is initialized by the bridge driver,
> >> +	 * wait for this.
> >> +	 */
> >> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> >> +	if (!ep)
> >> +		return dev_err_probe(ov01a10->dev, -EPROBE_DEFER,
> >> +				     "waiting for fwnode graph endpoint\n");
> >> +
> >> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> >> +	fwnode_handle_put(ep);
> >> +	if (ret)
> >> +		return dev_err_probe(ov01a10->dev, ret, "parsing endpoint\n");
> >> +
> >> +	ret = v4l2_link_freq_to_bitmap(ov01a10->dev,
> >> +				       bus_cfg.link_frequencies,
> >> +				       bus_cfg.nr_of_link_frequencies,
> >> +				       link_freq_menu_items,
> >> +				       ARRAY_SIZE(link_freq_menu_items),
> >> +				       &link_freq_bitmap);
> >> +	if (ret)
> >> +		goto check_hwcfg_error;
> >> +
> >> +	/* v4l2_link_freq_to_bitmap() guarantees at least 1 bit is set */
> >> +	ov01a10->link_freq_index = ffs(link_freq_bitmap) - 1;
> >> +
> >> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV01A10_DATA_LANES) {
> >> +		ret = dev_err_probe(ov01a10->dev, -EINVAL,
> >> +				    "number of CSI2 data lanes %u is not supported\n",
> >> +				    bus_cfg.bus.mipi_csi2.num_data_lanes);
> >> +		goto check_hwcfg_error;
> > 
> > You don't need this goto at the end.
> 
> Thank you for the review. I prefer to leave this goto in place even
> though this is the last error check so that if extra checks are added
> later on after this block the error handling is still correct.
> 
> But if you feel strongly about removing the goto I can remove it for
> v2 of this series.
> 
> Please let me know how you want to proceed with this.

I don't feel strongly about it, you can leave the goto but I would
prefer to replace check_hwcfg_error by check_hwcfg_done if we need it
when the function succeeds (we realistically expect the function to
succeed more than fail)

Again, this is really not that important, I am fine with whatever you
send in the v2

--
Kind Regards
Mehdi Djait

Re: [PATCH 06/25] media: i2c: ov01a10: Fix test-pattern disabling

[Mehdi Djait]

Hi Hans,

On Tue, Oct 28, 2025 at 04:52:54PM +0100, Hans de Goede wrote:
> Hi Medhi,
> 
> On 28-Oct-25 4:38 PM, Mehdi Djait wrote:
> > Hi Hans,
> > 
> > On Tue, Oct 28, 2025 at 03:38:28PM +0100, Hans de Goede wrote:
> >> Hi Mehdi,
> >>
> >> Thank you for all the reviews and testing.
> >>
> >> On 28-Oct-25 1:08 PM, Mehdi Djait wrote:
> >>> Hi Hans,
> >>>
> >>> Thank you for the patches!
> >>>
> >>> On Tue, Oct 14, 2025 at 07:40:14PM +0200, Hans de Goede wrote:
> >>>> When the test-pattern control gets set to 0 (Disabled) 0 should be written
> >>>> to the test-pattern register, rather then doing nothing.
> >>>>
> >>>
> >>> A small question: Do you see any difference between test_pattern 1 and
> >>> test_pattern 2 or I did not look hard enough at the screen ?
> >>
> >> IIRC the one has the colors fading (a bit) from left to right and
> >> the other from top to bottom ?
> > 
> > I see:
> > 1 and 2 are the same ?!
> > 3 fading from left -> right
> > 4 fading from top  ->  bottom
> 
> That might very well be correct. Unfortunately I no longer
> have access to the Dell XPS 13 9320 I tested this on, so I cannot
> confirm.
> 
> I think I should squash the following fix into this one:
> 
> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> index 6a1ab5fa70a2..1fe643cb1e6b 100644
> --- a/drivers/media/i2c/ov01a10.c
> +++ b/drivers/media/i2c/ov01a10.c
> @@ -215,9 +215,8 @@ static const struct reg_sequence ov01a1s_regs[] = {
>  static const char * const ov01a10_test_pattern_menu[] = {
>  	"Disabled",
>  	"Color Bar",
> +	"Left-Right Darker Color Bar",
>  	"Top-Bottom Darker Color Bar",

This should be changed to: "Bottom-Top Darker Color Bar"

> -	"Right-Left Darker Color Bar",
> -	"Color Bar type 4",
>  };
>  
>  static const s64 link_freq_menu_items[] = {
> @@ -318,7 +317,7 @@ static int ov01a10_update_digital_gain(struct ov01a10 *ov01a10, u32 d_gain)
>  static int ov01a10_test_pattern(struct ov01a10 *ov01a10, u32 pattern)
>  {
>  	if (pattern)
> -		pattern = (pattern - 1) | OV01A10_TEST_PATTERN_ENABLE;
> +		pattern |= OV01A10_TEST_PATTERN_ENABLE;
>  
>  	return cci_write(ov01a10->regmap, OV01A10_REG_TEST_PATTERN, pattern,
>  			 NULL);
> 
> This skips setting 0 as the pattern, since 0 is the same as 1,
> removes the weird "Color Bar type 4" from the menu and fixes
> the order of the 2 fading controls.
> 
> Can you give this a test ?

It works as expected. Thank you for the patch

--
Kind Regards
Mehdi Djait

Re: [PATCH 20/25] media: i2c: ov01a10: Remove values set by controls from global_setting[]

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:28PM +0200, Hans de Goede wrote:
> Registers 0x3501 (exposure), 0x3508 (analogue-gain) and 0x4503 (test-
> pattern) are already set through __v4l2_ctrl_handler_setup() drop them
> from ov01a10_global_setting[].
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 16/25] media: i2c: ov01a10: Add cropping support / allow arbitrary sizes

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:24PM +0200, Hans de Goede wrote:
> Remove the fixed mode list and add cropping support. The main reason for
> doing this is to allow libcamera to select 1292x812 instead of 1280x800
> so that after the extra border which the CPU debayer code needs libcamera
> can output 1280x720 instead of 1276x720.
> 
> This in turn allows google-meet to use 720p instead of it falling back
> to a pretty bad 360p.
> 
> This has been tested on a Dell XPS 9320, with both libcamera as well as
> with Intel's out-of-tree psys driver + proprietary userspace stack.
> 
> Libcamera asks for 1292x812 where as the Intel stack asks for 1280x800
> and neither stack explicitly sets the crop-window. Hence the need for
> ov01a10_set_format() to adjust the crop-window if necessary.
> 
> Note the differentiating between pattern_size and border_size is done in
> preparation for adding support for the monochrome OV01A1B model where
> coordinates still need to be aligned to a multiple of 2, but there will
> be no need for a border (border_size=0).
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2337593
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---

[..]

> +static int ov01a10_set_mode(struct ov01a10 *ov01a10)
> +{
> +	struct v4l2_mbus_framefmt *fmt = ov01a10_get_active_format(ov01a10);
> +	int ret = 0;
> +
> +	cci_write(ov01a10->regmap, OV01A10_REG_X_ADDR_START, 0, &ret);
> +	cci_write(ov01a10->regmap, OV01A10_REG_Y_ADDR_START, 0, &ret);
> +	cci_write(ov01a10->regmap, OV01A10_REG_X_ADDR_END,
> +		  OV01A10_NATIVE_WIDTH - 1, &ret);
> +	cci_write(ov01a10->regmap, OV01A10_REG_Y_ADDR_END,
> +		  OV01A10_NATIVE_HEIGHT - 1, &ret);
> +	cci_write(ov01a10->regmap, OV01A10_REG_X_OUTPUT_SIZE,
> +		  fmt->width, &ret);
> +	cci_write(ov01a10->regmap, OV01A10_REG_Y_OUTPUT_SIZE,
> +		  fmt->height, &ret);
> +	/* HTS register is in units of 2 pixels */
> +	cci_write(ov01a10->regmap, OV01A10_REG_HTS,
> +		  OV01A10_HTS_DEF / 2, &ret);
> +	/* OV01A10_REG_VTS is set by vblank control */
> +	/* OV01A10_REG_X_WIN is set by hlip control */

nit: typo here hlip -> hflip

--
Kind Regards
Mehdi Djait

Re: [PATCH 21/25] media: i2c: ov01a10: Add ov01a10_sensor_cfg struct

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:29PM +0200, Hans de Goede wrote:
> Add a struct with some sensor variant (ov01a10 / ov01a1b / ov01a1s)
> specific settings.
> 
> This is a preparation patch for adding support for the ov01a1s sensor
> which uses the same sensor with a different (RGBI) color-filter.
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 22/25] media: i2c: ov01a10: Optimize setting h/vflip values

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:30PM +0200, Hans de Goede wrote:
> Since ov01a10_global_setting[] sets the initial format1 register value,
> there is no need to do a read-write-modify when setting the flip controls.
> 
> Only write format1 when setting the flip-controls and remove the now
> unnecessary format1 register init from ov01a10_global_setting[].
> 

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 23/25] media: i2c: ov01a10: Add ov01a1b support

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:31PM +0200, Hans de Goede wrote:
> Add support for the ov01a1b model which is the exact same sensor as
> the ov01a10 without a color-filter.
> 
> Note since there is no color-filter there is also no need to shift
> the crop-window when flipping, so the crop window set by userspace may
> cover the full sensor (border_size=0).
> 

Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 24/25] media: i2c: ov01a10: Add ov01a1s support

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:32PM +0200, Hans de Goede wrote:
> The ov01a1s sensor is the exact same sensor as the ov01a10 with a different
> (RGBI) color-filter.
> 

Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>

Re: [PATCH 25/25] media: i2c: ov01a10: Register tweaks for ov01a1s model

[Mehdi Djait]

Hi Hans,

Thank you for the patches!

On Tue, Oct 14, 2025 at 07:40:33PM +0200, Hans de Goede wrote:
> The out of tree drivers from:
> https://github.com/intel/ipu6-drivers/tree/master/drivers/media/i2c
> 
> Have a separate driver for the ov01a1s. This driver is 95% the same as
> the ov01a10 driver (same sensor, different color-filters) but there are
> a couple of registers for which the out of tree ov01a1s driver uses
> different values.
> 
> Add a per model reg_sequence to struct ov01a10_sensor_cfg and apply
> the register tweaks from the out of tree driver to the ov01a1s model.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/ov01a10.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> index 058ff311a289..08cab1e4be1d 100644
> --- a/drivers/media/i2c/ov01a10.c
> +++ b/drivers/media/i2c/ov01a10.c
> @@ -161,7 +161,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
>  	{0x3815, 0x01},
>  	{0x3816, 0x01},
>  	{0x3817, 0x01},
> -	{0x3822, 0x13},
>  	{0x3832, 0x28},
>  	{0x3833, 0x10},
>  	{0x3b00, 0x00},
> @@ -186,7 +185,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
>  	{0x4800, 0x64},
>  	{0x481f, 0x34},
>  	{0x4825, 0x33},
> -	{0x4837, 0x11},
>  	{0x4881, 0x40},
>  	{0x4883, 0x01},
>  	{0x4890, 0x00},
> @@ -203,6 +201,17 @@ static const struct reg_sequence ov01a10_global_setting[] = {
>  	{0x0325, 0xc2},
>  };
>  
> +static const struct reg_sequence ov01a10_regs[] = {
> +	{0x3822, 0x13},
> +	{0x4837, 0x11},
> +};
> +
> +static const struct reg_sequence ov01a1s_regs[] = {
> +	{0x3822, 0x03},
> +	{0x4837, 0x14},
> +	{0x373d, 0x24},
> +};

How about the 0x4800 register ?
we write 0x64 for the ov01a10 driver but the out-of-tree driver for
ov01a1s does not write that value

--
Kind Regards
Mehdi Djait

Re: [PATCH 25/25] media: i2c: ov01a10: Register tweaks for ov01a1s model

[Hans de Goede]

Hi Medhi,

On 7-Nov-25 10:17 AM, Mehdi Djait wrote:
> Hi Hans,
> 
> Thank you for the patches!
> 
> On Tue, Oct 14, 2025 at 07:40:33PM +0200, Hans de Goede wrote:
>> The out of tree drivers from:
>> https://github.com/intel/ipu6-drivers/tree/master/drivers/media/i2c
>>
>> Have a separate driver for the ov01a1s. This driver is 95% the same as
>> the ov01a10 driver (same sensor, different color-filters) but there are
>> a couple of registers for which the out of tree ov01a1s driver uses
>> different values.
>>
>> Add a per model reg_sequence to struct ov01a10_sensor_cfg and apply
>> the register tweaks from the out of tree driver to the ov01a1s model.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/media/i2c/ov01a10.c | 28 ++++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
>> index 058ff311a289..08cab1e4be1d 100644
>> --- a/drivers/media/i2c/ov01a10.c
>> +++ b/drivers/media/i2c/ov01a10.c
>> @@ -161,7 +161,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
>>  	{0x3815, 0x01},
>>  	{0x3816, 0x01},
>>  	{0x3817, 0x01},
>> -	{0x3822, 0x13},
>>  	{0x3832, 0x28},
>>  	{0x3833, 0x10},
>>  	{0x3b00, 0x00},
>> @@ -186,7 +185,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
>>  	{0x4800, 0x64},
>>  	{0x481f, 0x34},
>>  	{0x4825, 0x33},
>> -	{0x4837, 0x11},
>>  	{0x4881, 0x40},
>>  	{0x4883, 0x01},
>>  	{0x4890, 0x00},
>> @@ -203,6 +201,17 @@ static const struct reg_sequence ov01a10_global_setting[] = {
>>  	{0x0325, 0xc2},
>>  };
>>  
>> +static const struct reg_sequence ov01a10_regs[] = {
>> +	{0x3822, 0x13},
>> +	{0x4837, 0x11},
>> +};
>> +
>> +static const struct reg_sequence ov01a1s_regs[] = {
>> +	{0x3822, 0x03},
>> +	{0x4837, 0x14},
>> +	{0x373d, 0x24},
>> +};
> 
> How about the 0x4800 register ?
> we write 0x64 for the ov01a10 driver but the out-of-tree driver for
> ov01a1s does not write that value

Right. I left that one in for all models on purpose.

Looking at other similar OVxxxxx sensor datasheets the 0x4800
register is the MIPI_CTRL00 register which controls things
like disabling the CSI clock during blank periods to save power.

Since this is in no way related to the installed color filter
I went with the assumption that the tweaks (changes from power-
on-rest default) done for the ov01a10 were done on purpose and
that these would also benefit the other 2 variants.

I did test the ov01a1b and ov01a1s with this setting and they
worked fine.

So this does deviate from what the ov01a1s driver does but it
is on purpose (and for the ov01a1b we have no other driver
to reference).

Also note that the ov01a1s already works fine without this
commit, but as the commit msg says I assume these tweaks were
done for a reason so I'm adding them here.

Regards,

Hans

Re: [PATCH 25/25] media: i2c: ov01a10: Register tweaks for ov01a1s model

[Mehdi Djait]

Hi Hans,

On Thu, Nov 13, 2025 at 10:54:06AM +0100, Hans de Goede wrote:
> Hi Medhi,
> 
> On 7-Nov-25 10:17 AM, Mehdi Djait wrote:
> > Hi Hans,
> > 
> > Thank you for the patches!
> > 
> > On Tue, Oct 14, 2025 at 07:40:33PM +0200, Hans de Goede wrote:
> >> The out of tree drivers from:
> >> https://github.com/intel/ipu6-drivers/tree/master/drivers/media/i2c
> >>
> >> Have a separate driver for the ov01a1s. This driver is 95% the same as
> >> the ov01a10 driver (same sensor, different color-filters) but there are
> >> a couple of registers for which the out of tree ov01a1s driver uses
> >> different values.
> >>
> >> Add a per model reg_sequence to struct ov01a10_sensor_cfg and apply
> >> the register tweaks from the out of tree driver to the ov01a1s model.
> >>
> >> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >> ---
> >>  drivers/media/i2c/ov01a10.c | 28 ++++++++++++++++++++++++++--
> >>  1 file changed, 26 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
> >> index 058ff311a289..08cab1e4be1d 100644
> >> --- a/drivers/media/i2c/ov01a10.c
> >> +++ b/drivers/media/i2c/ov01a10.c
> >> @@ -161,7 +161,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
> >>  	{0x3815, 0x01},
> >>  	{0x3816, 0x01},
> >>  	{0x3817, 0x01},
> >> -	{0x3822, 0x13},
> >>  	{0x3832, 0x28},
> >>  	{0x3833, 0x10},
> >>  	{0x3b00, 0x00},
> >> @@ -186,7 +185,6 @@ static const struct reg_sequence ov01a10_global_setting[] = {
> >>  	{0x4800, 0x64},
> >>  	{0x481f, 0x34},
> >>  	{0x4825, 0x33},
> >> -	{0x4837, 0x11},
> >>  	{0x4881, 0x40},
> >>  	{0x4883, 0x01},
> >>  	{0x4890, 0x00},
> >> @@ -203,6 +201,17 @@ static const struct reg_sequence ov01a10_global_setting[] = {
> >>  	{0x0325, 0xc2},
> >>  };
> >>  
> >> +static const struct reg_sequence ov01a10_regs[] = {
> >> +	{0x3822, 0x13},
> >> +	{0x4837, 0x11},
> >> +};
> >> +
> >> +static const struct reg_sequence ov01a1s_regs[] = {
> >> +	{0x3822, 0x03},
> >> +	{0x4837, 0x14},
> >> +	{0x373d, 0x24},
> >> +};
> > 
> > How about the 0x4800 register ?
> > we write 0x64 for the ov01a10 driver but the out-of-tree driver for
> > ov01a1s does not write that value
> 
> Right. I left that one in for all models on purpose.
> 
> Looking at other similar OVxxxxx sensor datasheets the 0x4800
> register is the MIPI_CTRL00 register which controls things
> like disabling the CSI clock during blank periods to save power.
> 
> Since this is in no way related to the installed color filter
> I went with the assumption that the tweaks (changes from power-
> on-rest default) done for the ov01a10 were done on purpose and
> that these would also benefit the other 2 variants.
> 
> I did test the ov01a1b and ov01a1s with this setting and they
> worked fine.
> 
> So this does deviate from what the ov01a1s driver does but it
> is on purpose (and for the ov01a1b we have no other driver
> to reference).
> 
> Also note that the ov01a1s already works fine without this
> commit, but as the commit msg says I assume these tweaks were
> done for a reason so I'm adding them here.

Ack.

Thank you for the explanation.

--
Kind Regards
Mehdi Djait