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