From: Pratyush Yadav <p.yadav@ti.com>
To: Rob Herring <robh+dt@kernel.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
Richard Weinberger <richard@nod.at>,
Vignesh Raghavendra <vigneshr@ti.com>,
Mark Brown <broonie@kernel.org>,
MTD Maling List <linux-mtd@lists.infradead.org>,
<devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-spi <linux-spi@vger.kernel.org>, Nishanth Menon <nm@ti.com>,
Michael Walle <michael@walle.cc>,
Tudor Ambarus <tudor.ambarus@microchip.com>
Subject: Re: [PATCH 1/2] spi: dt-bindings: add schema listing slave-specific properties
Date: Thu, 10 Jun 2021 18:55:53 +0530 [thread overview]
Message-ID: <20210610132551.rqtswf7zend2f6xw@ti.com> (raw)
In-Reply-To: <CAL_JsqK5hu6NSVxiTpPvH_jHA97WTj3=DyJhDf5g1hwXXZ8MZA@mail.gmail.com>
On 09/06/21 03:51PM, Rob Herring wrote:
> On Wed, Jun 9, 2021 at 6:17 AM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > Many SPI controllers need to add properties to slave devices. This could
> > be the delay in clock or data lines, etc. These properties are
> > controller specific but need to be defined in the slave node because
> > they are per-slave and there can be multiple slaves attached to a
> > controller.
> >
> > If these properties are not added to the slave binding, then the dtbs
> > check emits a warning. But these properties do not make much sense in
> > the slave binding because they are controller-specific and they will
> > just pollute every slave binding. So this binding is added to collect
> > all such properties from all such controllers. Slave bindings should
> > simply refer to this binding and they should be rid of the warnings.
>
> Thanks for working on this. I haven't thought of any better solution
> than this approach.
>
> > There are some limitations with this approach. Firstly, there is no way
> > to specify required properties. The schema contains properties for all
> > controllers and there is no way to know which controller is being used.
>
> If required properties are a function of the controller and not the
> controller+device, then the controller schema can list required
> properties for child nodes.
For the Cadence controller at least, these should be controller
specific. Controller+device dependent properties might exist but I don't
think we have to worry about them for now.
>
> > Secondly, there is no way to restrict additional properties. Since this
> > schema will be used with an allOf operator, additionalProperties needs
> > to be true. In addition, the slave schema will have to set
> > unevaluatedProperties: false.
>
> No issue there other than unevaluatedProperties is currently not implemented.
>
> > Despite these limitations, this appears to be the best solution to this
> > problem that doesn't involve modifying existing tools or schema specs.
> >
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >
> > .../bindings/spi/cdns,qspi-nor.yaml | 33 ------------
> > .../bindings/spi/spi-slave-props.yaml | 52 +++++++++++++++++++
>
> I think you need 2+ files here. A common one that's just an 'allOf' of
> all the controller specific schemas and then controller specific child
> node schemas.
Ok.
>
> > 2 files changed, 52 insertions(+), 33 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-props.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > index 0e7087cc8bf9..0730e6a8dc4a 100644
> > --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > @@ -74,39 +74,6 @@ properties:
> > items:
> > enum: [ qspi, qspi-ocp ]
> >
> > -# subnode's properties
> > -patternProperties:
> > - "@[0-9a-f]+$":
> > - type: object
> > - description:
> > - Flash device uses the below defined properties in the subnode.
> > -
> > - properties:
> > - cdns,read-delay:
> > - $ref: /schemas/types.yaml#/definitions/uint32
> > - description:
> > - Delay for read capture logic, in clock cycles.
> > -
> > - cdns,tshsl-ns:
> > - description:
> > - Delay in nanoseconds for the length that the master mode chip select
> > - outputs are de-asserted between transactions.
> > -
> > - cdns,tsd2d-ns:
> > - description:
> > - Delay in nanoseconds between one chip select being de-activated
> > - and the activation of another.
> > -
> > - cdns,tchsh-ns:
> > - description:
> > - Delay in nanoseconds between last bit of current transaction and
> > - deasserting the device chip select (qspi_n_ss_out).
> > -
> > - cdns,tslch-ns:
> > - description:
> > - Delay in nanoseconds between setting qspi_n_ss_out low and
> > - first bit transfer.
> > -
> > required:
> > - compatible
> > - reg
> > diff --git a/Documentation/devicetree/bindings/spi/spi-slave-props.yaml b/Documentation/devicetree/bindings/spi/spi-slave-props.yaml
> > new file mode 100644
> > index 000000000000..b2248e01dc43
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-slave-props.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/spi/spi-slave-props.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Slave-specific properties for a SPI bus.
> > +
> > +description: |
> > + Many SPI controllers need to add properties to slave devices. This could be
> > + the delay in clock or data lines, etc. These properties are controller
> > + specific but need to be defined in the slave node because they are per-slave
> > + and there can be multiple slaves attached to a controller.
> > +
> > + If these properties are not added to the slave binding, then the dtbs check
> > + emits a warning. But these properties do not make much sense in the slave
> > + binding because they are controller-specific and they will just pollute every
> > + slave binding. So this binding is added to collect all such properties from
> > + all such controllers. Slave bindings should simply refer to this binding and
> > + they should be rid of the warnings.
>
> I don't think this paragraph belongs in the schema.
Ok.
>
> > +
> > +maintainers:
> > + - Pratyush Yadav <p.yadav@ti.com>
> > +
> > +properties:
> > + # cdns,qspi-nor.yaml
> > + cdns,read-delay:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + Delay for read capture logic, in clock cycles.
> > +
> > + cdns,tshsl-ns:
> > + description:
> > + Delay in nanoseconds for the length that the master mode chip select
> > + outputs are de-asserted between transactions.
> > +
> > + cdns,tsd2d-ns:
> > + description:
> > + Delay in nanoseconds between one chip select being de-activated
> > + and the activation of another.
> > +
> > + cdns,tchsh-ns:
> > + description:
> > + Delay in nanoseconds between last bit of current transaction and
> > + deasserting the device chip select (qspi_n_ss_out).
> > +
> > + cdns,tslch-ns:
> > + description:
> > + Delay in nanoseconds between setting qspi_n_ss_out low and
> > + first bit transfer.
> > +
> > +additionalProperties: true
> > --
> > 2.30.0
> >
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
next prev parent reply other threads:[~2021-06-10 13:26 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 11:17 [PATCH 0/2] Add bindings for slave-specific SPI controller properties Pratyush Yadav
2021-06-09 11:17 ` [PATCH 1/2] spi: dt-bindings: add schema listing slave-specific properties Pratyush Yadav
2021-06-09 20:51 ` Rob Herring
2021-06-10 13:25 ` Pratyush Yadav [this message]
2021-06-09 11:17 ` [PATCH 2/2] dt-bindings: mtd: spi-nor: Add a reference to spi-slave-props.yaml Pratyush Yadav
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210610132551.rqtswf7zend2f6xw@ti.com \
--to=p.yadav@ti.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=michael@walle.cc \
--cc=miquel.raynal@bootlin.com \
--cc=nm@ti.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).