| Message ID | 20190808151221.24254-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Aug 08, 2019 at 04:12:17PM +0100, Kieran Bingham wrote: > The "Opened device" statement occurs before the buffertype_ is set. > > This causes all devices to report that they are [out] devices at open() > regardless of their type. > > As the message operates in the past-tense, move the statement to the end > of the function when all work has been completed. > > Fixes: 04d5be7f76fe ("libcamera: v4l2_device: Inherit from Loggable to > print device node name") No need to wrap this line :-) > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/v4l2_videodevice.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index c43d7cc557a0..81098dd70190 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -314,10 +314,6 @@ int V4L2VideoDevice::open() > return ret; > } > > - LOG(V4L2, Debug) > - << "Opened device " << caps_.bus_info() << ": " > - << caps_.driver() << ": " << caps_.card(); > - > if (!caps_.hasStreaming()) { > LOG(V4L2, Error) << "Device does not support streaming I/O"; > return -EINVAL; > @@ -352,6 +348,10 @@ int V4L2VideoDevice::open() > fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); > fdEvent_->setEnabled(false); > > + LOG(V4L2, Debug) > + << "Opened device " << caps_.bus_info() << ": " > + << caps_.driver() << ": " << caps_.card(); > + > return 0; > } >
On 08/08/2019 21:26, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Aug 08, 2019 at 04:12:17PM +0100, Kieran Bingham wrote: >> The "Opened device" statement occurs before the buffertype_ is set. >> >> This causes all devices to report that they are [out] devices at open() >> regardless of their type. >> >> As the message operates in the past-tense, move the statement to the end >> of the function when all work has been completed. >> >> Fixes: 04d5be7f76fe ("libcamera: v4l2_device: Inherit from Loggable to >> print device node name") > > No need to wrap this line :-) Is Fixes an exception to the rule? (I usually just use vim's autowrap - so I don't take much consideration to the actual wrap.) >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Collected > >> --- >> src/libcamera/v4l2_videodevice.cpp | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index c43d7cc557a0..81098dd70190 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -314,10 +314,6 @@ int V4L2VideoDevice::open() >> return ret; >> } >> >> - LOG(V4L2, Debug) >> - << "Opened device " << caps_.bus_info() << ": " >> - << caps_.driver() << ": " << caps_.card(); >> - >> if (!caps_.hasStreaming()) { >> LOG(V4L2, Error) << "Device does not support streaming I/O"; >> return -EINVAL; >> @@ -352,6 +348,10 @@ int V4L2VideoDevice::open() >> fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); >> fdEvent_->setEnabled(false); >> >> + LOG(V4L2, Debug) >> + << "Opened device " << caps_.bus_info() << ": " >> + << caps_.driver() << ": " << caps_.card(); >> + >> return 0; >> } >> >
Hi Kieran, On Thu, Aug 08, 2019 at 04:12:17PM +0100, Kieran Bingham wrote: > The "Opened device" statement occurs before the buffertype_ is set. > > This causes all devices to report that they are [out] devices at open() > regardless of their type. > > As the message operates in the past-tense, move the statement to the end > of the function when all work has been completed. > > Fixes: 04d5be7f76fe ("libcamera: v4l2_device: Inherit from Loggable to > print device node name") With the fixes tag on a single line Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/v4l2_videodevice.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index c43d7cc557a0..81098dd70190 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -314,10 +314,6 @@ int V4L2VideoDevice::open() > return ret; > } > > - LOG(V4L2, Debug) > - << "Opened device " << caps_.bus_info() << ": " > - << caps_.driver() << ": " << caps_.card(); > - > if (!caps_.hasStreaming()) { > LOG(V4L2, Error) << "Device does not support streaming I/O"; > return -EINVAL; > @@ -352,6 +348,10 @@ int V4L2VideoDevice::open() > fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); > fdEvent_->setEnabled(false); > > + LOG(V4L2, Debug) > + << "Opened device " << caps_.bus_info() << ": " > + << caps_.driver() << ": " << caps_.card(); > + > return 0; > } > > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Fri, Aug 09, 2019 at 11:03:32AM +0100, Kieran Bingham wrote: > On 08/08/2019 21:26, Laurent Pinchart wrote: > > On Thu, Aug 08, 2019 at 04:12:17PM +0100, Kieran Bingham wrote: > >> The "Opened device" statement occurs before the buffertype_ is set. > >> > >> This causes all devices to report that they are [out] devices at open() > >> regardless of their type. > >> > >> As the message operates in the past-tense, move the statement to the end > >> of the function when all work has been completed. > >> > >> Fixes: 04d5be7f76fe ("libcamera: v4l2_device: Inherit from Loggable to > >> print device node name") > > > > No need to wrap this line :-) > > Is Fixes an exception to the rule? I think so. So are SoB or Rb lines. > (I usually just use vim's autowrap - so I don't take much consideration > to the actual wrap.) > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Collected > > >> --- > >> src/libcamera/v4l2_videodevice.cpp | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >> index c43d7cc557a0..81098dd70190 100644 > >> --- a/src/libcamera/v4l2_videodevice.cpp > >> +++ b/src/libcamera/v4l2_videodevice.cpp > >> @@ -314,10 +314,6 @@ int V4L2VideoDevice::open() > >> return ret; > >> } > >> > >> - LOG(V4L2, Debug) > >> - << "Opened device " << caps_.bus_info() << ": " > >> - << caps_.driver() << ": " << caps_.card(); > >> - > >> if (!caps_.hasStreaming()) { > >> LOG(V4L2, Error) << "Device does not support streaming I/O"; > >> return -EINVAL; > >> @@ -352,6 +348,10 @@ int V4L2VideoDevice::open() > >> fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); > >> fdEvent_->setEnabled(false); > >> > >> + LOG(V4L2, Debug) > >> + << "Opened device " << caps_.bus_info() << ": " > >> + << caps_.driver() << ": " << caps_.card(); > >> + > >> return 0; > >> } > >>
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index c43d7cc557a0..81098dd70190 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -314,10 +314,6 @@ int V4L2VideoDevice::open() return ret; } - LOG(V4L2, Debug) - << "Opened device " << caps_.bus_info() << ": " - << caps_.driver() << ": " << caps_.card(); - if (!caps_.hasStreaming()) { LOG(V4L2, Error) << "Device does not support streaming I/O"; return -EINVAL; @@ -352,6 +348,10 @@ int V4L2VideoDevice::open() fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); fdEvent_->setEnabled(false); + LOG(V4L2, Debug) + << "Opened device " << caps_.bus_info() << ": " + << caps_.driver() << ": " << caps_.card(); + return 0; }
The "Opened device" statement occurs before the buffertype_ is set. This causes all devices to report that they are [out] devices at open() regardless of their type. As the message operates in the past-tense, move the statement to the end of the function when all work has been completed. Fixes: 04d5be7f76fe ("libcamera: v4l2_device: Inherit from Loggable to print device node name") Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/v4l2_videodevice.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)