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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/02/2021 12:28, Laurent Pinchart wrote:
> 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@ideasonboard.com>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  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@jmondi.org>
>>> +M:	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> +M:	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> +M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> +L:	linux-media@vger.kernel.org
>>> +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@openwrt.org>
>>>  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 ?

If that makes sense, then that's the alternative option. I don't know if it
can be used with anything else but rdacm20/21.

Either way, this needs to be fixed.

Regards,

	Hans

> 
>> Can you provide a patch fixing that?
>