[libcamera-devel] [PATCH v2 4/5] libcamera: camera_sensor: Add controls() method

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Dec 28, 2020 at 05:55:59PM +0100, Jacopo Mondi wrote:
> Add a controls() method to retrieve the map of libcamera controls
> registered by the CameraSensor class.
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h | 1 +
>  src/libcamera/camera_sensor.cpp            | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 841c7f4bef0f..10877b9d1d9d 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -62,6 +62,7 @@ public:
>  	int setControls(ControlList *ctrls);
>  
>  	const ControlList &properties() const { return properties_; }
> +	const ControlInfoMap &controls() const { return controls_; }
>  	int sensorInfo(CameraSensorInfo *info) const;
>  
>  protected:
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 337d73c524bf..ea57943647b1 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -653,6 +653,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
>   * \return The list of camera sensor properties
>   */
>  
> +/**
> + * \fn CameraSensor::controls()
> + * \brief Retrieve the camera sensor controls info

This needs to describe what those controls are. You're using libcamera
controls at the moment, which is OK as an interim measure, but we'll
need to define sensor-specific controls sooner than latter. This should
be mentioned, with a warning stating that those controls must not be
copied verbatim to the controls exposed by the Camera to applications.
Similarly, the subdevControls() documentation should state that the
function is deprecated.

This brings a second question, which is whether using controls for this
is actually the right API. There's little that the camera sensor needs
to expose, and a specific API may make more sense.

> + * \return The map of camera sensor controls info
> + */
> +
>  /**
>   * \brief Write controls to the sensor
>   * \param[in] ctrls The list of controls to write

-- 
Regards,

Laurent Pinchart