<div dir="ltr"><div dir="ltr"><div><div dir="ltr" class="gmail_signature"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">> </font><span style="color:rgb(34,34,34)">So accepting 'm00_f_ov8858' at all ... seems to be really tricky.</span></div><div style="color:rgb(136,136,136);font-size:small"><span style="color:rgb(34,34,34)"><br></span></div><div style="font-size:small">What I was trying to say in my earlier email is that the only requirement for the v4l2 "name" field be unique, although there are conventions. See comments above the regex logic in libcamera/src/libcamera/v4l2_subdevice.cpp.</div><div style="font-size:small"><br></div><div style="font-size:small">The current driver isn't doing anything that breaks the rules here as far as I can tell.</div><div style="font-size:small"><br></div><div style="font-size:small">A quick fix might be to modify the regex. A better fix might be to duplicate the logic in the Android HAL, which correctly reports the sensor as an "ov8858". Modifying the driver is an option as well, but for reasons pointed out above that will be problematic due to the downstream convention.</div><div style="font-size:small"><br></div><div style="font-size:small">-Nicholas</div></div></div></div></div></div></div><br></div><br><img width="0" height="0" class="mailtrack-img" alt="" style="display:flex" src="https://mailtrack.io/trace/mail/9aeb27fb0ef376149c18d30c644f1e6b2afdddbd.png?u=4192442"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 31, 2022 at 8:58 AM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Nicholas,<br>
<br>
Quoting Nicholas Roth via libcamera-devel (2022-10-30 23:05:00)<br>
> Currently, libcamera does not have information for the ov8858 sensor<br>
> used in the PinePhone Pro, a phone designed to run Linux.<br>
> <br>
> This commit adds metadata, especially that sensor gain is reported and<br>
> set in 1/16 discrete increments.<br>
> <br>
> For more information, see "5.8 manual exposure compensation/ manual<br>
> gain compensation" in [0] and the driver in [1].<br>
> <br>
> [0] <a href="http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf" rel="noreferrer" target="_blank">http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf</a><br>
> [1] <a href="https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c" rel="noreferrer" target="_blank">https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c</a><br>
> <br>
> Signed-off-by: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
> ---<br>
> src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++<br>
> src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++<br>
> 2 files changed, 25 insertions(+)<br>
> <br>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp<br>
> index 35056bec..f2040cbd 100644<br>
> --- a/src/ipa/libipa/camera_sensor_helper.cpp<br>
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp<br>
> @@ -476,6 +476,17 @@ public:<br>
> };<br>
> REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)<br>
> <br>
> +class CameraSensorHelperOv8858 : public CameraSensorHelper<br>
> +{<br>
> +public:<br>
> + CameraSensorHelperOv8858()<br>
> + {<br>
> + gainType_ = AnalogueGainLinear;<br>
> + gainConstants_.linear = { 1, 0, 0, 16 };<br>
> + }<br>
> +};<br>
> +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)<br>
<br>
"My OV8858 is the second on the rear." Suddenly this doesn't work.<br>
- So we can only use "ov8858" here.<br>
<br>
We 'could' add a dupliate entry as <br>
<br>
+/*<br>
+ * \todo: Deprecated: The PinePhonePro has devices listed as m00_f_ov8858.<br>
+ * This entry should be removed as soon as the ov8858 driver is<br>
+ * accepted into a mainline kernel.<br>
+ */<br>
+REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)<br>
+REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)<br>
<br>
But the problem there, is that suddenly users will find it 'works' until<br>
the driver gets into mainline, when it breaks. Given that the correct<br>
approach at that point will be to update to use the mainline driver,<br>
perhaps that's ok ... but I fear potentially unhappy users.<br>
<br>
And perhaps also worryingly, it removes the impetus to get the driver<br>
mainlined!<br>
<br>
So accepting 'm00_f_ov8858' at all ... seems to be really tricky.<br>
<br>
With just ov8858, and the 1.12 fix below this patch could probably go in<br>
quite rapidly though. You could easily have the <br>
<br>
+REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)<br>
<br>
line locally while developing, but I'd expect that your device should be<br>
quickly moving along with the mainline integration, so you probably<br>
wouldn't need it.<br>
<br>
<br>
<br>
> +<br>
> class CameraSensorHelperOv8865 : public CameraSensorHelper<br>
> {<br>
> public:<br>
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp<br>
> index e5f27f06..d0757c15 100644<br>
> --- a/src/libcamera/camera_sensor_properties.cpp<br>
> +++ b/src/libcamera/camera_sensor_properties.cpp<br>
> @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen<br>
> */<br>
> },<br>
> } },<br>
> + { "m00_f_ov8858", {<br>
<br>
And it won't be as short to support here here, I guess the whole table<br>
could be duplicated, but actually - missing camera-sensor properties<br>
don't cause the camera to fail to load - so I think this should simply<br>
be kept as ov8858 only.<br>
<br>
<br>
> + .unitCellSize = { 1200, 1200 },<br>
<br>
<a href="http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf" rel="noreferrer" target="_blank">http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf</a> states<br>
this should be 1120, 1120.<br>
<br>
>> 1.12 μm x 1.12 μm pixel with OmniBSI-3™ technology<br>
<br>
> + .testPatternModes = {<br>
> + { controls::draft::TestPatternModeOff, 0 },<br>
> + { controls::draft::TestPatternModeColorBars, 1 },<br>
> + /*<br>
> + * No best corresponding test pattern for:<br>
> + * 1: "Vertical Color Bar Type 1",<br>
> + * 2: "Vertical Color Bar Type 2",<br>
> + * 3: "Vertical Color Bar Type 3",<br>
> + * 4: "Vertical Color Bar Type 4"<br>
> + */<br>
> + },<br>
> + } },<br>
> { "ov8865", {<br>
> .unitCellSize = { 1400, 1400 },<br>
> .testPatternModes = {<br>
> -- <br>
> 2.34.1<br>
><br>
</blockquote></div></div>