[libcamera-devel] [PATCH v7 2/5] libcamera: add model() for retriving model name in V4L2Subdevice

[Laurent Pinchart]

Hi Han-lin,

Thank you for the patch.

On Thu, Dec 02, 2021 at 10:03:14PM +0800, Han-Lin Chen wrote:
> CameraSensor retrives model name from media entity. Move the heuristics
> method into V4L2Subdevice, so CameraLens can reuse the function.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  4 ++
>  src/libcamera/camera_sensor.cpp             | 32 +---------------
>  src/libcamera/v4l2_subdevice.cpp            | 42 +++++++++++++++++++++
>  3 files changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 97b89fb9..763c4dec 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -61,6 +61,8 @@ public:
>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  		      Whence whence = ActiveFormat);
>  
> +	std::string model();

Let's make it

	const std::string &model();

to avoid copies.

> +
>  	static std::unique_ptr<V4L2Subdevice>
>  	fromEntityName(const MediaDevice *media, const std::string &entity);
>  
> @@ -75,6 +77,8 @@ private:
>  					    unsigned int code);
>  
>  	const MediaEntity *entity_;
> +
> +	std::string model_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..6151b32e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -13,7 +13,6 @@
>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
> -#include <regex>
>  #include <string.h>
>  
>  #include <libcamera/property_ids.h>
> @@ -347,34 +346,7 @@ void CameraSensor::initTestPatternModes(
>  
>  int CameraSensor::initProperties()
>  {
> -	/*
> -	 * Extract the camera sensor model name from the media entity name.
> -	 *
> -	 * There is no standardized naming scheme for sensor entities in the
> -	 * Linux kernel at the moment.
> -	 *
> -	 * - The most common rule, used by I2C sensors, associates the model
> -	 *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> -	 *
> -	 * - When the sensor exposes multiple subdevs, the model name is
> -	 *   usually followed by a function name, as in the smiapp driver (e.g.
> -	 *   'jt8ew9 pixel_array 0-0010').
> -	 *
> -	 * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> -	 *
> -	 * Other schemes probably exist. As a best effort heuristic, use the
> -	 * part of the entity name before the first space if the name contains
> -	 * an I2C address, and use the full entity name otherwise.
> -	 */
> -	std::string entityName = entity_->name();
> -	std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> -	std::smatch match;
> -
> -	if (std::regex_search(entityName, match, i2cRegex))
> -		model_ = entityName.substr(0, entityName.find(' '));
> -	else
> -		model_ = entityName;
> -
> +	model_ = subdev_->model();
>  	properties_.set(properties::Model, utils::toAscii(model_));
>  
>  	/* Generate a unique ID for the sensor. */
> @@ -832,7 +804,7 @@ int CameraSensor::generateId()
>  	/*
>  	 * Virtual sensors not described in firmware
>  	 *
> -	 * Verify it's a platform device and construct ID from the deive path
> +	 * Verify it's a platform device and construct ID from the device path
>  	 * and model of sensor.
>  	 */
>  	if (devPath.find("/sys/devices/platform/", 0) == 0) {
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 023e2328..17ec9fdf 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <fcntl.h>
>  #include <iomanip>
> +#include <regex>
>  #include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
> @@ -442,6 +443,47 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  	return 0;
>  }
>  
> +/**
> + * \brief Retrieve the model name
> + * \return The model name of the device

As there's no concept of model name in the V4L2 subdev API, I think this
would benefit for some more information.

/**
 * \brief Retrieve the model name of the device
 *
 * The model name allows identification of the specific device model. This can
 * be used to infer device characteristics, for instance to determine the
 * analogue gain model of a camera sensor based on the sensor model name.
 *
 * Neither the V4L2 API nor the Media Controller API expose an explicit model
 * name. This function implements a heuristics to extract the model name from
 * the subdevice's entity name. This should produce accurate results for
 * I2C-based devices. If the heuristics can't match a known naming pattern,
 * the function returns the full entity name.
 *
 * \return The model name of the device
 */

> + */
> +std::string V4L2Subdevice::model()
> +{
> +	if (!model_.empty())
> +		return model_;
> +
> +	/*
> +	 * Extract model name from the media entity name.
> +	 *
> +	 * There is no standardized naming scheme for sensor entities in the
> +	 * Linux kernel at the moment.

It's not just sensors anymore, s/sensor/sensor or other/

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

> +	 *
> +	 * - The most common rule, used by I2C sensors, associates the model
> +	 *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> +	 *
> +	 * - When the sensor exposes multiple subdevs, the model name is
> +	 *   usually followed by a function name, as in the smiapp driver (e.g.
> +	 *   'jt8ew9 pixel_array 0-0010').
> +	 *
> +	 * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> +	 *
> +	 * Other schemes probably exist. As a best effort heuristic, use the
> +	 * part of the entity name before the first space if the name contains
> +	 * an I2C address, and use the full entity name otherwise.
> +	 */
> +	std::string entityName = entity_->name();
> +	std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> +	std::smatch match;
> +
> +	std::string model;
> +	if (std::regex_search(entityName, match, i2cRegex))
> +		model_ = entityName.substr(0, entityName.find(' '));
> +	else
> +		model_ = entityName;
> +
> +	return model_;
> +}
> +
>  /**
>   * \brief Create a new video subdevice instance from \a entity in media device
>   * \a media

-- 
Regards,

Laurent Pinchart