From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: arm64: dts: imx93: Add parallel display output nodes
Date: Tue, 03 Mar 2026 13:09:10 +1000 [thread overview]
Message-ID: <review-patch3-20260302-v6-18-topic-imx93-parallel-display-v10-3-634fe2778c7a@pengutronix.de> (raw)
In-Reply-To: <20260302-v6-18-topic-imx93-parallel-display-v10-3-634fe2778c7a@pengutronix.de>
Patch Review
**Subject:** `[PATCH v10 3/3] arm64: dts: imx93: Add parallel display output nodes`
This patch adds the DT nodes for the LCDIF display controller and wires it up to the PDFC bridge.
**`assigned-clock-rates` count mismatch:**
```dts
assigned-clocks = <&clk IMX93_CLK_MEDIA_AXI>,
<&clk IMX93_CLK_MEDIA_APB>,
<&clk IMX93_CLK_MEDIA_DISP_PIX>;
assigned-clock-parents = <&clk IMX93_CLK_SYS_PLL_PFD1>,
<&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>,
<&clk IMX93_CLK_VIDEO_PLL>;
assigned-clock-rates = <400000000>, <133333333>;
```
Three clocks are listed in `assigned-clocks` and `assigned-clock-parents`, but `assigned-clock-rates` only has two values. The third clock (`MEDIA_DISP_PIX`) has its parent set to `VIDEO_PLL` but no rate specified. This is intentionally leaving the pixel clock rate to be set at runtime (which is correct since the pixel clock rate depends on the display mode), but it would be clearer to explicitly pass `<0>` as the third rate to indicate "don't set a rate" (the standard DT convention). Some DT validation tools may warn about the length mismatch.
**The `dpi-bridge` compatible uses `nxp,imx93-pdfc` in the common dtsi:**
```dts
dpi_bridge: dpi-bridge {
compatible = "nxp,imx93-pdfc";
```
Since `imx91_93_common.dtsi` is shared between i.MX91 and i.MX93, using the `nxp,imx93-pdfc` compatible here means i.MX91-based boards would also use this compatible. If i.MX91 should use `nxp,imx91-pdfc` (as defined in the binding), then the compatible should be overridden in the i.MX91-specific dtsi file, or the common dtsi should use the i.MX91 compatible with a fallback. Currently there doesn't appear to be an i.MX91-specific override.
**LCDIF node looks correct:**
The `lcdif` node at `0x4ae30000` with appropriate clocks, power-domains, and port wiring to the PDFC bridge looks well-structured. The imx93.dtsi addition of `endpoint@1` (LDB) and `endpoint@2` (DSI) stubs for the other two bridge outputs is a clean forward-looking pattern.
**The patch closes the `media_blk_ctrl` node and adds `lcdif` as a sibling:**
```dts
+ };
+
+ lcdif: display-controller@4ae30000 {
```
This correctly makes `lcdif` a sibling of `media_blk_ctrl` in the SoC bus, not a child. This is the right approach since the LCDIF has its own register space.
**Verdict:** Mostly looks good. The `assigned-clock-rates` length mismatch is a minor DT style issue. The compatible string usage in the common dtsi should be considered in light of i.MX91 support.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-03-03 3:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-02 16:10 [PATCH v10 0/3] Add i.MX91/93 parallel display support Marco Felsch
2026-03-02 16:10 ` [PATCH v10 1/3] dt-bindings: soc: imx93-media-blk-ctrl: Add PDFC subnode to schema and example Marco Felsch
2026-03-03 3:09 ` Claude review: " Claude Code Review Bot
2026-03-02 16:10 ` [PATCH v10 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support Marco Felsch
2026-03-03 3:09 ` Claude review: " Claude Code Review Bot
2026-03-02 16:10 ` [PATCH v10 3/3] arm64: dts: imx93: Add parallel display output nodes Marco Felsch
2026-03-03 3:09 ` Claude Code Review Bot [this message]
2026-03-03 3:04 ` [PATCH v10 0/3] Add i.MX91/93 parallel display support Liu Ying
2026-03-03 3:09 ` Claude review: " Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-03 10:34 [PATCH v11 0/3] " Marco Felsch
2026-03-03 10:34 ` [PATCH v11 3/3] arm64: dts: imx93: Add parallel display output nodes Marco Felsch
2026-03-03 21:36 ` Claude review: " Claude Code Review Bot
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=review-patch3-20260302-v6-18-topic-imx93-parallel-display-v10-3-634fe2778c7a@pengutronix.de \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox