[PATCH 3/3] arm64: dts: mediatek: Add Acelink EW-7886CAX

[Rafał Miłecki]

On 20.11.2023 15:17, AngeloGioacchino Del Regno wrote:
> Il 17/11/23 11:43, Rafał Miłecki ha scritto:
>> From: Rafał Miłecki <rafal at milecki.pl>
>>
>> Acelink EW-7886CAX is an MT7986A (AKA Filogic 830) based access point.
>> It has 512 MiB of RAM, one 2.5 Gbps PoE (802.3at) Ethernet port and
>> on-SoC Wi-Fi.
>>
>> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
>> ---
>>   arch/arm64/boot/dts/mediatek/Makefile         |   1 +
>>   .../mediatek/mt7986a-acelink-ew-7886cax.dts   | 175 ++++++++++++++++++
>>   2 files changed, 176 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-acelink-ew-7886cax.dts
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
>> index e6e7592a3645..9ff2ab6c5e4d 100644
>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>> @@ -8,6 +8,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-evb.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-x20-dev.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-acelink-ew-7886cax.dts b/arch/arm64/boot/dts/mediatek/mt7986a-acelink-ew-7886cax.dts
>> new file mode 100644
>> index 000000000000..18d19281dfdb
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-acelink-ew-7886cax.dts
>> @@ -0,0 +1,175 @@
>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +
>> +/dts-v1/;
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/leds/common.h>
>> +
>> +#include "mt7986a.dtsi"
>> +
>> +/ {
>> +    model = "Acelink EW-7886CAX";
>> +    compatible = "acelink,ew-7886cax", "mediatek,mt7986a";
>> +
>> +    aliases {
>> +        serial0 = &uart0;
>> +    };
>> +
>> +    chosen {
>> +        stdout-path = "serial0:115200n8";
>> +    };
>> +
>> +    memory at 40000000 {
>> +        reg = <0 0x40000000 0 0x20000000>;
>> +        device_type = "memory";
>> +    };
>> +
>> +    keys {
>> +        compatible = "gpio-keys";
>> +
>> +        key-restart {
>> +            label = "Reset";
>> +            gpios = <&pio 7 GPIO_ACTIVE_LOW>;
>> +            linux,code = <KEY_RESTART>;
>> +        };
>> +    };
>> +
>> +    leds {
>> +        compatible = "gpio-leds";
>> +
>> +        led-0 {
> 
> Please, reorder by name
> 
>              color =    ...
>              function = ...
>              gpios = ...

Can you explain why and if there is a place I can find rules to follow
regarding such aspects? I really would like to just be aware of all
rules and don't waste anyone's time for such details.

FWIW I checked Documentation/devicetree/bindings/*.rst (after few years
I admit) but I couldn't find anything there about properties order.

If we currently don't have rules I don't really think we should enforce
following per-maintainer preferences. I really don't object your
suggestions but there is simply no way to remember each maintainer's
rules. We simply have too many subsystems and architectures boards.


>> +            function = LED_FUNCTION_STATUS;
>> +            color = <LED_COLOR_ID_RED>;
>> +            gpios = <&pio 18 GPIO_ACTIVE_HIGH>;
>> +        };
>> +
>> +        led-1 {
>> +            function = LED_FUNCTION_STATUS;
>> +            color = <LED_COLOR_ID_GREEN>;
>> +            gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
>> +        };
>> +
>> +        led-2 {
>> +            function = LED_FUNCTION_STATUS;
>> +            color = <LED_COLOR_ID_BLUE>;
>> +            gpios = <&pio 20 GPIO_ACTIVE_HIGH>;
>> +        };
>> +    };
>> +};
>> +
>> +&watchdog {
> 
> Ordering again, watchdog goes before wifi

Actually I ordered those in what I believe to be the most natural
numeric order. All those blocks references are ordered by the order
they appear in the included file and those follow numeric order I
believe.

I'd go as far as to claim using alhapbetical labels order is pretty
weak. Labels can be renamed and that would require reordering a lot of
.dts entries. On the other hand SoC's MMIO accessible hardware blocks
are quite unlikely to get reordered.


>> +    status = "okay";
>> +};
>> +
>> +&trng {
>> +    status = "okay";
>> +};
>> +
>> +&crypto {
> 
> crypto goes first.
> 
> crypto, eth, pcie_phy, spi0, trng, watchdog, wifi.
> 
>> +    status = "okay";
>> +};
>> +
>> +&uart0 {
>> +    status = "okay";
>> +};
>> +
>> +&spi0 {
>> +    status = "okay";
>> +
>> +    flash at 0 {
>> +        compatible = "spi-nand";
>> +        reg = <0>;
> 
> compatible
> reg
> #address/size cells
> spi-max-frequency
> spi-rx-bus-width
> spi-tx-bus-width
> 
>> +        spi-max-frequency = <52000000>;
>> +        spi-tx-bus-width = <4>;
>> +        spi-rx-bus-width = <4>;
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        partitions: partitions {
>> +            compatible = "fixed-partitions";
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +
>> +            partition at 0 {
>> +                label = "bootloader";
>> +                reg = <0x0 0x100000>;
> 
> label, reg, read-only
> 
>> +                read-only;
>> +            };
>> +
>> +            partition at 100000 {
>> +                label = "u-boot-env";
> 
> reg first, label last
> 
>> +                reg = <0x100000 0x80000>;
>> +            };
>> +
>> +            factory: partition at 180000 {
> 
> Why do you have a phandle here? This has no use, please remove.
> 
>> +                label = "factory";
>> +                reg = <0x180000 0x200000>;
>> +                read-only;
>> +                compatible = "nvmem-cells";
> 
> compatible
> reg
> label
> read-only;
> 
>> +
>> +                nvmem-layout {
>> +                    compatible = "fixed-layout";
>> +                    #address-cells = <1>;
>> +                    #size-cells = <1>;
>> +
>> +                    eeprom: eeprom at 0 {
>> +                        reg = <0x0 0x1000>;
>> +                    };
>> +
>> +                    macaddr: macaddr at 4 {
>> +                        reg = <0x4 0x6>;
>> +                    };
>> +                };
>> +            };
>> +
>> +            partition at 380000 {
>> +                label = "fip";
>> +                reg = <0x380000 0x200000>;
> 
> reg first, label last
> 
>> +            };
>> +
>> +            partition at 580000 {
>> +                label = "ubi";
>> +                reg = <0x580000 0x4000000>;
>> +            };
>> +        };
>> +    };
>> +};
>> +
> 
> Regards,
> Angelo