[libcamera-devel] [PATCH 1/4] libcamera: device_enumerator: Move lookupDeviceNode() to child classes

[Kieran Bingham]

Hi Laurent,

On 12/09/2019 21:03, Laurent Pinchart wrote:
> The lookupDeviceNode() method is declared as pure virtual in the base
> DeviceEnumerator class, but is only called by derived classes. Move it
> to the DeviceEnumeratorSysfs and DeviceEnumeratorUdev. This allows
> changing the udev version to take a dev_t instead of separate
> major/minor, as that's what both the caller and the callee end up using.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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

> ---
>  src/libcamera/device_enumerator.cpp           | 13 -------------
>  src/libcamera/device_enumerator_sysfs.cpp     | 11 +++++++++++
>  src/libcamera/device_enumerator_udev.cpp      | 19 ++++++++++++-------
>  src/libcamera/include/device_enumerator.h     |  2 --
>  .../include/device_enumerator_sysfs.h         |  2 +-
>  .../include/device_enumerator_udev.h          |  2 +-
>  6 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index e76438afdd65..0b596bcec363 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -306,17 +306,4 @@ std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)
>  	return nullptr;
>  }
>  
> -/**
> - * \fn DeviceEnumerator::lookupDeviceNode(int major, int minor)
> - * \brief Lookup device node path from device number
> - * \param[in] major The device major number
> - * \param[in] minor The device minor number
> - *
> - * Translate a device number given as \a major and \a minor to a device node
> - * path.
> - *
> - * \return the device node path on success, or an empty string if the lookup
> - * fails
> - */
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> index 78a7da8d79e5..ad26affb38a3 100644
> --- a/src/libcamera/device_enumerator_sysfs.cpp
> +++ b/src/libcamera/device_enumerator_sysfs.cpp
> @@ -112,6 +112,17 @@ int DeviceEnumeratorSysfs::populateMediaDevice(const std::shared_ptr<MediaDevice
>  	return 0;
>  }
>  
> +/**
> + * \brief Lookup device node path from device number
> + * \param[in] major The device major number
> + * \param[in] minor The device minor number
> + *
> + * Translate a device number given as \a major and \a minor to a device node
> + * path.
> + *
> + * \return The device node path on success, or an empty string if the lookup
> + * fails
> + */
>  std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor)
>  {
>  	std::string deviceNode;
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 40853e77902f..e0c646c997de 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -178,10 +178,9 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
>  		if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)
>  			continue;
>  
> -		std::string deviceNode = lookupDeviceNode(entity->deviceMajor(),
> -							  entity->deviceMinor());
>  		dev_t devnum = makedev(entity->deviceMajor(),
>  				       entity->deviceMinor());
> +		std::string deviceNode = lookupDeviceNode(devnum);
>  
>  		/* Take device from orphan list first, if it is in the list. */
>  		if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {
> @@ -205,14 +204,21 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
>  	return pendingNodes;
>  }
>  
> -std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)
> +/**
> + * \brief Lookup device node path from device number
> + * \param[in] devnum The device number
> + *
> + * Translate a device number given as \a devnum to a device node path.
> + *
> + * \return The device node path on success, or an empty string if the lookup
> + * fails
> + */
> +std::string DeviceEnumeratorUdev::lookupDeviceNode(dev_t devnum)
>  {
>  	struct udev_device *device;
>  	const char *name;
> -	dev_t devnum;
>  	std::string deviceNode = std::string();
>  
> -	devnum = makedev(major, minor);
>  	device = udev_device_new_from_devnum(udev_, 'c', devnum);
>  	if (!device)
>  		return std::string();
> @@ -246,8 +252,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
>  		return 0;
>  	}
>  
> -	std::string deviceNode = lookupDeviceNode(entity->deviceMajor(),
> -						  entity->deviceMinor());
> +	std::string deviceNode = lookupDeviceNode(devnum);
>  	if (deviceNode.empty())
>  		return -EINVAL;
>  
> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> index c5d26f1a883e..770f42772270 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -50,8 +50,6 @@ protected:
>  
>  private:
>  	std::vector<std::shared_ptr<MediaDevice>> devices_;
> -
> -	virtual std::string lookupDeviceNode(int major, int minor) = 0;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h
> index 242b22b289b0..9063f6a75e11 100644
> --- a/src/libcamera/include/device_enumerator_sysfs.h
> +++ b/src/libcamera/include/device_enumerator_sysfs.h
> @@ -24,7 +24,7 @@ public:
>  
>  private:
>  	int populateMediaDevice(const std::shared_ptr<MediaDevice> &media);
> -	std::string lookupDeviceNode(int major, int minor) final;
> +	std::string lookupDeviceNode(int major, int minor);
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h
> index 5bdcdea65c35..fb7cac8011a1 100644
> --- a/src/libcamera/include/device_enumerator_udev.h
> +++ b/src/libcamera/include/device_enumerator_udev.h
> @@ -47,7 +47,7 @@ private:
>  
>  	int addUdevDevice(struct udev_device *dev);
>  	int populateMediaDevice(const std::shared_ptr<MediaDevice> &media);
> -	std::string lookupDeviceNode(int major, int minor) final;
> +	std::string lookupDeviceNode(dev_t devnum);
>  
>  	int addV4L2Device(dev_t devnum);
>  	void udevNotify(EventNotifier *notifier);
> 

-- 
Regards
--
Kieran