[libcamera-devel] [PATCH v3] libcamera: Add OV5647 sensor properties

[Kieran Bingham]

Hi Vedant,

On 21/06/2021 15:17, Vedant Paranjape wrote:
> Brief specifications available at
> https://cdn.sparkfun.com/datasheets/Dev/RaspberryPi/ov5647_full.pdf
> 
>> pixel size: 1.4 μm x 1.4 μm
> 
> Change in this patch is referenced from Page 5: key specifications
> section of the above linked pdf
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> ---
>  src/libcamera/camera_sensor_properties.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> index f660743a..43030e8b 100644
> --- a/src/libcamera/camera_sensor_properties.cpp
> +++ b/src/libcamera/camera_sensor_properties.cpp
> @@ -81,6 +81,11 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ 1, controls::draft::TestPatternModeColorBars },
>  			},
>  		} },
> +		{ "ov5647", {
> +			.unitCellSize = { 1400, 1400 },
> +			/* \todo fill test pattern modes for ov5647. */

There was discussion on this:

>> So the todo is likely sufficient, but I'd be more explicit saying there
>> are no current supported test modes in the kernel.
> 
> I'd say there's no \todo needed in that case, if the driver doesn't
> support test patterns, the implementation here is enough.


So - it's either, remove the todo, or make the comment explicit that
there is no current test pattern support in the kernel, which ever feels
most appropriate to you.


Either of those is fine. But a todo that won't get done due to lacking
kernel support is a bit meaningless otherwise.

Also, I had given a tag on your first patch.

When posting a new revision, you should collect the tags given and add
them to your commit so they don't get lost.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



> +			.testPatternModes = {},
> +		} },
>  		{ "ov5693", {
>  			.unitCellSize = { 1400, 1400 },
>  			.testPatternModes = {
>