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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardcoded backlight `enable-gpios` in the shared panel overlay breaks native LVDS backlight control on i.MX8MP for the G215HVN011 display.
- [Low] Orphaned `enable-active-high` property left in `reg_panel_vcc`.
- [Low] Mathematically inaccurate comment regarding the LVDS clock derivation.
--

--- Patch [2]: [PATCH v6 2/7] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
commit 261fc4aa3222bc2beb75e81148227597b2b74f39
Author: Marek Vasut <[email protected]>
Subject: 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 populated
with a Lontium LT9211 bridge, supporting multiple panels on the eDM SBC. It
establishes base board DTS updates and panel-specific definitions.

> 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

[ ... ]

> +&backlight {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_panel_backlight>;
> +	enable-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>;
> +	/* 6.5 POWER ON/OFF SEQUENCE, T6 >= 10 ms */
> +	post-pwm-on-delay-ms = <10>;
> +	/* 6.5 POWER ON/OFF SEQUENCE, T7 >= 0 ms */
> +	pwm-off-delay-ms = <10>;
> +	/* 5.2 BACKLIGHT UNIT 200Hz..20kHz, value below in ns */
> +	pwms = <&pwm1 0 66666 0>;	/* 15 kHz = 66666ns */
> +	status = "okay";
> +};

[ ... ]

> 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

[ ... ]

> +&backlight {
> +	power-supply = <&reg_panel_bl_supply>;
> +};

[Severity: High]
Will the display stay dark on the i.MX8MP native LVDS interface?

The shared panel definition imx8mm-data-modul-edm-sbc-overlay-lvds-g215hvn011.dtsi
above hardcodes enable-gpios to &gpio3 0 and sets its associated pinctrl.

While that is the correct pin for the i.MX8MM DSI-to-LVDS adapter, the native
LVDS interface on i.MX8MP uses &gpiowifi 0 instead. The i.MX8MP-specific
overlay sets power-supply here but fails to override the enable-gpios and
pinctrl-0 properties inherited from the shared dtsi.

Because the backlight driver will request the wrong GPIO, the backlight enable
pin will not be asserted correctly.

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> --- a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts

[ ... ]

> @@ -63,7 +63,6 @@ reg_panel_vcc: regulator-panel-vcc {
>  		regulator-name = "PANEL_VCC";
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
> -		gpio = <&gpio3 6 0>;
>  		enable-active-high;
>  		/* Disabled by default, unless display board plugged in. */
>  		status = "disabled";

[Severity: Low]
Does this leave an orphaned enable-active-high property?

The gpio assignment was removed, but enable-active-high remains.
It looks like this was cleaned up completely in the i.MX8MP baseboard dts.

[ ... ]

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g156hce-l01.dtso b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g156hce-l01.dtso
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g156hce-l01.dtso

[ ... ]

> +&media_blk_ctrl {
> +	/*
> +	 * The G156HCE-L01 panel requires 141.86 MHz LVDS clock.
> +	 * Set IMX8MP_VIDEO_PLL1 to 993.2 MHz , since 993.2 MHz / 7 = 141.86 MHz .
> +	 */

[Severity: Low]
This is a minor typo, but should 993.2 MHz be 993.02 MHz here?

993.2 / 7 does not equal 141.86, and the code below correctly configures
it to 993020000 (993.02 MHz).

> +	assigned-clock-rates = <500000000>, <200000000>,
> +			       <0>, <0>, <500000000>, <993020000>;
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2