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

[sashiko-bot@kernel.org]

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 <m.wilczynski@samsung.com>

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.

> diff --git a/Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml b/Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml
> new file mode 100644
> index 0000000000000..bd35f191835a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml
> @@ -0,0 +1,95 @@
[ ... ]
> +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