Re: [PATCH v5 1/6] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The backlight enable GPIO is hardcoded for the i.MX8MM adapter in the G215HVN011 panel overlay, breaking the backlight on the i.MX8MP native LVDS board.
- [Medium] Incorrect voltage mapping for state 0x3 in the AP63300 regulator configuration, which could lead to severe overvoltage.
- [Medium] The common abstraction file contains an adapter-specific dangling phandle reference to `&gpio_display`.
- [Low] The patch modifies a core SoC file (`imx8mp.dtsi`) to add the `spba5` label, which is undocumented in the commit message.
- [Low] The MP2328 voltage divider calculation comment is mathematically incorrect for the specified resistors.
- [Low] The MIPI DSI burst-clock-frequency comment for the G215HVN01.1 panel is incorrect.
--

--- Patch [1]: [PATCH v5 1/6] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
commit 0857188ec12e2f2c2c27ef0295f8548ec44a3213
Author: Marek Vasut <marex@nabladev.com>
arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC

This commit adds device tree overlays for the DSI-to-LVDS adapter 
eDM-MOD-iMX8Mm-LVDS populated with Lontium LT9211 bridge. It provides 
support for multiple panels and is intended for use with both the 
i.MX8M Mini and i.MX8M Plus eDM SBCs.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-common.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-common.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-common.dtsi
[ ... ]
> +	reg_backlight_pwm_level: regulator-backlight-pwm-level {
> +		compatible = "regulator-gpio";
> +		regulator-name = "Backlight_SEL_PWM";
> +		regulator-type = "voltage";
> +		gpios = <&gpio_display 2 GPIO_ACTIVE_HIGH>; /* SEL_PWM */

[Severity: Medium]
Does this create a dangling phandle reference when included by the native 
i.MX8MP adapter overlay? It looks like gpio_display is undefined in that 
context, which might inject a leaky abstraction into the common file.

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-lvds-common.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-lvds-common.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-lvds-common.dtsi
[ ... ]
> +&reg_panel_vcc {
> +	/*
> +	 * AP63300 voltage divider settings:
> +	 *   R1=16k2
> +	 *   R2=5k23 with optional series Rs=7k68 (5V) or Rt=1k5 (12V)
> +	 *
> +	 * 1 / Rx = (1 / R2) [ + (1 / Rs)][ + (1 / Rt)]
> +	 * Vout = 0.8 * ((R1 / Rx) + 1)
> +	 */
> +	gpios = <&gpio_display 1 GPIO_ACTIVE_HIGH>,	/* 5V */
> +		<&gpio_display 0 GPIO_ACTIVE_HIGH>;	/* 12V */
> +	states = <3300000 0x0>,
> +		 <5000000 0x1>,
> +		 <12000000 0x2>,
> +		 <3900000 0x3>;

[Severity: Medium]
Is the state mapping for 0x3 safe to leave at 3900000? Based on the 
hardware calculations, it appears this state would actually output around 
13.6V. Could this lead to severe overvoltage and hardware damage if a 
future panel requests 3.9V and the regulator framework switches to this 
state?

> +};

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-lvds-g215hvn011.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-lvds-g215hvn011.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-edm-mod-imx8mm-lvds-g215hvn011.dtsi
[ ... ]
> +&mipi_dsi {
> +	samsung,burst-clock-frequency = <864000000>;	/* RX ByteClock ~27 MHz */

[Severity: Low]
Is the comment regarding the RX ByteClock frequency accurate here? An 
864 MHz burst clock usually yields a byte clock of 108 MHz rather than 
27 MHz.

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-lvds-g215hvn011.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-lvds-g215hvn011.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc-overlay-lvds-g215hvn011.dtsi
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2022-2026 Marek Vasut
> + */
> +
> +#include "imx8mm-data-modul-edm-sbc-overlay-lvds-5v0.dtsi"
> +#include "imx8mm-data-modul-edm-sbc-overlay-lvds-dual.dtsi"
> +
> +&backlight {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_panel_backlight>;
> +	enable-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>;

[Severity: High]
Does hardcoding this i.MX8MM-specific GPIO break the backlight when this 
overlay is used on the native i.MX8MP LVDS board? It seems this bypasses 
the abstract reg_panel_bl regulator. When the i.MX8MP overlay includes 
this file, it overrides the power supply but does not override this 
enable-gpios property, potentially causing it to toggle the wrong pin.

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g215hvn011.dtso b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g215hvn011.dtso
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g215hvn011.dtso
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2023-2026 Marek Vasut
> + */
> +
> +#include "imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds.dtsi"
> +#include "imx8mm-data-modul-edm-sbc-overlay-lvds-g215hvn011.dtsi"
> +
> +&backlight {
> +	power-supply = <&reg_panel_bl_supply>;
> +};

[Severity: High]
(Related to the previous point) Should this also override enable-gpios to 
prevent the i.MX8MM specific pin from being inherited?

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds.dtsi
[ ... ]
> +&{/} {
> +	reg_panel_bl_supply: regulator-panel-bl-supply {
> +		compatible = "regulator-gpio";
> +		regulator-type = "voltage";
> +		regulator-name = "PANEL_BL_SUPPLY";
> +		enable-gpios = <&gpiolvds 0 0>;
> +		enable-active-high;
> +		status = "okay";
> +
> +		/*
> +		 * MP2328 voltage divider settings:
> +		 *   R1=51k1
> +		 *   R2=5k62 with optional series Rs=2k21 (12V)
> +		 *
> +		 * 1 / Rx = (1 / R2) [ + (1 / Rs)][ + (1 / Rt)]
> +		 * Vout = 0.5 + ((R1 / Rx) * 0.5)
> +		 */

[Severity: Low]
Is the mathematical formula in this comment accurate for this circuit?
If the resistors were placed in parallel, the formula 
1 / Rx = (1 / R2) + (1 / Rs) with R1=51.1k, R2=5.62k, and Rs=2.21k would 
yield 16.6V instead of 12V. Does the hardware actually switch between the 
resistors rather than placing them in parallel?

[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 90d7bb8f5619e..42a3216daed44 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1437,7 +1437,7 @@ aips5: bus@30df0000 {
>  			#access-controller-cells = <3>;
>  			ranges = <0x30c00000 0x30c00000 0x400000>;
>  
> -			spba-bus@30c00000 {
> +			spba5: spba-bus@30c00000 {
>  				compatible = "fsl,spba-bus", "simple-bus";
>  				reg = <0x30c00000 0x100000>;
>  				#address-cells = <1>;

[Severity: Low]
Could this core SoC DTSI modification be split into a separate patch?
Adding the spba5 label here modifies a common architectural file silently 
during a board-level overlay patch, and it does not appear to be 
mentioned in the commit message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716055751.279033-1-marex@nabladev.com?part=1