[libcamera-devel] [PATCH v2 3/4] libcamera: Add link handling functions

[Laurent Pinchart]

Hi Jacopo,

On Tuesday, 8 January 2019 21:52:42 EET Jacopo Mondi wrote:
> On Tue, Jan 08, 2019 at 08:18:45PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 8 January 2019 19:04:06 EET Jacopo Mondi wrote:
> >> Add a function to the MediaLink class to set the state of a link to
> >> enabled or disabled. The function makes use of an internal MediaDevice
> >> method, which is defined private and only accessible by the MediaLink
> >> setEnable() function itself.
> >> 
> >> Also add to MediaDevice a function to reset all links registered in the
> >> media graph.
> > 
> > I would have split this to a separate patch, but it's not a big deal.
> > 
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >> v1->v2:
> >> - Add resetLinks() function
> >> - s/enable()/setEnable()
> >> - s/setLink()/setupLink()
> >> - Pass MediaLink and not two MediaPads to MediaDevice::setupLink
> >> 
> >>  src/libcamera/include/media_device.h |  4 ++
> >>  src/libcamera/include/media_object.h |  1 +
> >>  src/libcamera/media_device.cpp       | 75 ++++++++++++++++++++++++++++
> >>  src/libcamera/media_object.cpp       | 29 +++++++++++
> >>  4 files changed, 109 insertions(+)
> >> 
> >> diff --git a/src/libcamera/include/media_device.h
> >> b/src/libcamera/include/media_device.h index 397d349..0f423aa 100644
> >> --- a/src/libcamera/include/media_device.h
> >> +++ b/src/libcamera/include/media_device.h
> >> @@ -45,6 +45,7 @@ public:
> >>  	MediaLink *link(const MediaEntity *source, unsigned int sourceIdx,
> >>  			const MediaEntity *sink, unsigned int sinkIdx);
> >>  	MediaLink *link(const MediaPad *source, const MediaPad *sink);
> >> 
> > > +	int resetLinks();
> > 
> > I wonder if this should be called disableLinks() or disableAllLinks() to
> > better reflect the purpose.
> 
> I find reset confusing as well, although it is used already, but I
> find the enable/disable terms better... I'll go with disableLinks()
> 
> >>  private:
> >>  	std::string driver_;
> >> 
> >> @@ -65,6 +66,9 @@ private:
> >>  	bool populateEntities(const struct media_v2_topology &topology);
> >>  	bool populatePads(const struct media_v2_topology &topology);
> >>  	bool populateLinks(const struct media_v2_topology &topology);
> >> +
> >> +	friend int MediaLink::setEnable(bool enable);
> >> +	int setupLink(const MediaLink *link, unsigned int flags);
> >>  };
> >>  
> >>  } /* namespace libcamera */
> >> diff --git a/src/libcamera/include/media_object.h
> >> b/src/libcamera/include/media_object.h index d92aab3..bb46fac 100644
> >> --- a/src/libcamera/include/media_object.h
> >> +++ b/src/libcamera/include/media_object.h
> >> @@ -41,6 +41,7 @@ public:
> >>  	MediaPad *source() const { return source_; }
> >>  	MediaPad *sink() const { return sink_; }
> >>  	unsigned int flags() const { return flags_; }
> >> +	int setEnable(bool enable);
> > 
> > s/setEnable/setEnabled/ ?
> 
> It's maybe stupid but, since we have disableLinks() should we have an
> enable() and a disable() function?

It's a good question. As the property is "enabled", having enabled() and 
setEnabled() accessors is simple and logical. enable() and disable() would be 
as well, but it would be a special case. I'm tempted to stick to setEnabled() 
to follow the generic rule.

> >>  private:
> >>  	friend class MediaDevice;
> >> 

[snip]

-- 
Regards,

Laurent Pinchart