[libcamera-devel] [PATCH 1/6] libcamera: camera_sensor: Add dev() operation

[Jacopo Mondi]

Hi Laurent,

On Sun, Jun 09, 2019 at 01:21:17PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Sun, Jun 09, 2019 at 01:05:13AM +0100, Kieran Bingham wrote:
> > On 03/06/2019 12:31, Laurent Pinchart wrote:
> > > On Sun, Jun 02, 2019 at 03:04:30PM +0200, Jacopo Mondi wrote:
> > >> Add dev() operation to the CameraSensor class to access the
> > >> V4L2Subdevice backing the camera sensor.
> > >>
> > >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >> ---
> > >>  src/libcamera/camera_sensor.cpp       | 6 ++++++
> > >>  src/libcamera/include/camera_sensor.h | 1 +
> > >>  2 files changed, 7 insertions(+)
> > >>
> > >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > >> index 2b9d8fa593c1..8cbef8bccbef 100644
> > >> --- a/src/libcamera/camera_sensor.cpp
> > >> +++ b/src/libcamera/camera_sensor.cpp
> > >> @@ -130,6 +130,12 @@ int CameraSensor::init()
> > >>   * \return The sensor media entity
> > >>   */
> > >>
> > >> +/**
> > >> + * \fn CameraSensor::dev()
> > >
> > > Shouldn't this be called device() ?
> >
> > Or perhaps even ::subdev() as that is the field which is returned.
> >
> > > What if the camera sensor is exposed
> > > through multiple subdevs ?
> >
> > CameraSensor only supports a single subdev currently ... so I guess that
> > would then be considered if CameraSensor were to ever be extended?
> >
> > Can you imagine a CameraSensor exposing more than one subdev in the near
> > future?
>
> The smiapp driver exposes sensors through multiple subdevs, so that's
> certainly something we'll need to support at some point.
>

Oh, I see.

> > (In the context of needing to set controls on each subdev specifically?)
>
> I'm thinking it could be a good idea to abstract sensor controls in the
> CameraSensor class, and avoiding accessing the subdevs directly.
>

Considering it is likely CameraSensor will be subclassed by pipeline
handlers that need to do so so, providing a set/getControl that can be
specialized and applied on as many subdevices as necessary for the
sensor might be a good idea. I think I need to have it in this series
if we are to drop the here introduced "dev()" method.

> > We might have separate subdevs for handling say, flash-light controls,
> > but I think perhaps we would handle that in a separate class?
>
> Yes, that would be handled separately.
>

Separate class or sensor-specific sub-class ?

> > >> + * \brief Retrieve the sensor V4L2 subdevice
> > >> + * \return The sensor V4L2 subdevice
> > >> + */
> > >> +
> > >>  /**
> > >>   * \fn CameraSensor::mbusCodes()
> > >>   * \brief Retrieve the media bus codes supported by the camera sensor
> > >> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > >> index b823480241a7..6cdf833a27bf 100644
> > >> --- a/src/libcamera/include/camera_sensor.h
> > >> +++ b/src/libcamera/include/camera_sensor.h
> > >> @@ -33,6 +33,7 @@ public:
> > >>  	int init();
> > >>
> > >>  	const MediaEntity *entity() const { return entity_; }
> > >> +	V4L2Subdevice *dev() const { return subdev_; }
> > >>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > >>  	const std::vector<Size> &sizes() const { return sizes_; }
> > >>  	const Size &resolution() const;
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190610/232c9daf/attachment.sig>