[libcamera-devel,2/3] libcamera: MediaDevice: Create entities with major and minor numbers

128 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

Extend the MediaEntity object with device node major and minor numbers,
and retrieve them from the media graph using interfaces. They will be
used by the DeviceEnumerator to retrieve the devnode path.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/include/media_device.h |  2 +
 src/libcamera/include/media_object.h |  9 +++-
 src/libcamera/media_device.cpp       | 65 +++++++++++++++++++++++++++-
 src/libcamera/media_object.cpp       | 46 ++++++++++++++++++--
 4 files changed, 116 insertions(+), 6 deletions(-)

On Wednesday, 2 January 2019 02:49:02 EET Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Extend the MediaEntity object with device node major and minor numbers,
> and retrieve them from the media graph using interfaces. They will be
> used by the DeviceEnumerator to retrieve the devnode path.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/include/media_device.h |  2 +
>  src/libcamera/include/media_object.h |  9 +++-
>  src/libcamera/media_device.cpp       | 65 +++++++++++++++++++++++++++-
>  src/libcamera/media_object.cpp       | 46 ++++++++++++++++++--
>  4 files changed, 116 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h
> b/src/libcamera/include/media_device.h index 3fcdb4b4d5f8..8d491a87867c
> 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -54,6 +54,8 @@ private:
> 
>  	std::vector<MediaEntity *> entities_;
> 
> +	struct media_v2_interface *findInterface(const struct media_v2_topology
> &topology, +						 unsigned int entityId);
>  	bool populateEntities(const struct media_v2_topology &topology);
>  	bool populatePads(const struct media_v2_topology &topology);
>  	bool populateLinks(const struct media_v2_topology &topology);
> diff --git a/src/libcamera/include/media_object.h
> b/src/libcamera/include/media_object.h index 65b55085a3b0..950a33286690
> 100644
> --- a/src/libcamera/include/media_object.h
> +++ b/src/libcamera/include/media_object.h
> @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject
>  {
>  public:
>  	const std::string &name() const { return name_; }
> +	unsigned int major() const { return major_; }
> +	unsigned int minor() const { return minor_; }
> 
>  	const std::vector<MediaPad *> &pads() const { return pads_; }
> 
>  	const MediaPad *getPadByIndex(unsigned int index) const;
>  	const MediaPad *getPadById(unsigned int id) const;
> 
> +	int setDeviceNode(const std::string &devnode);
> +
>  private:
>  	friend class MediaDevice;
> 
> -	MediaEntity(const struct media_v2_entity *entity);
> +	MediaEntity(const struct media_v2_entity *entity,
> +		    unsigned int major = 0, unsigned int minor = 0);
>  	MediaEntity(const MediaEntity &) = delete;
>  	~MediaEntity();
> 
>  	std::string name_;
>  	std::string devnode_;
> +	unsigned int major_;
> +	unsigned int minor_;
> 
>  	std::vector<MediaPad *> pads_;
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 605e504be124..70b3fff3f492 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -208,6 +208,7 @@ int MediaDevice::populate()
>  	struct media_v2_entity *ents = nullptr;
>  	struct media_v2_link *links = nullptr;
>  	struct media_v2_pad *pads = nullptr;
> +	struct media_v2_interface *interfaces = nullptr;
>  	__u64 version = -1;
>  	int ret;
> 
> @@ -221,6 +222,7 @@ int MediaDevice::populate()
>  		topology.ptr_entities = reinterpret_cast<__u64>(ents);
>  		topology.ptr_links = reinterpret_cast<__u64>(links);
>  		topology.ptr_pads = reinterpret_cast<__u64>(pads);
> +		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
> 
>  		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
>  		if (ret < 0) {
> @@ -236,10 +238,12 @@ int MediaDevice::populate()
>  		delete[] links;
>  		delete[] ents;
>  		delete[] pads;
> +		delete[] interfaces;
> 
>  		ents = new media_v2_entity[topology.num_entities];
>  		links = new media_v2_link[topology.num_links];
>  		pads = new media_v2_pad[topology.num_pads];
> +		interfaces = new media_v2_interface[topology.num_interfaces];
> 
>  		version = topology.topology_version;
>  	}
> @@ -253,6 +257,7 @@ int MediaDevice::populate()
>  	delete[] links;
>  	delete[] ents;
>  	delete[] pads;
> +	delete[] interfaces;
> 
>  	if (!valid_) {
>  		clear();
> @@ -367,6 +372,45 @@ void MediaDevice::clear()
>   * \brief Global list of media entities in the media graph
>   */
> 
> +/**
> + * \brief Find the interface associated with an entity
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + * \param entityId The entity id
> + * \return A pointer to the interface if found, or nullptr otherwise
> + */
> +struct media_v2_interface *MediaDevice::findInterface(const struct
> media_v2_topology &topology, +			       unsigned int entityId)
> +{
> +	struct media_v2_link *links = reinterpret_cast<struct media_v2_link *>
> +						(topology.ptr_links);
> +	unsigned int ifaceId = -1;
> +
> +	for (unsigned int i = 0; i < topology.num_links; ++i) {
> +		/* Search for the interface to entity link. */
> +		if (links[i].sink_id != entityId)
> +			continue;
> +
> +		if ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> +		    MEDIA_LNK_FL_INTERFACE_LINK)
> +			continue;
> +
> +		ifaceId = links[i].source_id;
> +	}
> +
> +	if (ifaceId == static_cast<unsigned int>(-1))
> +		return nullptr;
> +
> +	struct media_v2_interface *ifaces = reinterpret_cast<struct
> media_v2_interface *> +						(topology.ptr_interfaces);
> +
> +	for (unsigned int i = 0; i < topology.num_interfaces; ++i) {
> +		if (ifaces[i].id == ifaceId)
> +			return &ifaces[i];
> +	}
> +
> +	return nullptr;
> +}
> +
>  /*
>   * For each entity in the media graph create a MediaEntity and store a
>   * reference in the media device objects map and entities list.
> @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct
> media_v2_topology &topology) (topology.ptr_entities);
> 
>  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> -		MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> +		/*
> +		 * Find the interface linked to this entity to get the device
> +		 * node major and minor numbers.
> +		 */
> +		struct media_v2_interface *iface =
> +			findInterface(topology, mediaEntities[i].id);
> +		if (!iface) {
> +			LOG(Error) << "Failed to find interface link for "
> +				   << "entity with id: " << mediaEntities[i].id;
> +			return false;
> +		}

The above five lines should be dropped. So much for testing patches after 
sending them out :-S

> +
> +		MediaEntity *entity;
> +		if (iface)
> +			entity = new MediaEntity(&mediaEntities[i],
> +						 iface->devnode.major,
> +						 iface->devnode.minor);
> +		else
> +			entity = new MediaEntity(&mediaEntities[i]);
> +
>  		if (!addObject(entity)) {
>  			delete entity;
>  			return false;
> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> index b64dcc3c8fb4..69e5cc74264d 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -6,9 +6,8 @@
>   */
> 
>  #include <errno.h>
> -#include <fcntl.h>
>  #include <string.h>
> -#include <sys/stat.h>
> +#include <unistd.h>
> 
>  #include <string>
>  #include <vector>
> @@ -212,6 +211,20 @@ void MediaPad::addLink(MediaLink *link)
>   * \return The entity name
>   */
> 
> +/**
> + * \fn MediaEntity::major()
> + * \brief Retrieve the major number of the interface associated with the
> entity + * \return The interface major number, or 0 if the entity isn't
> associated with + * an interface
> + */
> +
> +/**
> + * \fn MediaEntity::minor()
> + * \brief Retrieve the minor number of the interface associated with the
> entity + * \return The interface minor number, or 0 if the entity isn't
> associated with + * an interface
> + */
> +
>  /**
>   * \fn MediaEntity::pads()
>   * \brief Retrieve all pads of the entity
> @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned int
> index) const * \param id The pad id
>   * \return The pad identified by \a id, or nullptr if no such pad exist
>   */
> +
>  const MediaPad *MediaEntity::getPadById(unsigned int id) const
>  {
>  	for (MediaPad *p : pads_) {
> @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int
> id) const return nullptr;
>  }
> 
> +/**
> + * \brief Set the path to the device node for the associated interface
> + * \param devnode The interface device node path associated with this
> entity + * \return 0 on success, or a negative error code if the device
> node can't be + * accessed
> + */
> +int MediaEntity::setDeviceNode(const std::string &devnode)
> +{
> +	/* Make sure the device node can be accessed. */
> +	int ret = ::access(devnode.c_str(), R_OK | W_OK);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(Error) << "Device node " << devnode << " can't be accessed: "
> +			   << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Construct a MediaEntity
>   * \param entity The media entity kernel data
> + * \param major The major number of the entity associated interface
> + * \param minor The minor number of the entity associated interface
>   */
> -MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> -	: MediaObject(entity->id), name_(entity->name)
> +MediaEntity::MediaEntity(const struct media_v2_entity *entity,
> +			 unsigned int major, unsigned int minor)
> +	: MediaObject(entity->id), name_(entity->name),
> +	  major_(major), minor_(minor)
>  {
>  }

Hi Laurent,

On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> Extend the MediaEntity object with device node major and minor numbers,
> and retrieve them from the media graph using interfaces. They will be
> used by the DeviceEnumerator to retrieve the devnode path.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/include/media_device.h |  2 +
>  src/libcamera/include/media_object.h |  9 +++-
>  src/libcamera/media_device.cpp       | 65 +++++++++++++++++++++++++++-
>  src/libcamera/media_object.cpp       | 46 ++++++++++++++++++--
>  4 files changed, 116 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 3fcdb4b4d5f8..8d491a87867c 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -54,6 +54,8 @@ private:
>
>  	std::vector<MediaEntity *> entities_;
>
> +	struct media_v2_interface *findInterface(const struct media_v2_topology &topology,
> +						 unsigned int entityId);

To respect the 80-col, I dropped 'struct' from media_v2_ variables in
the 'populate*()' functions.

>  	bool populateEntities(const struct media_v2_topology &topology);
>  	bool populatePads(const struct media_v2_topology &topology);
>  	bool populateLinks(const struct media_v2_topology &topology);
> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h
> index 65b55085a3b0..950a33286690 100644
> --- a/src/libcamera/include/media_object.h
> +++ b/src/libcamera/include/media_object.h
> @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject
>  {
>  public:
>  	const std::string &name() const { return name_; }
> +	unsigned int major() const { return major_; }
> +	unsigned int minor() const { return minor_; }
>
>  	const std::vector<MediaPad *> &pads() const { return pads_; }
>
>  	const MediaPad *getPadByIndex(unsigned int index) const;
>  	const MediaPad *getPadById(unsigned int id) const;
>
> +	int setDeviceNode(const std::string &devnode);
> +

I'm fine adding it here, but it is not currently used.

>  private:
>  	friend class MediaDevice;
>
> -	MediaEntity(const struct media_v2_entity *entity);
> +	MediaEntity(const struct media_v2_entity *entity,
> +		    unsigned int major = 0, unsigned int minor = 0);
>  	MediaEntity(const MediaEntity &) = delete;
>  	~MediaEntity();
>
>  	std::string name_;
>  	std::string devnode_;
> +	unsigned int major_;
> +	unsigned int minor_;
>
>  	std::vector<MediaPad *> pads_;
>
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 605e504be124..70b3fff3f492 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -208,6 +208,7 @@ int MediaDevice::populate()
>  	struct media_v2_entity *ents = nullptr;
>  	struct media_v2_link *links = nullptr;
>  	struct media_v2_pad *pads = nullptr;
> +	struct media_v2_interface *interfaces = nullptr;

That's my OCD, but could you move this line 3 lines up? This gives
lines a nicer ordering

>  	__u64 version = -1;
>  	int ret;
>
> @@ -221,6 +222,7 @@ int MediaDevice::populate()
>  		topology.ptr_entities = reinterpret_cast<__u64>(ents);
>  		topology.ptr_links = reinterpret_cast<__u64>(links);
>  		topology.ptr_pads = reinterpret_cast<__u64>(pads);
> +		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);

Same here

>
>  		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
>  		if (ret < 0) {
> @@ -236,10 +238,12 @@ int MediaDevice::populate()
>  		delete[] links;
>  		delete[] ents;
>  		delete[] pads;
> +		delete[] interfaces;

Same here

>
>  		ents = new media_v2_entity[topology.num_entities];
>  		links = new media_v2_link[topology.num_links];
>  		pads = new media_v2_pad[topology.num_pads];
> +		interfaces = new media_v2_interface[topology.num_interfaces];

And here.

>
>  		version = topology.topology_version;
>  	}
> @@ -253,6 +257,7 @@ int MediaDevice::populate()
>  	delete[] links;
>  	delete[] ents;
>  	delete[] pads;
> +	delete[] interfaces;

And here.

The patch will look weird, but the resulting code will be nicer.

>
>  	if (!valid_) {
>  		clear();
> @@ -367,6 +372,45 @@ void MediaDevice::clear()
>   * \brief Global list of media entities in the media graph
>   */
>
> +/**
> + * \brief Find the interface associated with an entity
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + * \param entityId The entity id
> + * \return A pointer to the interface if found, or nullptr otherwise
> + */

This is a private function, we can document it, I agree, but leave
doxygen commands out.

> +struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topology &topology,
> +			       unsigned int entityId)
> +{
> +	struct media_v2_link *links = reinterpret_cast<struct media_v2_link *>
> +						(topology.ptr_links);

	media_v2_link *links = reinterpret_cast<media_v2_link *>
                               (topology.ptr_links);

To make it the same as in other populate*() functions

> +	unsigned int ifaceId = -1;

unsigned int initialized with -1 ?

> +
> +	for (unsigned int i = 0; i < topology.num_links; ++i) {
> +		/* Search for the interface to entity link. */
> +		if (links[i].sink_id != entityId)
> +			continue;
> +
> +		if ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> +		    MEDIA_LNK_FL_INTERFACE_LINK)
> +			continue;
> +
> +		ifaceId = links[i].source_id;

break?

> +	}
> +
> +	if (ifaceId == static_cast<unsigned int>(-1))
> +		return nullptr;
> +
> +	struct media_v2_interface *ifaces = reinterpret_cast<struct media_v2_interface *>
> +						(topology.ptr_interfaces);

drop struct here as well?

> +
> +	for (unsigned int i = 0; i < topology.num_interfaces; ++i) {
> +             if (ifaces[i].id == ifaceId)
> +			return &ifaces[i];
> +	}
> +
> +	return nullptr;
> +}
> +
>  /*
>   * For each entity in the media graph create a MediaEntity and store a
>   * reference in the media device objects map and entities list.
> @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
>  					 (topology.ptr_entities);
>
>  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> -		MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> +		/*
> +		 * Find the interface linked to this entity to get the device
> +		 * node major and minor numbers.
> +		 */
> +		struct media_v2_interface *iface =
> +			findInterface(topology, mediaEntities[i].id);
> +		if (!iface) {
> +			LOG(Error) << "Failed to find interface link for "
> +				   << "entity with id: " << mediaEntities[i].id;
> +			return false;
> +		}

As you pointed out already, it is fine to have entities without an
interface associated.

> +
> +		MediaEntity *entity;
> +		if (iface)
> +			entity = new MediaEntity(&mediaEntities[i],
> +						 iface->devnode.major,
> +						 iface->devnode.minor);
> +		else
> +			entity = new MediaEntity(&mediaEntities[i]);
> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> index b64dcc3c8fb4..69e5cc74264d 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -6,9 +6,8 @@
>   */
>
>  #include <errno.h>
> -#include <fcntl.h>
>  #include <string.h>
> -#include <sys/stat.h>
> +#include <unistd.h>
>
>  #include <string>
>  #include <vector>
> @@ -212,6 +211,20 @@ void MediaPad::addLink(MediaLink *link)
>   * \return The entity name
>   */
>
> +/**
> + * \fn MediaEntity::major()
> + * \brief Retrieve the major number of the interface associated with the entity
> + * \return The interface major number, or 0 if the entity isn't associated with
> + * an interface
> + */
> +
> +/**
> + * \fn MediaEntity::minor()
> + * \brief Retrieve the minor number of the interface associated with the entity
> + * \return The interface minor number, or 0 if the entity isn't associated with
> + * an interface
> + */
> +
>  /**
>   * \fn MediaEntity::pads()
>   * \brief Retrieve all pads of the entity
> @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned int index) const
>   * \param id The pad id
>   * \return The pad identified by \a id, or nullptr if no such pad exist
>   */
> +

Is this needed?

>  const MediaPad *MediaEntity::getPadById(unsigned int id) const
>  {
>  	for (MediaPad *p : pads_) {
> @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int id) const
>  	return nullptr;
>  }
>
> +/**
> + * \brief Set the path to the device node for the associated interface
> + * \param devnode The interface device node path associated with this entity
> + * \return 0 on success, or a negative error code if the device node can't be
> + * accessed
> + */
> +int MediaEntity::setDeviceNode(const std::string &devnode)
> +{
> +	/* Make sure the device node can be accessed. */
> +	int ret = ::access(devnode.c_str(), R_OK | W_OK);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(Error) << "Device node " << devnode << " can't be accessed: "
> +			   << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Construct a MediaEntity
>   * \param entity The media entity kernel data
> + * \param major The major number of the entity associated interface
> + * \param minor The minor number of the entity associated interface

Should we say they're defaulted to 0 ?

Thanks
   j


>   */
> -MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> -	: MediaObject(entity->id), name_(entity->name)
> +MediaEntity::MediaEntity(const struct media_v2_entity *entity,
> +			 unsigned int major, unsigned int minor)
> +	: MediaObject(entity->id), name_(entity->name),
> +	  major_(major), minor_(minor)
>  {
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:
> On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > Extend the MediaEntity object with device node major and minor numbers,
> > and retrieve them from the media graph using interfaces. They will be
> > used by the DeviceEnumerator to retrieve the devnode path.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  src/libcamera/include/media_device.h |  2 +
> >  src/libcamera/include/media_object.h |  9 +++-
> >  src/libcamera/media_device.cpp       | 65 +++++++++++++++++++++++++++-
> >  src/libcamera/media_object.cpp       | 46 ++++++++++++++++++--
> >  4 files changed, 116 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/libcamera/include/media_device.h
> > b/src/libcamera/include/media_device.h index 3fcdb4b4d5f8..8d491a87867c
> > 100644
> > --- a/src/libcamera/include/media_device.h
> > +++ b/src/libcamera/include/media_device.h
> > 
> > @@ -54,6 +54,8 @@ private:
> >  	std::vector<MediaEntity *> entities_;
> > 
> > +	struct media_v2_interface *findInterface(const struct media_v2_topology
> > &topology,
> > +						 unsigned int entityId);
> 
> To respect the 80-col, I dropped 'struct' from media_v2_ variables in
> the 'populate*()' functions.

C++ allows us to do that, but I've kept it to emphasize that we're dealing 
with kernel structures. We're free to pick one option or another, but it 
should be applied globally.

> >  	bool populateEntities(const struct media_v2_topology &topology);
> >  	bool populatePads(const struct media_v2_topology &topology);
> >  	bool populateLinks(const struct media_v2_topology &topology);
> > diff --git a/src/libcamera/include/media_object.h
> > b/src/libcamera/include/media_object.h index 65b55085a3b0..950a33286690
> > 100644
> > --- a/src/libcamera/include/media_object.h
> > +++ b/src/libcamera/include/media_object.h
> > @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject
> >  {
> >  public:
> >  	const std::string &name() const { return name_; }
> > +	unsigned int major() const { return major_; }
> > +	unsigned int minor() const { return minor_; }
> > 
> >  	const std::vector<MediaPad *> &pads() const { return pads_; }
> >  	
> >  	const MediaPad *getPadByIndex(unsigned int index) const;
> >  	const MediaPad *getPadById(unsigned int id) const;
> > 
> > +	int setDeviceNode(const std::string &devnode);
> > +
> 
> I'm fine adding it here, but it is not currently used.

I think it was added here in your original patch :-)

> >  private:
> >  	friend class MediaDevice;
> > 
> > -	MediaEntity(const struct media_v2_entity *entity);
> > +	MediaEntity(const struct media_v2_entity *entity,
> > +		    unsigned int major = 0, unsigned int minor = 0);
> >  	MediaEntity(const MediaEntity &) = delete;
> >  	~MediaEntity();
> >  	
> >  	std::string name_;
> >  	std::string devnode_;
> > +	unsigned int major_;
> > +	unsigned int minor_;
> > 
> >  	std::vector<MediaPad *> pads_;
> > 
> > diff --git a/src/libcamera/media_device.cpp
> > b/src/libcamera/media_device.cpp index 605e504be124..70b3fff3f492 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp
> > @@ -208,6 +208,7 @@ int MediaDevice::populate()
> >  	struct media_v2_entity *ents = nullptr;
> >  	struct media_v2_link *links = nullptr;
> >  	struct media_v2_pad *pads = nullptr;
> > +	struct media_v2_interface *interfaces = nullptr;
> 
> That's my OCD, but could you move this line 3 lines up? This gives
> lines a nicer ordering

I wanted to keep the same order as in the media_v2_topology structure, but I 
now realize that interfaces come just after entities (and links after pads). 
How about reordering them in that order in a follow-up patch ?

[snip]

> > @@ -367,6 +372,45 @@ void MediaDevice::clear()
> >   * \brief Global list of media entities in the media graph
> >   */
> > 
> > +/**
> > + * \brief Find the interface associated with an entity
> > + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> > + * \param entityId The entity id
> > + * \return A pointer to the interface if found, or nullptr otherwise
> > + */
> 
> This is a private function, we can document it, I agree, but leave
> doxygen commands out.

Why so ? I think we should standardize on a single syntax for documentation.

> > +struct media_v2_interface *MediaDevice::findInterface(const struct
> > media_v2_topology &topology, +			       unsigned int entityId)
> > +{
> > +	struct media_v2_link *links = reinterpret_cast<struct media_v2_link *>
> > +						(topology.ptr_links);
> 
> 	media_v2_link *links = reinterpret_cast<media_v2_link *>
>                                (topology.ptr_links);
> 
> To make it the same as in other populate*() functions

Let's discuss struct media_v2_link vs. media_v2_link globally as mentioned 
above, and update the code accordingly.

> > +	unsigned int ifaceId = -1;
> 
> unsigned int initialized with -1 ?

The compiler doesn't warn :-)

> > +
> > +	for (unsigned int i = 0; i < topology.num_links; ++i) {
> > +		/* Search for the interface to entity link. */
> > +		if (links[i].sink_id != entityId)
> > +			continue;
> > +
> > +		if ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> > +		    MEDIA_LNK_FL_INTERFACE_LINK)
> > +			continue;
> > +
> > +		ifaceId = links[i].source_id;
> 
> break?

Indeed. Fixed.

> > +	}
> > +
> > +	if (ifaceId == static_cast<unsigned int>(-1))
> > +		return nullptr;
> > +
> > +	struct media_v2_interface *ifaces = reinterpret_cast<struct
> > media_v2_interface *>
> > +						(topology.ptr_interfaces);
> 
> drop struct here as well?
> 
> > +
> > +	for (unsigned int i = 0; i < topology.num_interfaces; ++i) {
> > +             if (ifaces[i].id == ifaceId)
> > +			return &ifaces[i];
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> >  /*
> >   * For each entity in the media graph create a MediaEntity and store a
> >   * reference in the media device objects map and entities list.
> > @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct
> > media_v2_topology &topology)> 
> >  					 (topology.ptr_entities);
> >  	
> >  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> > 
> > -		MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> > +		/*
> > +		 * Find the interface linked to this entity to get the device
> > +		 * node major and minor numbers.
> > +		 */
> > +		struct media_v2_interface *iface =
> > +			findInterface(topology, mediaEntities[i].id);
> > +		if (!iface) {
> > +			LOG(Error) << "Failed to find interface link for "
> > +				   << "entity with id: " << mediaEntities[i].id;
> > +			return false;
> > +		}
> 
> As you pointed out already, it is fine to have entities without an
> interface associated.

Yes, I've dropped this check.

> > +
> > +		MediaEntity *entity;
> > +		if (iface)
> > +			entity = new MediaEntity(&mediaEntities[i],
> > +						 iface->devnode.major,
> > +						 iface->devnode.minor);
> > +		else
> > +			entity = new MediaEntity(&mediaEntities[i]);
> > diff --git a/src/libcamera/media_object.cpp
> > b/src/libcamera/media_object.cpp index b64dcc3c8fb4..69e5cc74264d 100644
> > --- a/src/libcamera/media_object.cpp
> > +++ b/src/libcamera/media_object.cpp

[snip]

> > @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned
> > int index) const
> >   * \param id The pad id
> >   * \return The pad identified by \a id, or nullptr if no such pad exist
> >   */
> > +
> 
> Is this needed?

Not at all, dropped.

> >  const MediaPad *MediaEntity::getPadById(unsigned int id) const
> >  {
> >  	for (MediaPad *p : pads_) {
> > @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int
> > id) const
> >  	return nullptr;
> >  }
> > 
> > +/**
> > + * \brief Set the path to the device node for the associated interface
> > + * \param devnode The interface device node path associated with this
> > entity + * \return 0 on success, or a negative error code if the device
> > node can't be + * accessed
> > + */
> > +int MediaEntity::setDeviceNode(const std::string &devnode)
> > +{
> > +	/* Make sure the device node can be accessed. */
> > +	int ret = ::access(devnode.c_str(), R_OK | W_OK);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		LOG(Error) << "Device node " << devnode << " can't be accessed: "
> > +			   << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Construct a MediaEntity
> >   * \param entity The media entity kernel data
> > + * \param major The major number of the entity associated interface
> > + * \param minor The minor number of the entity associated interface
> 
> Should we say they're defaulted to 0 ?

Doxygen shows the default value in the function prototype already, I don't 
think we need to duplicate that information.

> >   */
> > -MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> > -	: MediaObject(entity->id), name_(entity->name)
> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity,
> > +			 unsigned int major, unsigned int minor)
> > +	: MediaObject(entity->id), name_(entity->name),
> > +	  major_(major), minor_(minor)
> > 
> >  {
> >  }

Hi Laurent,

On Wed, Jan 02, 2019 at 12:55:00PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:
> > On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Extend the MediaEntity object with device node major and minor numbers,
> > > and retrieve them from the media graph using interfaces. They will be
> > > used by the DeviceEnumerator to retrieve the devnode path.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >
> > >  src/libcamera/include/media_device.h |  2 +
> > >  src/libcamera/include/media_object.h |  9 +++-
> > >  src/libcamera/media_device.cpp       | 65 +++++++++++++++++++++++++++-
> > >  src/libcamera/media_object.cpp       | 46 ++++++++++++++++++--
> > >  4 files changed, 116 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/media_device.h
> > > b/src/libcamera/include/media_device.h index 3fcdb4b4d5f8..8d491a87867c
> > > 100644
> > > --- a/src/libcamera/include/media_device.h
> > > +++ b/src/libcamera/include/media_device.h
> > >
> > > @@ -54,6 +54,8 @@ private:
> > >  	std::vector<MediaEntity *> entities_;
> > >
> > > +	struct media_v2_interface *findInterface(const struct media_v2_topology
> > > &topology,
> > > +						 unsigned int entityId);
> >
> > To respect the 80-col, I dropped 'struct' from media_v2_ variables in
> > the 'populate*()' functions.
>
> C++ allows us to do that, but I've kept it to emphasize that we're dealing
> with kernel structures. We're free to pick one option or another, but it
> should be applied globally.

I'm not a fan of omitting 'struct' neither, but in that case the code
was more readable and fit in 80 columns.

Let's define a rule globally. I would tend to keep struct
Please not the populate*() functions will have to be re-adjusted, I
could do that, send a patch, and then you can rebase on top, or you
could do it here.

>
> > >  	bool populateEntities(const struct media_v2_topology &topology);
> > >  	bool populatePads(const struct media_v2_topology &topology);
> > >  	bool populateLinks(const struct media_v2_topology &topology);
> > > diff --git a/src/libcamera/include/media_object.h
> > > b/src/libcamera/include/media_object.h index 65b55085a3b0..950a33286690
> > > 100644
> > > --- a/src/libcamera/include/media_object.h
> > > +++ b/src/libcamera/include/media_object.h
> > > @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject
> > >  {
> > >  public:
> > >  	const std::string &name() const { return name_; }
> > > +	unsigned int major() const { return major_; }
> > > +	unsigned int minor() const { return minor_; }
> > >
> > >  	const std::vector<MediaPad *> &pads() const { return pads_; }
> > >
> > >  	const MediaPad *getPadByIndex(unsigned int index) const;
> > >  	const MediaPad *getPadById(unsigned int id) const;
> > >
> > > +	int setDeviceNode(const std::string &devnode);
> > > +
> >
> > I'm fine adding it here, but it is not currently used.
>
> I think it was added here in your original patch :-)
>

Told you: "I'm fine adding it here"
:)


> > >  private:
> > >  	friend class MediaDevice;
> > >
> > > -	MediaEntity(const struct media_v2_entity *entity);
> > > +	MediaEntity(const struct media_v2_entity *entity,
> > > +		    unsigned int major = 0, unsigned int minor = 0);
> > >  	MediaEntity(const MediaEntity &) = delete;
> > >  	~MediaEntity();
> > >
> > >  	std::string name_;
> > >  	std::string devnode_;
> > > +	unsigned int major_;
> > > +	unsigned int minor_;
> > >
> > >  	std::vector<MediaPad *> pads_;
> > >
> > > diff --git a/src/libcamera/media_device.cpp
> > > b/src/libcamera/media_device.cpp index 605e504be124..70b3fff3f492 100644
> > > --- a/src/libcamera/media_device.cpp
> > > +++ b/src/libcamera/media_device.cpp
> > > @@ -208,6 +208,7 @@ int MediaDevice::populate()
> > >  	struct media_v2_entity *ents = nullptr;
> > >  	struct media_v2_link *links = nullptr;
> > >  	struct media_v2_pad *pads = nullptr;
> > > +	struct media_v2_interface *interfaces = nullptr;
> >
> > That's my OCD, but could you move this line 3 lines up? This gives
> > lines a nicer ordering
>
> I wanted to keep the same order as in the media_v2_topology structure, but I
> now realize that interfaces come just after entities (and links after pads).
> How about reordering them in that order in a follow-up patch ?

How about reordering them in reverse-xmas-tree order in a follow-up
patch instead :)

Ultimate bikeshedding here... I'm fine with both actually

>
> [snip]
>
> > > @@ -367,6 +372,45 @@ void MediaDevice::clear()
> > >   * \brief Global list of media entities in the media graph
> > >   */
> > >
> > > +/**
> > > + * \brief Find the interface associated with an entity
> > > + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> > > + * \param entityId The entity id
> > > + * \return A pointer to the interface if found, or nullptr otherwise
> > > + */
> >
> > This is a private function, we can document it, I agree, but leave
> > doxygen commands out.
>
> Why so ? I think we should standardize on a single syntax for documentation.
>

Ok for consistency. Be aware Doxygen does not parse them though.

> > > +struct media_v2_interface *MediaDevice::findInterface(const struct
> > > media_v2_topology &topology, +			       unsigned int entityId)
> > > +{
> > > +	struct media_v2_link *links = reinterpret_cast<struct media_v2_link *>
> > > +						(topology.ptr_links);
> >
> > 	media_v2_link *links = reinterpret_cast<media_v2_link *>
> >                                (topology.ptr_links);
> >
> > To make it the same as in other populate*() functions
>
> Let's discuss struct media_v2_link vs. media_v2_link globally as mentioned
> above, and update the code accordingly.
>
> > > +	unsigned int ifaceId = -1;
> >
> > unsigned int initialized with -1 ?
>
> The compiler doesn't warn :-)
>

So for the purpose of using this variable to check for a match, this
should read s/-1/UINT_MAX/ ?

> > > +
> > > +	for (unsigned int i = 0; i < topology.num_links; ++i) {
> > > +		/* Search for the interface to entity link. */
> > > +		if (links[i].sink_id != entityId)
> > > +			continue;
> > > +
> > > +		if ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> > > +		    MEDIA_LNK_FL_INTERFACE_LINK)
> > > +			continue;
> > > +
> > > +		ifaceId = links[i].source_id;
> >
> > break?
>
> Indeed. Fixed.
>
> > > +	}
> > > +
> > > +	if (ifaceId == static_cast<unsigned int>(-1))
> > > +		return nullptr;

I wonder what is the value of making it unsigned, and having to go
through a static cast to use -1 as UINT_MAX.

I mean, I now if you declare it as signed int, it has then to be
casted to unsigned when comparing it with ids in kernel structure, but
what about checking here for

        if (i == topology.num_links)

instead, and keep using unsigned for ifaceId (dropping the assignament and
comparison to -1) ?

> > > +
> > > +	struct media_v2_interface *ifaces = reinterpret_cast<struct
> > > media_v2_interface *>
> > > +						(topology.ptr_interfaces);
> >
> > drop struct here as well?
> >
> > > +
> > > +	for (unsigned int i = 0; i < topology.num_interfaces; ++i) {
> > > +             if (ifaces[i].id == ifaceId)
> > > +			return &ifaces[i];
> > > +	}
> > > +
> > > +	return nullptr;
> > > +}
> > > +
> > >  /*
> > >   * For each entity in the media graph create a MediaEntity and store a
> > >   * reference in the media device objects map and entities list.
> > > @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct
> > > media_v2_topology &topology)>
> > >  					 (topology.ptr_entities);
> > >
> > >  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> > >
> > > -		MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> > > +		/*
> > > +		 * Find the interface linked to this entity to get the device
> > > +		 * node major and minor numbers.
> > > +		 */
> > > +		struct media_v2_interface *iface =
> > > +			findInterface(topology, mediaEntities[i].id);
> > > +		if (!iface) {
> > > +			LOG(Error) << "Failed to find interface link for "
> > > +				   << "entity with id: " << mediaEntities[i].id;
> > > +			return false;
> > > +		}
> >
> > As you pointed out already, it is fine to have entities without an
> > interface associated.
>
> Yes, I've dropped this check.
>
> > > +
> > > +		MediaEntity *entity;
> > > +		if (iface)
> > > +			entity = new MediaEntity(&mediaEntities[i],
> > > +						 iface->devnode.major,
> > > +						 iface->devnode.minor);
> > > +		else
> > > +			entity = new MediaEntity(&mediaEntities[i]);
> > > diff --git a/src/libcamera/media_object.cpp
> > > b/src/libcamera/media_object.cpp index b64dcc3c8fb4..69e5cc74264d 100644
> > > --- a/src/libcamera/media_object.cpp
> > > +++ b/src/libcamera/media_object.cpp
>
> [snip]
>
> > > @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned
> > > int index) const
> > >   * \param id The pad id
> > >   * \return The pad identified by \a id, or nullptr if no such pad exist
> > >   */
> > > +
> >
> > Is this needed?
>
> Not at all, dropped.
>
> > >  const MediaPad *MediaEntity::getPadById(unsigned int id) const
> > >  {
> > >  	for (MediaPad *p : pads_) {
> > > @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int
> > > id) const
> > >  	return nullptr;
> > >  }
> > >
> > > +/**
> > > + * \brief Set the path to the device node for the associated interface
> > > + * \param devnode The interface device node path associated with this
> > > entity + * \return 0 on success, or a negative error code if the device
> > > node can't be + * accessed
> > > + */
> > > +int MediaEntity::setDeviceNode(const std::string &devnode)
> > > +{
> > > +	/* Make sure the device node can be accessed. */
> > > +	int ret = ::access(devnode.c_str(), R_OK | W_OK);
> > > +	if (ret < 0) {
> > > +		ret = -errno;
> > > +		LOG(Error) << "Device node " << devnode << " can't be accessed: "
> > > +			   << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * \brief Construct a MediaEntity
> > >   * \param entity The media entity kernel data
> > > + * \param major The major number of the entity associated interface
> > > + * \param minor The minor number of the entity associated interface
> >
> > Should we say they're defaulted to 0 ?
>
> Doxygen shows the default value in the function prototype already, I don't
> think we need to duplicate that information.
>
Ah fine, I didn't know.

Fine with me, with the few bit addressed (as you like the most, that's
minor stuff) please push this.

Thanks
  j

> > >   */
> > > -MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> > > -	: MediaObject(entity->id), name_(entity->name)
> > > +MediaEntity::MediaEntity(const struct media_v2_entity *entity,
> > > +			 unsigned int major, unsigned int minor)
> > > +	: MediaObject(entity->id), name_(entity->name),
> > > +	  major_(major), minor_(minor)
> > >
> > >  {
> > >  }
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi Jacopo,

On Wednesday, 2 January 2019 13:13:40 EET Jacopo Mondi wrote:
> On Wed, Jan 02, 2019 at 12:55:00PM +0200, Laurent Pinchart wrote:
> > On Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:
> > > On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:
> > > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > > 
> > > > Extend the MediaEntity object with device node major and minor
> > > > numbers, and retrieve them from the media graph using interfaces. They
> > > > will be used by the DeviceEnumerator to retrieve the devnode path.
> > > > 
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  src/libcamera/include/media_device.h |  2 +
> > > >  src/libcamera/include/media_object.h |  9 +++-
> > > >  src/libcamera/media_device.cpp       | 65 ++++++++++++++++++++++++++-
> > > >  src/libcamera/media_object.cpp       | 46 ++++++++++++++++++--
> > > >  4 files changed, 116 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/src/libcamera/include/media_device.h
> > > > b/src/libcamera/include/media_device.h index
> > > > 3fcdb4b4d5f8..8d491a87867c
> > > > 100644
> > > > --- a/src/libcamera/include/media_device.h
> > > > +++ b/src/libcamera/include/media_device.h
> > > > 
> > > > @@ -54,6 +54,8 @@ private:
> > > >  	std::vector<MediaEntity *> entities_;
> > > > 
> > > > +	struct media_v2_interface *findInterface(const struct
> > > > media_v2_topology
> > > > &topology,
> > > > +						 unsigned int entityId);
> > > 
> > > To respect the 80-col, I dropped 'struct' from media_v2_ variables in
> > > the 'populate*()' functions.
> > 
> > C++ allows us to do that, but I've kept it to emphasize that we're dealing
> > with kernel structures. We're free to pick one option or another, but it
> > should be applied globally.
> 
> I'm not a fan of omitting 'struct' neither, but in that case the code
> was more readable and fit in 80 columns.
> 
> Let's define a rule globally. I would tend to keep struct
> Please not the populate*() functions will have to be re-adjusted, I
> could do that, send a patch, and then you can rebase on top, or you
> could do it here.

Keeping the struct keyword has my preference too. You can submit a patch on 
top of the master branch, or I can do it if you prefer.

> > > >  	bool populateEntities(const struct media_v2_topology &topology);
> > > >  	bool populatePads(const struct media_v2_topology &topology);
> > > >  	bool populateLinks(const struct media_v2_topology &topology);

[snip]

> > > > diff --git a/src/libcamera/media_device.cpp
> > > > b/src/libcamera/media_device.cpp index 605e504be124..70b3fff3f492
> > > > 100644
> > > > --- a/src/libcamera/media_device.cpp
> > > > +++ b/src/libcamera/media_device.cpp
> > > > @@ -208,6 +208,7 @@ int MediaDevice::populate()
> > > > 
> > > >  	struct media_v2_entity *ents = nullptr;
> > > >  	struct media_v2_link *links = nullptr;
> > > >  	struct media_v2_pad *pads = nullptr;
> > > > 
> > > > +	struct media_v2_interface *interfaces = nullptr;
> > > 
> > > That's my OCD, but could you move this line 3 lines up? This gives
> > > lines a nicer ordering
> > 
> > I wanted to keep the same order as in the media_v2_topology structure, but
> > I now realize that interfaces come just after entities (and links after
> > pads). How about reordering them in that order in a follow-up patch ?
> 
> How about reordering them in reverse-xmas-tree order in a follow-up
> patch instead :)

Follow-up patch it should be as I've pushed this patch already :-)

> Ultimate bikeshedding here... I'm fine with both actually
> 
> > [snip]
> > 
> > > > @@ -367,6 +372,45 @@ void MediaDevice::clear()
> > > >   * \brief Global list of media entities in the media graph
> > > >   */
> > > > 
> > > > +/**
> > > > + * \brief Find the interface associated with an entity
> > > > + * \param topology The media topology as returned by
> > > > MEDIA_IOC_G_TOPOLOGY
> > > > + * \param entityId The entity id
> > > > + * \return A pointer to the interface if found, or nullptr otherwise
> > > > + */
> > > 
> > > This is a private function, we can document it, I agree, but leave
> > > doxygen commands out.
> > 
> > Why so ? I think we should standardize on a single syntax for
> > documentation.
> 
> Ok for consistency. Be aware Doxygen does not parse them though.

I know. That's a bit of a shame.

> > > > +struct media_v2_interface *MediaDevice::findInterface(const struct
> > > > media_v2_topology &topology,
> > > > +			       unsigned int entityId)
> > > > +{
> > > > +	struct media_v2_link *links = reinterpret_cast<struct media_v2_link
> > > > *>
> > > > +						(topology.ptr_links);
> > > 	
> > > 	media_v2_link *links = reinterpret_cast<media_v2_link *>
> > > 	
> > >                                (topology.ptr_links);
> > > 
> > > To make it the same as in other populate*() functions
> > 
> > Let's discuss struct media_v2_link vs. media_v2_link globally as mentioned
> > above, and update the code accordingly.
> > 
> > > > +	unsigned int ifaceId = -1;
> > > 
> > > unsigned int initialized with -1 ?
> > 
> > The compiler doesn't warn :-)
> 
> So for the purpose of using this variable to check for a match, this
> should read s/-1/UINT_MAX/ ?
> 
> > > > +
> > > > +	for (unsigned int i = 0; i < topology.num_links; ++i) {
> > > > +		/* Search for the interface to entity link. */
> > > > +		if (links[i].sink_id != entityId)
> > > > +			continue;
> > > > +
> > > > +		if ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> > > > +		    MEDIA_LNK_FL_INTERFACE_LINK)
> > > > +			continue;
> > > > +
> > > > +		ifaceId = links[i].source_id;
> > > 
> > > break?
> > 
> > Indeed. Fixed.
> > 
> > > > +	}
> > > > +
> > > > +	if (ifaceId == static_cast<unsigned int>(-1))
> > > > +		return nullptr;
> 
> I wonder what is the value of making it unsigned, and having to go
> through a static cast to use -1 as UINT_MAX.
> 
> I mean, I now if you declare it as signed int, it has then to be
> casted to unsigned when comparing it with ids in kernel structure, but
> what about checking here for
> 
>         if (i == topology.num_links)
> 
> instead, and keep using unsigned for ifaceId (dropping the assignament and
> comparison to -1) ?

It's a good point. Feel free to submit a follow-up patch if you think this 
should be fixed.

> > > > +
> > > > +	struct media_v2_interface *ifaces = reinterpret_cast<struct
> > > > media_v2_interface *>
> > > > +						(topology.ptr_interfaces);
> > > 
> > > drop struct here as well?
> > > 
> > > > +
> > > > +	for (unsigned int i = 0; i < topology.num_interfaces; ++i) {
> > > > +             if (ifaces[i].id == ifaceId)
> > > > +			return &ifaces[i];
> > > > +	}
> > > > +
> > > > +	return nullptr;
> > > > +}

[snip]