[libcamera-devel] [PATCH 1/2] ipa: libipa: Add OV2685 Camera Sensor Helper

[Umang Jain]

Hi Kieran

On 3/1/23 8:31 PM, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi (2023-02-28 15:30:51)
>> Hi Kieran
>>
>> On Mon, Feb 27, 2023 at 08:42:53PM +0000, Kieran Bingham via libcamera-devel wrote:
>>> Provide a CameraSensorHelper for the OV2685, along with the
>>> corresponding camera sensor properties.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> ---
>>>   src/ipa/libipa/camera_sensor_helper.cpp    | 11 +++++++++++
>>>   src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++
>>>   2 files changed, 25 insertions(+)
>>>
>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>>> index 7977d7eb6c07..d1051cc25656 100644
>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>> @@ -450,6 +450,17 @@ public:
>>>   };
>>>   REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>>>
>>> +class CameraSensorHelperOv2685 : public CameraSensorHelper
>>> +{
>>> +public:
>>> +     CameraSensorHelperOv2685()
>>> +     {
>>> +             gainType_ = AnalogueGainLinear;
>>> +             gainConstants_.linear = { 1, 0, 0, 128 };
>> The sensor manual I have doesn't specify a gain model at all.
>>
>> I've seen this working and it's reasonable, I would add a coment to
>> record that the gain model is not documented, but otherwise... ship
>> it!
> Is this ok for you ?
>
> +               /*
> +                * The Sensor Manual doesn't appear to document the gain model.
> +                * This has been validated with some empirical testing only.
> +                */
>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>
>>> +     }
>>> +};
>>> +REGISTER_CAMERA_SENSOR_HELPER("ov2685", CameraSensorHelperOv2685)
>>> +
>>>   class CameraSensorHelperOv2740 : public CameraSensorHelper
>>>   {
>>>   public:
>>> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
>>> index b8c532699684..7652c5f3e24c 100644
>>> --- a/src/libcamera/camera_sensor_properties.cpp
>>> +++ b/src/libcamera/camera_sensor_properties.cpp
>>> @@ -133,6 +133,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>>                                { controls::draft::TestPatternModePn9, 4 },
>>>                        },
>>>                } },
>>> +             { "ov2685", {
>>> +                     .unitCellSize = { 1750, 1750 },
>>> +                     .testPatternModes = {
>>> +                             { controls::draft::TestPatternModeOff, 0 },
>>> +                             { controls::draft::TestPatternModeColorBars, 1},
>>> +                             { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
>>> +                             /*
>>> +                              * Also exposed by the driver:

This doesn't tell convey the what's the issue here?  Is it a case of 
exposed by driver but not found while testing?

In that case, I would align with one of the pre-existing comment(s) from 
other entries:

     /*
      * No corresponding test pattern mode for:
     ....

or similar.

In any case, minor nitpick so,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>



>>> +                              *  - OV2685_TEST_PATTERN_RANDOM,
>>> +                              *  - OV2685_TEST_PATTERN_BW_SQUARE,
>>> +                              *  - OV2685_TEST_PATTERN_COLOR_SQUARE,
>>> +                              */
>>> +                     },
>>> +             } },
>>>                { "ov2740", {
>>>                        .unitCellSize = { 1400, 1400 },
>>>                        .testPatternModes = {
>>> --
>>> 2.34.1
>>>