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