[libcamera-devel] [PATCH v4 1/5] libcamera: v4l2_device: Add method to lookup device path

[Kieran Bingham]

Hi Niklas,

This looks fine to me.

On 29/07/2020 10:21, Niklas Söderlund wrote:
> Add a method to lookup a V4L2 devices path in sysfs.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v3
> - s/the device path/the device path in sysfs/
> ---
>  include/libcamera/internal/v4l2_device.h |  1 +
>  src/libcamera/v4l2_device.cpp            | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index bf643f2ec966bb33..3b605aab343b3b94 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -30,6 +30,7 @@ public:
>  	int setControls(ControlList *ctrls);
>  
>  	const std::string &deviceNode() const { return deviceNode_; }
> +	std::string devicePath() const;

I wonder if it warrants a simple addition in the test/v4l2_videodevice/
but I'm not sure what it would actually do other than simply call it and
make sure 'something' returns, which might not be much added value.


But for this addition anyway:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  
>  protected:
>  	V4L2Device(const std::string &deviceNode);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 56ea1ddda2c1425f..1cb8181ab6fd5a8d 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <fcntl.h>
>  #include <iomanip>
> +#include <limits.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <sys/syscall.h>
> @@ -350,6 +351,29 @@ int V4L2Device::setControls(ControlList *ctrls)
>  	return ret;
>  }
>  
> +/**
> + * \brief Retrieve the device path in sysfs
> + *
> + * The device path in sysfs describes the device backing the V4L2 device.
> + *
> + * \todo When switching to C++17 use std::filesystem:: in the implementation.
> + * \todo Query udev for this information as the path created for vdev here
> + * migh not be correct.
> + *
> + * \return The device path in sysfs
> + */
> +std::string V4L2Device::devicePath() const
> +{
> +	std::string vdev = basename(deviceNode_.c_str());
> +	std::string sysfs = "/sys/class/video4linux/" + vdev + "/device";
> +
> +	char path[PATH_MAX];
> +	if (!realpath(sysfs.c_str(), path))
> +		LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_;
> +
> +	return path;
> +}
> +
>  /**
>   * \brief Perform an IOCTL system call on the device node
>   * \param[in] request The IOCTL request code
> 

-- 
Regards
--
Kieran