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.2Hi 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
>
>
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
> >
> >
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
> > >
> > >
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
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
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
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
© 2016 - 2026 Red Hat, Inc.