[libcamera-devel] [PATCH v5 09/10] libcamera: camera_sensor: Initialize VIMC properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 05, 2021 at 01:31:27PM +0100, Jacopo Mondi wrote:
> The VIMC driver does not yet support all the features required
> for all sensor drivers. As it is the main testing platforms and the
> driver changes might take a long time to land in the developments
> and testing platforms, temporary close the gap by skipping driver
> validation and initializing properties with static information such
> as the sensor resolution.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index de6a0202d19a..759a12d16f72 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -70,6 +70,7 @@ protected:
>  private:
>  	int generateId();
>  	int validateSensorDriver();
> +	void initVIMCDefaultProperties();

s/VIMC/Vimc/

>  	int initProperties();
>  
>  	const MediaEntity *entity_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 0e8aff27b712..046474c03f4a 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/media_device.h"
>  
>  #include <algorithm>
>  #include <float.h>
> @@ -207,6 +208,21 @@ int CameraSensor::init()
>  	 */
>  	resolution_ = sizes_.back();
>  
> +	/*
> +	 * VIMC is a bit special, as it does not yet support all the
> +	 * mandatory requirements regular sensors have to respect.
> +	 *
> +	 * Do not validate the driver if it's VIMC and initialize the
> +	 * the sensor properties with static information.

s/the the/the/

(You could also reflow the text up to 80 columns)

> +	 *
> +	 * \todo Remove the special case once the VIMC driver has been
> +	 * updated in all test platforms.
> +	 */
> +	if (entity_->device()->driver() == "vimc") {
> +		initVIMCDefaultProperties();

You could have inlined the function here as it's really small, but it's
no big deal.

Did you btw implement the corresponding changes in vimc yet ? If not I
can help.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		return initProperties();
> +	}
> +
>  	ret = validateSensorDriver();
>  	if (ret)
>  		return ret;
> @@ -306,6 +322,17 @@ int CameraSensor::validateSensorDriver()
>  	return 0;
>  }
>  
> +/*
> + * \brief Initialize properties that cannot be intialized by the
> + * regular initProperties() function for VIMC
> + */
> +void CameraSensor::initVIMCDefaultProperties()
> +{
> +	pixelArraySize_ = resolution();
> +	activeArea_ = Rectangle(pixelArraySize_);
> +	analogueCrop_ = activeArea_;
> +}
> +
>  int CameraSensor::initProperties()
>  {
>  	/*

-- 
Regards,

Laurent Pinchart