Re: [PATCH v2 13/16] dt-bindings: i2c: tegra-bpmp: Convert to json-schema

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

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

On Wed, Dec 01, 2021 at 12:42:07PM -0600, Rob Herring wrote:
> On Wed, Dec 1, 2021 at 11:42 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Nov 29, 2021 at 07:44:32PM -0600, Rob Herring wrote:
> > > On Fri, Nov 19, 2021 at 03:38:36PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Convert the NVIDIA Tegra186 (and later) BPMP I2C bindings from the
> > > > free-form text format to json-schema.
> > > >
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > > Changes in v2:
> > > > - add missing additionalProperties: false
> > > >
> > > >  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt | 42 -------------------
> > > >  .../i2c/nvidia,tegra186-bpmp-i2c.yaml         | 42 +++++++++++++++++++
> > > >  2 files changed, 42 insertions(+), 42 deletions(-)
> > > >  delete mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> > > >  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> > > > deleted file mode 100644
> > > > index ab240e10debc..000000000000
> > > > --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> > > > +++ /dev/null
> > > > @@ -1,42 +0,0 @@
> > > > -NVIDIA Tegra186 BPMP I2C controller
> > > > -
> > > > -In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
> > > > -devices, such as the I2C controller for the power management I2C bus. Software
> > > > -running on other CPUs must perform IPC to the BPMP in order to execute
> > > > -transactions on that I2C bus. This binding describes an I2C bus that is
> > > > -accessed in such a fashion.
> > > > -
> > > > -The BPMP I2C node must be located directly inside the main BPMP node. See
> > > > -../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
> > > > -
> > > > -This node represents an I2C controller. See ../i2c/i2c.txt for details of the
> > > > -core I2C binding.
> > > > -
> > > > -Required properties:
> > > > -- compatible:
> > > > -    Array of strings.
> > > > -    One of:
> > > > -    - "nvidia,tegra186-bpmp-i2c".
> > > > -- #address-cells: Address cells for I2C device address.
> > > > -    Single-cell integer.
> > > > -    Must be <1>.
> > > > -- #size-cells:
> > > > -    Single-cell integer.
> > > > -    Must be <0>.
> > > > -- nvidia,bpmp-bus-id:
> > > > -    Single-cell integer.
> > > > -    Indicates the I2C bus number this DT node represent, as defined by the
> > > > -    BPMP firmware.
> > > > -
> > > > -Example:
> > > > -
> > > > -bpmp {
> > > > -   ...
> > > > -
> > > > -   i2c {
> > > > -           compatible = "nvidia,tegra186-bpmp-i2c";
> > > > -           #address-cells = <1>;
> > > > -           #size-cells = <0>;
> > > > -           nvidia,bpmp-bus-id = <5>;
> > > > -   };
> > > > -};
> > > > diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.yaml
> > > > new file mode 100644
> > > > index 000000000000..351e12124959
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.yaml
> > > > @@ -0,0 +1,42 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/i2c/nvidia,tegra186-bpmp-i2c.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: NVIDIA Tegra186 (and later) BPMP I2C controller
> > > > +
> > > > +maintainers:
> > > > +  - Thierry Reding <thierry.reding@gmail.com>
> > > > +  - Jon Hunter <jonathanh@nvidia.com>
> > > > +
> > > > +description: |
> > > > +  In Tegra186 and later, the BPMP (Boot and Power Management Processor)
> > > > +  owns certain HW devices, such as the I2C controller for the power
> > > > +  management I2C bus. Software running on other CPUs must perform IPC to
> > > > +  the BPMP in order to execute transactions on that I2C bus. This
> > > > +  binding describes an I2C bus that is accessed in such a fashion.
> > > > +
> > > > +  The BPMP I2C node must be located directly inside the main BPMP node.
> > > > +  See ../firmware/nvidia,tegra186-bpmp.yaml for details of the BPMP
> > > > +  binding.
> > > > +
> > > > +  This node represents an I2C controller. See ../i2c/i2c.txt for details
> > > > +  of the core I2C binding.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: nvidia,tegra186-bpmp-i2c
> > > > +
> > >
> > > > +  "#address-cells":
> > > > +    const: 1
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > >
> > > Covered by i2c-controller.yaml. Add a reference and then use
> > > unevaluatedProperties.
> >
> > About that: I've recently noticed that this doesn't seem to work
> > properly. I'm using branch draft2020-12 from your github and my
> 
> Use dtschema main/master branch. That branch is likely stale.

That seems to have helped somewhat. I do occasionally see warnings now
about unevaluated properties being unexpected. I can still reproduce the
issue, though, see below.

> > understanding was that this should give us support for
> > unevaluatedProperties. And indeed, it no longer complains about
> > #address-cells and #size-cells if I remove them from this binding,
> > presumably because it gets them from i2c-controller.yaml.
> >
> > However, a side-effect seems to be that now it also ignores any
> > properties that aren't defined anywhere. So for example if I touch
> > up the example in firmware/nvidia,tegra186-bpmp.yaml and add a bogus
> > "foo-bar = <0>;" property in the BPMP I2C node, then it'll blindly
> > accept that as valid.
> 
> Do you have unevaluatedProperties within the i2c node? It only applies
> to 1 level, and you can't have a parent+child schema evaluated with
> another child (or parent+child) schema. This is why the graph schema
> is done the way it is and why we're splitting spi-controller.yaml
> child node schema out to spi-peripheral.yaml.

Let me give an example based on a schema that's already upstream. So
looking at this:

	Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml

it does include spi-controller.yaml via an allOf: [ $ref: ... ], so it
uses unevaluatedProperties to validate against any generic SPI
controller properties. For example, #address-cells and #size-cells are
validated based on the schema from spi-controller.yaml.

However, if I now apply the following patch to add an undocumented
property to the example, then validation doesn't fail as I would expect
it to.

--- >8 ---
diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml
index 35a8045b2c70..e9342faf5194 100644
--- a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml
+++ b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad.yaml
@@ -104,6 +104,7 @@ examples:
             resets = <&tegra_car 211>;
             dmas = <&apbdma 5>, <&apbdma 5>;
             dma-names = "rx", "tx";
+            foo-something = <42>;
 
             flash@0 {
                     compatible = "spi-nor";
--- >8 ---

I would expect the validation to fail for foo-something because it isn't
defined in any of the schemas.

Or is this one of the cases that you mentioned above. I must admit I did
not follow what exactly is expected to work and what isn't. The QSPI
controller example from above seems simple enough.

Thierry

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