Re: [PATCH 0/3] dt-bindings: tpm: Clean all the things

From: Rob Herring <robh@kernel.org>
To: Lukas Wunner <lukas@wunner.de>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	devicetree@vger.kernel.org, linux-integrity@vger.kernel.org,
	Lino Sanfilippo <LinoSanfilippo@gmx.de>
Subject: Re: [PATCH 0/3] dt-bindings: tpm: Clean all the things
Date: Tue, 21 Nov 2023 09:10:26 -0700	[thread overview]
Message-ID: <20231121161026.GC1845293-robh@kernel.org> (raw)
In-Reply-To: <cover.1700555862.git.lukas@wunner.de>

On Tue, Nov 21, 2023 at 10:48:40AM +0100, Lukas Wunner wrote:
> Rob asked me to consolidate and convert the TPM dt-bindings:
> https://lore.kernel.org/all/20230927115300.GA1587935-robh@kernel.org/
> 
> I came across several issues:
> 
> First, this pull request is needed to cope with ibm,#dma-address-cells
> and ibm,#dma-size-cells properties used in ibm,vtpm.yaml:
> 
>   https://github.com/devicetree-org/dt-schema/pull/116

Now applied.

> Second, the compatible string "google,cr50" refers to a chip which has
> both an i2c and an spi interface (see drivers/char/tpm/tpm_tis_i2c_cr50.c
> as well as tpm_tis_spi_main.c)  This confuses the schema validator
> because it thinks that "google,cr50" may only use i2c properties but
> tcg,tpm_tis-spi.yaml contains an spi example, which causes some warnings.
> It seems deeper code changes to the schema validator are required to
> avoid them:
> 
>   Documentation/devicetree/bindings/tpm/tcg,tpm_tis-spi.example.dtb: tpm@0: compatible:1: 'tcg,tpm-tis-i2c' was expected
>   from schema $id: http://devicetree.org/schemas/tpm/tcg,tpm-tis-i2c.yaml#
>   Documentation/devicetree/bindings/tpm/tcg,tpm_tis-spi.example.dtb: tpm@0: Unevaluated properties are not allowed ('compatible', 'spi-max-frequency' were unexpected)
>   from schema $id: http://devicetree.org/schemas/tpm/tcg,tpm-tis-i2c.yaml#

You either need to put "google,cr50" into its own schema file with a 
custom 'select' that omits the generic compatibles or the spi and i2c 
schemas need a custom select omitting "google,cr50" or having just the 
generic compatible. 

However, none of the current users of "google,cr50" have a fallback 
compatible, so perhaps its own schema file with no fallback is the 
answer. Is the fallback useful and is it worth updating all the users? 
Probably not since we have to keep support for "google,cr50" explicitly 
in the driver.

> Third, the schema validator raises warnings about three properties
> I've defined in tpm-common.yaml:  "linux,sml-base" and "linux,sml-size"
> are nested in a oneOf/allOf clause and "lpcpd-gpios" is nested in an
> allOf/if-then clause.  This seems to confuse the validator:
> 
>   Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.example.dtb: tpm@57: Unevaluated properties are not allowed ('linux,sml-base', 'linux,sml-size' were unexpected)
>   from schema $id: http://devicetree.org/schemas/tpm/tcg,tpm-tis-i2c.yaml#

Should be fixed with my suggestion in patch 1.

>   Documentation/devicetree/bindings/tpm/tcg,tpm-tis-i2c.example.dtb: tpm@13: Unevaluated properties are not allowed ('lpcpd-gpios' was unexpected)
>   from schema $id: http://devicetree.org/schemas/tpm/tcg,tpm-tis-i2c.yaml#

The issue is here:

+      properties:
+        compatible:
+          contains:
+            const: st,st33zp24

That's an exact match, not a pattern. You could do 'pattern: '^st,st33zp24'
or an enum with both compatible strings.

Rob