[1/5] spi: dt-bindings: add binding doc for spi-qpic-snand

  Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash
Interface.

Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---
 .../bindings/spi/qcom,spi-qpic-snand.yaml     | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
  

On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote:

> +  clocks:
> +    minItems: 2
> +    maxItems: 3
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 3

The driver requests the clocks by name but this does not document the
expected set of names.  The driver also unconditionally requests all
three clocks so won't work with only two clocks.
  

On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote:
> Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash
> Interface.
> 
> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
>  .../bindings/spi/qcom,spi-qpic-snand.yaml     | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
> new file mode 100644
> index 000000000000..fa7484ce1319
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/qcom,spi-qpic-snand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QPIC NAND controller
> +
> +maintainers:
> +  - Md sadre Alam <quic_mdalam@quicinc.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq9574-snand
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 3
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 3
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-controller.yaml#
> +  - if:

> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq9574-snand
> +
> +    then:
> +      properties:
> +        dmas:
> +          items:
> +            - description: tx DMA channel
> +            - description: rx DMA channel
> +            - description: cmd DMA channel
> +
> +        dma-names:
> +          items:
> +            - const: tx
> +            - const: rx
> +            - const: cmd

None of this complexity here is needed, you have only one device in this
binding and therefore can define these properties at the top level.

Cheers,
Conor.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> +    qpic_nand: spi@79b0000 {
> +        compatible = "qcom,ipq9574-snand";
> +        reg = <0x1ac00000 0x800>;
> +
> +        clocks = <&gcc GCC_QPIC_CLK>,
> +                 <&gcc GCC_QPIC_AHB_CLK>,
> +                 <&gcc GCC_QPIC_IO_MACRO_CLK>;
> +        clock-names = "core", "aon", "iom";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        flash@0 {
> +            compatible = "spi-nand";
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +            nand-ecc-engine = <&qpic_nand>;
> +            nand-ecc-strength = <4>;
> +            nand-ecc-step-size = <512>;
> +            };
> +        };
> -- 
> 2.34.1
>
  

On 15/02/2024 14:48, Md Sadre Alam wrote:
> Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash
> Interface.
> 

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
>  .../bindings/spi/qcom,spi-qpic-snand.yaml     | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
> new file mode 100644
> index 000000000000..fa7484ce1319
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml

Filename like compatible.

> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/qcom,spi-qpic-snand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QPIC NAND controller
> +
> +maintainers:
> +  - Md sadre Alam <quic_mdalam@quicinc.com>
> +

Provide description which will describe hardware.

> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq9574-snand
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 3

You must document the items (could be sufficient in clock-names if the
names are obvious).


Why the clocks are flexible? This given IPQ9574 has variable clock
inputs? Please explain.

> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 3
> +

required goes here.

> +allOf:
> +  - $ref: /schemas/spi/spi-controller.yaml#


> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq9574-snand
> +
> +    then:
> +      properties:
> +        dmas:
> +          items:
> +            - description: tx DMA channel
> +            - description: rx DMA channel
> +            - description: cmd DMA channel
> +
> +        dma-names:
> +          items:
> +            - const: tx
> +            - const: rx
> +            - const: cmd

No clue why it is here, move it to top level.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> +    qpic_nand: spi@79b0000 {

Drop unused label

> +        compatible = "qcom,ipq9574-snand";
> +        reg = <0x1ac00000 0x800>;
> +
> +        clocks = <&gcc GCC_QPIC_CLK>,
> +                 <&gcc GCC_QPIC_AHB_CLK>,
> +                 <&gcc GCC_QPIC_IO_MACRO_CLK>;
> +        clock-names = "core", "aon", "iom";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        flash@0 {
> +            compatible = "spi-nand";
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +            nand-ecc-engine = <&qpic_nand>;
> +            nand-ecc-strength = <4>;
> +            nand-ecc-step-size = <512>;
> +            };

Fix indentation.

> +        };

Best regards,
Krzysztof
  

On 2/15/2024 7:52 PM, Mark Brown wrote:
> On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote:
> 
>> +  clocks:
>> +    minItems: 2
>> +    maxItems: 3
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    maxItems: 3
> 
> The driver requests the clocks by name but this does not document the
> expected set of names.  The driver also unconditionally requests all
> three clocks so won't work with only two clocks.

Thanks for reviewing, Will document the clock name in next patch.
By mistake i have given minItems = 2 and maxItems = 3 , will fix
this in next patch.
  

