[libcamera-devel] [PATCH v2 4/7] libcamera: sensor: Add OV5670 camera sensor

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 06, 2020 at 07:52:44PM +0100, Jacopo Mondi wrote:
> Add OV5670CameraSensor class to handle Omnivision OV5670 image sensor
> and register it to the camera sensor factory.

s/to the/with the/

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp  |  1 +
>  src/libcamera/meson.build        |  1 +
>  src/libcamera/sensor/meson.build |  3 +++
>  src/libcamera/sensor/ov5670.cpp  | 43 ++++++++++++++++++++++++++++++++
>  src/libcamera/sensor/ov5670.h    | 24 ++++++++++++++++++
>  5 files changed, 72 insertions(+)
>  create mode 100644 src/libcamera/sensor/meson.build
>  create mode 100644 src/libcamera/sensor/ov5670.cpp
>  create mode 100644 src/libcamera/sensor/ov5670.h
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index fc8452b607a0..06d10295a80e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "camera_sensor.h"
> +#include "sensor/ov5670.h"

I don't think this is needed.

>  
>  #include <algorithm>
>  #include <float.h>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index ecc5b5fe4023..7dd7358b174e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -58,6 +58,7 @@ includes = [
>  
>  subdir('pipeline')
>  subdir('proxy')
> +subdir('sensor')
>  
>  libudev = dependency('libudev', required : false)
>  
> diff --git a/src/libcamera/sensor/meson.build b/src/libcamera/sensor/meson.build
> new file mode 100644
> index 000000000000..7af70370cf5c
> --- /dev/null
> +++ b/src/libcamera/sensor/meson.build
> @@ -0,0 +1,3 @@
> +libcamera_sources += files([
> +    'ov5670.cpp',
> +])
> diff --git a/src/libcamera/sensor/ov5670.cpp b/src/libcamera/sensor/ov5670.cpp
> new file mode 100644
> index 000000000000..de6011875a2d
> --- /dev/null
> +++ b/src/libcamera/sensor/ov5670.cpp
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.

Happy new year :-)

> + *
> + * ov5670.cpp - OV5670 camera sensor
> + */
> +
> +#include "ov5670.h"
> +#include "camera_sensor.h"
> +
> +/**
> + * \file ov5670.h
> + * \brief Omnivision OV5670 image sensor handler

As for the previous patch, let's not use the name "handler".

> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class OV5670CameraSensor

Would OV5670 be a long-enough class name ?

The same way we don't document pipeline handlers, I think you can add
src/libcamera/sensor/ to the doxygen exclude list.

> + * \brief Camera sensor handler for Omnivision OV5670 image sensor
> + */
> +
> +/**
> + * \brief Retrieve the name of the sensor entity supported by the handler
> + * \return The supported sensor entity name
> + */
> +const char *OV5670CameraSensor::entityName()
> +{
> +	return "ov5670";
> +}
> +
> +/**
> + * \brief Construct the ov5670 sensor handler
> + * \param[in] entity The media entity representing the sensor
> + */
> +OV5670CameraSensor::OV5670CameraSensor(const MediaEntity *entity)
> +	: CameraSensor(entity)
> +{
> +}
> +
> +REGISTER_CAMERA_SENSOR(OV5670);
> +
> +}; /* namespace libcamera */
> diff --git a/src/libcamera/sensor/ov5670.h b/src/libcamera/sensor/ov5670.h
> new file mode 100644
> index 000000000000..4acf02a8b06b
> --- /dev/null
> +++ b/src/libcamera/sensor/ov5670.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ov5670.h - OV5670 camera sensor
> + */
> +#ifndef __LIBCAMERA_SENSOR_OV5670_H__
> +#define __LIBCAMERA_SENSOR_OV5670_H__
> +
> +#include "camera_sensor.h"
> +
> +namespace libcamera {
> +
> +class OV5670CameraSensor final : public CameraSensor
> +{
> +public:
> +	static const char *entityName();
> +
> +	OV5670CameraSensor(const MediaEntity *entity);
> +};

I think you can move this to ov5670.cpp and remove ov5670.h.

This patch will become pretty small, I'd squash it with 6/7.

> +
> +}; /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SENSOR_OV5670_H__ */

-- 
Regards,

Laurent Pinchart