On Thu, Jun 18, 2020 at 8:16 AM Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxxx> > > > > > > Convert the Tegra host1x controller bindings from the free-form text > > > format to json-schema. > > > > > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> > > > --- > > > .../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 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. > > > + 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. > > 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. :) Rob
