diff mbox series

[1/2] spi: dt-bindings: add schema listing slave-specific properties

Message ID 20210609111707.9555-2-p.yadav@ti.com
State New
Headers show
Series Add bindings for slave-specific SPI controller properties | expand

Commit Message

Pratyush Yadav June 9, 2021, 11:17 a.m. UTC
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.

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.
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.

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 +++++++++++++++++++
 2 files changed, 52 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-slave-props.yaml

Comments

Rob Herring June 9, 2021, 8:51 p.m. UTC | #1
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.

> 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.

>  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.

> +
> +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
>
Pratyush Yadav June 10, 2021, 1:25 p.m. UTC | #2
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
> >
diff mbox series

Patch

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.
+
+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