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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Jul 25, 2025 at 11:38:29AM -0400, Frank Li wrote:
> 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@nxp.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@nxp.com>
> > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
> > > Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.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?

There are tools such as include-what-you-use, but I haven't tested that
one with the kernel.

> > > +
> > > +#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.

OK. I wonder if you could then replace some (most ? all ?) of the
read-update-write sequences with plain writes.

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