Re: [PATCH v2] dt-bindings: spi: convert Freescale DSPI to dt-schema

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

On Tue, Nov 15, 2022 at 02:46:21PM +0100, Krzysztof Kozlowski wrote:
> > +$id: http://devicetree.org/schemas/spi/fsl,spi-fsl-dspi.yaml
> 
> Why second "fsl" in file name? It does not patch compatibles and
> duplicates the vendor. We do not have compatibles "nxp,imx6-nxp".

Ok, which file name would be good then? There are 9 different (all SoC
specific) compatible strings, surely the convention of naming the file
after a compatible string has some limitations...

> > +$schema: http://devicetree.org/meta-schemas/core.yaml
> > +
> > +title: Freescale DSPI Controller
> > +
> > +maintainers:
> > +  - Vladimir Oltean <olteanv@xxxxxxxxx>
> > +
> > +allOf:
> > +  - $ref: "spi-controller.yaml#"
> 
> Drop quotes.
> 
> > +
> > +properties:
> > +  compatible:
> > +    description:
> > +      Some integrations can have a single compatible string containing their
> > +      SoC name (LS1012A, LS1021A, ...). Others require their SoC compatible
> > +      string, plus a fallback compatible string (either on LS1021A or on
> > +      LS2085A).
> 
> Why? The fsl,ls1012a-dspi device is either compatible with
> fsl,ls1021a-v1.0-dspi or not. It cannot be both - compatible and not
> compatible.

LS1012A is compatible with LS1021A to the extent that it works when
treated like a LS1021A. LS1012A has a FIFO size of 8 SPI words, LS1021A
of just 4. Treating it like LS1021A means roughly half the performance,
but it still works.

I didn't invent any of this. When I took over the driver, there were
device trees like this all over the place:

		dspi: spi@2100000 {
			compatible = "fsl,ls1012a-dspi", "fsl,ls1021a-v1.0-dspi";
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0x0 0x2100000 0x0 0x10000>;
			interrupts = <0 64 IRQ_TYPE_LEVEL_HIGH>;
			clock-names = "dspi";
			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
					    QORIQ_CLK_PLL_DIV(1)>;
			spi-num-chipselects = <5>;
			big-endian;
			status = "disabled";
		};

but the Linux driver pre-~5.7 always relied on the fallback compatible
string (LS1021A in this case). I'm working with what's out in the field,
haven't changed a thing there.

> > +    oneOf:
> > +      - enum:
> > +          - fsl,ls1012a-dspi
> > +          - fsl,ls1021a-v1.0-dspi
> > +          - fsl,ls1028a-dspi
> > +          - fsl,ls2085a-dspi
> > +          - fsl,lx2160a-dspi
> > +          - fsl,vf610-dspi
> > +      - items:
> > +          - enum:
> > +              - fsl,ls1012a-dspi
> > +              - fsl,ls1028a-dspi
> > +              - fsl,ls1043a-dspi
> > +              - fsl,ls1046a-dspi
> > +              - fsl,ls1088a-dspi
> > +          - const: fsl,ls1021a-v1.0-dspi
> > +      - items:
> > +          - enum:
> > +              - fsl,ls2080a-dspi
> > +              - fsl,lx2160a-dspi
> > +          - const: fsl,ls2085a-dspi
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: dspi
> > +
> > +  dmas:
> > +    maxItems: 2
> > +
> > +  dma-names:
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +
> > +  spi-num-chipselects:
> 
> Would be nice to deprecated it in separate patches. There is num-cs
> property.

Will add this on my TODO list. Right now I'm just converting what exists.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/fsl,qoriq-clockgen.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        spi@2100000 {
> > +            compatible = "fsl,ls1028a-dspi", "fsl,ls1021a-v1.0-dspi";
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            reg = <0x0 0x2100000 0x0 0x10000>;
> 
> reg by convention is a second property.

ok.

> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index dca677f9e1b9..a475e757f8da 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -101,6 +101,7 @@ properties:
> >  # The controller specific properties go here.
> >  allOf:
> >    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> > +  - $ref: fsl,spi-fsl-dspi-peripheral-props.yaml#
> >    - $ref: samsung,spi-peripheral-props.yaml#
> >    - $ref: nvidia,tegra210-quad-peripheral-props.yaml#
> >  
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c242098a34f9..c75ae49c85b5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8160,7 +8160,8 @@ FREESCALE DSPI DRIVER
> >  M:	Vladimir Oltean <olteanv@xxxxxxxxx>
> >  L:	linux-spi@xxxxxxxxxxxxxxx
> >  S:	Maintained
> > -F:	Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
> > +F:	Documentation/devicetree/bindings/spi/fsl,spi-fsl-dspi.yaml
> 
> Instead: Documentation/devicetree/bindings/spi/fsl,spi-fsl-dspi*

ok...