[libcamera-devel] [PATCH v6 1/9] libcamera: device_enumerator: Add method to lookup sysfs path

[Jacopo Mondi]

Hi Niklas,

On Mon, Aug 03, 2020 at 11:17:25PM +0200, Niklas Söderlund wrote:
> Add an implementation to lookup a device nodes sysfs interface path
> based on the assumption that device name under /dev matches the one
> under /sysfs. Subclasses of the DeviceEnumerator may override this to
> create more clever methods to lookup the path.

s/method/implementation

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/internal/device_enumerator.h |  2 ++
>  src/libcamera/device_enumerator.cpp            | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h
> index a9850400229d2b53..11961f4f9304f6d7 100644
> --- a/include/libcamera/internal/device_enumerator.h
> +++ b/include/libcamera/internal/device_enumerator.h
> @@ -45,6 +45,8 @@ public:
>
>  	std::shared_ptr<MediaDevice> search(const DeviceMatch &dm);
>
> +	virtual std::string lookupSysfsPath(const std::string &deviceNode);

That's a quite peculiar name I would say. Any reason I have missed why
you retained the 'lookup' part ? As we don't have 'get' in getters,
I'm not sure if 'lookup' should be there.

Why not just 'sysfsPath()' ?

> +
>  	Signal<> devicesAdded;
>
>  protected:
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index 647974b17341f44b..ef2451ac29159bf0 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -318,4 +318,18 @@ std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)
>  	return nullptr;
>  }
>
> +/**
> + * \brief Lookup a nodes sysfs path

Lookup a video device node path in sysfs

> + * \param[in] deviceNode device node to lookup

The video device node path in /dev/

> + *
> + * Lookup \a deviceNode sysfs interface.

I know nothing about sysfs, but to me a device node has several
representation in sysfs, by device hierarchy, by class, by device node
devnums etc.

This one returns a very precise one, I would mention that

> + *
> + * \return Path in sysfs
> + */
> +std::string DeviceEnumerator::lookupSysfsPath(const std::string &deviceNode)
> +{
> +	std::string dev = basename(deviceNode.c_str());
> +	return "/sys/class/video4linux/" + dev;
> +}
> +
>  } /* namespace libcamera */
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel