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

[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

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>

[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

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>

[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

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>

[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

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

[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

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>

[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

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