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

9055 diff mbox series

Add a method to lookup a V4L2 devices path in sysfs.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@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(+)

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@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@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@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
>

Hi Kieran,

Thanks for your feedback.

On 2020-07-29 11:28:12 +0100, Kieran Bingham wrote:
> 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@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@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.

The error is fatal so if nothing is returned we would terminate 
execution ;-)

> 
> 
> But for this addition anyway:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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