[PATCH 0/3] Fix analogue gain maximum for ov9282

[PATCH 0/3] Fix analogue gain maximum for ov9282

[Richard Leitner]

This series fixes/increases the analogue gain maximum value of the
ov9282 sensor driver to the limits specified in the datasheet.

It furthermore introduces a new register definition (including its
values) and uses available defined registers instead of addresses where
possible.

This is basically a first improvement/cleanup series for the ov9282
driver with likely more to follow.

All register addresses/values are based on the OV9281 datasheet v1.01
(09.18.2015).

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
Richard Leitner (3):
      media: i2c: ov9282: use register definitions
      media: i2c: ov9282: add AEC Manual register definition
      media: i2c: ov9282: fix analogue gain maximum

 drivers/media/i2c/ov9282.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)
---
base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd

Best regards,
-- 
Richard Leitner <richard.leitner@linux.dev>

[PATCH 1/3] media: i2c: ov9282: use register definitions

[Richard Leitner]

For better readability use already available register definitions in
hard-coded common and mode register structs.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 87e5d7ce5a47ee23a721ea39c0ab314c6fca6007..c926842257893c4da3319b847fab9908b5bdaec6 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -296,8 +296,8 @@ static const struct ov9282_reg mode_1280x800_regs[] = {
 	{0x3813, 0x08},
 	{0x3814, 0x11},
 	{0x3815, 0x11},
-	{0x3820, 0x40},
-	{0x3821, 0x00},
+	{OV9282_REG_TIMING_FORMAT_1, 0x40},
+	{OV9282_REG_TIMING_FORMAT_2, 0x00},
 	{0x4003, 0x40},
 	{0x4008, 0x04},
 	{0x4009, 0x0b},
@@ -327,8 +327,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3813, 0x08},
 	{0x3814, 0x11},
 	{0x3815, 0x11},
-	{0x3820, 0x3c},
-	{0x3821, 0x84},
+	{OV9282_REG_TIMING_FORMAT_1, 0x3c},
+	{OV9282_REG_TIMING_FORMAT_2, 0x84},
 	{0x4003, 0x40},
 	{0x4008, 0x02},
 	{0x4009, 0x05},
