[libcamera-devel] [PATCH v6 5/5] ipa: libcamera: add support for ov8858 sensor

[Kieran Bingham]

Hi Nicholas,

Quoting Nicholas Roth via libcamera-devel (2022-10-30 23:05:00)
> Currently, libcamera does not have information for the ov8858 sensor
> used in the PinePhone Pro, a phone designed to run Linux.
> 
> This commit adds metadata, especially that sensor gain is reported and
> set in 1/16 discrete increments.
> 
> For more information, see "5.8 manual exposure compensation/ manual
> gain compensation" in [0] and the driver in [1].
> 
> [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf
> [1] https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c
> 
> Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
> ---
>  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 35056bec..f2040cbd 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -476,6 +476,17 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
>  
> +class CameraSensorHelperOv8858 : public CameraSensorHelper
> +{
> +public:
> +       CameraSensorHelperOv8858()
> +       {
> +               gainType_ = AnalogueGainLinear;
> +               gainConstants_.linear = { 1, 0, 0, 16 };
> +       }
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)

"My OV8858 is the second on the rear." Suddenly this doesn't work.
- So we can only use "ov8858" here.

We 'could' add a dupliate entry as 

+/*
+ * \todo: Deprecated: The PinePhonePro has devices listed as m00_f_ov8858.
+ *        This entry should be removed as soon as the ov8858 driver is
+ *        accepted into a mainline kernel.
+ */
+REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)
+REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)

But the problem there, is that suddenly users will find it 'works' until
the driver gets into mainline, when it breaks. Given that the correct
approach at that point will be to update to use the mainline driver,
perhaps that's ok ... but I fear potentially unhappy users.

And perhaps also worryingly, it removes the impetus to get the driver
mainlined!

So accepting 'm00_f_ov8858' at all ... seems to be really tricky.

With just ov8858, and the 1.12 fix below this patch could probably go in
quite rapidly though. You could easily have the 

+REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)

line locally while developing, but I'd expect that your device should be
quickly moving along with the mainline integration, so you probably
wouldn't need it.



> +
>  class CameraSensorHelperOv8865 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> index e5f27f06..d0757c15 100644
> --- a/src/libcamera/camera_sensor_properties.cpp
> +++ b/src/libcamera/camera_sensor_properties.cpp
> @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>                                  */
>                         },
>                 } },
> +               { "m00_f_ov8858", {

And it won't be as short to support here here, I guess the whole table
could be duplicated, but actually - missing camera-sensor properties
don't cause the camera to fail to load - so I think this should simply
be kept as ov8858 only.


> +                       .unitCellSize = { 1200, 1200 },

http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf states
this should be 1120, 1120.

 >> 1.12 μm x 1.12 μm pixel with OmniBSI-3™ technology

> +                       .testPatternModes = {
> +                               { controls::draft::TestPatternModeOff, 0 },
> +                               { controls::draft::TestPatternModeColorBars, 1 },
> +                               /*
> +                                * No best corresponding test pattern for:
> +                                * 1: "Vertical Color Bar Type 1",
> +                                * 2: "Vertical Color Bar Type 2",
> +                                * 3: "Vertical Color Bar Type 3",
> +                                * 4: "Vertical Color Bar Type 4"
> +                                */
> +                       },
> +               } },
>                 { "ov8865", {
>                         .unitCellSize = { 1400, 1400 },
>                         .testPatternModes = {
> -- 
> 2.34.1
>