Message ID | 20190414013506.10515-6-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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
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(); + 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. */ +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. */ - ret = dev.open(); - if (ret) - return ret; - - ret = dev.open(); - if (!ret) - return ret; - - dev.close(); - ret = dev.populate(); if (ret) return ret;
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(-)