[libcamera-devel] libipa: camera_sensor_helper: Add IMX327 helper

18640 diff mbox series

From: Benjamin Bara <benjamin.bara@skidata.com>

Add support for the Sony IMX327, which is added to the kernel with
commit 2d41947ec2c0 ("media: i2c: imx290: Add support for imx327
variant"). It is basically a derivate of the IMX290, therefore also
derive the helper.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++
 1 file changed, 5 insertions(+)

Quoting Benjamin Bara via libcamera-devel (2023-05-19 16:48:13)
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Add support for the Sony IMX327, which is added to the kernel with
> commit 2d41947ec2c0 ("media: i2c: imx290: Add support for imx327
> variant"). It is basically a derivate of the IMX290, therefore also
> derive the helper.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 21cdabd1..2eebd7ab 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -428,6 +428,11 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290)
>  
> +class CameraSensorHelperImx327 : public CameraSensorHelperImx290
> +{
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx327)

This looks reasonable.

I wonder if we even need to have the new class at all? Does this work:


  REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290)
 +REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx290)

?

I would indeed rather add references to existing compatibles than
duplicate entire Helper classes where the result would be identical for
equivalent devices so either way:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
>  class CameraSensorHelperImx296 : public CameraSensorHelper
>  {
>  public:
> -- 
> 2.34.1
>

Hello

On Fri, May 19, 2023 at 04:57:37PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Benjamin Bara via libcamera-devel (2023-05-19 16:48:13)
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > Add support for the Sony IMX327, which is added to the kernel with
> > commit 2d41947ec2c0 ("media: i2c: imx290: Add support for imx327
> > variant"). It is basically a derivate of the IMX290, therefore also
> > derive the helper.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index 21cdabd1..2eebd7ab 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -428,6 +428,11 @@ public:
> >  };
> >  REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290)
> >
> > +class CameraSensorHelperImx327 : public CameraSensorHelperImx290
> > +{
> > +};
> > +REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx327)
>
> This looks reasonable.
>
> I wonder if we even need to have the new class at all? Does this work:
>
>
>   REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290)
>  +REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx290)
>
> ?
>
> I would indeed rather add references to existing compatibles than
> duplicate entire Helper classes where the result would be identical for
> equivalent devices so either way:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

With Kieran's suggestions, if possible. Either waysL
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> > +
> >  class CameraSensorHelperImx296 : public CameraSensorHelper
> >  {
> >  public:
> > --
> > 2.34.1
> >

Hi!

On Fri, 19 May 2023 at 17:57, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> I would indeed rather add references to existing compatibles than
> duplicate entire Helper classes where the result would be identical for
> equivalent devices.

Yes, I think so too - but unfortunately it leads to an error for now, as
it would redefine:
libcamera::ipa::CameraSensorHelperFactory<libcamera::ipa::CameraSensorHelperImx290>
libcamera::ipa::global_CameraSensorHelperImx290Factory’

Regards
Benjamin

Quoting Benjamin Bara (2023-05-19 17:40:55)
> Hi!
> 
> On Fri, 19 May 2023 at 17:57, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> > I would indeed rather add references to existing compatibles than
> > duplicate entire Helper classes where the result would be identical for
> > equivalent devices.
> 
> Yes, I think so too - but unfortunately it leads to an error for now, as
> it would redefine:
> libcamera::ipa::CameraSensorHelperFactory<libcamera::ipa::CameraSensorHelperImx290>
> libcamera::ipa::global_CameraSensorHelperImx290Factory’

Well then lets stick with your patch and we can see if we can optimise
reuse in the future.

I'll queue the patch up for merge.

Thanks,

Kieran

> 
> Regards
> Benjamin

On Fri, May 19, 2023 at 05:54:33PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Benjamin Bara (2023-05-19 17:40:55)
> > On Fri, 19 May 2023 at 17:57, Kieran Bingham wrote:
> > > I would indeed rather add references to existing compatibles than
> > > duplicate entire Helper classes where the result would be identical for
> > > equivalent devices.
> > 
> > Yes, I think so too - but unfortunately it leads to an error for now, as
> > it would redefine:
> > libcamera::ipa::CameraSensorHelperFactory<libcamera::ipa::CameraSensorHelperImx290>
> > libcamera::ipa::global_CameraSensorHelperImx290Factory’
> 
> Well then lets stick with your patch and we can see if we can optimise
> reuse in the future.
> 
> I'll queue the patch up for merge.

The alphabetical order is now messed up :-( Could you please send a fix
?