Re: [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants

[Icenowy Zheng]

在 2026-06-29一的 11:47 +0800，Joey Lu写道：
> 
> On 6/26/2026 5:33 PM, Icenowy Zheng wrote:
> > 在 2026-06-26五的 10:26 +0100，Conor Dooley写道：
> > > On Fri, Jun 26, 2026 at 05:00:35PM +0800, Icenowy Zheng wrote:
> > > > 在 2026-06-26五的 08:19 +0100，Conor Dooley写道：
> > > > > On Fri, Jun 26, 2026 at 01:27:21PM +0800, Icenowy Zheng
> > > > > wrote:
> > > > > > 在 2026-06-25四的 17:33 +0100，Conor Dooley写道：
> > > > > > > On Thu, Jun 25, 2026 at 05:44:43PM +0800, Joey Lu wrote:
> > > > > > > > +allOf:
> > > > > > > > +  - if:
> > > > > > > > +      properties:
> > > > > > > > +        compatible:
> > > > > > > > +          contains:
> > > > > > > > +            const: thead,th1520-dc8200
> > > > > > > > +    then:
> > > > > > > > +      properties:
> > > > > > > > +        clocks:
> > > > > > > > +          minItems: 5
> > > > > > > > +          maxItems: 5
> > > > > > > > +
> > > > > > > > +        clock-names:
> > > > > > > > +          minItems: 5
> > > > > > > > +          maxItems: 5
> > > > > > > All the maxItems here repeat the maximum constraint and
> > > > > > > do
> > > > > > > nothing.
> > > > > > > 
> > > > > > > Since you didn't change the minimum constraint at the top
> > > > > > > level,
> > > > > > > your
> > > > > > > minItems also do nothing.
> > > > > > > 
> > > > > > > > +
> > > > > > > > +        resets:
> > > > > > > > +          minItems: 3
> > > > > > > > +          maxItems: 3
> > > > > > > > +
> > > > > > > > +        reset-names:
> > > > > > > > +          minItems: 3
> > > > > > > > +          maxItems: 3
> > > > > > > > +
> > > > > > > > +      required:
> > > > > > > > +        - resets
> > > > > > > > +        - reset-names
> > > > > > > Both conditional sections have this, but the original
> > > > > > > binding
> > > > > > > doesn't
> > > > > > > require these for the thead device. This is a functional
> > > > > > > change
> > > > > > > therefore and shouldn't be in a patch calling itself
> > > > > > > "generalise
> > > > > > > for
> > > > > > > single ended variants".
> > > > > > Well yes they're required.
> > > > > > 
> > > > > > Should I send a patch adding the `thead,th1520-dc8200` part
> > > > > > of
> > > > > > the
> > > > > > schema?
> > > > > If you mean the code above, no. Adding a conditional section
> > > > > when
> > > > > there's only that compatible doesn't make sense.
> > > > > 
> > > > > What you could do is just add it at the top level though,
> > > > > which
> > > > > would
> > > > > also benefit this patch since it'd not have to be
> > > > > conditionally
> > > > > added
> > > > > for the new nuvoton device.
> > > > > Just note in your commit message about what the ABI impact of
> > > > > the
> > > > > change
> > > > > to required properties is (effectively nothing because it's
> > > > > optional
> > > > > in
> > > > > the driver and the only user has the properties).
> > > > Okay, I will craft such a patch and send it.
> > > > 
> > > > > > > > +
> > > > > > > > +        resets:
> > > > > > > > +          minItems: 1
> > > > > > > > +          maxItems: 1
> > > > > > > > +
> > > > > > > > +        reset-names:
> > > > > > > > +          items:
> > > > > > > > +            - const: core
> > > > > > > This is just maxItems: 1.
> > > > > > Well the implicit rules of DT binding schemas are quite
> > > > > > weird...
> > > > > I don't think it is that strange, as the binding has
> > > > >    reset-names:
> > > > >      items:
> > > > >        - const: core
> > > > >        - const: axi
> > > > >        - const: ahb
> > > > Ah does the list constraint the order of items? If it
> > > > constrains
> > > > the
> > > It does, yes.
> > > Alternatively, using an enum permits free ordering.
> > Ah in this case this should be converted to an enum, I think.
> > 
> > Should I send a patch for converting it?
> > 
> > Thanks,
> > Icenowy
> Thank you all for the detailed review and discussion, it really
> helped
> clarify the right approach.
> 
> Since I will supply all four clocks with the same phandle for
> core/axi/ahb,
> and only one reset "core" for MA35D1, the ordering constraint in the
> `items` list is not a problem, "core" is already the first entry.
> There
> is no need to convert to an enum.
> 
> Regarding the clock situation for the MA35D1: I agree with supplying
> all
> four clocks (core, axi, ahb, pix0) in the devicetree, even though the
> MA35D1 clock controller gates core/axi/ahb with a single bit. The DT
> will
> use the same clock phandle for core, axi, and ahb:
> 
>    clocks = <&clk X>, <&clk X>, <&clk X>, <&pix_clk Y>;
>    clock-names = "core", "axi", "ahb", "pix0";
> 
> This correctly models the hardware topology. Since all three names

No, this doesn't correctly model the hardware topology -- this will
lead to clk_get_rate() return the rate of DC core clock when checking
the AXI clock rate, which is problematic because both clocks are
limiting the performance of the DC.

> resolve
> to the same underlying clock node, the CCF's standard enable
> refcounting
> handles the shared gate correctly without any custom implementation
> needed.
> I will also revert the change in patch 4/7 that made axi and ahb
> clocks
> optional, since they will now always be provided in the devicetree.
> 
> Regarding moving `resets` and `reset-names` to the top-level
> `required:`,
> I will wait for Icenowy's patch to land before sending v6 to avoid
> duplicating the work.

The patch is sent.

> 
> In v6 I will update patch 1/7 with:
> - Update the subject to "dt-bindings: display: verisilicon,dc: add
>    support for nuvoton,ma35d1-dcu"
> - Lower `clocks`/`clock-names` `minItems` to 4 at the top level
> - Remove the `thead,th1520-dc8200` conditional block entirely

I think this conditional block will still be needed, because it will
need to constrain the minItems to ensure all clocks / resets are
populated.

Thanks,
Icenowy

> - Keep only the `nuvoton,ma35d1-dcu` conditional block, using only
>    `maxItems: 4` for clocks/clock-names and `maxItems: 1` for
>    resets/reset-names to tighten the top-level constraints
> 
> BR,
> Joey
> > > > order, it partly breaks the intention of having names; if it
> > > > does
> > > > not
> > > > constrain the order, it needs to be clarified that the required
> > > > 1
> > > > reset
> > > > is core instead of the other two.
> > > Given the discussion we're having on the clocks, I wonder if this
> > > is
> > > also an oversimplification and the IP has three resets inputs
> > > hooked
> > > up
> > > to one output of the reset controller (or 3 outputs controlled by
> > > one
> > > bit..).