Messages in this thread | | | Subject | Re: [PATCH 2/3] dt-bindings: SPI: Add Ingenic SFC bindings. | From | Zhou Yanjie <> | Date | Sun, 24 Jul 2022 00:50:40 +0800 |
| |
Hi Krzysztof,
On 2022/7/23 上午1:46, Krzysztof Kozlowski wrote: > On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote: >> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC, >> and the X2000 SoC from Ingenic. >> >> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> >> --- >> .../devicetree/bindings/spi/ingenic,sfc.yaml | 64 ++++++++++++++++++++++ >> 1 file changed, 64 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml >> >> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml >> new file mode 100644 >> index 00000000..b7c4cf4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml >> @@ -0,0 +1,64 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml# > File name should be rather based on first compatible, so > ingenic,x1000-sfc.yaml
No offense, does it really need to be named that way? I can't seem to find documentation with instructions on this :(
The use of "ingenic,sfc.yaml" indicates that this is the documentation for the SFC module for all Ingenic SoCs, without misleading people into thinking it's only for a specific model of SoC. And there seem to be many other yaml documents that use similar names (eg. fsl,spi-fsl-qspi.yaml, spi-rockchip.yaml, spi-nxp-fspi.yaml, ingenic,spi.yaml, spi-sifive.yaml, omap-spi.yaml), maybe these yaml files that are not named with first compatible are also for the same consideration. :)
>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Bindings for SFC in Ingenic SoCs >> + >> +maintainers: >> + - 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> >> + >> +description: >> + The SPI Flash Controller in Ingenic SoCs. >> + > You miss here allOf referencing spi-controller.
Sure, will add it in the next version.
>> +properties: >> + compatible: >> + oneOf: >> + - enum: >> + - ingenic,x1000-sfc >> + - ingenic,x1600-sfc >> + - ingenic,x2000-sfc >> + - items: >> + - enum: >> + - ingenic,x1830-sfc >> + - const: ingenic,x1000-sfc >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + clock-names: >> + const: sfc > Remove the clock-names entirely, no benefits.
Sure.
>> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - clock-names >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/ingenic,x1000-cgu.h> >> + >> + sfc: spi@13440000 { >> + compatible = "ingenic,x1000-sfc"; > > Use 4 spaces for example indentation.
My fault, will fix it in the next version.
>> + reg = <0x13440000 0x1000>; >> + >> + interrupt-parent = <&intc>; >> + interrupts = <7>; >> + >> + clocks = <&cgu X1000_CLK_SFC>; >> + clock-names = "sfc"; >> + >> + status = "disabled"; > No status in example.
Sure, will remove it in the next version.
Thanks and beset regards!
> >> + }; >> +... > > Best regards, > Krzysztof
| |