Re: [PATCH v8 1/5] media: i2c: Add driver for RDACM21 camera module

[Laurent Pinchart]

Hi Hans,

On Mon, Feb 08, 2021 at 12:21:01PM +0100, Hans Verkuil wrote:
> On 14/01/2021 18:04, Jacopo Mondi wrote:
> > The RDACM21 is a GMSL camera supporting 1280x1080 resolution images
> > developed by IMI based on an Omnivision OV10640 sensor, an Omnivision
> > OV490 ISP and a Maxim MAX9271 GMSL serializer.
> > 
> > The driver uses the max9271 library module, to maximize code reuse with
> > other camera module drivers using the same serializer, such as rdacm20.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> >  MAINTAINERS                 |  12 +
> >  drivers/media/i2c/Kconfig   |  13 +
> >  drivers/media/i2c/Makefile  |   2 +
> >  drivers/media/i2c/rdacm21.c | 623 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 650 insertions(+)
> >  create mode 100644 drivers/media/i2c/rdacm21.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 14adf87d90c7..1822d73ed615 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14967,6 +14967,18 @@ F:	drivers/media/i2c/max9271.c
> >  F:	drivers/media/i2c/max9271.h
> >  F:	drivers/media/i2c/rdacm20.c
> > 
> > +RDACM21 Camera Sensor
> > +M:	Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > +M:	Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > +M:	Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > +M:	Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > +L:	linux-media@xxxxxxxxxxxxxxx
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> > +F:	drivers/media/i2c/max9271.c
> > +F:	drivers/media/i2c/max9271.h
> > +F:	drivers/media/i2c/rdacm21.c
> > +
> >  RDC R-321X SoC
> >  M:	Florian Fainelli <florian@xxxxxxxxxxx>
> >  S:	Maintained
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 2b9d81e4794a..d500edb8638b 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -1212,6 +1212,19 @@ config VIDEO_RDACM20
> >  	  This camera should be used in conjunction with a GMSL
> >  	  deserialiser such as the MAX9286.
> > 
> > +config VIDEO_RDACM21
> > +	tristate "IMI RDACM21 camera support"
> > +	depends on I2C
> > +	select V4L2_FWNODE
> > +	select VIDEO_V4L2_SUBDEV_API
> > +	select MEDIA_CONTROLLER
> > +	help
> > +	  This driver supports the IMI RDACM21 GMSL camera, used in
> > +	  ADAS systems.
> > +
> > +	  This camera should be used in conjunction with a GMSL
> > +	  deserialiser such as the MAX9286.
> > +
> >  config VIDEO_RJ54N1
> >  	tristate "Sharp RJ54N1CB0C sensor support"
> >  	depends on I2C && VIDEO_V4L2
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index a3149dce21bb..85b1edc62508 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -124,6 +124,8 @@ obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
> >  obj-$(CONFIG_VIDEO_MAX9286)	+= max9286.o
> >  rdacm20-camera_module-objs	:= rdacm20.o max9271.o
> >  obj-$(CONFIG_VIDEO_RDACM20)	+= rdacm20-camera_module.o
> > +rdacm21-camera_module-objs	:= rdacm21.o max9271.o
> > +obj-$(CONFIG_VIDEO_RDACM21)	+= rdacm21-camera_module.o
> >  obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> 
> This isn't right. The max9271 code exports various functions, but since it is
> included with *two* modules (rdacm20/1-camera_module.ko) it is exported twice.
> 
> Since max9271 is not a self-contained driver it should not export symbols.

Shouldn't we instead make max9271 a stand-alone module ?

> Can you provide a patch fixing that?

-- 
Regards,

Laurent Pinchart