linux-kernel - Re: [PATCH v3 2/4] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <aIOk9W/RBKmS8uVQ@lizhi-Precision-Tower-5810>
Date: Fri, 25 Jul 2025 11:38:29 -0400
From: Frank Li <Frank.li@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Rui Miguel Silva <rmfrfs@...il.com>,
	Martin Kepplinger <martink@...teo.de>,
	Purism Kernel Team <kernel@...i.sm>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Alice Yuan <alice.yuan@....com>,
	Robert Chiras <robert.chiras@....com>,
	Zhipeng Wang <zhipeng.wang_1@....com>
Subject: Re: [PATCH v3 2/4] media: nxp: add V4L2 subdev driver for camera
 parallel interface (CPI)

On Fri, Jul 25, 2025 at 03:04:04AM +0300, Laurent Pinchart wrote:
> Hi Frank,
>
> Thank you for the patch.
>
> On Tue, Jul 08, 2025 at 01:48:43PM -0400, Frank Li via B4 Relay wrote:
> > From: Alice Yuan <alice.yuan@....com>
> >
> > Add a V4L2 sub-device driver for the CPI controller found on i.MX8QXP,
> > i.MX8QM, and i.MX93 SoCs. This controller supports parallel camera sensors
> > and enables image data capture through a parallel interface.
> >
> > Signed-off-by: Alice Yuan <alice.yuan@....com>
> > Signed-off-by: Robert Chiras <robert.chiras@....com>
> > Signed-off-by: Zhipeng Wang <zhipeng.wang_1@....com>
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> > change in v3
> > - replace csi with cpi
> > - use __free(fwnode_handle) to simpilfy code
> > - remove imx91 driver data, which is the same as imx93
> >
> > change in v2
> > - remove MODULE_ALIAS
> > - use devm_pm_runtime_enable() and cleanup remove function
> > - change output format to 1x16. controller convert 2x8 to 1x16 format
> > ---
> >  MAINTAINERS                                   |   1 +
> >  drivers/media/platform/nxp/Kconfig            |  11 +
> >  drivers/media/platform/nxp/Makefile           |   1 +
> >  drivers/media/platform/nxp/imx-parallel-cpi.c | 920 ++++++++++++++++++++++++++
> >  4 files changed, 933 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8ae0667d2bb41fb6a1549bd3b2b33f326cbd1303..14842a3b860a6f23846f12a684eedcbb9eb69e19 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15112,6 +15112,7 @@ F:	Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
> >  F:	Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> >  F:	Documentation/devicetree/bindings/media/nxp,imx8mq-mipi-csi2.yaml
> >  F:	drivers/media/platform/nxp/imx-mipi-csis.c
> > +F:	drivers/media/platform/nxp/imx-parallel-cpi.c
> >  F:	drivers/media/platform/nxp/imx7-media-csi.c
> >  F:	drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> >
> > diff --git a/drivers/media/platform/nxp/Kconfig b/drivers/media/platform/nxp/Kconfig
> > index 40e3436669e213fdc5da70821dc0b420e1821f4f..504ae1c6494f331c16124a224421ac7acd433ba5 100644
> > --- a/drivers/media/platform/nxp/Kconfig
> > +++ b/drivers/media/platform/nxp/Kconfig
> > @@ -39,6 +39,17 @@ config VIDEO_IMX_MIPI_CSIS
> >  	  Video4Linux2 sub-device driver for the MIPI CSI-2 CSIS receiver
> >  	  v3.3/v3.6.3 found on some i.MX7 and i.MX8 SoCs.
> >
> > +config VIDEO_IMX_PARALLEL_CPI
> > +	tristate "NXP i.MX9/i.MX8 Parallel CPI Driver"
>
> I'd write it in numerical order, "i.MX8/i.MX9"
>
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	depends on VIDEO_DEV
> > +	select MEDIA_CONTROLLER
> > +	select V4L2_FWNODE
> > +	select VIDEO_V4L2_SUBDEV_API
> > +	help
> > +	  Video4Linux2 sub-device driver for PARALLEL CPI receiver found
> > +	  on some iMX8 and iMX9 SoCs.
> > +
> >  source "drivers/media/platform/nxp/imx8-isi/Kconfig"
> >
> >  # mem2mem drivers
> > diff --git a/drivers/media/platform/nxp/Makefile b/drivers/media/platform/nxp/Makefile
> > index 4d90eb71365259ebdda84ea58483e1c4131d3ac7..5346919d2f1083b51ec99b66981c5d38b3df960c 100644
> > --- a/drivers/media/platform/nxp/Makefile
> > +++ b/drivers/media/platform/nxp/Makefile
> > @@ -7,5 +7,6 @@ obj-y += imx8-isi/
> >  obj-$(CONFIG_VIDEO_IMX7_CSI) += imx7-media-csi.o
> >  obj-$(CONFIG_VIDEO_IMX8MQ_MIPI_CSI2) += imx8mq-mipi-csi2.o
> >  obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx-mipi-csis.o
> > +obj-$(CONFIG_VIDEO_IMX_PARALLEL_CPI) += imx-parallel-cpi.o
> >  obj-$(CONFIG_VIDEO_IMX_PXP) += imx-pxp.o
> >  obj-$(CONFIG_VIDEO_MX2_EMMAPRP) += mx2_emmaprp.o
> > diff --git a/drivers/media/platform/nxp/imx-parallel-cpi.c b/drivers/media/platform/nxp/imx-parallel-cpi.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..718f02bf70c4d0ae74ecf842c1ecb1a1afbcdd45
> > --- /dev/null
> > +++ b/drivers/media/platform/nxp/imx-parallel-cpi.c
> > @@ -0,0 +1,920 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * i.MX Parallel CPI receiver driver.
> > + *
> > + * Copyright 2019-2025 NXP
> > + *
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
>
> I don't think you use this.
>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/interrupt.h>
>
> I don't think you use this either.
>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/mfd/syscon.h>
>
> Alphabetical order please.
>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
>
> This doesn't seem needed.
>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/reset.h>
>
> Same here.
>
> > +#include <linux/spinlock.h>
>
> And here. Please verify all headers, drop the ones you don't need, and
> add any you may have forgotten. That includes the media/ headers.

Do you have good method to check it?

>
> > +
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-fwnode.h>
> > +#include <media/v4l2-mc.h>
> > +#include <media/v4l2-subdev.h>

...

> > +
> > +	/* Software Reset */
> > +	imx_cpi_sw_reset(pcpidev);
> > +
> > +	/* Config PL Data Type */
> > +	val = readl(pcpidev->regs + pdata->if_ctrl_reg);
> > +	val |= IF_CTRL_REG_DATA_TYPE(DATA_TYPE_OUT_YUV444);
>
> Don't you need to clear the field first ? Same everywhere else where
> applicable.

imx_cpi_sw_reset() clean all register.

Frank

>
> > +	writel(val, pcpidev->regs + pdata->if_ctrl_reg);
> > +
> > +	/* Enable sync Force */
> > +	val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
> > +	val |= (CPI_CTRL_REG_HSYNC_FORCE_EN | CPI_CTRL_REG_VSYNC_FORCE_EN);
> > +	writel(val, pcpidev->regs + pdata->interface_ctrl_reg);

....

> --
> Regards,
>
> Laurent Pinchart