[libcamera-devel] [RFC 03/11] libcamera: media_device: Only read device information in populate()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Apr 14, 2019 at 03:34:58AM +0200, Niklas Söderlund wrote:
> There is no reason to reread the MEDIA_IOC_DEVICE_INFO information every
> time the media device is opened. Move it populate() where it will be
> read once together the other information about the media device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/media_device.cpp | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index ecb00a1d5abffe80..54706bb73a7591d5 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -131,9 +131,6 @@ bool MediaDevice::acquire()
>   * with the device node associated with the media device, the device node must
>   * be opened.
>   *
> - * This function also retrieves media device information from the device node,
> - * which can be queried through driver().

This should go to the documentation of the populate() function (in a
modified form to integrate nicely with it).

> - *
>   * If the device is already open the function returns -EBUSY.
>   *
>   * \return 0 on success or a negative error code otherwise
> @@ -155,20 +152,6 @@ int MediaDevice::open()
>  	}
>  	fd_ = ret;
>  
> -	struct media_device_info info = { };
> -	ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> -	if (ret) {
> -		ret = -errno;
> -		LOG(MediaDevice, Error)
> -			<< "Failed to get media device info "
> -			<< ": " << strerror(-ret);
> -		return ret;
> -	}
> -
> -	driver_ = info.driver;
> -	model_ = info.model;
> -	version_ = info.media_version;
> -
>  	return 0;
>  }
>  
> @@ -224,6 +207,20 @@ int MediaDevice::populate()
>  	if (ret)
>  		return ret;
>  
> +	struct media_device_info info = {};
> +	ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(MediaDevice, Error)
> +			<< "Failed to get media device info "

While at it you could merge the ": " wit the previous line (and remove
the space after "info").

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +			<< ": " << strerror(-ret);
> +		goto err_out;
> +	}
> +
> +	driver_ = info.driver;
> +	model_ = info.model;
> +	version_ = info.media_version;
> +
>  	/*
>  	 * Keep calling G_TOPOLOGY until the version number stays stable.
>  	 */

-- 
Regards,

Laurent Pinchart