All of lore.kernel.org
 help / color / mirror / Atom feed
From: Frank Li <Frank.li@oss.nxp.com>
To: sashiko-reviews@lists.linux.dev
Cc: Marek Vasut <marex@nabladev.com>,
	Frank.Li@kernel.org, imx@lists.linux.dev,
	devicetree@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org
Subject: Re: [PATCH v7 2/7] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC
Date: Thu, 30 Jul 2026 10:31:13 -0500	[thread overview]
Message-ID: <amtuQUa0aPGdlRCT@SMW015318> (raw)
In-Reply-To: <20260729220634.9AEC31F000E9@smtp.kernel.org>

On Wed, Jul 29, 2026 at 10:06:34PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Incorrect backlight enable GPIO and pinctrl applied for the g215hvn011 panel on the native i.MX8MP LVDS interface.
> - [Medium] The `assigned-clock-rates` property in `&media_blk_ctrl` lacks the required `assigned-clocks` property, causing the LVDS panel clock configuration to be silently ignored.
> --
>
> --- Patch [2]: [PATCH v7 2/7] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC ---
> commit 57ce2b382e55c1c98951209e8e0a8f4ea3f2c65c
> Author: Marek Vasut <marex@nabladev.com>
>
> arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC
>
> This patch adds DT overlays for the DSI-to-LVDS adapter populated with a
> Lontium LT9211 bridge on Data Modul i.MX8M Mini and Plus eDM SBCs. It
> supports multiple panels for both the bridge and native i.MX8MP LVDS
> interfaces.
>
> > 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
> > @@ -0,0 +1,30 @@
> [ ... ]
> > +&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 */
>
> [ ... ]
>
> > 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
> > index 0000000000000..a6d13c3609796
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g215hvn011.dtso
> > @@ -0,0 +1,36 @@
> [ ... ]
> > +&backlight {
> > +	power-supply = <&reg_panel_bl_supply>;
> > +};
>
> [Severity: Medium]
> Will this result in the wrong GPIO being toggled for the native i.MX8MP LVDS
> interface?
>
> The shared imx8mm-data-modul-edm-sbc-overlay-lvds-g215hvn011.dtsi file hardcodes
> enable-gpios to &gpio3 0 and defines a pinctrl. However, the native i.MX8MP
> LVDS interface uses the IO expander's &gpiowifi 0 (via reg_panel_bl in
> edm-sbc-imx8mp-lvds.dtsi).
>
> Since this overlay only overrides power-supply and does not remove the
> inapplicable pinctrl or override enable-gpios, won't the backlight driver
> toggle the incorrect baseboard adapter GPIO pin instead of the intended one?

I am not sure what's bad impact to wrong toggle enable-gpios. need check
this,

change exist ones to keep consistenet, all use enable-gpios or power-supply.


>
> [ ... ]
>
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g101ice-l01.dtso b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g101ice-l01.dtso
> > new file mode 100644
> > index 0000000000000..817d4ec62d0e8
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc-overlay-edm-sbc-imx8mp-lvds-g101ice-l01.dtso
> > @@ -0,0 +1,24 @@
> [ ... ]
> > +&media_blk_ctrl {
> > +	/*
> > +	 * The G101ICE-L01 panel requires 71.1 MHz LVDS clock.
> > +	 * Set IMX8MP_VIDEO_PLL1 to 497.7 MHz , since 497.7 MHz / 7 = 71.1 MHz .
> > +	 */
> > +	assigned-clock-rates = <500000000>, <200000000>,
> > +			       <0>, <0>, <500000000>, <497700000>;
> > +};
>
> [Severity: Medium]
> Will this clock configuration be silently ignored by the clock framework?
>
> This overlay (and several others for the native LVDS interface) defines
> assigned-clock-rates without providing or inheriting an accompanying
> assigned-clocks array.
>
> The base board file imx8mp-data-modul-edm-sbc.dts does not include
> imx8mp-nominal.dtsi, where the assigned-clocks array is typically defined.
>
> When assigned-clocks is missing, of_parse_phandle_with_args() returns -ENOENT,
> and __set_clk_rates() silently skips setting the rates. Should assigned-clocks
> be explicitly defined here so the panel clock frequency is correctly set

This is most like false alarm. But for better read purpose, suggest
write whole group property

assign-clocks
assign-clock-parents
assign-clock-rates

Incase some change order in soc dtsi and forget update here and cause
wrong.

I met similar issue before.

the similar case is clocks and clock-names, should appear in pair at
every where.

Frank

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

  reply	other threads:[~2026-07-30 15:31 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-29 21:47 [PATCH v7 1/7] arm64: dts: imx8mp: Add DT label to SPBA5 bus Marek Vasut
2026-07-29 21:47 ` [PATCH v7 2/7] arm64: dts: imx8mm: imx8mp: Add LVDS DTOs for Data Modul i.MX8M Mini and Plus eDM SBC Marek Vasut
2026-07-29 22:06   ` sashiko-bot
2026-07-30 15:31     ` Frank Li [this message]
2026-07-30 19:14       ` Marek Vasut
2026-07-29 21:47 ` [PATCH v7 3/7] arm64: dts: imx8mm: imx8mp: Add legacy board " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 4/7] arm64: dts: imx8mm: imx8mp: Add HDMI " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 5/7] arm64: dts: imx8mm: imx8mp: Add CM " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 6/7] arm64: dts: imx8mm: imx8mp: Add FIO1 " Marek Vasut
2026-07-29 21:47 ` [PATCH v7 7/7] arm64: dts: imx8mm: imx8mp: Add FIO1-Audio " Marek Vasut
2026-07-29 22:10   ` sashiko-bot
2026-07-30 15:17     ` Frank Li
2026-07-30 19:34       ` Marek Vasut
2026-07-31 15:23         ` Frank Li
2026-07-31 15:56           ` Marek Vasut

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=amtuQUa0aPGdlRCT@SMW015318 \
    --to=frank.li@oss.nxp.com \
    --cc=Frank.Li@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=imx@lists.linux.dev \
    --cc=marex@nabladev.com \
    --cc=robh@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.