[libcamera-devel] [PATCH v4 5/8] libcamera: camera_sensor: Expose the camera device

[Niklas Söderlund]

Hi Laurent,

Thanks for your comment.

On 2021-01-10 16:03:43 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 08, 2021 at 04:28:54PM +0100, Niklas Söderlund wrote:
> > On 2020-12-17 15:12:16 +0100, Jacopo Mondi wrote:
> > > On Tue, Dec 15, 2020 at 01:48:08AM +0100, Niklas Söderlund wrote:
> > > > Expose the device backing the CameraSensor instance.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > ---
> > > > * Changes since v3
> > > > - New, needed to be able to move the DelayedControls handling out of
> > > >   CameraSensor.
> > > > ---
> > > >  include/libcamera/internal/camera_sensor.h | 2 ++
> > > >  src/libcamera/camera_sensor.cpp            | 6 ++++++
> > > >  2 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index f80d836161a564e7..a70e024aa327f69b 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -61,6 +61,8 @@ public:
> > > >  	ControlList getControls(const std::vector<uint32_t> &ids);
> > > >  	int setControls(ControlList *ctrls);
> > > >
> > > > +	V4L2Subdevice *device() { return subdev_.get(); }
> > > > +
> > > >  	const ControlList &properties() const { return properties_; }
> > > >  	int sensorInfo(CameraSensorInfo *info) const;
> > > >
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index 1628ba9c892b0eaf..df45b2b803617bfd 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -486,6 +486,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> > > >  	return subdev_->getControls(ids);
> > > >  }
> > > >
> > > > +/**
> > > > + * \fn CameraSensor::device()
> > > > + * \brief Retrieve the camera sensor device
> > > > + * \return The camera sensor device
> > > > + */
> > > > +
> > > 
> > > I wonder if subdevice() would be better.
> > > A minor though
> > 
> > I thin device is 'nicer' as what it does is retrieve the device which 
> > represents the sensor. That being said I don't feel strongly about it.  
> > And hopefully we can remove this method in the future and not expose it 
> > outside CameraSensor. This is only needed until we decide how to best 
> > integrate DelayedControls inside CameraSensro.
> 
> Could you add a
> 
> 	\todo Remove this function by integrating DelayedControl with
> 	CameraSensor
> 
> ? With that,

Good idea.

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

Thanks.

> 
> Do you have plans to address this integration ?

Yes, I had a go at the integration in v1 and v2 but was pushed back to 
make this series less controversial. I hope to find a way to integrate 
this ASAP as I think some of the things this series introduce in 
pipeline handlers are quiet ugly :-) I think the killer combo for the 
first integration step will be the addition of the camera 
database/configuration files.

> 
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > 
> > Thanks.
> > 
> > > >  /**
> > > >   * \fn CameraSensor::properties()
> > > >   * \brief Retrieve the camera sensor properties
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund