[libcamera-devel] [PATCH v3 3/6] libcamera: pipeline_handler: uvcvideo: register all Cameras along with a devnum

[Paul Elder]

Hi Laurent,

Thank you for the review.

On Fri, Dec 27, 2019 at 04:48:51PM +0200, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Dec 23, 2019 at 01:26:17AM -0600, Paul Elder wrote:
> > Register all UVC Cameras along with its device number, to eventually
> 
> s/its device number/their device numbers/

ack

> > allow the V4L2 compatibility layer to match against it.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > ---
> > New in v3
> > ---
> >  src/libcamera/pipeline/uvcvideo.cpp | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 3043366b..096c1c09 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -7,6 +7,7 @@
> >  
> >  #include <algorithm>
> >  #include <iomanip>
> > +#include <sys/sysmacros.h>
> >  #include <tuple>
> >  
> >  #include <libcamera/camera.h>
> > @@ -294,17 +295,19 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  		return false;
> >  
> >  	std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
> > +	dev_t devnum = 0;
> >  
> >  	/* Locate and initialise the camera data with the default video node. */
> >  	for (MediaEntity *entity : media->entities()) {
> >  		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> >  			if (data->init(entity))
> >  				return false;
> > +			devnum = makedev(entity->deviceMajor(), entity->deviceMinor());
> 
> You can move this line after the loop, and merge it with the devnum
> variable declaration. I would actually move it after the "Could not find
> a default video device" check and make it a separate check with a
> dedicated error message. Or even drop the check completely as it really
> shouldn't happen (in which case you can move the devnum calculation to
> the registration below).

I don't think I can, because entity is only available inside the loop. I
would have to save it, in which case I might as well just extract the
devnum here.

> With this fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  			break;
> >  		}
> >  	}
> >  
> > -	if (!data) {
> > +	if (!data || !devnum) {
> >  		LOG(UVC, Error) << "Could not find a default video device";
> >  		return false;
> >  	}
> > @@ -312,7 +315,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  	/* Create and register the camera. */
> >  	std::set<Stream *> streams{ &data->stream_ };
> >  	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> > -	registerCamera(std::move(camera), std::move(data));
> > +	registerCamera(std::move(camera), std::move(data), devnum);
> >  
> >  	/* Enable hot-unplug notifications. */
> >  	hotplugMediaDevice(media);


Thanks,

Paul