[libcamera-devel] [PATCH v5 07/19] libcamera: v4l2_subdevice: Create device from entity name

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Mar 26, 2019 at 09:38:50AM +0100, Jacopo Mondi wrote:
> Add a static method to V4L2Subdevice class to create a V4L2Subdevice
> instance from a media entity name.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_subdevice.h |  5 +++++
>  src/libcamera/v4l2_subdevice.cpp       | 22 ++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index f42cccabda94..0067109931d1 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -18,6 +18,8 @@
>  
>  namespace libcamera {
>  
> +class MediaDevice;
> +
>  struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	uint32_t width;
> @@ -29,6 +31,9 @@ struct V4L2SubdeviceFormat {
>  class V4L2Subdevice : protected Loggable
>  {
>  public:
> +	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> +					     const std::string &name);
> +
I would move this down, as static methods are usually put after the
non-static ones.

I would also rename the name argument to entity (I forgot to mention
this for patch 06/19, it applies there too).

I wonder if there would be an advantage in turning this into a
constructor for the V4L2Subdevice class, but in any case that could be
done later, so, with the above and below issues fixed,

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

>  	explicit V4L2Subdevice(const MediaEntity *entity);
>  	V4L2Subdevice(const V4L2Subdevice &) = delete;
>  	V4L2Subdevice &operator=(const V4L2Subdevice &) = delete;
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 10925f9fe497..e28857c9aeb1 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -16,6 +16,7 @@
>  
>  #include "geometry.h"
>  #include "log.h"
> +#include "media_device.h"
>  #include "media_object.h"
>  #include "v4l2_subdevice.h"
>  
> @@ -101,6 +102,27 @@ const std::string V4L2SubdeviceFormat::toString() const
>   * any device left open will be closed, and any resources released.
>   */
>  
> +/**
> + * \brief Create a new video subdevice instance from entity with \a name in
> + * media device \a media
> + * \param[in] media The media device where the entity is registered
> + * \param[in] name The media entity name
> + *
> + * Releasing memory of the newly created instance is responsibility of the

s/Releasing memory of the/Deleting the/
s/is responsibility/is the responsibility/

(applies to 06/19 too)

> + * caller of this function.
> + *
> + * \return A newly created V4L2Subdevice on success, nullptr otherwise
> + */
> +V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> +					     const std::string &name)
> +{
> +	MediaEntity *entity = media->getEntityByName(name);
> +	if (!entity)
> +		return nullptr;
> +
> +	return new V4L2Subdevice(entity);
> +}
> +
>  /**
>   * \brief Create a V4L2 subdevice from a MediaEntity using its device node
>   * path

-- 
Regards,

Laurent Pinchart