Re: [PATCH v9 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:
- [Medium] The G215HVN011 panel overlay hardcodes the backlight enable GPIO, breaking the backlight functionality when used with the native i.MX8MP SBC LVDS interface.
- [Low] Mismatch between the mathematical comment and the actual assigned clock rate for the G156HCE-L01 panel.
- [Low] The built-in i.MX8MP LVDS overlay inherits and incorrectly configures the external adapter's SoC pinctrls for its regulators.
--

--- Patch [2]: [PATCH v9 2/7] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
commit 9eb4f5b623d7a9a7866c31e6663fe324b4e72663
Author: Marek Vasut <[email protected]>

arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC

Add DT overlay for the DSI-to-LVDS adapter eDM-MOD-iMX8Mm-LVDS populated
with Lontium LT9211 bridge. This adapter can be optionally populated onto
the eDM SBC.

> 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
> index 0000000000000..3585170bd59dd
> --- /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>;

[Severity: Medium]
Does hardcoding enable-gpios here break the backlight for the native
i.MX8MP SBC LVDS interface?

Since the native LVDS overlay does not override this property, the driver
will toggle the SoC GPIO3_IO00 instead of the correct I2C expander pin,
which might leave the screen black.

Could we abstract this using the reg_panel_bl node instead?

> 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
> index 0000000000000..86163d6ddd5c6
> --- /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 .
> +	 */
> +	assigned-clock-rates = <500000000>, <200000000>,
> +			       <0>, <0>, <500000000>, <993020000>;
> +};

[Severity: Low]
Is there a typo in the comment here? It mentions 993.2 MHz, but the code
configures 993020000 (993.02 MHz).

> 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
> index 0000000000000..95f4761869d75
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds.dtsi

[ ... ]

> +&reg_panel_bl {
> +	gpio = <&gpiowifi 0 0>;
> +	vin-supply = <&reg_panel_bl_supply>;
> +};
> +
> +&reg_panel_vcc {
> +	enable-gpios = <&gpiowifi 4 0>;

[Severity: Low]
Since this native LVDS overlay routes the regulator GPIOs to the gpiowifi
I2C expander, does it also need to delete the pinctrl-0 properties inherited
from the base tree and external adapter include?

Without /delete-property/ pinctrl-0; the regulators might still needlessly
request and configure the SoC pins via pinctrl.

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