Re: [PATCH v3 03/19] dt-bindings: mfd: Add starfive,jh7110-hdmi-subsystem

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Shared bus clocks and resets are incorrectly assigned to the child controller instead of the parent, which will cause a system bus fault when the PHY child accesses the shared registers.
- [Medium] Splitting a monolithic hardware block into an MFD purely to resolve a software probe ordering issue violates core Device Tree design principles.
- [Medium] The binding requires a bespoke MFD driver for a simple shared register space by omitting standard fallback compatible strings.
--

commit c934e468899df3820b545d81d3b4b4a9ae4b2097
Author: Michal Wilczynski [off-list ref]

dt-bindings: mfd: Add starfive,jh7110-hdmi-subsystem

Adds a device tree binding for the StarFive JH7110 HDMI subsystem.
The binding creates a multi-function device parent with phy and controller
children to resolve a circular clock dependency.

[ ... ]

+description:
+  The StarFive JH7110 HDMI block is a monolithic IP containing both
+  the digital controller logic and the analog PHY logic in a single
+  register space. It sits inside the video output subsystem, which owns

[Severity: Medium]
Since the hardware is a single monolithic IP block in a single register
space, does splitting it into a multi-function device parent and child nodes
conflict with the core device tree principle of describing hardware topology
rather than software abstraction limitations?

+  the NoC bus clock and reset gating this region and the power domain it
+  belongs to.
+
+properties:
+  compatible:
+    const: starfive,jh7110-hdmi-subsystem

[Severity: Medium]
Does limiting the compatible property strictly to this string without allowing
standard fallbacks like "simple-mfd" force the creation of a bespoke driver
for a simple shared register space?

+
+  reg:
+    maxItems: 1

[ ... ]

+required:
+  - compatible
+  - reg
+  - phy
+  - controller
+
+additionalProperties: false

[Severity: High]
With this property set to false, it is impossible for the parent node to
request clocks or resets. In the example below, the module system clock
(JH7110_VOUTCLK_HDMI_TX_SYS) is assigned to the controller child node.

Because the controller depends on the phy child for its pixel clock, the phy
must probe first. When the phy probes and attempts read-modify-write
operations on the shared regmap, will this cause a system bus fault and hard
CPU lockup because the module system clock is not yet enabled by the
controller?

+
+examples:
+  - |
+    #include <dt-bindings/clock/starfive,jh7110-crg.h>
+    #include <dt-bindings/reset/starfive,jh7110-crg.h>
+
+    soc {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        hdmi@29590000 {
+            compatible = "starfive,jh7110-hdmi-subsystem";
+            reg = <0x29590000 0x4000>;
+
+            controller {
+                compatible = "starfive,jh7110-inno-hdmi-controller";
+                clocks = <&voutcrg JH7110_VOUTCLK_HDMI_TX_SYS>,

[Severity: High]
Can this clock assignment lead to the system bus fault mentioned above? It
seems the parent node should be responsible for enabling the bus clock before
the children probe.

+                         <&voutcrg JH7110_VOUTCLK_HDMI_TX_MCLK>,
+                         <&voutcrg JH7110_VOUTCLK_HDMI_TX_BCLK>,
+                         <&hdmi_phy>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=3