Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The TIAN variant overlay incorrectly uses `/ {` instead of `&{/} {`, meaning its root properties will not be applied to the base device tree.
- [High] The `&lcdif2` display controller node was accidentally deleted from the jutouch variant and not added to the common base DT, which will break the display.
- [Medium] In the TIAN overlay, `#address-cells` and `#size-cells` were mistakenly added to `&lcdif2` instead of `&lvds_bridge/ports`, leaving `port@1` without a `reg` property.
--
--- Patch [3]: [PATCH v5 3/5] arm64: dts: imx8mp-skov-revc: convert to overlays ---
commit 3f7282529fd194eae3895a434d1dbbf37a64e617
Author: Steffen Trumtrar <[email protected]>
arm64: dts: imx8mp-skov-revc: convert to overlays
The i.MX8MP Skov RevC jutouch 10" and Tian 7" variants are similar.
This commit moves the common bindings to imx8mp-skov-revc-touch.dts and
converts both variants to use overlays.
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-jutouch-jt101tm023.dtso b/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-jutouch-jt101tm023.dtso
> index 3e320d6dea3a2..b50960879e82d 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-jutouch-jt101tm023.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-jutouch-jt101tm023.dtso
[ ... ]
> @@ -42,38 +40,20 @@ touchscreen@2a {
> };
> };
>
> -&lcdif2 {
> - status = "okay";
> -};
> -
[Severity: High]
Does removing the &lcdif2 node here leave the display controller disabled?
Because this node was not moved to the common imx8mp-skov-revc-touch.dts
base tree, it appears the controller might remain disabled as defined in
the SoC base imx8mp.dtsi, which could break display output for this variant.
> &lvds_bridge {
> - assigned-clocks = <&clk IMX8MP_CLK_MEDIA_LDB>,
> - <&clk IMX8MP_VIDEO_PLL1>;
> - assigned-clock-parents = <&clk IMX8MP_VIDEO_PLL1_OUT>;
> /* IMX8MP_VIDEO_PLL1 = IMX8MP_CLK_MEDIA_DISP2_PIX * 7 */
> assigned-clock-rates = <0>, <506800000>;
> - status = "okay";
>
> ports {
[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-tian-g07017.dtso b/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-tian-g07017.dtso
> index 9a562c011f2c2..4f37723dbe8a1 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-tian-g07017.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-skov-revc-tian-g07017.dtso
> @@ -1,8 +1,10 @@
> // SPDX-License-Identifier: (GPL-2.0 OR MIT)
>
> /dts-v1/;
> +/plugin/;
>
> -#include "imx8mp-skov-reva.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
>
> / {
> model = "SKOV IMX8MP CPU revC - TIAN G07017";
[Severity: High]
Does this correctly target the base tree's root? In an overlay, using
/ { ... } defines the root node of the overlay DTB itself rather than
creating a fragment targeting the base tree. Should this use &{/} {
like the jutouch variant so properties like the model and the panel node
are properly merged into the base device tree?
> @@ -21,13 +23,9 @@ in_lvds0: endpoint {
> };
> };
>
[ ... ]
> @@ -45,16 +43,14 @@ touchscreen@38 {
> };
>
> &lcdif2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
[Severity: Medium]
Were these properties intended for the &lvds_bridge/ports node instead?
Adding them to &lcdif2 when there are no unit-addressed children seems
incorrect.
> status = "okay";
> };
>
> &lvds_bridge {
> - assigned-clocks = <&clk IMX8MP_CLK_MEDIA_LDB>,
> - <&clk IMX8MP_VIDEO_PLL1>;
> - assigned-clock-parents = <&clk IMX8MP_VIDEO_PLL1_OUT>;
> /* IMX8MP_VIDEO_PLL1 = IMX8MP_CLK_MEDIA_DISP2_PIX * 2 * 7 */
> assigned-clock-rates = <0>, <358400000>;
> - status = "okay";
>
> ports {
> port@1 {
[Severity: Medium]
Does port@1 need a reg = <1> property? Without the #address-cells and
#size-cells on the ports node and the reg property here, this can cause
dtc warnings and might fail parsing by the display drivers.
> @@ -64,18 +60,3 @@ ldb_lvds_ch0: endpoint {
> };
> };
> };
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-v7-0-topic-imx8mp-skov-dts-jutouch-7inch-v5-0-67faa5ee0bf6@pengutronix.de?part=3
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.