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
next prev parent 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).