Re: [PATCH v5 2/3] dt-bindings: mfd: aspeed,ast2x00-scu: Describe AST2700 SCU0

> > > > AST2700 consists of two interconnected SoC instances, each with its own
> > > > System Control Unit (SCU). The SCU0 provides pin control, interrupt
> > > > controllers, clocks, resets, and address-space mappings for the
> > > > Secondary and Tertiary Service Processors (SSP and TSP).
> > > >
> > > > Describe the SSP/TSP address mappings using the standard
> > > > memory-region and memory-region-names properties.
> > > >
> > > > Disallow legacy child nodes that are not present on AST2700, including
> > > > p2a-control and smp-memram. The latter is unnecessary as software can
> > > > access the scratch registers via the SCU syscon.
> > > >
> > > > Also allow the AST2700 SoC0 pin controller to be described as a child
> > > > node of the SCU0, and add an example illustrating the SCU0 layout,
> > > > including reserved-memory, interrupt controllers, and pinctrl.
> > > >
> > > > Signed-off-by: Billy Tsai <[email protected]>
> > > > ---
> > > >  .../bindings/mfd/aspeed,ast2x00-scu.yaml           | 117 +++++++++++++++++++++
> > > >  1 file changed, 117 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
> > > > index a87f31fce019..86d51389689c 100644
> > > > --- a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
> > > > +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
> > > > @@ -46,6 +46,9 @@ properties:
> > > >    '#reset-cells':
> > > >      const: 1
> > > >
> > > > +  memory-region: true
> > > > +  memory-region-names: true
> >
> > > Missing constraints. From where did you take such syntax (so I can fix
> > > it)?
> >
> > The intention was to constrain these properties conditionally for
> > AST2700 SCU0 as done further down in the patch.
> >
> > I can update the binding so that memory-region and memory-region-names
> > have baseline constraints (e.g. minItems and maxItems), and then refine them in the
> > conditional branches for AST2700SCU0, AST2700SCU1 and others
> >
> >   memory-region:
> >     minItems: 2
> >     maxItems: 3
> >   memory-region-names:
> >     minItems: 2
> >     maxItems: 3

> As of this patch, you don't need that. You can just define the regions
> and names at the top-level. And the conditional schema only needs to
> disallow them for the appropriate case.

Based on your suggestion, I will simplify the schema and define
memory-region and memory-region-names at the top-level without item
constraints, and only disallow them for the non-AST2700 cases.

The updated structure would look like:

    memory-region:
      description:
        Reserved memory regions used by AST2700 SCU to configure
        coprocessor address mapping windows.

    memory-region-names:
      description:
        Names corresponding to the AST2700 coprocessor mapping windows
        listed in memory-region.

    ...

    - if:
        properties:
          compatible:
            contains:
              anyOf:
                - const: aspeed,ast2700-scu0
                - const: aspeed,ast2700-scu1
      then:
        patternProperties:
          '^p2a-control@[0-9a-f]+$': false
          '^smp-memram@[0-9a-f]+$': false
      else:
        properties:
          memory-region: false
          memory-region-names: false

Does this match what you had in mind?

Thanks

Billy Tsai