All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Akash Asthana <akashast@codeaurora.org>,
	agross@kernel.org, mark.rutland@arm.com, robh+dt@kernel.org
Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, mgautam@codeaurora.org,
	rojay@codeaurora.org, skakit@codeaurora.org,
	Akash Asthana <akashast@codeaurora.org>
Subject: Re: [PATCH 1/2] dt-bindings: spi: Convert QSPI bindings to YAML
Date: Wed, 19 Feb 2020 18:29:41 -0800	[thread overview]
Message-ID: <158216578112.184098.9357700822184458798@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <1581932974-21654-2-git-send-email-akashast@codeaurora.org>

Quoting Akash Asthana (2020-02-17 01:49:33)
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.yaml
> new file mode 100644
> index 0000000..977070a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/spi/qcom,spi-qcom-qspi.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Qualcomm Quad Serial Peripheral Interface (QSPI)
> +
> +maintainers:
> + - Mukesh Savaliya <msavaliy@codeaurora.org>
> + - Akash Asthana <akashast@codeaurora.org>
> +
> +description: |

Drop the | because it doesn't look like any formatting needs to be
maintained in the text for the description.

> + The QSPI controller allows SPI protocol communication in single, dual, or quad
> + wire transmission modes for read/write access to slaves such as NOR flash.
> +
> +allOf:
> +  - $ref: /spi/spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,sdm845-qspi
> +      - const: qcom,qspi-v1
> +
> +  reg:
> +    description: Base register location and length.

Drop description? It doesn't seem useful.

> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: iface
> +      - const: core
> +
> +  clocks:
> +    items:
> +      - description: AHB clock
> +      - description: QSPI core clock.

Please drop the full-stop on core clock.

> +
> +  "#address-cells":
> +     const: 1
> +
> +  "#size-cells":
> +    const: 0

Aren't these two unnecessary because they're covered by the
spi-controller.yaml binding?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-names
> +  - clocks
> +  - "#address-cells"
> +  - "#size-cells"

These last two are also covered by spi-controller binding.

> +
> +

Why two newlines instead of one?

> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    soc: soc@0 {

Remove this node from example please.

> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        qspi: spi@88df000 {
> +            compatible = "qcom,sdm845-qspi", "qcom,qspi-v1";
> +            reg = <0 0x88df000 0 0x600>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
> +            clock-names = "iface", "core";
> +            clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
> +                <&gcc GCC_QSPI_CORE_CLK>;

Weird tabbing here. Just use spaces and align it up.

> +
> +                flash@0 {
> +                    compatible = "jedec,spi-nor";
> +                    reg = <0>;
> +                    spi-max-frequency = <25000000>;
> +                    spi-tx-bus-width = <2>;
> +                    spi-rx-bus-width = <2>;
> +                };

Is this flash node necessary for the example?

> +        };
> +    };
> +

Nitpick: Why newline here?

> +...

  reply	other threads:[~2020-02-20  2:29 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-17  9:49 [PATCH 0/2] Convert QSPI binding to YAML and add interconnect doc Akash Asthana
2020-02-17  9:49 ` [PATCH 1/2] dt-bindings: spi: Convert QSPI bindings to YAML Akash Asthana
2020-02-20  2:29   ` Stephen Boyd [this message]
2020-03-13 10:51     ` Akash Asthana
2020-02-17  9:49 ` [PATCH 2/2] dt-bindings: spi: Add interconnect binding for QSPI Akash Asthana
2020-02-20  2:30   ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=158216578112.184098.9357700822184458798@swboyd.mtv.corp.google.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=akashast@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mgautam@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=rojay@codeaurora.org \
    --cc=skakit@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.