Re: [PATCH 1/4] dt-bindings: spi: Add YAML schemas for the generic SPI options

[Maxime Ripard <maxime.ripard@xxxxxxxxxxx>]

Hi,

On Tue, May 07, 2019 at 09:35:28AM -0500, Rob Herring wrote:
> On Tue, May 7, 2019 at 8:48 AM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> >
> > The SPI controllers have a bunch of generic options that are needed in a
> > device tree. Add a YAML schemas for those.
>
> I'd started on this one, but was planning to move it to the schema
> repository. The issue there is re-licensing (adding BSD 2 clause).
> Maybe better to just move it later.

I just found out that dt-doc-validate also chokes on the reference
URI. Maybe I should just submit it to the repo then once that is
settled?

> > +properties:
> > +  $nodename:
> > +    pattern: "^spi(@[a-zA-Z0-9]+)?$"
>
> I think we want just "(@.*)". At a minimum, you need to allow for ','.
> It would be the a bus schema for the parent which should validate unit
> addresses, so we should pretty much just allow anything here.

The issue with this is that it will also match any node starting with
spi. In the Allwinner case, that also means the pinctrl nodes with spi
pins in them, but I'm sure we can find more corner cases.

> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  cs-gpios:
> > +    description: |
> > +      GPIOs used as chip selects.
> > +      If that property is used, the number of chip selects will be
> > +      increased automatically with max(cs-gpios, hardware chip selects).
> > +
> > +      So if, for example, the controller has 2 CS lines, and the
> > +      cs-gpios looks like this
> > +        cs-gpios = <&gpio1 0 0>, <0>, <&gpio1 1 0>, <&gpio1 2 0>;
> > +
> > +      Then it should be configured so that num_chipselect = 4, with
> > +      the following mapping
> > +        cs0 : &gpio1 0 0
> > +        cs1 : native
> > +        cs2 : &gpio1 1 0
> > +        cs3 : &gpio1 2 0
> > +
> > +  num-cs:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Total number of chip selects.
> > +
> > +  spi-slave:
> > +    $ref: /schemas/types.yaml#/definitions/flag
>
> "type: boolean" is sufficient here. Maybe we should just remove
> 'flag'. OTOH, maybe consistency with other types and the abstraction
> is better as we could add to the flag schema.

I was trying to be consistent. Do you want me to remove it?

> > +      spi-rx-bus-width:
> > +        allOf:
> > +          - $ref: /schemas/types.yaml#/definitions/uint32
> > +          - enum: [ 1, 2, 4, 8 ]
>
> Is the old doc out of date and 8 is allowed now?

It's not, it's a copy and paste mistake.

I'll respin this with your fixes, thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com