Hello, Gentle ping for review. On Mon, Nov 28, 2022 at 10:02:01AM +0200, Laurent Pinchart wrote: > From: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxx> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > --- > 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, -- Regards, Laurent Pinchart
