[libcamera-devel,v2,3/5] libcamera: media_object: Set devnode in MediaEntity
diff mbox series

Message ID 20190116135949.2097-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: pipeline: Add Intel IPU3 pipeline handler
Related show

Commit Message

Jacopo Mondi Jan. 16, 2019, 1:59 p.m. UTC 
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@jmondi.org>
---
 src/libcamera/include/media_object.h |  1 +
 src/libcamera/media_object.cpp       | 11 +++++++++++
 2 files changed, 12 insertions(+)

Comments

Niklas Söderlund Jan. 16, 2019, 3:20 p.m. UTC | #1 
Hi Jacopo,

Thanks for your patch.

On 2019-01-16 14:59:47 +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@jmondi.org>

I would have split this in two patches at it clearly does two different 
things. For small patches like this it's not a huge issue but as patch 
size grows the cost of review increases, at least for me.

Also with smaller patches you can apply from the bottom once they 
received proper review and decrease the size of the next version of a 
series and release fixes earlier to master, if appropriate of course.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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_; }
>  	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)
> + *
> + * \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;
>  }
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 18, 2019, 1:09 a.m. UTC | #2 
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@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.

>  	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@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;
>  }
>
Jacopo Mondi Jan. 21, 2019, 10:27 a.m. UTC | #3 
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@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@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
Laurent Pinchart Jan. 21, 2019, 12:08 p.m. UTC | #4 
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@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@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;
> >> }
> >>

Patch
diff mbox series 

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_; }
 	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)
+ *
+ * \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;
 }