@@ -358,8 +358,8 @@ static const struct ov9282_reg mode_640x400_regs[] = {
 	{0x3813, 0x04},
 	{0x3814, 0x31},
 	{0x3815, 0x22},
-	{0x3820, 0x60},
-	{0x3821, 0x01},
+	{OV9282_REG_TIMING_FORMAT_1, 0x60},
+	{OV9282_REG_TIMING_FORMAT_2, 0x01},
 	{0x4008, 0x02},
 	{0x4009, 0x05},
 	{0x400c, 0x00},

-- 
2.47.2

[PATCH 2/3] media: i2c: ov9282: add AEC Manual register definition

[Richard Leitner]

Add #define's for the "AEC MANUAL" (0x3503) register and its
values/flags. Use those in the registers single usage within the
`common_regs` struct.

All values are based on the OV9281 datasheet v1.01 (09.18.2015).

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c926842257893c4da3319b847fab9908b5bdaec6..c882a021cf18852237bf9b9524d3de0c5b48cbcb 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -44,6 +44,15 @@
 #define OV9282_EXPOSURE_STEP	1
 #define OV9282_EXPOSURE_DEFAULT	0x0282
 
+/* AEC/AGC manual */
+#define OV9282_REG_AEC_MANUAL		0x3503
+#define OV9282_DIGFRAC_GAIN_DELAY	BIT(6)
+#define OV9282_GAIN_CHANGE_DELAY	BIT(5)
+#define OV9282_GAIN_DELAY		BIT(4)
+#define OV9282_GAIN_PREC16_EN		BIT(3)
+#define OV9282_GAIN_MANUAL_AS_SENSGAIN	BIT(2)
+#define OV9282_AEC_MANUAL_DEFAULT	0x00
+
 /* Analog gain control */
 #define OV9282_REG_AGAIN	0x3509
 #define OV9282_AGAIN_MIN	0x10
@@ -214,7 +223,7 @@ static const struct ov9282_reg common_regs[] = {
 	{0x3030, 0x10},
 	{0x3039, 0x32},
 	{0x303a, 0x00},
-	{0x3503, 0x08},
+	{OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
 	{0x3505, 0x8c},
 	{0x3507, 0x03},
 	{0x3508, 0x00},

-- 
2.47.2

[PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Richard Leitner]

The sensors analogue gain is stored within two "LONG GAIN" registers
(0x3508 and 0x3509) where the first one holds the upper 5 bits of the
value. The second register (0x3509) holds the lower 4 bits of the gain
value in its upper 4 bits. The lower 4 register bits are fraction bits.

This patch changes the gain control to adhere to the datasheet and
make the "upper gain register" (0x3508) accessible via the gain control,
resulting in a new maximum of 0x1fff instead of 0xff.

As the "upper gain register" is now written during exposure/gain update
remove the hard-coded 0x00 write to it from common_regs.

We cover only the "real gain format" use-case. The "sensor gain
format" one is ignored as based on the hard-coded "AEC MANUAL" register
configuration it is disabled.

All values are based on the OV9281 datasheet v1.01 (09.18.2015).

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c882a021cf18852237bf9b9524d3de0c5b48cbcb..e6effb2b42d4d5d0ca3d924df59c60512f9ce65d 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -54,11 +54,11 @@
 #define OV9282_AEC_MANUAL_DEFAULT	0x00
 
 /* Analog gain control */
-#define OV9282_REG_AGAIN	0x3509
-#define OV9282_AGAIN_MIN	0x10
-#define OV9282_AGAIN_MAX	0xff
-#define OV9282_AGAIN_STEP	1
-#define OV9282_AGAIN_DEFAULT	0x10
+#define OV9282_REG_AGAIN	0x3508
+#define OV9282_AGAIN_MIN	0x0010
+#define OV9282_AGAIN_MAX	0x1fff
+#define OV9282_AGAIN_STEP	0x0001
+#define OV9282_AGAIN_DEFAULT	0x0010
 
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
@@ -226,7 +226,6 @@ static const struct ov9282_reg common_regs[] = {
 	{OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
 	{0x3505, 0x8c},
 	{0x3507, 0x03},
-	{0x3508, 0x00},
 	{0x3610, 0x80},
 	{0x3611, 0xa0},
 	{0x3620, 0x6e},
@@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 	if (ret)
 		goto error_release_group_hold;
 
-	ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
+	ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
+	if (ret)
+		goto error_release_group_hold;
+
+	ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);
 
 error_release_group_hold:
 	ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);

-- 
2.47.2

Re: [PATCH 1/3] media: i2c: ov9282: use register definitions

[Dave Stevenson]

Hi Richard

Thanks for the patch

On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> For better readability use already available register definitions in
> hard-coded common and mode register structs.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/ov9282.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 87e5d7ce5a47ee23a721ea39c0ab314c6fca6007..c926842257893c4da3319b847fab9908b5bdaec6 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -296,8 +296,8 @@ static const struct ov9282_reg mode_1280x800_regs[] = {
>         {0x3813, 0x08},
>         {0x3814, 0x11},
>         {0x3815, 0x11},
> -       {0x3820, 0x40},
> -       {0x3821, 0x00},
> +       {OV9282_REG_TIMING_FORMAT_1, 0x40},
> +       {OV9282_REG_TIMING_FORMAT_2, 0x00},
>         {0x4003, 0x40},
>         {0x4008, 0x04},
>         {0x4009, 0x0b},
> @@ -327,8 +327,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>         {0x3813, 0x08},
>         {0x3814, 0x11},
>         {0x3815, 0x11},
> -       {0x3820, 0x3c},
> -       {0x3821, 0x84},
> +       {OV9282_REG_TIMING_FORMAT_1, 0x3c},
> +       {OV9282_REG_TIMING_FORMAT_2, 0x84},
>         {0x4003, 0x40},
>         {0x4008, 0x02},
>         {0x4009, 0x05},
> @@ -358,8 +358,8 @@ static const struct ov9282_reg mode_640x400_regs[] = {
>         {0x3813, 0x04},
>         {0x3814, 0x31},
>         {0x3815, 0x22},
> -       {0x3820, 0x60},
> -       {0x3821, 0x01},
> +       {OV9282_REG_TIMING_FORMAT_1, 0x60},
> +       {OV9282_REG_TIMING_FORMAT_2, 0x01},
>         {0x4008, 0x02},
>         {0x4009, 0x05},
>         {0x400c, 0x00},
>
> --
> 2.47.2
>
>

Re: [PATCH 2/3] media: i2c: ov9282: add AEC Manual register definition

[Dave Stevenson]

Hi Richard

Thanks for the patch

On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> Add #define's for the "AEC MANUAL" (0x3503) register and its
> values/flags. Use those in the registers single usage within the
> `common_regs` struct.
>
> All values are based on the OV9281 datasheet v1.01 (09.18.2015).
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/ov9282.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c926842257893c4da3319b847fab9908b5bdaec6..c882a021cf18852237bf9b9524d3de0c5b48cbcb 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -44,6 +44,15 @@
>  #define OV9282_EXPOSURE_STEP   1
>  #define OV9282_EXPOSURE_DEFAULT        0x0282
>
> +/* AEC/AGC manual */
> +#define OV9282_REG_AEC_MANUAL          0x3503
> +#define OV9282_DIGFRAC_GAIN_DELAY      BIT(6)
> +#define OV9282_GAIN_CHANGE_DELAY       BIT(5)
> +#define OV9282_GAIN_DELAY              BIT(4)
> +#define OV9282_GAIN_PREC16_EN          BIT(3)
> +#define OV9282_GAIN_MANUAL_AS_SENSGAIN BIT(2)
> +#define OV9282_AEC_MANUAL_DEFAULT      0x00
> +
>  /* Analog gain control */
>  #define OV9282_REG_AGAIN       0x3509
>  #define OV9282_AGAIN_MIN       0x10
> @@ -214,7 +223,7 @@ static const struct ov9282_reg common_regs[] = {
>         {0x3030, 0x10},
>         {0x3039, 0x32},
>         {0x303a, 0x00},
> -       {0x3503, 0x08},
> +       {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
>         {0x3505, 0x8c},
>         {0x3507, 0x03},
>         {0x3508, 0x00},
>
> --
> 2.47.2
>
>

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Dave Stevenson]

Hi Richard

On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> The sensors analogue gain is stored within two "LONG GAIN" registers
> (0x3508 and 0x3509) where the first one holds the upper 5 bits of the
> value. The second register (0x3509) holds the lower 4 bits of the gain
> value in its upper 4 bits. The lower 4 register bits are fraction bits.
>
> This patch changes the gain control to adhere to the datasheet and
> make the "upper gain register" (0x3508) accessible via the gain control,
> resulting in a new maximum of 0x1fff instead of 0xff.
>
> As the "upper gain register" is now written during exposure/gain update
> remove the hard-coded 0x00 write to it from common_regs.
>
> We cover only the "real gain format" use-case. The "sensor gain
> format" one is ignored as based on the hard-coded "AEC MANUAL" register
> configuration it is disabled.
>
> All values are based on the OV9281 datasheet v1.01 (09.18.2015).

My web searches turn up a 1.53 from Jan 2019 -
http://www.sinotimes-tech.com/product/20220217221034589.pdf
That lists 0x3508 as DEBUG, not LONG_GAIN.

The current range allows analogue gain to x15.9375.
Expanding it to 0x1ff.f would be up to x511.9375. I believe that
equates to ~54dB as we're scaling voltages, not power. The spec sheet
for the sensor lists S/N of 38dB and dynamic range of 68dB, so x511
will be almost pure noise.

Doing a very basic test using i2ctransfer to set gain values whilst
the sensor is running suggests that the image is the same regardless
of bits 2-4 of 0x3508. Setting either bits 0 or 1 increases the gain
by around x8.5, but they don't combine.

Overall can I ask how you've tested that a range up to 0x1fff works,
and on which module? I currently don't believe this works as intended.

  Dave

> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c882a021cf18852237bf9b9524d3de0c5b48cbcb..e6effb2b42d4d5d0ca3d924df59c60512f9ce65d 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -54,11 +54,11 @@
>  #define OV9282_AEC_MANUAL_DEFAULT      0x00
>
>  /* Analog gain control */
> -#define OV9282_REG_AGAIN       0x3509
> -#define OV9282_AGAIN_MIN       0x10
> -#define OV9282_AGAIN_MAX       0xff
> -#define OV9282_AGAIN_STEP      1
> -#define OV9282_AGAIN_DEFAULT   0x10
> +#define OV9282_REG_AGAIN       0x3508
> +#define OV9282_AGAIN_MIN       0x0010
> +#define OV9282_AGAIN_MAX       0x1fff
> +#define OV9282_AGAIN_STEP      0x0001
> +#define OV9282_AGAIN_DEFAULT   0x0010
>
>  /* Group hold register */
>  #define OV9282_REG_HOLD                0x3308
> @@ -226,7 +226,6 @@ static const struct ov9282_reg common_regs[] = {
>         {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
>         {0x3505, 0x8c},
>         {0x3507, 0x03},
> -       {0x3508, 0x00},
>         {0x3610, 0x80},
>         {0x3611, 0xa0},
>         {0x3620, 0x6e},
> @@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>         if (ret)
>                 goto error_release_group_hold;
>
> -       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
> +       if (ret)
> +               goto error_release_group_hold;
> +
> +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);
>
>  error_release_group_hold:
>         ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
>
> --
> 2.47.2
>
>

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Laurent Pinchart]

On Tue, Feb 25, 2025 at 03:30:16PM +0000, Dave Stevenson wrote:
> On Tue, 25 Feb 2025 at 13:09, Richard Leitner wrote:
> >
> > The sensors analogue gain is stored within two "LONG GAIN" registers
> > (0x3508 and 0x3509) where the first one holds the upper 5 bits of the
> > value. The second register (0x3509) holds the lower 4 bits of the gain
> > value in its upper 4 bits. The lower 4 register bits are fraction bits.
> >
> > This patch changes the gain control to adhere to the datasheet and
> > make the "upper gain register" (0x3508) accessible via the gain control,
> > resulting in a new maximum of 0x1fff instead of 0xff.
> >
> > As the "upper gain register" is now written during exposure/gain update
> > remove the hard-coded 0x00 write to it from common_regs.
> >
> > We cover only the "real gain format" use-case. The "sensor gain
> > format" one is ignored as based on the hard-coded "AEC MANUAL" register
> > configuration it is disabled.
> >
> > All values are based on the OV9281 datasheet v1.01 (09.18.2015).
> 
> My web searches turn up a 1.53 from Jan 2019 -
> http://www.sinotimes-tech.com/product/20220217221034589.pdf
> That lists 0x3508 as DEBUG, not LONG_GAIN.
> 
> The current range allows analogue gain to x15.9375.
> Expanding it to 0x1ff.f would be up to x511.9375. I believe that
> equates to ~54dB as we're scaling voltages, not power. The spec sheet
> for the sensor lists S/N of 38dB and dynamic range of 68dB, so x511
> will be almost pure noise.
> 
> Doing a very basic test using i2ctransfer to set gain values whilst
> the sensor is running suggests that the image is the same regardless
> of bits 2-4 of 0x3508. Setting either bits 0 or 1 increases the gain
> by around x8.5, but they don't combine.
> 
> Overall can I ask how you've tested that a range up to 0x1fff works,
> and on which module? I currently don't believe this works as intended.
> 
>   Dave
> 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index c882a021cf18852237bf9b9524d3de0c5b48cbcb..e6effb2b42d4d5d0ca3d924df59c60512f9ce65d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -54,11 +54,11 @@
> >  #define OV9282_AEC_MANUAL_DEFAULT      0x00
> >
> >  /* Analog gain control */
> > -#define OV9282_REG_AGAIN       0x3509
> > -#define OV9282_AGAIN_MIN       0x10
> > -#define OV9282_AGAIN_MAX       0xff
> > -#define OV9282_AGAIN_STEP      1
> > -#define OV9282_AGAIN_DEFAULT   0x10
> > +#define OV9282_REG_AGAIN       0x3508
> > +#define OV9282_AGAIN_MIN       0x0010
> > +#define OV9282_AGAIN_MAX       0x1fff
> > +#define OV9282_AGAIN_STEP      0x0001
> > +#define OV9282_AGAIN_DEFAULT   0x0010
> >
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD                0x3308
> > @@ -226,7 +226,6 @@ static const struct ov9282_reg common_regs[] = {
> >         {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
> >         {0x3505, 0x8c},
> >         {0x3507, 0x03},
> > -       {0x3508, 0x00},
> >         {0x3610, 0x80},
> >         {0x3611, 0xa0},
> >         {0x3620, 0x6e},
> > @@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >         if (ret)
> >                 goto error_release_group_hold;
> >
> > -       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
> > +       if (ret)
> > +               goto error_release_group_hold;
> > +
> > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);

Ignoring Dave's functional review for a moment to focus on the code:
16-bit registers should ideally use the v4l2-cci helpers. It would be
nice to convert the driver to them.

> >
> >  error_release_group_hold:
> >         ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> >

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Richard Leitner]

[-- Attachment #1: Type: text/plain, Size: 6551 bytes --]

Hi Dave,
thanks for the quick and detailled reply!

On Tue, Feb 25, 2025 at 03:30:16PM +0000, Dave Stevenson wrote:
> Hi Richard
> 
> On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.dev> wrote:
> >
> > The sensors analogue gain is stored within two "LONG GAIN" registers
> > (0x3508 and 0x3509) where the first one holds the upper 5 bits of the
> > value. The second register (0x3509) holds the lower 4 bits of the gain
> > value in its upper 4 bits. The lower 4 register bits are fraction bits.
> >
> > This patch changes the gain control to adhere to the datasheet and
> > make the "upper gain register" (0x3508) accessible via the gain control,
> > resulting in a new maximum of 0x1fff instead of 0xff.
> >
> > As the "upper gain register" is now written during exposure/gain update
> > remove the hard-coded 0x00 write to it from common_regs.
> >
> > We cover only the "real gain format" use-case. The "sensor gain
> > format" one is ignored as based on the hard-coded "AEC MANUAL" register
> > configuration it is disabled.
> >
> > All values are based on the OV9281 datasheet v1.01 (09.18.2015).
> 
> My web searches turn up a 1.53 from Jan 2019 -
> http://www.sinotimes-tech.com/product/20220217221034589.pdf
> That lists 0x3508 as DEBUG, not LONG_GAIN.

Thanks. That helps a lot :-)

> 
> The current range allows analogue gain to x15.9375.
> Expanding it to 0x1ff.f would be up to x511.9375. I believe that
> equates to ~54dB as we're scaling voltages, not power. The spec sheet
> for the sensor lists S/N of 38dB and dynamic range of 68dB, so x511
> will be almost pure noise.
> 
> Doing a very basic test using i2ctransfer to set gain values whilst
> the sensor is running suggests that the image is the same regardless
> of bits 2-4 of 0x3508. Setting either bits 0 or 1 increases the gain
> by around x8.5, but they don't combine.
> 
> Overall can I ask how you've tested that a range up to 0x1fff works,
> and on which module? I currently don't believe this works as intended.

I've done some basic testing on a vision components OV9281 module.
Nonetheless it seems I should have done more testing... I just ran a
"gain sweep" test and it seems you are perfectly right...

The lower two bits of 0x3508 have some kind of offset and "flattening" effect
on the applied gain, like shown in the plot (X is the gain, Y is the reported
mean brightness of the picture, read by magick).

  45 +---------------------------------------------------------------------+
     |         +           +           +           +           +           |
     |                     A                     AA                        |
  40 |-+                AAA                    AA                        +-|
     |                 AA                 A AAA                            |
  35 |-+             AA                AAA A                             +-|
     |              AA              AAAA                             AAAAAA|
     |            AA             AAAA                          AAAAAA      |
  30 |-+         AA           AAAA                       AAAAAA          +-|
     |         AA           AAA                    AAAAAA                  |
     |         A           A                         A                     |
  25 |-+     AA                                                          +-|
     |      A                                                              |
     |     A                                                               |
  20 |-+ AA                                                              +-|
     |   A                                                                 |
  15 |-AA                                                                +-|
     |A                                                                    |
     |A        +           +           +           +           +           |
  10 +---------------------------------------------------------------------+
            0x0080      0x0100      0x0180      0x0200      0x0280      0x0300
                                      gain

This pattern repeats up to 0x1fff, so I guess all other bits of 0x3508
have no effect on the gain (like shown in the plot attached as png, as
it got way to big for ascii).

I'm sorry for the inconvenience caused... I've somehow messed up my
initial tests.

Thank you again for your feedback!

So please feel free to ignore this patch. Should I send a new series
with just the two minor patches of this series? Or should I include them
in the next series for the ov9282 driver?

regards;rl

> 
>   Dave
> 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index c882a021cf18852237bf9b9524d3de0c5b48cbcb..e6effb2b42d4d5d0ca3d924df59c60512f9ce65d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -54,11 +54,11 @@
> >  #define OV9282_AEC_MANUAL_DEFAULT      0x00
> >
> >  /* Analog gain control */
> > -#define OV9282_REG_AGAIN       0x3509
> > -#define OV9282_AGAIN_MIN       0x10
> > -#define OV9282_AGAIN_MAX       0xff
> > -#define OV9282_AGAIN_STEP      1
> > -#define OV9282_AGAIN_DEFAULT   0x10
> > +#define OV9282_REG_AGAIN       0x3508
> > +#define OV9282_AGAIN_MIN       0x0010
> > +#define OV9282_AGAIN_MAX       0x1fff
> > +#define OV9282_AGAIN_STEP      0x0001
> > +#define OV9282_AGAIN_DEFAULT   0x0010
> >
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD                0x3308
> > @@ -226,7 +226,6 @@ static const struct ov9282_reg common_regs[] = {
> >         {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
> >         {0x3505, 0x8c},
> >         {0x3507, 0x03},
> > -       {0x3508, 0x00},
> >         {0x3610, 0x80},
> >         {0x3611, 0xa0},
> >         {0x3620, 0x6e},
> > @@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >         if (ret)
> >                 goto error_release_group_hold;
> >
> > -       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
> > +       if (ret)
> > +               goto error_release_group_hold;
> > +
> > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);
> >
> >  error_release_group_hold:
> >         ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> >
> > --
> > 2.47.2
> >
> >

[-- Attachment #2: plot4.png --]
[-- Type: image/png, Size: 18808 bytes --]

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Richard Leitner]

Hi Laurent,

On Tue, Feb 25, 2025 at 05:46:07PM +0200, Laurent Pinchart wrote:
> On Tue, Feb 25, 2025 at 03:30:16PM +0000, Dave Stevenson wrote:
> > On Tue, 25 Feb 2025 at 13:09, Richard Leitner wrote:

...

> > > @@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> > >         if (ret)
> > >                 goto error_release_group_hold;
> > >
> > > -       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
> > > +       if (ret)
> > > +               goto error_release_group_hold;
> > > +
> > > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);
> 
> Ignoring Dave's functional review for a moment to focus on the code:
> 16-bit registers should ideally use the v4l2-cci helpers. It would be
> nice to convert the driver to them.

Thanks for the feedback! I will take a look at the v4l2-cci helpers.

regards;rl

> 
> > >
> > >  error_release_group_hold:
> > >         ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> > >
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Dave Stevenson]

On Wed, 26 Feb 2025 at 11:07, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> Hi Dave,
> thanks for the quick and detailled reply!
>
> On Tue, Feb 25, 2025 at 03:30:16PM +0000, Dave Stevenson wrote:
> > Hi Richard
> >
> > On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.dev> wrote:
> > >
> > > The sensors analogue gain is stored within two "LONG GAIN" registers
> > > (0x3508 and 0x3509) where the first one holds the upper 5 bits of the
> > > value. The second register (0x3509) holds the lower 4 bits of the gain
> > > value in its upper 4 bits. The lower 4 register bits are fraction bits.
> > >
> > > This patch changes the gain control to adhere to the datasheet and
> > > make the "upper gain register" (0x3508) accessible via the gain control,
> > > resulting in a new maximum of 0x1fff instead of 0xff.
> > >
> > > As the "upper gain register" is now written during exposure/gain update
> > > remove the hard-coded 0x00 write to it from common_regs.
> > >
> > > We cover only the "real gain format" use-case. The "sensor gain
> > > format" one is ignored as based on the hard-coded "AEC MANUAL" register
> > > configuration it is disabled.
> > >
> > > All values are based on the OV9281 datasheet v1.01 (09.18.2015).
> >
> > My web searches turn up a 1.53 from Jan 2019 -
> > http://www.sinotimes-tech.com/product/20220217221034589.pdf
> > That lists 0x3508 as DEBUG, not LONG_GAIN.
>
> Thanks. That helps a lot :-)
>
> >
> > The current range allows analogue gain to x15.9375.
> > Expanding it to 0x1ff.f would be up to x511.9375. I believe that
> > equates to ~54dB as we're scaling voltages, not power. The spec sheet
> > for the sensor lists S/N of 38dB and dynamic range of 68dB, so x511
> > will be almost pure noise.
> >
> > Doing a very basic test using i2ctransfer to set gain values whilst
> > the sensor is running suggests that the image is the same regardless
> > of bits 2-4 of 0x3508. Setting either bits 0 or 1 increases the gain
> > by around x8.5, but they don't combine.
> >
> > Overall can I ask how you've tested that a range up to 0x1fff works,
> > and on which module? I currently don't believe this works as intended.
>
> I've done some basic testing on a vision components OV9281 module.
> Nonetheless it seems I should have done more testing... I just ran a
> "gain sweep" test and it seems you are perfectly right...
>
> The lower two bits of 0x3508 have some kind of offset and "flattening" effect
> on the applied gain, like shown in the plot (X is the gain, Y is the reported
> mean brightness of the picture, read by magick).
>
>   45 +---------------------------------------------------------------------+
>      |         +           +           +           +           +           |
>      |                     A                     AA                        |
>   40 |-+                AAA                    AA                        +-|
>      |                 AA                 A AAA                            |
>   35 |-+             AA                AAA A                             +-|
>      |              AA              AAAA                             AAAAAA|
>      |            AA             AAAA                          AAAAAA      |
>   30 |-+         AA           AAAA                       AAAAAA          +-|
>      |         AA           AAA                    AAAAAA                  |
>      |         A           A                         A                     |
>   25 |-+     AA                                                          +-|
>      |      A                                                              |
>      |     A                                                               |
>   20 |-+ AA                                                              +-|
>      |   A                                                                 |
>   15 |-AA                                                                +-|
>      |A                                                                    |
>      |A        +           +           +           +           +           |
>   10 +---------------------------------------------------------------------+
>             0x0080      0x0100      0x0180      0x0200      0x0280      0x0300
>                                       gain
>
> This pattern repeats up to 0x1fff, so I guess all other bits of 0x3508
> have no effect on the gain (like shown in the plot attached as png, as
> it got way to big for ascii).
>
> I'm sorry for the inconvenience caused... I've somehow messed up my
> initial tests.

No problem - this is why we review and test patches.
My general view would be that anything over x64 on analogue gain would
be unusual, and on most sensors x16 is about the limit of useful gain.

> Thank you again for your feedback!
>
> So please feel free to ignore this patch. Should I send a new series
> with just the two minor patches of this series? Or should I include them
> in the next series for the ov9282 driver?

Up to Sakari.
The other two patches have my R-b responses, so taking those two and
ignoring this one should be fairly simple, but it just depends upon
workflows.

  Dave

> regards;rl
>
> >
> >   Dave
> >
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 17 ++++++++++-------
> > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index c882a021cf18852237bf9b9524d3de0c5b48cbcb..e6effb2b42d4d5d0ca3d924df59c60512f9ce65d 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -54,11 +54,11 @@
> > >  #define OV9282_AEC_MANUAL_DEFAULT      0x00
> > >
> > >  /* Analog gain control */
> > > -#define OV9282_REG_AGAIN       0x3509
> > > -#define OV9282_AGAIN_MIN       0x10
> > > -#define OV9282_AGAIN_MAX       0xff
> > > -#define OV9282_AGAIN_STEP      1
> > > -#define OV9282_AGAIN_DEFAULT   0x10
> > > +#define OV9282_REG_AGAIN       0x3508
> > > +#define OV9282_AGAIN_MIN       0x0010
> > > +#define OV9282_AGAIN_MAX       0x1fff
> > > +#define OV9282_AGAIN_STEP      0x0001
> > > +#define OV9282_AGAIN_DEFAULT   0x0010
> > >
> > >  /* Group hold register */
> > >  #define OV9282_REG_HOLD                0x3308
> > > @@ -226,7 +226,6 @@ static const struct ov9282_reg common_regs[] = {
> > >         {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
> > >         {0x3505, 0x8c},
> > >         {0x3507, 0x03},
> > > -       {0x3508, 0x00},
> > >         {0x3610, 0x80},
> > >         {0x3611, 0xa0},
> > >         {0x3620, 0x6e},
> > > @@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> > >         if (ret)
> > >                 goto error_release_group_hold;
> > >
> > > -       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
> > > +       if (ret)
> > > +               goto error_release_group_hold;
> > > +
> > > +       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);
> > >
> > >  error_release_group_hold:
> > >         ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> > >
> > > --
> > > 2.47.2
> > >
> > >

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Sakari Ailus]

Hi Dave, Richard,

On Wed, Feb 26, 2025 at 12:52:46PM +0000, Dave Stevenson wrote:
> > So please feel free to ignore this patch. Should I send a new series
> > with just the two minor patches of this series? Or should I include them
> > in the next series for the ov9282 driver?
> 
> Up to Sakari.
> The other two patches have my R-b responses, so taking those two and
> ignoring this one should be fairly simple, but it just depends upon
> workflows.

I can pick the two first patches of the set now.

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

[Richard Leitner]

Hi Sakari,

On Wed, Feb 26, 2025 at 01:01:05PM +0000, Sakari Ailus wrote:
> Hi Dave, Richard,
> 
> On Wed, Feb 26, 2025 at 12:52:46PM +0000, Dave Stevenson wrote:
> > > So please feel free to ignore this patch. Should I send a new series
> > > with just the two minor patches of this series? Or should I include them
> > > in the next series for the ov9282 driver?
> > 
> > Up to Sakari.
> > The other two patches have my R-b responses, so taking those two and
> > ignoring this one should be fairly simple, but it just depends upon
> > workflows.
> 
> I can pick the two first patches of the set now.

Thanks. That would be great!

regards;rl

> 
> -- 
> Regards,
> 
> Sakari Ailus