| Message ID | 20190602130435.18780-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. 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@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() ? What if the camera sensor is exposed through multiple subdevs ? > + * \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;
Hi Laurent, On 03/06/2019 12:31, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > 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@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? (In the context of needing to set controls on each subdev specifically?) We might have separate subdevs for handling say, flash-light controls, but I think perhaps we would handle that in a separate 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; >
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@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. > (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. > 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. > >> + * \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;
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@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
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@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;
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() + * \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;
Add dev() operation to the CameraSensor class to access the V4L2Subdevice backing the camera sensor. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera_sensor.cpp | 6 ++++++ src/libcamera/include/camera_sensor.h | 1 + 2 files changed, 7 insertions(+)