Re: [PATCH 1/7] dt-bindings: tpm: Introduce trivial-tpms.yaml

[Alexander Steffen <Alexander.Steffen@infineon.com>]

On 01.10.22 00:15, Rob Herring wrote:
> On Fri, Sep 30, 2022 at 12:03 PM Alexander Steffen
> <Alexander.Steffen@infineon.com> wrote:
>>
>> Most TPM devices are very similar and only need a few common properties
>> to describe them. However, they may use more properties than other
>> trivial I2C or SPI devices, e.g. powered-while-suspended. Therefore,
>> move them to their own trivial-tpms.yaml.
>>
>> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
>> ---
>>   .../bindings/security/tpm/trivial-tpms.yaml   | 54 +++++++++++++++++++
>>   .../devicetree/bindings/trivial-devices.yaml  | 16 ------
>>   2 files changed, 54 insertions(+), 16 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/trivial-tpms.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/trivial-tpms.yaml b/Documentation/devicetree/bindings/security/tpm/trivial-tpms.yaml
>> new file mode 100644
>> index 000000000000..fadd4ca96554
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/trivial-tpms.yaml
>> @@ -0,0 +1,54 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/security/tpm/trivial-tpms.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Trivial TPM devices that have simple device tree bindings
>> +
>> +maintainers:
>> +  - linux-integrity@vger.kernel.org
>> +
>> +description: |
>> +  This is a list of trivial TPM devices that share the same properties and
>> +  therefore have simple device tree bindings.
>> +
>> +  If a device needs more specific bindings, such as properties to
>> +  describe some aspect of it, there needs to be a specific binding
>> +  document for it just like any other devices.
>> +
>> +properties:
>> +  reg:
>> +    maxItems: 1
> 
> blank line
> 
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  spi-max-frequency: true
> 
> The SPI based devices need to reference spi-peripheral-props.yaml. So
> I think these are going to need to be split up by bus some.
> 
>> +
>> +  compatible:
> 
> compatible goes first by convention.

I had copied all three from trivial-devices.yaml ;-)

The style fixes are easy. But do you really think I should split 
trivial-tpms.yaml into i2c-tpms.yaml, spi-tpms.yaml, etc.? After all, 
trivial-devices.yaml also contains a mix of I2C and SPI devices.

Also, what about devices like "google,cr50", that support both I2C and 
SPI? Can they appear in two YAML files at the same time?

>> +    contains:
> 
> 'contains' can not be used here. That allows any other compatible
> strings to be present. It's got to be exact lists of what are valid
> combinations.

So what exactly are valid combinations then? If I look at what is in 
use, I find three possible combinations:

arch/arm/boot/dts/am335x-moxa-uc-2100-common.dtsi: compatible = 
"tcg,tpm_tis-spi"
arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts: compatible = 
"infineon,slb9670", "tcg,tpm_tis-spi";
arch/arm64/boot/dts/freescale/imx8mq-kontron-pitx-imx8m.dts: compatible 
= "infineon,slb9670";

It is either a generic identifier or a specific device or both. Is it 
correct to allow all three variants? If so, how to specify that as YAML, 
ideally without duplicating any of the identifiers?

>> +      enum:
>> +          # i2c trusted platform module (TPM)
>> +        - atmel,at97sc3204t
>> +          # Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
>> +        - infineon,slb9635tt
>> +          # Infineon SLB9645 I2C TPM (new protocol, max 400khz)
>> +        - infineon,slb9645tt
>> +          # Infineon SLB9673 I2C TPM 2.0
>> +        - infineon,slb9673
>> +          # i2c trusted platform module (TPM)
>> +        - nuvoton,npct501
>> +          # i2c trusted platform module (TPM2)
>> +        - nuvoton,npct601
>> +          # Socionext SynQuacer TPM MMIO module
>> +        - socionext,synquacer-tpm-mmio
>> +          # i2c trusted platform module (TPM)
>> +        - winbond,wpct301
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +...
>> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
>> index 61746755c107..1cc7f82c0822 100644
>> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
>> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
>> @@ -47,8 +47,6 @@ properties:
>>             - ams,iaq-core
>>               # i2c serial eeprom (24cxx)
>>             - at,24c08
>> -            # i2c trusted platform module (TPM)
>> -          - atmel,at97sc3204t
>>               # ATSHA204 - i2c h/w symmetric crypto module
>>             - atmel,atsha204
>>               # ATSHA204A - i2c h/w symmetric crypto module
>> @@ -135,12 +133,6 @@ properties:
>>             - infineon,ir38164
>>               # Infineon IR38263 Voltage Regulator
>>             - infineon,ir38263
>> -            # Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
>> -          - infineon,slb9635tt
>> -            # Infineon SLB9645 I2C TPM (new protocol, max 400khz)
>> -          - infineon,slb9645tt
>> -            # Infineon SLB9673 I2C TPM 2.0
>> -          - infineon,slb9673
>>               # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
>>             - infineon,tlv493d-a1b6
>>               # Infineon Multi-phase Digital VR Controller xdpe11280
>> @@ -289,10 +281,6 @@ properties:
>>             - national,lm85
>>               # I2C ±0.33°C Accurate, 12-Bit + Sign Temperature Sensor and Thermal Window Comparator
>>             - national,lm92
>> -            # i2c trusted platform module (TPM)
>> -          - nuvoton,npct501
>> -            # i2c trusted platform module (TPM2)
>> -          - nuvoton,npct601
>>               # Nuvoton Temperature Sensor
>>             - nuvoton,w83773g
>>               # OKI ML86V7667 video decoder
>> @@ -327,8 +315,6 @@ properties:
>>             - silabs,si7020
>>               # Skyworks SKY81452: Six-Channel White LED Driver with Touch Panel Bias Supply
>>             - skyworks,sky81452
>> -            # Socionext SynQuacer TPM MMIO module
>> -          - socionext,synquacer-tpm-mmio
>>               # SparkFun Qwiic Joystick (COM-15168) with i2c interface
>>             - sparkfun,qwiic-joystick
>>               # i2c serial eeprom (24cxx)
>> @@ -383,8 +369,6 @@ properties:
>>             - winbond,w83793
>>               # Vicor Corporation Digital Supervisor
>>             - vicor,pli1209bc
>> -            # i2c trusted platform module (TPM)
>> -          - winbond,wpct301
>>
>>   required:
>>     - compatible
>> --
>> 2.25.1
>>