Re: [PATCH v4 1/2] arm64: dts: imx8mq-evk: Add OV5640 camera support via overlays

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dual camera setup fails due to shared reset GPIO exclusivity causing `-EBUSY` and unexpected hardware resets.
- [Medium] Overlay-specific pinmux configurations and unconditional hogs pollute the base device tree.
--

commit d401c57ae9a7ef9c71733905b73812d521acf9a9
Author: Robby Cai <robby.cai@nxp.com>

arm64: dts: imx8mq-evk: Add OV5640 camera support via overlays

This patch adds device tree overlays for single and dual OV5640 camera
setups on the i.MX8MQ EVK board. It configures media pipelines for CSI1
and CSI2, noting that both sensors share a common reset GPIO and MCLK
source.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso
> new file mode 100644
> index 0000000000000..1e9931802cdc4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi1.dtso

[ ... ]

> +	camera@3c {
> +		compatible = "ovti,ov5640";
> +		reg = <0x3c>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_camera1_pwdn>;
> +		clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		clock-names = "xclk";
> +		assigned-clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		assigned-clock-parents = <&clk IMX8MQ_SYS2_PLL_200M>;
> +		assigned-clock-rates = <20000000>;
> +		powerdown-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;

[Severity: High]
Is it safe to assign this reset GPIO (&gpio1 6) when it is also assigned
identically in the CSI2 overlay?

Since the GPIO framework enforces exclusive access, won't the second camera
to probe fail with -EBUSY?

Additionally, when ov5640_powerup_sequence() toggles this shared reset line
during power-on, couldn't this silently hard-reset the other active camera
and break its stream?

> +		DOVDD-supply = <&sw4_reg>;
> +		AVDD-supply = <&reg_2v8>;
> +		DVDD-supply = <&reg_1v5>;

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso
> new file mode 100644
> index 0000000000000..fd247b3b59825
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk-ov5640-csi2.dtso

[ ... ]

> +	camera@3c {
> +		compatible = "ovti,ov5640";
> +		reg = <0x3c>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_camera2_pwdn>;
> +		clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		clock-names = "xclk";
> +		assigned-clocks = <&clk IMX8MQ_CLK_CLKO2>;
> +		assigned-clock-parents = <&clk IMX8MQ_SYS2_PLL_200M>;
> +		assigned-clock-rates = <20000000>;
> +		powerdown-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;

[Severity: High]
This assigns the exact same reset GPIO (&gpio1 6) as the CSI1 overlay,
which can cause probe contention and hardware state corruption as described
above.

> +		DOVDD-supply = <&sw4_reg>;
> +		AVDD-supply = <&reg_2v8>;
> +		DVDD-supply = <&reg_1v5>;

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> index d48f901487d49..1260bb1d9069b 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts

[ ... ]

> @@ -532,12 +546,34 @@ &wdog1 {
>  };
>  
>  &iomuxc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_mclk>, <&pinctrl_camera_reset>;

[Severity: Medium]
Does unconditionally hogging these camera-specific pins in the base device
tree unnecessarily consume GPIOs when the optional camera overlays are not
applied?

>  	pinctrl_buck2: vddarmgrp {
>  		fsl,pins = <
>  			MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13		0x19
>  		>;
>  	};
>  
> +	pinctrl_camera1_pwdn: camera1pwdngrp {

[Severity: Medium]
Could defining these overlay-specific pin groups in the base device tree,
rather than isolating them within their respective overlays, break the
modularity of the device tree overlays?

> +		fsl,pins = <
> +			MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3		0x19
> +		>;
> +	};
> +
> +	pinctrl_camera2_pwdn: camera2pwdngrp {
> +		fsl,pins = <
> +			MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5		0x19
> +		>;
> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619100532.3779934-1-robby.cai@oss.nxp.com?part=1