| Message ID | 20240430-loongson1-nand-v7-1-60787c314fa4@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Loongson-1 NAND | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hello, devnull+keguang.zhang.gmail.com@kernel.org wrote on Tue, 30 Apr 2024 19:11:10 +0800: > From: Keguang Zhang <keguang.zhang@gmail.com> > > Add devicetree binding document for Loongson-1 NAND Controller. > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > --- > Changes in v7: > - rename the file to loongson,ls1b-nfc.yaml > > Changes in v6: > - A newly added patch > --- > .../devicetree/bindings/mtd/loongson,ls1b-nfc.yaml | 66 ++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml > new file mode 100644 > index 000000000000..a69f22b9fd9e > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml > @@ -0,0 +1,66 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mtd/loongson,ls1b-nfc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Loongson-1 NAND Controller > + > +maintainers: > + - Keguang Zhang <keguang.zhang@gmail.com> > + > +allOf: > + - $ref: nand-controller.yaml > + > +properties: > + compatible: > + oneOf: > + - const: loongson,ls1b-nfc What is the rationale behind this choice? Seems like the b variant has two possible implementations and should always be preceded by a more specific compatible. As there is currently no description of this controller upstream, I would not care too much about any out-of-tree description and directly go for a clean description. > + - items: > + - enum: > + - loongson,ls1a-nfc > + - loongson,ls1c-nfc > + - const: loongson,ls1b-nfc > + > + reg: > + maxItems: 1 > + > + dmas: > + maxItems: 1 > + > + dma-names: > + const: rxtx > + > +patternProperties: > + "^nand@[0-3]$": > + type: object > + $ref: raw-nand-chip.yaml > + > + unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - dmas > + - dma-names Should DMA props be required? > + > +unevaluatedProperties: false > + > +examples: > + - | > + nand-controller@1fe78000 { > + compatible = "loongson,ls1b-nfc"; > + reg = <0x1fe78000 0x40>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + dmas = <&dma 0>; > + dma-names = "rxtx"; There is a preferred spacing for DT nodes, see: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html > + > + nand@0 { > + reg = <0>; > + nand-use-soft-ecc-engine; > + nand-ecc-algo = "hamming"; These two properties are not needed. Unless there is no hardware ECC capability on this controller and in this case you need to ensure the properties are present in the schema. > + }; > + }; > Thanks, Miquèl
On Mon, May 6, 2024 at 3:14 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > devnull+keguang.zhang.gmail.com@kernel.org wrote on Tue, 30 Apr 2024 > 19:11:10 +0800: > > > From: Keguang Zhang <keguang.zhang@gmail.com> > > > > Add devicetree binding document for Loongson-1 NAND Controller. > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > --- > > Changes in v7: > > - rename the file to loongson,ls1b-nfc.yaml > > > > Changes in v6: > > - A newly added patch > > --- > > .../devicetree/bindings/mtd/loongson,ls1b-nfc.yaml | 66 ++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml > > new file mode 100644 > > index 000000000000..a69f22b9fd9e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml > > @@ -0,0 +1,66 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mtd/loongson,ls1b-nfc.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Loongson-1 NAND Controller > > + > > +maintainers: > > + - Keguang Zhang <keguang.zhang@gmail.com> > > + > > +allOf: > > + - $ref: nand-controller.yaml > > + > > +properties: > > + compatible: > > + oneOf: > > + - const: loongson,ls1b-nfc > > What is the rationale behind this choice? Seems like the b variant has > two possible implementations and should always be preceded by a more > specific compatible. > > As there is currently no description of this controller upstream, I > would not care too much about any out-of-tree description and directly > go for a clean description. > Excuse me, should I add a description for this property? > > + - items: > > + - enum: > > + - loongson,ls1a-nfc > > + - loongson,ls1c-nfc > > + - const: loongson,ls1b-nfc > > + > > + reg: > > + maxItems: 1 > > + > > + dmas: > > + maxItems: 1 > > + > > + dma-names: > > + const: rxtx > > + > > +patternProperties: > > + "^nand@[0-3]$": > > + type: object > > + $ref: raw-nand-chip.yaml > > + > > + unevaluatedProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - dmas > > + - dma-names > > Should DMA props be required? > Yes. This NAND controller only works with DMA, which means the DMA is necessary. > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + nand-controller@1fe78000 { > > + compatible = "loongson,ls1b-nfc"; > > + reg = <0x1fe78000 0x40>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + dmas = <&dma 0>; > > + dma-names = "rxtx"; > > There is a preferred spacing for DT nodes, see: > https://docs.kernel.org/devicetree/bindings/dts-coding-style.html > Sorry. I don't get the meaning of preferred spacing. https://docs.kernel.org/devicetree/bindings/writing-schema.html says "For DTS examples in the schema, preferred is four-space indentation." Then I used four-space indentation. > > + > > + nand@0 { > > + reg = <0>; > > + nand-use-soft-ecc-engine; > > + nand-ecc-algo = "hamming"; > > These two properties are not needed. Unless there is no hardware ECC > capability on this controller and in this case you need to ensure the > properties are present in the schema. Exactly. This NAND controller doesn't support hardware ECC. 'nand-use-soft-ecc-engine' and 'nand-ecc-algo' are present in nand-chip.yaml. Is there anything else I should do? Thanks for your view! > > > + }; > > + }; > > > > > Thanks, > Miquèl
Hi, keguang.zhang@gmail.com wrote on Sat, 18 May 2024 16:01:01 +0800: > On Mon, May 6, 2024 at 3:14 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hello, > > > > devnull+keguang.zhang.gmail.com@kernel.org wrote on Tue, 30 Apr 2024 > > 19:11:10 +0800: > > > > > From: Keguang Zhang <keguang.zhang@gmail.com> > > > > > > Add devicetree binding document for Loongson-1 NAND Controller. > > > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > > --- > > > Changes in v7: > > > - rename the file to loongson,ls1b-nfc.yaml > > > > > > Changes in v6: > > > - A newly added patch > > > --- > > > .../devicetree/bindings/mtd/loongson,ls1b-nfc.yaml | 66 ++++++++++++++++++++++ > > > 1 file changed, 66 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml > > > new file mode 100644 > > > index 000000000000..a69f22b9fd9e > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml > > > @@ -0,0 +1,66 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mtd/loongson,ls1b-nfc.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Loongson-1 NAND Controller > > > + > > > +maintainers: > > > + - Keguang Zhang <keguang.zhang@gmail.com> > > > + > > > +allOf: > > > + - $ref: nand-controller.yaml > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - const: loongson,ls1b-nfc > > > > What is the rationale behind this choice? Seems like the b variant has > > two possible implementations and should always be preceded by a more > > specific compatible. > > > > As there is currently no description of this controller upstream, I > > would not care too much about any out-of-tree description and directly > > go for a clean description. > > > Excuse me, should I add a description for this property? No, description is not needed. But you are allowing the "loongson,ls1b-nfc" compatible alone, which I think is not relevant, unless you convince me it is :-) > > > + - items: > > > + - enum: > > > + - loongson,ls1a-nfc > > > + - loongson,ls1c-nfc > > > + - const: loongson,ls1b-nfc > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + dmas: > > > + maxItems: 1 > > > + > > > + dma-names: > > > + const: rxtx > > > + > > > +patternProperties: > > > + "^nand@[0-3]$": > > > + type: object > > > + $ref: raw-nand-chip.yaml > > > + > > > + unevaluatedProperties: false > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - dmas > > > + - dma-names > > > > Should DMA props be required? > > > Yes. This NAND controller only works with DMA, which means the DMA is necessary. Ok > > > > + > > > +unevaluatedProperties: false > > > + > > > +examples: > > > + - | > > > + nand-controller@1fe78000 { > > > + compatible = "loongson,ls1b-nfc"; > > > + reg = <0x1fe78000 0x40>; > > > + > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + dmas = <&dma 0>; > > > + dma-names = "rxtx"; > > > > There is a preferred spacing for DT nodes, see: > > https://docs.kernel.org/devicetree/bindings/dts-coding-style.html > > > Sorry. I don't get the meaning of preferred spacing. > https://docs.kernel.org/devicetree/bindings/writing-schema.html says > "For DTS examples in the schema, preferred is four-space indentation." > Then I used four-space indentation. I'm talking about the new lines (\n) between the properties. > > > > + > > > + nand@0 { > > > + reg = <0>; > > > + nand-use-soft-ecc-engine; > > > + nand-ecc-algo = "hamming"; > > > > These two properties are not needed. Unless there is no hardware ECC > > capability on this controller and in this case you need to ensure the > > properties are present in the schema. > > Exactly. This NAND controller doesn't support hardware ECC. > 'nand-use-soft-ecc-engine' and 'nand-ecc-algo' are present in nand-chip.yaml. > Is there anything else I should do? Thry are in nand-chip.yaml, which means they are allowed, but I want them mandatory: required: - foo - bar Thanks, Miquèl
diff --git a/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml new file mode 100644 index 000000000000..a69f22b9fd9e --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/loongson,ls1b-nfc.yaml @@ -0,0 +1,66 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mtd/loongson,ls1b-nfc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Loongson-1 NAND Controller + +maintainers: + - Keguang Zhang <keguang.zhang@gmail.com> + +allOf: + - $ref: nand-controller.yaml + +properties: + compatible: + oneOf: + - const: loongson,ls1b-nfc + - items: + - enum: + - loongson,ls1a-nfc + - loongson,ls1c-nfc + - const: loongson,ls1b-nfc + + reg: + maxItems: 1 + + dmas: + maxItems: 1 + + dma-names: + const: rxtx + +patternProperties: + "^nand@[0-3]$": + type: object + $ref: raw-nand-chip.yaml + + unevaluatedProperties: false + +required: + - compatible + - reg + - dmas + - dma-names + +unevaluatedProperties: false + +examples: + - | + nand-controller@1fe78000 { + compatible = "loongson,ls1b-nfc"; + reg = <0x1fe78000 0x40>; + + #address-cells = <1>; + #size-cells = <0>; + + dmas = <&dma 0>; + dma-names = "rxtx"; + + nand@0 { + reg = <0>; + nand-use-soft-ecc-engine; + nand-ecc-algo = "hamming"; + }; + };