[libcamera-devel] [PATCH v7 4/9] libcamera: camera_sensor: Generate a sensor ID

[Laurent Pinchart]

Hi Niklas,

Another comment I'm afraid.

On Tue, Aug 04, 2020 at 09:22:12PM +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Aug 04, 2020 at 06:13:53PM +0200, Niklas Söderlund wrote:
> > The ID is generated from information in the firmware description of the
> > sensor if available or from module and model information if the sensor
> > is virtual (for example VIMC).
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > ---
> > * Changes sinve v6
> > - Do not allow for subclassing
> > - Add support for virtual sensors
> > - Use new sysfs:: helper.
> > 
> > * Changes since v5
> > - Use new utils:: helper.
> > - Allow for subclassing
> > 
> > * Changes since v4
> > - Fix spelling.
> > 
> > * Changes since v3
> > - Update commit message.
> > - Add description of how ID are generated to comment.
> > ---
> >  include/libcamera/internal/camera_sensor.h |  3 ++
> >  src/libcamera/camera_sensor.cpp            | 38 ++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 06c8292ca30129de..0f65fca46877e89a 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -67,11 +67,14 @@ protected:
> >  	std::string logPrefix() const override;
> >  
> >  private:
> > +	int generateId();
> > +
> >  	const MediaEntity *entity_;
> >  	std::unique_ptr<V4L2Subdevice> subdev_;
> >  	unsigned int pad_;
> >  
> >  	std::string model_;
> > +	std::string id_;
> >  
> >  	V4L2Subdevice::Formats formats_;
> >  	Size resolution_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 350f49accad99c7b..3952d16b053ae20e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -9,6 +9,7 @@
> >  
> >  #include <algorithm>
> >  #include <float.h>
> > +#include <fstream>
> >  #include <iomanip>
> >  #include <limits.h>
> >  #include <math.h>
> > @@ -16,7 +17,9 @@
> >  
> >  #include <libcamera/property_ids.h>
> >  
> > +#include "libcamera/internal/file.h"
> >  #include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/utils.h"
> >  
> >  /**
> > @@ -204,6 +207,11 @@ int CameraSensor::init()
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	/* Generate a unique ID for the sensor. */
> > +	ret = generateId();
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* Retrieve and store the camera sensor properties. */
> >  	const ControlInfoMap &controls = subdev_->controls();
> >  	int32_t propertyValue;
> > @@ -541,4 +549,34 @@ std::string CameraSensor::logPrefix() const
> >  	return "'" + entity_->name() + "'";
> >  }
> >  
> > +int CameraSensor::generateId()
> > +{
> > +	const std::string devPath = subdev_->devicePath();
> > +
> > +	/* Try to get ID from firmware description. */
> > +	if (!sysfs::firmwareId(devPath, &id_))
> > +		return 0;
> > +
> > +	/* Virtual sensors not described in firmware, use modalias and model. */
> > +	std::string path = devPath + "/modalias";
> > +	if (File::exists(path)) {
> > +		std::ifstream file(path.c_str());
> > +
> > +		if (!file.is_open()) {
> > +			LOG(CameraSensor, Error) << "Failed to read modalias";
> > +			return -EINVAL;
> > +		}
> > +
> > +		std::string modalias;
> > +		std::getline(file, modalias);
> > +		file.close();
> > +
> > +		id_ = modalias + " " + model();
> > +		return 0;
> > +	}

This won't generate a unique name if the same driver handles multiple
devices. Would it make sense to instead use devPath stripped of the
/sys/devices/ prefix ? That would return platform/vimc.0 for vimc for
instance. If you want to restrict this to platform devices, you could
further check that the ID starts with platform/.

> > +
> > +	LOG(CameraSensor, Error) << "Can't generate sensor ID";
> > +	return -EINVAL;
> > +}
> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart