[libcamera-devel] [PATCH v2 3/5] libcamera: media_object: Set devnode in MediaEntity

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 21, 2019 at 11:27:45AM +0100, Jacopo Mondi wrote:
>  On Fri, Jan 18, 2019 at 03:09:57AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 16, 2019 at 02:59:47PM +0100, Jacopo Mondi wrote:
> >> The MediaEntity::setDeviceNode() function was designed to set the device
> >> node path associated with a MediaEntity. The function was there, but the
> >> devnode_ member field was never actually set. Fix this.
> >> 
> >> While at there add a getter method for the devnode_ member as it will
> >> soon be used.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >> src/libcamera/include/media_object.h |  1 +
> >> src/libcamera/media_object.cpp       | 11 +++++++++++
> >> 2 files changed, 12 insertions(+)
> >> 
> >> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h
> >> index a10f7e1..fad55a0 100644
> >> --- a/src/libcamera/include/media_object.h
> >> +++ b/src/libcamera/include/media_object.h
> >> @@ -85,6 +85,7 @@ class MediaEntity : public MediaObject
> >> public:
> >> 	const std::string &name() const { return name_; }
> >> 	unsigned int function() const { return function_; }
> >> +	const std::string &devnode() const { return devnode_; }
> > 
> > As the setter is called setDeviceNode(), should this be called
> > deviceNode() ? We usually try not to abbreviate when not necessary.
>  
>  I would like to have devnode() as the MediaDevice exposes a function
>  with the same purpose namde like that. Or I either change both or stay
>  with devnode() here. Is this a big deal?

I think consistency is important. I'm OK with both devnode and
deviceNode (possibly with a preference for the latter), so you can pick
the one you like best, but the getter and setter in MediaEntity should
be consistent. If they can be consistent with MediaDevice it's probably
best.

> >> 	unsigned int deviceMajor() const { return major_; }
> >> 	unsigned int deviceMinor() const { return minor_; }

This is why I prefer deviceNode() over devnode() by the way, to be
consistent with these two functions. I'll let you decide which one you
prefer.

> >> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> >> index 76dd326..4e90443 100644
> >> --- a/src/libcamera/media_object.cpp
> >> +++ b/src/libcamera/media_object.cpp
> >> @@ -263,6 +263,15 @@ void MediaPad::addLink(MediaLink *link)
> >> * \return The entity's function
> >> */
> >> 
> >> +/**
> >> + * \fn MediaEntity::devnode()
> >> + * \brief Retrieve the entity's device node path, if any
> >> + *
> >> + * \sa int MediaEntity::setDeviceNode(const std::string &devnode)
> > 
> > I think you can abbreviate that to \sa setDeviceNode()
> > 
> > With these addressed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> >> + *
> >> + * \return The entity's device node path, or an empty string if it is not set
> >> + */
> >> +
> >> /**
> >> * \fn MediaEntity::deviceMajor()
> >> * \brief Retrieve the major number of the interface associated with the entity
> >> @@ -330,6 +339,8 @@ int MediaEntity::setDeviceNode(const std::string &devnode)
> >> 		return ret;
> >> 	}
> >> 
> >> +	devnode_ = devnode;
> >> +
> >> 	return 0;
> >> }
> >> 

-- 
Regards,

Laurent Pinchart