| Message ID | 20200803211733.1037019-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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 */
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); + 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 + * \param[in] deviceNode device node to lookup + * + * Lookup \a deviceNode sysfs interface. + * + * \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 */
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/internal/device_enumerator.h | 2 ++ src/libcamera/device_enumerator.cpp | 14 ++++++++++++++ 2 files changed, 16 insertions(+)