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

[Pratyush Yadav <p.yadav@xxxxxx>]

On 09/06/21 03:51PM, Rob Herring wrote:
> On Wed, Jun 9, 2021 at 6:17 AM Pratyush Yadav <p.yadav@xxxxxx> 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@xxxxxx>
> > ---
> >
> >  .../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@xxxxxx>
> > +
> > +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.