RE: [EXTERNAL] Re: [PATCH v4 2/5] dt-bindings: spi: cadence: Add MRVL overlay bindings documentation for Cadence XSPI

[Witold Sadowski]

> On 09/05/2024 03:05, Witold Sadowski wrote:
> > Add new bindings for v2 Marvell xSPI overlay:
> > mrvl,xspi-nor  compatible string
> 
> Where?
> 
> Why double space?
> 
> subject prefix: spi goes first
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are explained
> here:
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.kernel.org_doc_html_latest_devicetree_bindings_submitting-
> 2Dpatches.html-23i-2Dfor-2Dpatch-
> 2Dsubmitters&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=GKgcn-g6ZX-
> JmCL3S2qKgVQhvhv7hu2n8En-
> dZbLTa8&m=qeqJlq7clOFz9f6HAtdYw0Qlc95bPthAbwzLip4BHM3fDtQ_rIx-
> R6WPFdKtjM5T&s=d2T7Rq5gLIrXRKVDPv9VDiGgiQVD7GFAxw7lFJ1tgvg&e=
> 

Looks like old compatible string was included in commit message.

> 
> > New compatible string to distinguish between orginal and modified xSPI
> > block
> >
> > PHY configuration registers
> > Allow to change orginal xSPI PHY configuration values. If not set, and
> > Marvell overlay is enabled, safe defaults will be written into xSPI
> > PHY
> >
> > Optional base for xfer register set
> > Additional reg field to allocate xSPI Marvell overlay XFER block
> 
> I have troubles reading this. Is this some sort of one long sentence or a
> list?
> 
> >
> > Signed-off-by: Witold Sadowski <wsadowski@xxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/spi/cdns,xspi.yaml    | 78 +++++++++++++++----
> >  1 file changed, 65 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > index eb0f92468185..094f8b7ffc49 100644
> > --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > @@ -17,22 +17,43 @@ description: |
> >
> >  allOf:
> >    - $ref: spi-controller.yaml#
> > +  - if:
> 
> Move the allOf after required block.
> 
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: marvell,cn10-xspi-nor
> > +    then:
> > +      properties:
> > +        reg-names:
> > +          items:
> > +            - const: io
> > +            - const: sdma
> > +            - const: aux
> > +            - const: xferbase
> > +        reg:
> > +          items:
> > +            - description: address and length of the controller
> register set
> > +            - description: address and length of the Slave DMA data
> port
> > +            - description: address and length of the auxiliary
> registers
> > +            - description: address and length of the xfer registers
> > +    else:
> > +      properties:
> > +        reg-names:
> > +          items:
> > +            - const: io
> > +            - const: sdma
> > +            - const: aux
> > +        reg:
> > +          items:
> > +            - description: address and length of the controller
> register set
> > +            - description: address and length of the Slave DMA data
> port
> > +            - description: address and length of the auxiliary
> > + registers
> >
> >  properties:
> >    compatible:
> > -    const: cdns,xspi-nor
> > -
> > -  reg:
> > -    items:
> > -      - description: address and length of the controller register set
> > -      - description: address and length of the Slave DMA data port
> > -      - description: address and length of the auxiliary registers
> 
> Widest constraints stay here.
> 
> > -
> > -  reg-names:
> > -    items:
> > -      - const: io
> > -      - const: sdma
> > -      - const: aux
> 
> Widest constraints stay here.

Yaml file will be reworked to match guidelines.

> 
> > +    enum:
> > +      - cdns,xspi-nor
> > +      - marvell,cn10-xspi-nor
> >
> >    interrupts:
> >      maxItems: 1
> > @@ -68,6 +89,37 @@ examples:
> >                  reg = <0>;
> >              };
> >
> > +            flash@1 {
> > +                compatible = "jedec,spi-nor";
> > +                spi-max-frequency = <75000000>;
> > +                reg = <1>;
> > +            };
> > +        };
> > +    };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        spi@d0010000 {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "marvell,cn10-xspi-nor";
> > +            reg = <0x0 0xa0010000 0x0 0x1040>,
> > +                  <0x0 0xb0000000 0x0 0x1000>,
> > +                  <0x0 0xa0020000 0x0 0x100>,
> > +                  <0x0 0xa0090000 0x0 0x100>;
> 
> No need for new example for difference in one property.

Ok, I will keep only original one

> 
> Best regards,
> Krzysztof