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

From: Krzysztof Kozlowski
Date: Thu Feb 15 2024 - 14:02:46 EST


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@xxxxxxxxxxx>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
> Co-developed-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> Signed-off-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxx>
> +

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