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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250225154607.GA18426@pendragon.ideasonboard.com>
Date: Tue, 25 Feb 2025 17:46:07 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Richard Leitner <richard.leitner@...ux.dev>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

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@...ux.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