Re: [PATCH 07/38] dt-bindings: display: tegra: Convert to json-schema

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8664 bytes --]

On Thu, Jun 18, 2020 at 09:23:58AM -0600, Rob Herring wrote:
> On Thu, Jun 18, 2020 at 8:16 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Wed, Jun 17, 2020 at 05:13:26PM -0600, Rob Herring wrote:
> > > On Fri, Jun 12, 2020 at 04:18:32PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Convert the Tegra host1x controller bindings from the free-form text
> > > > format to json-schema.
> > > >
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >  .../display/tegra/nvidia,tegra20-host1x.txt   |  516 ------
> > > >  .../display/tegra/nvidia,tegra20-host1x.yaml  | 1418 +++++++++++++++++
> > > >  2 files changed, 1418 insertions(+), 516 deletions(-)
> > > >  delete mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> > > >  create mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
> 
> [...]
> 
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - nvidia,tegra124-host1x
> > > > +              - nvidia,tegra210-host1x
> > > > +              - nvidia,tegra186-host1x
> > > > +              - nvidia,tegra194-host1x
> > > > +    then:
> > > > +      patternProperties:
> > > > +        "^sor@[0-9a-f]+$":
> > > > +          description: |
> > > > +            The Serial Output Resource (SOR) can be used to drive HDMI, LVDS,
> > > > +            eDP and DP outputs.
> > > > +
> > > > +            See ../pinctrl/nvidia,tegra124-dpaux-padctl.txt for information
> > > > +            regarding the DPAUX pad controller bindings.
> > > > +          type: object
> > > > +          properties:
> > > > +            # required
> > > > +            compatible:
> > > > +              oneOf:
> > > > +                - const: nvidia,tegra124-sor
> > > > +                - items:
> > > > +                    - const: nvidia,tegra132-sor
> > > > +                    - const: nvidia,tegra124-sor
> > > > +                - const: nvidia,tegra210-sor
> > > > +                - const: nvidia,tegra210-sor1
> > > > +                - const: nvidia,tegra186-sor
> > > > +                - const: nvidia,tegra186-sor1
> > > > +                - const: nvidia,tegra194-sor
> > > > +
> > > > +            reg:
> > > > +              maxItems: 1
> > > > +
> > > > +            interrupts:
> > > > +              maxItems: 1
> > > > +
> > > > +            resets:
> > > > +              items:
> > > > +                - description: module reset
> > > > +
> > > > +            reset-names:
> > > > +              items:
> > > > +                - const: sor
> > > > +
> > > > +            status:
> > > > +              $ref: "/schemas/dt-core.yaml#/properties/status"
> > >
> > > 'status' should never need to be listed.
> >
> > This seems to be needed at least when I try to validate against a single
> > binding, like so:
> >
> >         $ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml dtbs_check
> >
> > I assume that that somehow prevents the tooling from looking at any of
> > the other bindings, which in turn then causes status and other standard
> > properties to never be defined and then it flags them as extra and
> > causes a failure.
> 
> I'm surprised using DT_SCHEMA_FILES makes a difference. I'm guessing
> that has your 'unevaluatedProperties' support. If so, that means
> there's an unintended side effect that any common schema property
> becomes always allowed. That's good for 'status' and 'phandle', but
> not so much for 'reg', '*-gpios, '*-names', etc.

I don't think that's an unintended side-effect. If the property
validates against a schema it counts as evaluated, doesn't it? In order
to avoid that we would somehow have to restrict which schemas contribute
to the evaluatedProperties annotation and I don't think there's a way to
do that because we don't know which out of all the schemas is relevant.

> > I think I've even seen this trigger on dt_binding_check if I happened to
> > have status in there. Now, you've mentioned elsewhere that we shouldn't
> > use "status" in examples, so that would work around this. However, I
> > think I've seen this happen as well in examples that referenced some
> > node via phandle, and then dt_binding_check would emit an error about
> > phandle being undefined.
> >
> > Perhaps this is a problem with the tooling? Should we instruct the
> > scripts to always include the core schema even if we're only testing a
> > single YAML file via DT_SCHEMA_FILES?
> 
> The purpose of DT_SCHEMA_FILES is to see warnings just from that
> schema file. If the core schema was warning free, we could add that,
> but it's not. Plus that wouldn't solve the problem here. 'status' and
> 'phandle' are added to each schema by the tooling (along with other
> things), not by another schema file (well, they are in another schema
> file, but they are added to each schema so that 'additionalProperties:
> false' works).
> 
> This is certainly a limitation in the tooling in that what you have is
> a bit different from the expected form. Generally it is expected that
> everything is defined under the top-level 'properties' and then any
> 'if/then' schema only add further constraints. However, you have the
> child nodes only defined under an if/then. We could fix that, but I'm
> not sure I want to. IMO, extensive use of if/then is a sign the schema
> should be split up. More on that below.

Okay, I see your point.

> > > > +            pinctrl-names: true
> > > > +            phandle:
> > > > +              $ref: "/schemas/types.yaml#/definitions/uint32"
> > >
> > > 'phandle' shouldn't need to be listed.
> > >
> > > > +
> > > > +          patternProperties:
> > > > +            "^pinctrl-[0-9]+$": true
> > >
> > > pinctrl properties are automatically added, but maybe not if under an
> > > 'if' schema. Really, I think probably either this should be split
> > > into multiple schema files or all of these child nodes should be
> > > described at the top-level. I'm not sure it's really important to define
> > > which set of child nodes belong or not for each chip.
> >
> > I'm not too worried about the set of child nodes for each chip, but I
> > think having this all in one file underlines the importance of the
> > hierarchy. If these were discrete bindings for each of the compatible
> > strings it'd be easy for someone to create them as standalone nodes in
> > device tree, but that's not something that would work. All of these
> > devices are children of host1x and they do depend on host1x for a lot
> > of the functionality, so the hierarchy must be respected.
> 
> I'm not saying don't describe the hierarchy.
> 
> The first option is 1 host1x schema file per SoC (roughly) and the
> 'host1x' parent node would be duplicated in each one. That doesn't
> worry me too much as it's all standard properties and not that many of
> them. Though you could have a common 'host1x-bus.yaml' just describing
> the parent node properties that each <soc>-host1x.yaml references.
> 
> The 2nd option is keep this as a single file, but just move every
> child node definition under the top-level 'patternProperties'. This
> option has the limitation that you can't enforce which child nodes are
> valid per SoC.

Okay, I'll give the first option a try and see where I end up.

> > > I'm stopping there. I think the rest is more of the same comments.
> >
> > I've made a pass over the whole file and fixed the issues that you
> > pointed out above in other places.
> >
> > Sounds like the biggest remaining issue is with the duplicated standard
> > properties. I'm not a huge fan of giving up on doing the right thing
> > because the tooling can't deal with it. I think we should fix the
> > tooling to do the right thing. So if there's something in the core DT
> > schema then it should apply regardless of what mode we run in. Much of
> > the above issues should go away once that's fixed.
> >
> > Any thoughts on making some of the schema files "always included"? I
> > haven't looked at this side of the tooling at all yet, so I'm not sure
> > how difficult that would be, but if you're okay with it conceptually I
> > can take a closer look.
> 
> Hopefully, it's clear why that doesn't help here. But don't worry,
> there's plenty of other work to do on the tooling. :)

Yes, I think I understand now.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]