Re: [PATCH v3 2/3] spi: dt-bindings: fsl-dspi: Convert to yaml format

[Frank Li <Frank.li@xxxxxxx>]

On Fri, Jun 21, 2024 at 03:42:11PM +0300, Vladimir Oltean wrote:
> On Thu, Jun 20, 2024 at 12:58:28PM -0400, Frank Li wrote:
> > Convert dt-binding spi-fsl-dspi.txt to yaml format.
> > 
> > Addtional changes during convert:
> > - compatible string "fsl,ls1028a-dspi" can be followed by
> > fsl,ls1021a-v1.0-dspi.
> > - Change "dspi0@4002c000" to "spi@4002c000" in example.
> > - Reorder properties in example.
> > - Use GIC include in example.
> > - Remove fsl,spi-cs-sck-delay and fsl,spi-sck-cs-delay by use common SPI
> > property.
> > - Use compatible string 'jedec,spi-nor' in example.
> > - Split peripheral part to fsl,spi-dspi-peripheral-props.yaml
> > 
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > 
> > ---
> > Use part of Vladimir Oltean's work at
> > https://lore.kernel.org/linux-spi/20221111224651.577729-1-vladimir.oltean@xxxxxxx/
> 
> Hm, you took part of that but gave no attribution? The portion below ---
> is also discarded when the patch is applied, so even the link is lost,
> FYI.

I am not sure what should be added in comments? 

Ref part of Vladimir Oltean's work at https:// ... (I am not sure if allow
http link).
 
Or coworked-with Vladimir Oltean ...
Or use seperated patch for your part.

What do you like?

Frank

> 
> > ---
> >  .../devicetree/bindings/spi/fsl,dspi.yaml          | 115 +++++++++++++++++++++
> >  .../spi/fsl,spi-dspi-peripheral-props.yaml         |  28 +++++
> 
> For consistency, could you name this fsl,dspi-peripheral-props.yaml?
> 
> >  .../devicetree/bindings/spi/spi-fsl-dspi.txt       |  65 ------------
> >  .../bindings/spi/spi-peripheral-props.yaml         |   1 +
> 
> No MAINTAINERS change for the schema path? There was a discussion with
> Krzysztof in the old thread.
> 
> >  4 files changed, 144 insertions(+), 65 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/fsl,dspi.yaml b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
> > new file mode 100644
> > index 0000000000000..924ba19aea017
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
> > @@ -0,0 +1,115 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/spi/fsl,dspi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM Freescale DSPI controller
> > +
> > +maintainers:
> > +  - Frank Li <Frank.Li@xxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - enum:
> > +          - fsl,vf610-dspi
> > +          - fsl,ls1021a-v1.0-dspi
> > +          - fsl,ls1012a-dspi
> > +          - fsl,ls1028a-dspi
> > +          - fsl,ls1043a-dspi
> > +          - fsl,ls1046a-dspi
> > +          - fsl,ls1088a-dspi
> > +          - fsl,ls2080a-dspi
> > +          - fsl,ls2085a-dspi
> > +          - fsl,lx2160a-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:
> > +          - const: fsl,ls2080a-dspi
> > +          - const: fsl,ls2085a-dspi
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: dspi
> > +
> > +  pinctrl-0: true
> > +
> > +  pinctrl-names:
> > +    items:
> > +      - const: default
> 
> I don't think that pinctrl properties need to be specified in the
> schema. Somehow, I think dt-schema applies
> dtschema/schemas/pinctrl/pinctrl-consumer.yaml by default every time.
> 
> > +
> > +  spi-num-chipselects:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: the number of the chipselect signals.
> 
> Worth mentioning that this is about _native_ chip select signals.
> cs-gpios don't count against this number.
> 
> > +
> > +  big-endian:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      If present the dspi device's registers are implemented
> > +      in big endian mode.
> 
> I'm not sure that this needs an explanation, it is an absolutely generic
> property with a universal meaning.
> 
> > +
> > +  bus-num:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: the slave chip chipselect signal number.
> 
> In fact, no, this is not a chip select number, the old documentation is
> wrong. It just gets assigned to the struct spi_controller :: bus_num.
> In my last submitted version I wrote "SoC-specific identifier for the
> SPI controller", that seems perfectly adequate.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - pinctrl-0
> > +  - pinctrl-names
> 
> interrupts and pinctrl are not required.
> 
> > +  - spi-num-chipselects
> > +
> > +allOf:
> > +  - $ref: spi-controller.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/vf610-clock.h>
> > +
> > +    spi@4002c000 {
> > +        compatible = "fsl,vf610-dspi";
> > +        reg = <0x4002c000 0x1000>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
> > +        clocks = <&clks VF610_CLK_DSPI0>;
> > +        clock-names = "dspi";
> > +        spi-num-chipselects = <5>;
> > +        bus-num = <0>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&pinctrl_dspi0_1>;
> > +        big-endian;
> > +
> > +        flash@0 {
> > +                compatible = "jedec,spi-nor";
> > +                reg = <0>;
> > +                spi-max-frequency = <16000000>;
> > +                spi-cpol;
> > +                spi-cpha;
> > +                spi-cs-setup-delay-ns = <100>;
> > +                spi-cs-hold-delay-ns = <50>;
> > +        };
> > +    };
> > +
> 
> Please remove newline at end of file.