Messages in this thread | | | Date | Wed, 29 Jun 2022 11:52:07 +0200 | Subject | Re: [PATCH v3 01/12] dt-bindings: wiiu: Document the Nintendo Wii U devicetree | From | Krzysztof Kozlowski <> |
| |
On 28/06/2022 15:31, Ash Logan wrote: > Adds schema for the various Wii U devicetree nodes used. > > Signed-off-by: Ash Logan <ash@heyquark.com> > --- > .../bindings/powerpc/nintendo/wiiu.yaml | 28 +++++++++++ > .../powerpc/nintendo/wiiu/espresso-pic.yaml | 42 +++++++++++++++++ > .../bindings/powerpc/nintendo/wiiu/gpu7.yaml | 41 +++++++++++++++++ > .../powerpc/nintendo/wiiu/latte-ahci.yaml | 43 +++++++++++++++++ > .../powerpc/nintendo/wiiu/latte-dsp.yaml | 35 ++++++++++++++ > .../powerpc/nintendo/wiiu/latte-pic.yaml | 46 +++++++++++++++++++ > .../powerpc/nintendo/wiiu/latte-sdhci.yaml | 40 ++++++++++++++++ > .../bindings/powerpc/nintendo/wiiu/latte.yaml | 25 ++++++++++ > 8 files changed, 300 insertions(+) > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/espresso-pic.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/gpu7.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-ahci.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-dsp.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-pic.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-sdhci.yaml > create mode 100644 Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte.yaml > > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu.yaml > new file mode 100644 > index 000000000000..5824b07928f5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu.yaml > @@ -0,0 +1,28 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > + > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U bindings > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + Nintendo Wii U video game console binding. > + > +properties: > + $nodename: > + const: "/" > + compatible: > + oneOf: > + - description: Nintendo Wii U video game console > + items: > + - const: nintendo,wiiu > + > +additionalProperties: true > + > +... > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/espresso-pic.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/espresso-pic.yaml > new file mode 100644 > index 000000000000..878a81595f5f > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/espresso-pic.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu/espresso-pic.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U "Espresso" interrupt controller > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + Interrupt controller found on the Nintendo Wii U for the "Espresso" processor. > + > +properties: > + compatible: > + oneOf:
No need for oneOf, just const.
> + - description: Nintendo Wii U "Espresso" interrupt controller
No need for description.
> + items: > + - const: nintendo,espresso-pic > + '#interrupt-cells': > + # Interrupt numbers 0-32 in one cell > + const: 1
Blank line between each property.
> + interrupt-controller: true > + reg: > + items: > + - description: Core registers
Just maxItems:1. Description is not helpful.
Missing required properties.
> + > +additionalProperties: false > + > +examples: > + - | > + espresso_pic: pic@c000078 {
Generic node name, so "interrupt-controller"
> + #interrupt-cells = <1>; > + interrupt-controller; > + > + compatible = "nintendo,espresso-pic"; > + reg = <0x0c000078 0x18>;
compatible and reg by convention go first in list of properties. This applies everywhere, also to your DTS.
> + }; > + > +... > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/gpu7.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/gpu7.yaml > new file mode 100644 > index 000000000000..e54d49015f36 > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/gpu7.yaml > @@ -0,0 +1,41 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu/gpu7.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U Latte "GPU7" graphics processor > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + GPU7 graphics processor, also known as "GX2", found in the Latte multifunction chip of the > + Nintendo Wii U. > + > +properties: > + compatible: > + oneOf: > + - description: Nintendo Wii U Latte "GPU7" graphics processor > + items: > + - const: nintendo,latte-gpu7 > + reg: > + items: > + - description: GpuF0MMReg registers > + interrupts: > + items: > + - description: Main interrupt, connected via Espresso PIC
Same comments as your interrupt controller patch. I guess these comments apply everywhere here.
Missing required properties.
> + > +# This will likely get bound to the Radeon driver one day, which will neccesitate extra properties > +additionalProperties: true
No, it must be false. It's okay to extend the binding later.
> + > +examples: > + - | > + gpu7@c200000 {
Generic node name, so "gpu".
> + compatible = "nintendo,latte-gpu7";
Wrong indentation. Use four spaces for DTS example.
> + reg = <0x0c200000 0x80000>; > + interrupts = <2>; > + interrupt-parent = <&espresso_pic>; > + }; > +... > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-ahci.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-ahci.yaml > new file mode 100644 > index 000000000000..7b9de4effcb0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-ahci.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu/latte-ahci.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U Latte AHCI controller > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + Nintendo Wii U AHCI SATA controller, as found in the Latte chip. > + > +properties: > + compatible: > + oneOf: > + - description: Nintendo Wii U Latte AHCI controller > + items: > + - const: nintendo,latte-ahci > + reg: > + items: > + - description: | > + HBA memory registers. Note that unlike the spec, space for only 6 ports exist, with 2 vendor > + registers afterwards, thus register space should be 0x408 long (0x100+0x80*6+0x8).
Here description can stay, so no need for maxItems.
> + interrupts: > + items: > + - description: Main HBA interrupt > + - description: Vendor debugging interrupt > +
This should also reference ahci schema (I think it is ready).
> +additionalProperties: false > + > +examples: > + - | > + sata: ahci@d160400 { > + compatible = "nintendo,latte-ahci"; > + reg = <0x0d160400 0x408>;
Fix indentation.
> + > + interrupt-parent = <&latte_pic>; > + interrupts = <38>, <28>; > + }; > +... > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-dsp.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-dsp.yaml > new file mode 100644 > index 000000000000..ed0b4c28ce48 > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-dsp.yaml > @@ -0,0 +1,35 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu/latte-dsp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U Latte DSP > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + Nintendo Wii U digital signal processor, as found in the Latte chip. > + > +properties: > + compatible: > + oneOf: > + - description: Nintendo Wii U Latte DSP > + items: > + - const: nintendo,latte-dsp > + reg: > + items: > + - description: DSP registers > + > +# Once more is known about this device more properties will likely be added > +additionalProperties: true
Again, no. All other comments also apply.
> + > +examples: > + - | > + latte_dsp: dsp@c005000 { > + compatible = "nintendo,latte-dsp"; > + reg = <0x0c005000 0x200>; > + }; > +... > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-pic.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-pic.yaml > new file mode 100644 > index 000000000000..a2df71fb9964 > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-pic.yaml > @@ -0,0 +1,46 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu/latte-pic.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U Latte interrupt controller > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + Interrupt controller found on the Nintendo Wii U for the "Latte" devices. > + > +properties: > + compatible: > + oneOf: > + - description: Nintendo Wii U Latte interrupt controller > + items: > + - const: nintendo,latte-pic > + '#interrupt-cells': > + # Interrupt numbers 0-64 in one cell. > + const: 1 > + interrupt-controller: true > + reg: > + items: > + - description: Core registers > + interrupts: > + items: > + - description: Cascade interrupt for Espresso PIC > + > +additionalProperties: false > + > +examples: > + - | > + latte_pic: pic@d800440 {
interrupt-controller.
All other comments apply.
> + #interrupt-cells = <1>; > + interrupt-controller; > + > + compatible = "nintendo,latte-pic"; > + reg = <0x0d800440 0x30>; > + interrupt-parent = <&espresso_pic>; > + interrupts = <24>; > + }; > +... > diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-sdhci.yaml b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-sdhci.yaml > new file mode 100644 > index 000000000000..4b9b0820edc1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/powerpc/nintendo/wiiu/latte-sdhci.yaml > @@ -0,0 +1,40 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/powerpc/nintendo/wiiu/latte-sdhci.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nintendo Wii U Latte SD Host controller > + > +maintainers: > + - Ash Logan <ash@heyquark.com> > + - Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > + > +description: | > + SDHCI hosts found on the Nintendo Wii U's Latte SoC for SD cards and SDIO devices. > + > +properties: > + compatible: > + oneOf: > + - description: Nintendo Wii U Latte SDHCI > + items: > + - const: nintendo,latte-sdhci > + - const: sdhci > + reg: > + items: > + - description: Core registers > + interrupts: > + items: > + - description: SDHCI interrupt
You need to reference mmc-controller bindings in allOf.
> + > +additionalProperties: false > + > +examples: > + - | > + sdcard_0: sdhci@d070000 {
Node name: mmc
The bindings look poor, several easy to fix mistakes. I suggest to open example-schema.yaml and start from there.
Best regards, Krzysztof
| |