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
