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

[Joey Lu <a0987203069@gmail.com>]

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 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.

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
- 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..).