[libcamera-devel] [RFC 05/11] libcamera: media_device: Make open() and close() private

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Apr 14, 2019 at 03:35:00AM +0200, Niklas Söderlund wrote:
> All external callers to open() and close() have been refactored and
> there is no need to expose these functions anymore, make them private.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/media_device.h          |  6 +-
>  src/libcamera/media_device.cpp                | 78 +++++++------------
>  test/media_device/media_device_print_test.cpp | 11 ---
>  3 files changed, 32 insertions(+), 63 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 883985055eb419dd..e513a2fee1b91a1b 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -30,9 +30,6 @@ public:
>  	void release();
>  	bool busy() const { return acquired_; }
>  
> -	int open();
> -	void close();
> -
>  	int populate();
>  	bool valid() const { return valid_; }
>  
> @@ -62,6 +59,9 @@ private:
>  	bool valid_;
>  	bool acquired_;
>  
> +	int open();
> +	void close();
> +

We usually declare functions before variables.

>  	std::map<unsigned int, MediaObject *> objects_;
>  	MediaObject *object(unsigned int id);
>  	bool addObject(MediaObject *object);
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 644c6143fb15e1fa..0138be526f886c90 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -124,55 +124,6 @@ void MediaDevice::release()
>   * \sa acquire(), release()
>   */
>  
> -/**
> - * \brief Open a media device and retrieve device information
> - *
> - * If the device is already open the function returns -EBUSY.
> - *
> - * \return 0 on success or a negative error code otherwise
> - */
> -int MediaDevice::open()
> -{
> -	if (fd_ != -1) {
> -		LOG(MediaDevice, Error) << "MediaDevice already open";
> -		return -EBUSY;
> -	}
> -
> -	int ret = ::open(deviceNode_.c_str(), O_RDWR);
> -	if (ret < 0) {
> -		ret = -errno;
> -		LOG(MediaDevice, Error)
> -			<< "Failed to open media device at "
> -			<< deviceNode_ << ": " << strerror(-ret);
> -		return ret;
> -	}
> -	fd_ = ret;
> -
> -	return 0;
> -}
> -
> -/**
> - * \brief Close the media device
> - *
> - * This function closes the media device node. It does not invalidate the media
> - * graph and all cached media objects remain valid and can be accessed normally.
> - * Once closed no operation interacting with the media device node can be
> - * performed until the device is opened again.
> - *
> - * Closing an already closed device is allowed and will not perform any
> - * operation.
> - *
> - * \sa open()
> - */
> -void MediaDevice::close()
> -{
> -	if (fd_ == -1)
> -		return;
> -
> -	::close(fd_);
> -	fd_ = -1;
> -}
> -
>  /**
>   * \brief Populate the media graph with media objects
>   *
> @@ -440,6 +391,35 @@ int MediaDevice::disableLinks()
>   * driver unloading for most devices. The media device is passed as a parameter.
>   */
>  

I would keep the documentation, it's still valuable.

> +int MediaDevice::open()
> +{
> +	if (fd_ != -1) {
> +		LOG(MediaDevice, Error) << "MediaDevice already open";
> +		return -EBUSY;
> +	}
> +
> +	int ret = ::open(deviceNode_.c_str(), O_RDWR);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(MediaDevice, Error)
> +			<< "Failed to open media device at "
> +			<< deviceNode_ << ": " << strerror(-ret);
> +		return ret;
> +	}
> +	fd_ = ret;
> +
> +	return 0;
> +}
> +
> +void MediaDevice::close()
> +{
> +	if (fd_ == -1)
> +		return;
> +
> +	::close(fd_);
> +	fd_ = -1;
> +}
> +
>  /**
>   * \var MediaDevice::objects_
>   * \brief Global map of media objects (entities, pads, links) keyed by their
> diff --git a/test/media_device/media_device_print_test.cpp b/test/media_device/media_device_print_test.cpp
> index ceffd538e13fca73..30d929b8c76387a7 100644
> --- a/test/media_device/media_device_print_test.cpp
> +++ b/test/media_device/media_device_print_test.cpp
> @@ -113,17 +113,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)
>  	MediaDevice dev(deviceNode);
>  	int ret;
>  
> -	/* Fuzzy open/close sequence. */

Do we need a fuzzy acquire/release sequence ?

> -	ret = dev.open();
> -	if (ret)
> -		return ret;
> -
> -	ret = dev.open();
> -	if (!ret)
> -		return ret;
> -
> -	dev.close();
> -
>  	ret = dev.populate();
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart