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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Aug 04, 2020 at 09:31:54AM +0200, Jacopo Mondi wrote:
> 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

s/nodes/node's/

> > 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

Isn't "method" an appropriate term too ?

> > 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()' ?

I also think it would be better.

Niklas, conceptual question, would it make sense to move this
implementation to the sysfs enumerator, keeping the function purely
virtual in the base class ?

> > +
> >  	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

I'd talk about character device instead of video device, as this is
generic.

Except it's not now that I read the implementation below :-) I should
have thought about it, but how about

std::string DeviceEnumerator::lookupSysfsPath(const std::string &deviceNode)
{
	struct stat st;
	int ret = stat(deviceNode.c_str(), &st);
	if (ret < 0) {
		LOG(DeviceEnumerator, Error)
			<< "Unable to stat '" << deviceNode << "'";
		return {};
	}

	std::ostringstream dev("/sys/dev/char/", std::ios_base::ate);
	dev << major(st.st_rdev) << ":" << minor(st.st_rdev);
	return dev.str();
}

This should work in all cases and wouldn't require udev at all. The code
could even be moved to V4L2Device::devicePath(), as it's the only user.
I think this would have my preference for now.

Another option is to add a sysfs.cpp file for this function and
tryLookupFirmwareID(). No need to add classes there, a simple
libcamera::sysfs namespace would do (maybe we'll refactor that in a
CharDev class later, if the need arises).

> > + * \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 */

-- 
Regards,

Laurent Pinchart