diff mbox series

[v2] media: ov5640: Fix analogue gain control

Message ID 20221128080201.15104-1-laurent.pinchart@ideasonboard.com
State New
Headers show
Series [v2] media: ov5640: Fix analogue gain control | expand

Commit Message

Laurent Pinchart Nov. 28, 2022, 8:02 a.m. UTC 
From: Paul Elder <paul.elder@ideasonboard.com>

Gain control is badly documented in publicly available (including
leaked) documentation.

There is an AGC pre-gain in register 0x3a13, expressed as a 6-bit value
(plus an enable bit in bit 6). The driver hardcodes it to 0x43, which
one application note states is equal to x1.047. The documentation also
states that 0x40 is equel to x1.000. The pre-gain thus seems to be
expressed as in 1/64 increments, and thus ranges from x1.00 to x1.984.
What the pre-gain does is however unspecified.

There is then an AGC gain limit, in registers 0x3a18 and 0x3a19,
expressed as a 10-bit "real gain format" value. One application note
sets it to 0x00f8 and states it is equal to x15.5, so it appears to be
expressed in 1/16 increments, up to x63.9375.

The manual gain is stored in registers 0x350a and 0x350b, also as a
10-bit "real gain format" value. It is documented in the application
note as a Q6.4 values, up to x63.9375.

One version of the datasheet indicates that the sensor supports a
digital gain:

  The OV5640 supports 1/2/4 digital gain. Normally, the gain is
  controlled automatically by the automatic gain control (AGC) block.

It isn't clear how that would be controlled manually.

There appears to be no indication regarding whether the gain controlled
through registers 0x350a and 0x350b is an analogue gain only or also
includes digital gain. The words "real gain" don't necessarily mean
"combined analogue and digital gains". Some OmniVision sensors (such as
the OV8858) are documented as supoprting different formats for the gain
values, selectable through a register bit, and they are called "real
gain format" and "sensor gain format". For that sensor, we have (one of)
the gain registers documented as

  0x3503[2]=0, gain[7:0] is real gain format, where low 4 bits are
  fraction bits, for example, 0x10 is 1x gain, 0x28 is 2.5x gain

  If 0x3503[2]=1, gain[7:0] is sensor gain format, gain[7:4] is coarse
  gain, 00000: 1x, 00001: 2x, 00011: 4x, 00111: 8x, gain[7] is 1,
  gain[3:0] is fine gain. For example, 0x10 is 1x gain, 0x30 is 2x gain,
  0x70 is 4x gain

(The second part of the text makes little sense)

"Real gain" may thus refer to the combination of the coarse and fine
analogue gains as a single value.

The OV5640 0x350a and 0x350b registers thus appear to control analogue
gain. The driver incorrectly uses V4L2_CID_GAIN as V4L2 has a specific
control for analogue gain, V4L2_CID_ANALOGUE_GAIN. Use it.

If registers 0x350a and 0x350b are later found to control digital gain
as well, the driver could then restrict the range of the analogue gain
control value to lower than x64 and add a separate digital gain control.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/ov5640.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Laurent Pinchart Dec. 22, 2022, 10:42 p.m. UTC | #1 
Hello,

Gentle ping for review.

On Mon, Nov 28, 2022 at 10:02:01AM +0200, Laurent Pinchart wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> Gain control is badly documented in publicly available (including
> leaked) documentation.
> 
> There is an AGC pre-gain in register 0x3a13, expressed as a 6-bit value
> (plus an enable bit in bit 6). The driver hardcodes it to 0x43, which
> one application note states is equal to x1.047. The documentation also
> states that 0x40 is equel to x1.000. The pre-gain thus seems to be
> expressed as in 1/64 increments, and thus ranges from x1.00 to x1.984.
> What the pre-gain does is however unspecified.
> 
> There is then an AGC gain limit, in registers 0x3a18 and 0x3a19,
> expressed as a 10-bit "real gain format" value. One application note
> sets it to 0x00f8 and states it is equal to x15.5, so it appears to be
> expressed in 1/16 increments, up to x63.9375.
> 
> The manual gain is stored in registers 0x350a and 0x350b, also as a
> 10-bit "real gain format" value. It is documented in the application
> note as a Q6.4 values, up to x63.9375.
> 
> One version of the datasheet indicates that the sensor supports a
> digital gain:
> 
>   The OV5640 supports 1/2/4 digital gain. Normally, the gain is
>   controlled automatically by the automatic gain control (AGC) block.
> 
> It isn't clear how that would be controlled manually.
> 
> There appears to be no indication regarding whether the gain controlled
> through registers 0x350a and 0x350b is an analogue gain only or also
> includes digital gain. The words "real gain" don't necessarily mean
> "combined analogue and digital gains". Some OmniVision sensors (such as
> the OV8858) are documented as supoprting different formats for the gain
> values, selectable through a register bit, and they are called "real
> gain format" and "sensor gain format". For that sensor, we have (one of)
> the gain registers documented as
> 
>   0x3503[2]=0, gain[7:0] is real gain format, where low 4 bits are
>   fraction bits, for example, 0x10 is 1x gain, 0x28 is 2.5x gain
> 
>   If 0x3503[2]=1, gain[7:0] is sensor gain format, gain[7:4] is coarse
>   gain, 00000: 1x, 00001: 2x, 00011: 4x, 00111: 8x, gain[7] is 1,
>   gain[3:0] is fine gain. For example, 0x10 is 1x gain, 0x30 is 2x gain,
>   0x70 is 4x gain
> 
> (The second part of the text makes little sense)
> 
> "Real gain" may thus refer to the combination of the coarse and fine
> analogue gains as a single value.
> 
> The OV5640 0x350a and 0x350b registers thus appear to control analogue
> gain. The driver incorrectly uses V4L2_CID_GAIN as V4L2 has a specific
> control for analogue gain, V4L2_CID_ANALOGUE_GAIN. Use it.
> 
> If registers 0x350a and 0x350b are later found to control digital gain
> as well, the driver could then restrict the range of the analogue gain
> control value to lower than x64 and add a separate digital gain control.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5640.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2d740397a5d4..c65c391bc1eb 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3458,7 +3458,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  	/* Auto/manual gain */
>  	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
>  					     0, 1, 1, 1);
> -	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
> +	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
>  					0, 1023, 1, 0);
>  
>  	ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
Jai Luthra Jan. 17, 2023, 9:14 a.m. UTC | #2 
Hi Laurent,

