Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 30 Mar 2022 16:44:07 +0530
From:   Kuldeep Singh <singh.kuldeep87k@gmail.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     linux-kernel@vger.kernel.org, Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        linux-arm-msm@vger.kernel.org, linux-spi@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH 1/2] spi: dt-bindings: qcom,spi-geni-qcom: convert to
 dtschema
Message-ID: <20220330111407.GA51399@9a2d8922b8f1>
References: <20220329112717.252647-1-krzysztof.kozlowski@linaro.org>
 <20220330062313.GA51331@9a2d8922b8f1>
 <ac686e08-008f-be08-db5e-b6d4d08df315@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ac686e08-008f-be08-db5e-b6d4d08df315@linaro.org>
Precedence: bulk

On Wed, Mar 30, 2022 at 08:48:38AM +0200, Krzysztof Kozlowski wrote:
> On 30/03/2022 08:23, Kuldeep Singh wrote:
> > On Tue, Mar 29, 2022 at 01:27:16PM +0200, Krzysztof Kozlowski wrote:
> >> Convert the GENI based Qualcomm Universal Peripheral (QUP) Serial
> >> Peripheral Interface (SPI) bindings to DT Schema.
> >>
> >> The original bindings in TXT were not complete, so add during conversion
> >> properties already used in DTS and/or in the driver: reg-names, dmas,
> >> interconnects, operating points and power-domains.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >>  .../bindings/spi/qcom,spi-geni-qcom.txt       |  39 ------
> >>  .../bindings/spi/qcom,spi-geni-qcom.yaml      | 131 ++++++++++++++++++
> >>  2 files changed, 131 insertions(+), 39 deletions(-)
> >>  delete mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
> >>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
> >> deleted file mode 100644
> >> index c8c1e913f4e7..000000000000
> >> --- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.txt
> >> +++ /dev/null
> >> @@ -1,39 +0,0 @@
> >> -GENI based Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
> >> -
> >> -The QUP v3 core is a GENI based AHB slave that provides a common data path
> >> -(an output FIFO and an input FIFO) for serial peripheral interface (SPI)
> >> -mini-core.
> >> -
> >> -SPI in master mode supports up to 50MHz, up to four chip selects, programmable
> >> -data path from 4 bits to 32 bits and numerous protocol variants.
> >> -
> >> -Required properties:
> >> -- compatible:		Must contain "qcom,geni-spi".
> >> -- reg:			Must contain SPI register location and length.
> >> -- interrupts:		Must contain SPI controller interrupts.
> >> -- clock-names:		Must contain "se".
> >> -- clocks:		Serial engine core clock needed by the device.
> >> -- #address-cells:	Must be <1> to define a chip select address on
> >> -			the SPI bus.
> >> -- #size-cells:		Must be <0>.
> >> -
> >> -SPI Controller nodes must be child of GENI based Qualcomm Universal
> >> -Peripharal. Please refer GENI based QUP wrapper controller node bindings
> >> -described in Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml.
> >> -
> >> -SPI slave nodes must be children of the SPI master node and conform to SPI bus
> >> -binding as described in Documentation/devicetree/bindings/spi/spi-bus.txt.
> >> -
> >> -Example:
> >> -	spi0: spi@a84000 {
> >> -		compatible = "qcom,geni-spi";
> >> -		reg = <0xa84000 0x4000>;
> >> -		interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
> >> -		clock-names = "se";
> >> -		clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>;
> >> -		pinctrl-names = "default", "sleep";
> >> -		pinctrl-0 = <&qup_1_spi_2_active>;
> >> -		pinctrl-1 = <&qup_1_spi_2_sleep>;
> >> -		#address-cells = <1>;
> >> -		#size-cells = <0>;
> >> -	};
> >> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
> >> new file mode 100644
> >> index 000000000000..a85ff02ba1db
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml
> >> @@ -0,0 +1,131 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/spi/qcom,spi-geni-qcom.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: GENI based Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
> >> +
> >> +maintainers:
> >> +  - Andy Gross <agross@kernel.org>
> >> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> >> +  - Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> +
> >> +description:
> >> +  The QUP v3 core is a GENI based AHB slave that provides a common data path
> >> +  (an output FIFO and an input FIFO) for serial peripheral interface (SPI)
> >> +  mini-core.
> >> +
> >> +  SPI in master mode supports up to 50MHz, up to four chip selects,
> >> +  programmable data path from 4 bits to 32 bits and numerous protocol variants.
> >> +
> >> +  SPI Controller nodes must be child of GENI based Qualcomm Universal
> >> +  Peripharal. Please refer GENI based QUP wrapper controller node bindings
> >> +  described in Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml.
> >> +
> >> +allOf:
> >> +  - $ref: /spi/spi-controller.yaml#
> > 
> > Rob sometime back sent an update on how to refer absolute paths.
> > Please see below:
> > https://lore.kernel.org/linux-spi/20220325215652.525383-1-robh@kernel.org/
> 
> Yes, this is wrong. I copied other existing schema without checking. :(

No issues.

> 
> > 
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: qcom,geni-spi
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +
> >> +  clock-names:
> >> +    const: se
> >> +
> >> +  dmas:
> >> +    maxItems: 2
> >> +
> >> +  dma-names:
> >> +    items:
> >> +      - const: tx
> >> +      - const: rx
> >> +
> >> +  interconnects:
> >> +    minItems: 2
> > 
> > We can skip minItems here.
> > As minimim value defaults to maximum if not defined.
> 
> True.
> 
> > 
> >> +    maxItems: 2
> >> +
> >> +  interconnect-names:
> >> +    items:
> >> +      - const: qup-core
> >> +      - const: qup-config
> > 
> > Some properties like clocks, dmas, dma-names, interconnect etc. are
> > defined as common child properties of geni based qup.
> > Please see Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml
> > 
> > Shouldn't we skip these entities here? as spi reference will anyway be
> > used in geni-se.yaml.
> 
> We could have them there, just like we could store all of this schema
> there. Having something half-here-half-there will not work, because this
> schema won't validate.
> 
> Therefore all of child properties from qcom,geni-se.yaml should be
> rather moved to child schema (which is included directly in my patch #2).

Sure, this looks good.
Probably we can remove common properties from geni-se.yaml once all
child nodes have their respective schemas.

-Kuldeep