[libcamera-devel] [PATCH v3] MediaLink: Make MediaLink::setEnabled account for existing
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 28 18:52:45 CEST 2020
On Fri, Aug 28, 2020 at 10:40:45AM +0100, Kieran Bingham wrote:
> Hi Dan,
>
> Thankyou, and yes - this git-send-mail version applies perfectly well
> with git-am.
>
> On 28/08/2020 07:25, Dan Scally wrote:
> > The setupLink function fails (ioctl returns EINVAL) when it passes the
> > MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with
> > MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's
> > equivalent media_setup_link() which ORs the new flags with the existing
> > values. This patch modifies the behaviour of setupLink() to behave the
> > same as media_setup_link() in media-ctl.
The commit message is now out of date I'm afraid, as the patch doesn't
modify setupLink() anymore. No worries, I'll update this to
The MediaDevice::setupLink() function fails (ioctl returns EINVAL) when
it passes only the MEDIA_LNK_FL_ENABLE flag to a link that is already
flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to
media-ctl's equivalent media_setup_link() which ORs the new flags with
the existing values. Fix this by preserving all flags but
MEDIA_LNK_FL_ENABLED in MediaLink::setEnabled().
The subject line seems to also have been truncated, I think it misses
"flags" at the end. It also should start with "libcamera: media_object:"
to match the usual style of commit messages.
Please let me know if you have any objection.
> >
> > Signed-off-by: Dan Scally <djrscally at gmail.com>
> > ---
> >
> > Changelog:
> >
> > v3 - Moved the change to MediaLink::setEnabled()
> >
> > v2 - Simplified by removing the call to link() to fetch a link that's
> > already passed as a parameter to the function.
> >
> > src/libcamera/media_object.cpp | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> > index ce77a72..a2e6a0d 100644
> > --- a/src/libcamera/media_object.cpp
> > +++ b/src/libcamera/media_object.cpp
> > @@ -115,7 +115,8 @@ LOG_DECLARE_CATEGORY(MediaDevice)
> > */
> > int MediaLink::setEnabled(bool enable)
> > {
> > - unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> > + unsigned int flags = (flags_ & ~MEDIA_LNK_FL_ENABLED)
> > + | (enable ? MEDIA_LNK_FL_ENABLED : 0);
>
> To fit our coding style, that second line should be aligned so that the
> | operator is below the = operator I think ... but that's really minor
> and is easily handled when applying.
>
> Other than that, this sounds like a better approach that v2 indeed,
> especially as now the flags_ is going to be kept consistent.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
With this addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > int ret = dev_->setupLink(this, flags);
> > if (ret)
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list