Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191217005424.226858-1-swboyd@chromium.org> <CAD=FV=UQAgd2R=ykTCnBZuOvFFKoWu4o-3Rq=GEdrc1KKSi9cQ@mail.gmail.com>
 <20191220231040.GA11384@bogus> <5e12cc29.1c69fb81.fe838.d5f3@mx.google.com>
In-Reply-To: <5e12cc29.1c69fb81.fe838.d5f3@mx.google.com>
From:   Andrey Pronin <apronin@chromium.org>
Date:   Thu, 16 Jan 2020 09:58:52 -0800
Message-ID: <CAP7wa8Lfgm5h5j0jvkYP+sFHnz6jRb1m8fCi-8AvPmNqfupenw@mail.gmail.com>
Subject: Re: [PATCH] dt-bindings: tpm: Convert cr50 binding to YAML
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Doug Anderson <dianders@chromium.org>,
        Rob Herring <robh@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jan 5, 2020 at 9:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rob Herring (2019-12-20 15:10:40)
> > On Tue, Dec 17, 2019 at 09:45:02AM -0800, Doug Anderson wrote:
> > > On Mon, Dec 16, 2019 at 4:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > diff --git a/Documentation/devicetree/bindings/security/tpm/google,cr50.yaml b/Documentation/devicetree/bindings/security/tpm/google,cr50.yaml
> > > > new file mode 100644
> > > > index 000000000000..8bfff0e757af
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/security/tpm/google,cr50.yaml
> > > > @@ -0,0 +1,52 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/tpm/google,cr50.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: H1 Secure Microcontroller with Cr50 Firmware on SPI Bus
> > > > +
> > > > +description:
> > > > +  H1 Secure Microcontroller running Cr50 firmware provides several functions,
> > > > +  including TPM-like functionality. It communicates over SPI using the FIFO
> > > > +  protocol described in the PTP Spec, section 6.
> > > > +
> > > > +maintainers:
> > > > +  - Andrey Pronin <apronin@chromium.org>
> > >
> > > Does Andrey agree to be the maintainer here?
>
> I Cced Andrey in hopes of eliciting a response.

Yes, I finally can confirm I agree to be the maintainer.

>
> > >
> > >
> > > I'd like to see if we can delete most of what you've written here.
> > > Specifically in "spi/spi-controller.yaml" you can see a really nice
> > > description of what SPI devices ought to look like.  Can we just
> > > reference that?  To do that I _think_ we actually need to break that
> > > description into a separate YAML file and then include it from there
> > > and here.  Maybe someone on the list can confirm or we can just post
> > > some patches for that?
>
> I'm not sure what to do here.
>
> > >
> > >
> > > > +properties:
> > > > +  compatible:
> > > > +    const: google,cr50
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > >
> > > I'm curious if you need a minItems here.  ...and if we don't somehow
> > > include it, should we follow 'spi-controller.yaml' and treat this like
> > > an int?
> >
> > Really, just 'true' is sufficient as you can't say which CS number it is
> > here.
>
> Ok.
>
> > >
> > >
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - spi-max-frequency
> > >
> > > Technically spi-max-frequency might not be required (the SPI binding
> > > doesn't list it as such), but I guess it was before...
> >
> > Generally, we expect a device knows its max and this should only be used
> > it a board has a lower value. However, sometimes there's exceptions.
> >
> > Shouldn't really be debate here unless the old binding doc was wrong.
>
> The old binding doc had it as required and the spi framework seems to
> bail out if this property isn't specified (see of_spi_parse_dt() for
> more details).
>
> >
> > >
> > >
> > > > +  - interrupts
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > > +    spi {
> > > > +      #address-cells = <0x1>;
> > > > +      #size-cells = <0x0>;
> > > > +      tpm@0 {
> > > > +          compatible = "google,cr50";
> > > > +          reg = <0>;
> > > > +          spi-max-frequency = <800000>;
> > > > +          interrupts = <50 IRQ_TYPE_EDGE_RISING>;
> > >
> > > I would tend to prefer seeing the interrupt parent in the example
> > > since it's pretty likely that the GPIO controller isn't the overall
> > > parent and likely that our interrupt is a GPIO.  I'm not sure the
> > > convention, though.
> >
> > Example is fine, but shouldn't be in the schema.
>
> Ok. Will add an interrupt parent like
>
>         interrupt-parent = <&gpio_controller>;
>


-- 
Andrey