Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v8 1/5] media: i2c: Add driver for RDACM21 camera module
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Jacopo Mondi <jacopo+renesas@jmondi.org>,
        kieran.bingham+renesas@ideasonboard.com,
        niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org,
        linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
        linux-kernel@vger.kernel.org, Hyun Kwon <hyunk@xilinx.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        sergei.shtylyov@gmail.com
References: <20210114170429.139762-1-jacopo+renesas@jmondi.org>
 <20210114170429.139762-2-jacopo+renesas@jmondi.org>
 <0b118385-70bc-01eb-8ddf-829c604d533b@xs4all.nl>
 <YCEgcPv1TWe0DN50@pendragon.ideasonboard.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <c0db4607-7e2b-ac88-c9d2-d1c5f75bbf86@xs4all.nl>
Date:   Mon, 8 Feb 2021 12:34:55 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <YCEgcPv1TWe0DN50@pendragon.ideasonboard.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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