diff mbox series

[2/2] media: imx258: Limit the max analogue gain to 480

Message ID 20210723112233.1361319-3-umang.jain@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series IMX258 driver fixes | expand

Commit Message

Umang Jain July 23, 2021, 11:22 a.m. UTC
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@ideasonboard.com>
---
 drivers/media/i2c/imx258.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Laurent Pinchart July 23, 2021, 11:44 a.m. UTC | #1
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.

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@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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
>
Laurent Pinchart July 23, 2021, 11:52 a.m. UTC | #2
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@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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
> >
Krzysztof Kozlowski July 23, 2021, 12:29 p.m. UTC | #3
On 23/07/2021 13:52, Laurent Pinchart 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).
> 

I don't work with this sensor anymore and I don't have it. I trust the
change is reasonable, but I did not check it in documentation.

Best regards,
Krzysztof
Dave Stevenson July 23, 2021, 12:36 p.m. UTC | #4
Hi Umang and Laurent

On Fri, 23 Jul 2021 at 12:52, Laurent Pinchart
<laurent.pinchart@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@ideasonboard.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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
diff mbox series

Patch

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