| Message ID | 20240215134856.1313239-2-quic_mdalam@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add QPIC SPI NAND driver | expand |
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 >
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 +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>; + }; + };