devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Rafał Miłecki" <zajec5@gmail.com>
To: AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>
Cc: "Nícolas F . R . A . Prado" <nfraprado@collabora.com>,
	"Macpaul Lin" <macpaul.lin@mediatek.com>,
	"Bernhard Rosenkränzer" <bero@baylibre.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Jernej Skrabec" <jernej.skrabec@gmail.com>,
	"Chris Morgan" <macromorgan@hotmail.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Sean Wang" <sean.wang@mediatek.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	"Rafał Miłecki" <rafal@milecki.pl>
Subject: Re: [PATCH 3/3] arm64: dts: mediatek: Add Acelink EW-7886CAX
Date: Mon, 20 Nov 2023 18:27:21 +0100	[thread overview]
Message-ID: <b13b6ad3-2ef9-482a-bec4-8f23de6c951f@gmail.com> (raw)
In-Reply-To: <0c3267e5-5371-4fd8-a0f6-360ff28c9dda@collabora.com>

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@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@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@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@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@0 {
>> +                label = "bootloader";
>> +                reg = <0x0 0x100000>;
> 
> label, reg, read-only
> 
>> +                read-only;
>> +            };
>> +
>> +            partition@100000 {
>> +                label = "u-boot-env";
> 
> reg first, label last
> 
>> +                reg = <0x100000 0x80000>;
>> +            };
>> +
>> +            factory: partition@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@0 {
>> +                        reg = <0x0 0x1000>;
>> +                    };
>> +
>> +                    macaddr: macaddr@4 {
>> +                        reg = <0x4 0x6>;
>> +                    };
>> +                };
>> +            };
>> +
>> +            partition@380000 {
>> +                label = "fip";
>> +                reg = <0x380000 0x200000>;
> 
> reg first, label last
> 
>> +            };
>> +
>> +            partition@580000 {
>> +                label = "ubi";
>> +                reg = <0x580000 0x4000000>;
>> +            };
>> +        };
>> +    };
>> +};
>> +
> 
> Regards,
> Angelo


  reply	other threads:[~2023-11-20 17:27 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-17 10:43 [PATCH 2/3] dt-bindings: arm64: dts: mediatek: Add Acelink EW-7886CAX access point Rafał Miłecki
2023-11-17 10:43 ` [PATCH 3/3] arm64: dts: mediatek: Add Acelink EW-7886CAX Rafał Miłecki
2023-11-20 14:17   ` AngeloGioacchino Del Regno
2023-11-20 17:27     ` Rafał Miłecki [this message]
2023-11-21 10:31       ` AngeloGioacchino Del Regno
2023-11-21 11:15         ` Rafał Miłecki
2023-11-19 13:43 ` [PATCH 2/3] dt-bindings: arm64: dts: mediatek: Add Acelink EW-7886CAX access point Conor Dooley
2023-11-24  9:49 ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b13b6ad3-2ef9-482a-bec4-8f23de6c951f@gmail.com \
    --to=zajec5@gmail.com \
    --cc=angelogioacchino.delregno@collabora.com \
    --cc=bero@baylibre.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=heiko@sntech.de \
    --cc=jernej.skrabec@gmail.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=macpaul.lin@mediatek.com \
    --cc=macromorgan@hotmail.com \
    --cc=matthias.bgg@gmail.com \
    --cc=nfraprado@collabora.com \
    --cc=rafal@milecki.pl \
    --cc=robh+dt@kernel.org \
    --cc=sean.wang@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).