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

973 diff mbox series

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@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(-)

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@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;

Hi Laurent,

Thanks for your feedback.

On 2019-04-17 02:14:54 +0300, Laurent Pinchart wrote:

snip

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

That is a good idea, I will add this for v1 to replace this test.

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