Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Sam Ravnborg <sam@xxxxxxxxxxxx>]

Hi Maxime.

On Mon, Mar 16, 2020 at 09:48:50PM +0100, Maxime Ripard wrote:
> Hi Sam,
> 
> On Sun, Mar 15, 2020 at 02:43:42PM +0100, Sam Ravnborg wrote:
> > Independent bindings can be SPI slaves which for example is
> > the case for several panel bindings.
> >
> > Move SPI slave properties to spi-slave.yaml so the independent
> > SPI slave bindings can include spi-slave.yaml rather than
> > duplicating the properties.
> >
> > Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Cc: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Mark Brown <broonie@xxxxxxxxxx>
> > Cc: linux-spi@xxxxxxxxxxxxxxx
> > ---
> >  .../bindings/spi/spi-controller.yaml          | 63 +-------------
> >  .../devicetree/bindings/spi/spi-slave.yaml    | 83 +++++++++++++++++++
> >  2 files changed, 86 insertions(+), 60 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/spi/spi-slave.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > index 1e0ca6ccf64b..99531c8d10dd 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > @@ -67,71 +67,14 @@ patternProperties:
> >    "^.*@[0-9a-f]+$":
> >      type: object
> >
> > +    allOf:
> > +      - $ref: spi-slave.yaml#
> > +
> >      properties:
> >        compatible:
> >          description:
> >            Compatible of the SPI device.
> >
> > -      reg:
> > -        minimum: 0
> > -        maximum: 256
> > -        description:
> > -          Chip select used by the device.
> > -
> > -      spi-3wire:
> > -        $ref: /schemas/types.yaml#/definitions/flag
> > -        description:
> > -          The device requires 3-wire mode.
> > -
> > -      spi-cpha:
> > -        $ref: /schemas/types.yaml#/definitions/flag
> > -        description:
> > -          The device requires shifted clock phase (CPHA) mode.
> > -
> > -      spi-cpol:
> > -        $ref: /schemas/types.yaml#/definitions/flag
> > -        description:
> > -          The device requires inverse clock polarity (CPOL) mode.
> > -
> > -      spi-cs-high:
> > -        $ref: /schemas/types.yaml#/definitions/flag
> > -        description:
> > -          The device requires the chip select active high.
> > -
> > -      spi-lsb-first:
> > -        $ref: /schemas/types.yaml#/definitions/flag
> > -        description:
> > -          The device requires the LSB first mode.
> > -
> > -      spi-max-frequency:
> > -        $ref: /schemas/types.yaml#/definitions/uint32
> > -        description:
> > -          Maximum SPI clocking speed of the device in Hz.
> > -
> > -      spi-rx-bus-width:
> > -        allOf:
> > -          - $ref: /schemas/types.yaml#/definitions/uint32
> > -          - enum: [ 1, 2, 4, 8 ]
> > -          - default: 1
> > -        description:
> > -          Bus width to the SPI bus used for MISO.
> > -
> > -      spi-rx-delay-us:
> > -        description:
> > -          Delay, in microseconds, after a read transfer.
> > -
> > -      spi-tx-bus-width:
> > -        allOf:
> > -          - $ref: /schemas/types.yaml#/definitions/uint32
> > -          - enum: [ 1, 2, 4, 8 ]
> > -          - default: 1
> > -        description:
> > -          Bus width to the SPI bus used for MOSI.
> > -
> > -      spi-tx-delay-us:
> > -        description:
> > -          Delay, in microseconds, after a write transfer.
> > -
> 
> I can see what you're trying to do, but you don't really need to.
> 
> All the SPI devices will be declared under a spi controller node that
> will validate its child nodes (and thus the devices) already.

This was the missing piece - thanks.
And as Mark put it "why is this suddenly an issue"?
Turns out this is already properly handled and I made up an issue.
Maybe Mark tried to explian it to me already...

> 
> Doing it this way would actually make all the checks happen twice,
> once as part of the SPI controller, once as part of the SPI device
> binding, without any good reason.

I had focus on validating the example in the binding
file and not the full picture.

One thing I do not see properly addressed, but maybe I just miss it.
What triggers that we catch properties that are not supposed to be
present?

If we see a unsupported property "foobar":

spi {
    ...
    panel {
       ....
       foobar = <1>;
    };
};

somewhere in a SPI slave binding we should catch this.
If for no other reasons that it could be a simple spelling mistake
that otherwise could go undetected for a long time.
But maybe this is really not feasible to do?

	Sam