[libcamera-devel] [PATCH 2/2] media: imx258: Limit the max analogue gain to 480

[Dave Stevenson]

Hi Umang and Laurent

On Fri, 23 Jul 2021 at 12:52, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Fri, Jul 23, 2021 at 02:44:40PM +0300, Laurent Pinchart wrote:
> > Hi Umang,
> >
> > Thank you for the patch.
> >
> > CC'ing Sakari. For future kernel patches, you can use the
> > ./scripts/get_maintainer.pl script in the kernel sources to get a list
> > of appropriate recipients. The list should be taken with a grain of salt
> > though, it has a tendency to return too many recipients. For this
> > particular patch, for instance, it also recommends Mauro and LKML.
> > Whether to CC the subsystem maintainer on every patch depends on the
> > subsystem (it's more common for small ones than big ones) and on the
> > maintainer's preferences. LKML is a catch-all mailing list with very
> > high traffic, and when more appropriate venues exist for patches, I
> > usually recommend skipping LKML.
>
> And expanding the CC list further to include Dave (for his contribution
> to the discussion), and Krzysztof and Bingbu (for their contributions to
> the driver, as reported by git log).
>
> > On Fri, Jul 23, 2021 at 04:52:33PM +0530, Umang Jain wrote:
> > > The range for analog gain mentioned in the datasheet is [0, 480].
> > > The real gain formula mentioned in the datasheet is:
> > >
> > >     Gain = 512 / (512 – X)
> > >
> > > Hence, values larger than 511 clearly makes no sense. The gain
> > > register field is also documented to be of 9-bits in the datasheet.
> > >
> > > Certainly, it is enough to infer that, the kernel driver currently
> > > advertises an arbitrary analog gain max. Fix it by rectifying the
> > > value as per the data sheet i.e. 480.
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

That certainly follows the datasheet that I have.

Gains up to code 480 work as expected. Up to 496 seems to work, but
going beyond that causes issues. Adopting the documented maximum value
is the safest approach.

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

> >
> > > ---
> > >  drivers/media/i2c/imx258.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > index 4e695096e5d0..81cdf37216ca 100644
> > > --- a/drivers/media/i2c/imx258.c
> > > +++ b/drivers/media/i2c/imx258.c
> > > @@ -47,7 +47,7 @@
> > >  /* Analog gain control */
> > >  #define IMX258_REG_ANALOG_GAIN             0x0204
> > >  #define IMX258_ANA_GAIN_MIN                0
> > > -#define IMX258_ANA_GAIN_MAX                0x1fff
> > > +#define IMX258_ANA_GAIN_MAX                480
> > >  #define IMX258_ANA_GAIN_STEP               1
> > >  #define IMX258_ANA_GAIN_DEFAULT            0x0
> > >
>
> --
> Regards,
>
> Laurent Pinchart