[libcamera-devel] [PATCH] ipa: libipa: Fix ov5670 analogue gain parameters

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Jul 05, 2021 at 06:01:20PM +0200, Jean-Michel Hautbois wrote:
> Based on datasheet, the gain multiplier is 128 and not 256 (the
> fraction bits are the 7 lower bits). Fix it.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7

You know about this already :-)

> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 23335ed6..cd0a19bb 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper
>  public:
>  	CameraSensorHelperOv5670()
>  	{
> -		analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };
> +		analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };

Interestingly, the OV5670 supports two different gain models at the
hardware level, with a register bit to select which model to use. Bit 2
in register 0x3503 selects the "real gain format" (linear) when set to
0, and the "sensor gain format" (exponential) when set to 1. I really
wonder why.

I've had a look at the driver, and quite interestingly, it selects the
linear gain model in all cases except for the 2560x1440 mode, where the
exponential gain model is used. Register 0x3503 is set to 0x04 towards
the beginning of the mode-specific register arrays for all modes, and
then set to 0x00 towards the end, except for 2560x1440 where the 0x00
entry is missing.

https://lore.kernel.org/linux-media/YOay2tIJxpBzYKzW@pendragon.ideasonboard.com/

I believe that's a driver bug, and this patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	}
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670)

-- 
Regards,

Laurent Pinchart