[libcamera-devel] [PATCH v3] MediaLink: Make MediaLink::setEnabled account for existing

[Kieran Bingham]

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.
> 
> 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>

>  
>  	int ret = dev_->setupLink(this, flags);
>  	if (ret)
> 

-- 
Regards
--
Kieran