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.