Thanks for the patch.

On 28/11/22 13:32, Laurent Pinchart wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> Gain control is badly documented in publicly available (including
> leaked) documentation.
> 
> There is an AGC pre-gain in register 0x3a13, expressed as a 6-bit value
> (plus an enable bit in bit 6). The driver hardcodes it to 0x43, which
> one application note states is equal to x1.047. The documentation also
> states that 0x40 is equel to x1.000. The pre-gain thus seems to be
> expressed as in 1/64 increments, and thus ranges from x1.00 to x1.984.
> What the pre-gain does is however unspecified.
> 
> There is then an AGC gain limit, in registers 0x3a18 and 0x3a19,
> expressed as a 10-bit "real gain format" value. One application note
> sets it to 0x00f8 and states it is equal to x15.5, so it appears to be
> expressed in 1/16 increments, up to x63.9375.
> 
> The manual gain is stored in registers 0x350a and 0x350b, also as a
> 10-bit "real gain format" value. It is documented in the application
> note as a Q6.4 values, up to x63.9375.
> 
> One version of the datasheet indicates that the sensor supports a
> digital gain:
> 
>    The OV5640 supports 1/2/4 digital gain. Normally, the gain is
>    controlled automatically by the automatic gain control (AGC) block.
> 
> It isn't clear how that would be controlled manually.
> 
> There appears to be no indication regarding whether the gain controlled
> through registers 0x350a and 0x350b is an analogue gain only or also
> includes digital gain. The words "real gain" don't necessarily mean
> "combined analogue and digital gains". Some OmniVision sensors (such as
> the OV8858) are documented as supoprting different formats for the gain
> values, selectable through a register bit, and they are called "real
> gain format" and "sensor gain format". For that sensor, we have (one of)
> the gain registers documented as
> 
>    0x3503[2]=0, gain[7:0] is real gain format, where low 4 bits are
>    fraction bits, for example, 0x10 is 1x gain, 0x28 is 2.5x gain
> 
>    If 0x3503[2]=1, gain[7:0] is sensor gain format, gain[7:4] is coarse
>    gain, 00000: 1x, 00001: 2x, 00011: 4x, 00111: 8x, gain[7] is 1,
>    gain[3:0] is fine gain. For example, 0x10 is 1x gain, 0x30 is 2x gain,
>    0x70 is 4x gain
> 
> (The second part of the text makes little sense)
> 
> "Real gain" may thus refer to the combination of the coarse and fine
> analogue gains as a single value.

LGTM.

In another Omnivision sensor (OV2312), they refer the same register pair 
as both "analogue gain" and "real gain" at different places in the 
datasheet - and it has separate digital gain registers as well.

> 
> The OV5640 0x350a and 0x350b registers thus appear to control analogue
> gain. The driver incorrectly uses V4L2_CID_GAIN as V4L2 has a specific
> control for analogue gain, V4L2_CID_ANALOGUE_GAIN. Use it.
> 
> If registers 0x350a and 0x350b are later found to control digital gain
> as well, the driver could then restrict the range of the analogue gain
> control value to lower than x64 and add a separate digital gain control.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Jai Luthra <j-luthra@ti.com>

> ---
>   drivers/media/i2c/ov5640.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2d740397a5d4..c65c391bc1eb 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3458,7 +3458,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>   	/* Auto/manual gain */
>   	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
>   					     0, 1, 1, 1);
> -	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
> +	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
>   					0, 1023, 1, 0);
>   
>   	ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2d740397a5d4..c65c391bc1eb 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -3458,7 +3458,7 @@  static int ov5640_init_controls(struct ov5640_dev *sensor)
 	/* Auto/manual gain */
 	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
 					     0, 1, 1, 1);
-	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
+	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
 					0, 1023, 1, 0);
 
 	ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,