linux-kernel - Re: [PATCH v4 06/11] media: rockchip: rkcif: add driver for mipi csi-2 host

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <137eeb26-65df-4dfa-bece-08a7758ae5c2@wolfvision.net>
Date: Thu, 6 Mar 2025 10:05:22 +0100
From: Michael Riesch <michael.riesch@...fvision.net>
To: Mehdi Djait <mehdi.djait@...ux.intel.com>
Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Gerald Loacker <gerald.loacker@...fvision.net>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Sebastian Fricke <sebastian.fricke@...labora.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Paul Kocialkowski <paulk@...-base.io>,
 Alexander Shiyan <eagle.alexander923@...il.com>,
 Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v4 06/11] media: rockchip: rkcif: add driver for mipi
 csi-2 host

Hi Mehdi,

On 3/4/25 20:41, Mehdi Djait wrote:
> Hi Michael,
> 
> thank you for the patches.
> 
> Sorry for the big delay!

No worries, thanks for your comments!

> 
> On Wed, Feb 19, 2025 at 11:16:37AM +0100, Michael Riesch wrote:
>> The Rockchip RK3568 MIPI CSI-2 Host is a CSI-2 bridge with one input port
>> and one output port. It receives the data with the help of an external
>> MIPI PHY (C-PHY or D-PHY) and passes it to the Rockchip RK3568 Video
>> Capture (VICAP) block. Add a V4L2 subdevice driver for this unit.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@...fvision.net>
>> ---
>>  drivers/media/platform/rockchip/rkcif/Makefile     |   1 +
>>  .../platform/rockchip/rkcif/rkcif-mipi-csi-host.c  | 731 +++++++++++++++++++++
>>  2 files changed, 732 insertions(+)
>>
>> diff --git a/drivers/media/platform/rockchip/rkcif/Makefile b/drivers/media/platform/rockchip/rkcif/Makefile
>> index 818424972c7b..0c18efd1f1b4 100644
>> --- a/drivers/media/platform/rockchip/rkcif/Makefile
>> +++ b/drivers/media/platform/rockchip/rkcif/Makefile
>> @@ -4,4 +4,5 @@ rockchip-cif-objs += rkcif-dev.o \
>>  	rkcif-capture-dvp.o \
>>  	rkcif-capture-mipi.o \
>>  	rkcif-interface.o \
>> +	rkcif-mipi-csi-host.o \
> 
> [..]
> 
>>  	rkcif-stream.o
>> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-host.c b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-host.c
>> new file mode 100644
>> index 000000000000..fa3f42b2dc55
> 
> SNIP
> 
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-mipi-csi-host.c
>> +
>> +static struct platform_driver rkcif_csi_drv = {
>> +	.driver = {
>> +		   .name = "rockchip-mipi-csi",
>> +		   .of_match_table = rkcif_csi_of_match,
>> +		   .pm = &rkcif_csi_pm_ops,
>> +	},
>> +	.probe = rkcif_csi_probe,
>> +	.remove = rkcif_csi_remove,
>> +};
>> +module_platform_driver(rkcif_csi_drv);
> 
> [..]
> 
> When adding the driver for this CSI-2 Host bridge, you added another
> call to  module_platform_driver()
> 
> but in the definition of this macro:
> 
> "Each module may only use this macro once, and
> calling it replaces module_init() and module_exit()"
> 
> and as you can see in the diff of the Makefile,
> rkcif-mipi-csi-host.0 is part of the same module as rkcif-dev.o, where
> you already call module_platform_driver()

Indeed. Found that only after submitting v4... :-/

> I think the solution here is to call
> platform_register_drivers(drivers, ARRAY_SIZE(drivers)) in rkcif-dev.c
> 
> with
> 
> static struct platform_driver * const drivers[] = {
> 	&rkcif_csi_drv,
> 	&rkcif_plat_drv,
> };
> 
> then define module_init() and module_exit()

Ah, I haven't thought of that approach. Actually, I was going to split
the cif part and the mipi host part into two modules. This would keep
things modular (literally). What do you think about that?

> 
> Btw. MODULE_DEVICE_TABLE() is missing both here and in rkcif-dev.c

Huh, indeed. Nice catch, thanks!

Best regards,
Michael

> 
>> +
>> +MODULE_DESCRIPTION("Rockchip MIPI CSI-2 Host platform driver");
>> +MODULE_LICENSE("GPL");
>>
>> -- 
>> 2.34.1
>>
> 
> --
> Kind Regards
> Mehdi Djait