On 2/15/2024 7:54 PM, Conor Dooley wrote:
> On Thu, Feb 15, 2024 at 07:18:52PM +0530, Md Sadre Alam wrote:
>> Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash
>> Interface.
>>
>> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>   .../bindings/spi/qcom,spi-qpic-snand.yaml     | 82 +++++++++++++++++++
>>   1 file changed, 82 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
>> new file mode 100644
>> index 000000000000..fa7484ce1319
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
>> @@ -0,0 +1,82 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/qcom,spi-qpic-snand.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm QPIC NAND controller
>> +
>> +maintainers:
>> +  - Md sadre Alam <quic_mdalam@quicinc.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq9574-snand
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 2
>> +    maxItems: 3
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    maxItems: 3
>> +
>> +allOf:
>> +  - $ref: /schemas/spi/spi-controller.yaml#
>> +  - if:
> 
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-snand
>> +
>> +    then:
>> +      properties:
>> +        dmas:
>> +          items:
>> +            - description: tx DMA channel
>> +            - description: rx DMA channel
>> +            - description: cmd DMA channel
>> +
>> +        dma-names:
>> +          items:
>> +            - const: tx
>> +            - const: rx
>> +            - const: cmd
> 
> None of this complexity here is needed, you have only one device in this
> binding and therefore can define these properties at the top level.

  Thanks for reviewing, Will fix this in next patch.
> 
> Cheers,
> Conor.
> 
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>> +    qpic_nand: spi@79b0000 {
>> +        compatible = "qcom,ipq9574-snand";
>> +        reg = <0x1ac00000 0x800>;
>> +
>> +        clocks = <&gcc GCC_QPIC_CLK>,
>> +                 <&gcc GCC_QPIC_AHB_CLK>,
>> +                 <&gcc GCC_QPIC_IO_MACRO_CLK>;
>> +        clock-names = "core", "aon", "iom";
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        flash@0 {
>> +            compatible = "spi-nand";
>> +            reg = <0>;
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            nand-ecc-engine = <&qpic_nand>;
>> +            nand-ecc-strength = <4>;
>> +            nand-ecc-step-size = <512>;
>> +            };
>> +        };
>> -- 
>> 2.34.1
>>
  

On 2/16/2024 12:32 AM, Krzysztof Kozlowski wrote:
> On 15/02/2024 14:48, Md Sadre Alam wrote:
>> Add device-tree binding documentation for QCOM QPIC-SNAND-NAND Flash
>> Interface.
>>
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Ok
> 
>> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>   .../bindings/spi/qcom,spi-qpic-snand.yaml     | 82 +++++++++++++++++++
>>   1 file changed, 82 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
>> new file mode 100644
>> index 000000000000..fa7484ce1319
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
> 
> Filename like compatible.
Ok
> 
>> @@ -0,0 +1,82 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/qcom,spi-qpic-snand.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm QPIC NAND controller
>> +
>> +maintainers:
>> +  - Md sadre Alam <quic_mdalam@quicinc.com>
>> +
> 
> Provide description which will describe hardware.
Ok
> 
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq9574-snand
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 2
>> +    maxItems: 3
> 
> You must document the items (could be sufficient in clock-names if the
> names are obvious).
Ok
> 
> 
> Why the clocks are flexible? This given IPQ9574 has variable clock
> inputs? Please explain.

  I have checked Hardware Spec. and clocks are fixed in IPQ9574. Will fix in next
  patch.
> 
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    maxItems: 3
>> +
> 
> required goes here.
Ok
> 
>> +allOf:
>> +  - $ref: /schemas/spi/spi-controller.yaml#
> 
> 
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-snand
>> +
>> +    then:
>> +      properties:
>> +        dmas:
>> +          items:
>> +            - description: tx DMA channel
>> +            - description: rx DMA channel
>> +            - description: cmd DMA channel
>> +
>> +        dma-names:
>> +          items:
>> +            - const: tx
>> +            - const: rx
>> +            - const: cmd
> 
> No clue why it is here, move it to top level.
Ok
> 
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>> +    qpic_nand: spi@79b0000 {
> 
> Drop unused label
Ok
> 
>> +        compatible = "qcom,ipq9574-snand";
>> +        reg = <0x1ac00000 0x800>;
>> +
>> +        clocks = <&gcc GCC_QPIC_CLK>,
>> +                 <&gcc GCC_QPIC_AHB_CLK>,
>> +                 <&gcc GCC_QPIC_IO_MACRO_CLK>;
>> +        clock-names = "core", "aon", "iom";
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        flash@0 {
>> +            compatible = "spi-nand";
>> +            reg = <0>;
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            nand-ecc-engine = <&qpic_nand>;
>> +            nand-ecc-strength = <4>;
>> +            nand-ecc-step-size = <512>;
>> +            };
> 
> Fix indentation.
Ok
> 
>> +        };
> 
> Best regards,
> Krzysztof
>