Re: [PATCH 5/5] arm64: dts: imx8qxp-mek: add parallel ov5640 camera support

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

Hi Frank,

On Tue, Jul 01, 2025 at 11:08:24AM -0400, Frank Li wrote:
> On Tue, Jul 01, 2025 at 02:06:51AM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 30, 2025 at 06:28:21PM -0400, Frank Li wrote:
> > > Add parallel ov5640 nodes in imx8qxp-mek and create overlay file to enable
> > > it because it can work at two mode: MIPI and parallel mode.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  arch/arm64/boot/dts/freescale/Makefile             |  3 ++
> > >  arch/arm64/boot/dts/freescale/imx8qxp-mek.dts      | 37 +++++++++++++++++++
> > >  .../dts/freescale/imx8x-mek-ov5640-parallel.dtso   | 43 ++++++++++++++++++++++
> > >  3 files changed, 83 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> > > index 02ef35578dbc7..a9fb11ccd3dea 100644
> > > --- a/arch/arm64/boot/dts/freescale/Makefile
> > > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > > @@ -330,6 +330,9 @@ dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek-pcie-ep.dtb
> > >  imx8qxp-mek-ov5640-csi-dtbs := imx8qxp-mek.dtb imx8qxp-mek-ov5640-csi.dtbo
> > >  dtb-${CONFIG_ARCH_MXC} += imx8qxp-mek-ov5640-csi.dtb
> > >
> > > +imx8qxp-mek-ov5640-parallel-dtbs := imx8qxp-mek.dtb imx8x-mek-ov5640-parallel.dtbo
> > > +dtb-${CONFIG_ARCH_MXC} += imx8qxp-mek-ov5640-parallel.dtb
> > > +
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8qxp-tqma8xqp-mba8xx.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8qxp-tqma8xqps-mb-smarc-2.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8ulp-evk.dtb
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
> > > index c95cb8acc360a..09eb85a9759e2 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
> > > @@ -487,6 +487,23 @@ pca6416: gpio@20 {
> > >  		#gpio-cells = <2>;
> > >  	};
> > >
> > > +	ov5640_pi: camera@3c {
> > > +		compatible = "ovti,ov5640";
> > > +		reg = <0x3c>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_parallel_csi>;
> > > +		clocks = <&pi0_misc_lpcg IMX_LPCG_CLK_0>;
> > > +		assigned-clocks = <&pi0_misc_lpcg IMX_LPCG_CLK_0>;
> > > +		assigned-clock-rates = <24000000>;
> > > +		clock-names = "xclk";
> > > +		powerdown-gpios = <&lsio_gpio3 2 GPIO_ACTIVE_HIGH>;
> > > +		reset-gpios = <&lsio_gpio3 3 GPIO_ACTIVE_LOW>;
> > > +		AVDD-supply = <&reg_2v8>;
> > > +		DVDD-supply = <&reg_1v5>;
> > > +		DOVDD-supply = <&reg_1v8>;
> > > +		status = "disabled"; /* Overlay enable it */
> > > +	};
> > > +
> >
> > As far as I can tell, the sensor isn't soldered on the board, but is an
> > external module connected through a cable. This DT node should therefore
> > be moved to the overlay.
> 
> It is fine for i.MX8QXP. I put here try to reuse overlay file as much as
> possible.
> 
> For example, imx93 have, 9x9, 11x11, 14x14 boards ...
> 
> Each board's reset-gpios have slice difference. If move whole to overlay
> files, we have to create difference overlay for each boards.
> 
> If keep here and set "status = okay" and update graphic links in overlay,
> we can reuse this overlay file for different boards.
> 
> for example: imx93-pcam.dtso, which simialr with imx8x-mek-ov5640-parallel.dtso.
> 
> So we simple use
> imx93-9x9-qsb.dtb + imx93-pcam.dtbo.
> imx93-11x11-evk.dtb + imx93-pcam.dtbo.
> imx93-14x14-evk.dtb + imx93-pcam.dtbo.
> 
> even for imx91 boards in future.

First of all, I want to say I feel your pain. I maintain many camera
sensor modules DT overlays out-of-tree, for different boards, and code
duplication is definitely annoying. Unfortunately, DT doesn't provide a
solution to this problem today. There was an attempt years ago to design
a "DT connector" that defined how connector signals were routed on a
board. This would allow a single overlay compatible with the connected
to be used with different base boards. Unfortunately, development
stalled.

