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

[Jacopo Mondi]

Hi Laurent,

On Fri, Jan 18, 2019 at 03:09:57AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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?


> >  	unsigned int deviceMajor() const { return major_; }
> >  	unsigned int deviceMinor() const { return minor_; }
> >
> > 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>
>

Thanks
  j

> > + *
> > + * \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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190121/cf92ba21/attachment-0001.sig>