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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jun 10, 2019 at 08:59:44AM +0200, Jacopo Mondi wrote:
> On Sun, Jun 09, 2019 at 01:21:17PM +0300, Laurent Pinchart wrote:
> > 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,

I don't think so, I expect it to be subclassed by implemetations
specific to sensor devices. Pipeline handlers should not subclass
CameraSensor.

> 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.

If it's not too difficult to add in this series I think it would be a
good idea. Note that in that case CameraSensor should expose libcamera
controls (and possibly even sensor controls that are not exposed to
applications), not V4L2 controls.

> >> 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 ?

I think separate classes, possibly with cross-pointers between them.

> >>>> + * \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