Until this problem gets solved, I'm afraid you'll have to duplicate
overlays for different base boards. The OV5640 is not part of the
imx8qxp-mek, so it should not be declared in the imx8qxp-mek.dts.

> > >  	cs42888: audio-codec@48 {
> > >  		compatible = "cirrus,cs42888";
> > >  		reg = <0x48>;
> > > @@ -865,6 +882,26 @@ IMX8QXP_MIPI_CSI0_MCLK_OUT_MIPI_CSI0_ACM_MCLK_OUT	0xC0000041
> > >  		>;
> > >  	};
> > >
> > > +	pinctrl_parallel_csi: parallelcsigrp {
> > > +		fsl,pins = <
> > > +			IMX8QXP_CSI_D00_CI_PI_D02				0xc0000041
> > > +			IMX8QXP_CSI_D01_CI_PI_D03				0xc0000041
> > > +			IMX8QXP_CSI_D02_CI_PI_D04				0xc0000041
> > > +			IMX8QXP_CSI_D03_CI_PI_D05				0xc0000041
> > > +			IMX8QXP_CSI_D04_CI_PI_D06				0xc0000041
> > > +			IMX8QXP_CSI_D05_CI_PI_D07				0xc0000041
> > > +			IMX8QXP_CSI_D06_CI_PI_D08				0xc0000041
> > > +			IMX8QXP_CSI_D07_CI_PI_D09				0xc0000041
> > > +
> > > +			IMX8QXP_CSI_MCLK_CI_PI_MCLK				0xc0000041
> > > +			IMX8QXP_CSI_PCLK_CI_PI_PCLK				0xc0000041
> > > +			IMX8QXP_CSI_HSYNC_CI_PI_HSYNC				0xc0000041
> > > +			IMX8QXP_CSI_VSYNC_CI_PI_VSYNC				0xc0000041
> > > +			IMX8QXP_CSI_EN_LSIO_GPIO3_IO02				0xc0000041
> > > +			IMX8QXP_CSI_RESET_LSIO_GPIO3_IO03			0xc0000041
> > > +		>;
> > > +	};
> >
> > Same for this one.
> >
> > > +
> > >  	pinctrl_pcieb: pcieagrp {
> > >  		fsl,pins = <
> > >  			IMX8QXP_PCIE_CTRL0_PERST_B_LSIO_GPIO4_IO00		0x06000021
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8x-mek-ov5640-parallel.dtso b/arch/arm64/boot/dts/freescale/imx8x-mek-ov5640-parallel.dtso
> > > new file mode 100644
> > > index 0000000000000..927d6640662f3
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8x-mek-ov5640-parallel.dtso
> > > @@ -0,0 +1,43 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Copyright 2025 NXP
> > > + */
> > > +
> > > +/dts-v1/;
> > > +/plugin/;
> > > +
> > > +#include <dt-bindings/gpio/gpio.h>
> > > +#include <dt-bindings/media/video-interfaces.h>
> > > +
> > > +&ov5640_pi {
> > > +	status = "okay";
> > > +
> > > +	port {
> > > +		ov5640_pi_ep: endpoint {
> > > +			remote-endpoint = <&parallel_csi_in>;
> > > +			data-lanes = <1 2>;
> >
> > data-lanes is not allowed for parallel buses.
> 
> Do you know there are method in dt_binding to restrict this?
> 
> Frank

We can use if-then-else blocks to restrict the allowed properties based
on the bus-type.

> > > +			bus-type = <MEDIA_BUS_TYPE_PARALLEL>;
> > > +			bus-width = <8>;
> > > +			vsync-active = <0>;
> > > +			hsync-active = <1>;
> > > +			pclk-sample = <1>;
> > > +		};
> > > +	};
> > > +};
> > > +
> > > +&parallel_csi {
> > > +	status = "okay";
> > > +
> > > +	ports {
> > > +		port@0 {
> > > +			parallel_csi_in: endpoint {
> > > +				remote-endpoint = <&ov5640_pi_ep>;
> > > +			};
> > > +		};
> > > +
> > > +	};
> > > +};
> > > +
> > > +&isi {
> > > +	status = "okay";
> > > +};

-- 
Regards,

Laurent Pinchart