Message-ID: <dbacc018-2631-6606-7562-27371cf45d6f@manjaro.org>
Date: Fri, 14 Nov 2025 03:26:47 +0100
From: "Dragan Simic" <dsimic@...jaro.org>
To: michael.opdenacker@...tcommit.com
Cc: "Rob Herring" <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>, "Heiko Stuebner" <heiko@...ech.de>, devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: add Tinkerboard 3 and 3S device tree

Hello Michael,

Thanks for this patch!  Please, see some comments below.

On Tuesday, November 11, 2025 18:20 CET, michael.opdenacker@...tcommit.com wrote:
> From: Michael Opdenacker <michael.opdenacker@...tcommit.com>
> 
> Add initial device tree support for Asus Tinkerboard 3 [1] and 3S [2],
> which are SBCs based on the Rockchip 3566 SoC.

For consistency and because it's a proper noun, this should be
s/Tinkerboard/Tinker Board/.

> The "3S" version ("S" for "storage") just adds a 16 GB eMMC
> and a "mask ROM" DIP switch (to mask the eMMC and enter "Mask ROM"
> mode for recovery) to the "3" version.
> 
> This adds support for:
> - Debug UART (/dev/ttyS2)
> - SD card (/dev/mmcblk1)
> - eMMC (/dev/mmcblk0, only on Tinkerboard 3S)
> - I2C:
>   - i2c0 (internal bus with a PMIC and regulators)
>   - i2c2 (internal bus with an at24 eeprom and an RTC device)
> - USB 2.0 ports
> - 2 GPIO LEDS
> 
> Link: https://tinker-board.asus.com/series/tinker-board-3.html [1]
> Link: https://tinker-board.asus.com/series/tinker-board-3s.html [2]

These two lines should read like this, to serve as references, with
an empty line afterwards, of course:

  [1] https://tinker-board.asus.com/series/tinker-board-3.html
  [2] https://tinker-board.asus.com/series/tinker-board-3s.html

> Signed-off-by: Michael Opdenacker <michael.opdenacker@...tcommit.com>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile         |   2 +
>  .../boot/dts/rockchip/rk3566-tinker-3.dts     |  14 +
>  .../boot/dts/rockchip/rk3566-tinker-3.dtsi    | 264 ++++++++++++++++++
>  .../boot/dts/rockchip/rk3566-tinker-3s.dts    |  30 ++
>  4 files changed, 310 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dts
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dtsi
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-tinker-3s.dts
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index ad684e3831bc..381831cab20c 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -130,6 +130,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-lubancat-1.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-nanopi-r3s.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-bigtreetech-cb2-manta.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-bigtreetech-pi2.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-tinker-3.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-tinker-3s.dtb

The board .dts/.dtb files should include "-board", i.e. these should
be "rk3566-tinker-board-3.dtb" and "rk3566-tinker-board-3s.dtb"
instead, because there's no real need for shortening.  These boards
are simply named "Tinker Board", which should be preserved.

>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-bpi-r2-pro.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-evb1-v10.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-fastrhino-r66s.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dts b/arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dts
> new file mode 100644
> index 000000000000..938af35b9004
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dts
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2025 Michael Opdenacker <michael.opdenacker@...tcommit.com>
> + */
> +
> +/dts-v1/;
> +
> +#include "rk3566-tinker-3.dtsi"

The same comment from above about the naming applies here (as well
as in other places), so we should have "rk3566-tinker-board-3.dtsi"
here instead.

> +
> +/ {
> +	model = "Rockchip RK3566 Asus Tinker Board 3";

For consistency and to avoid redundancy, the "Rockchip RK3566"
part should be removed.

> +	compatible = "asus,rk3566-tinker-3", "rockchip,rk3566";

Actually, the compatible should be "asus,rk3566-tinker-board-3"
instead, because there's no real need for shortening it.

> +};
> +
> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dtsi
> new file mode 100644
> index 000000000000..45269d33b0cb
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3566-tinker-3.dtsi

This .dtsi file should be named "rk3566-tinker-board-3.dtsi" instead,
because there's no need for shortening.  Please see also a comment
above, regarding the naming.

[snip]

> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-tinker-3s.dts b/arch/arm64/boot/dts/rockchip/rk3566-tinker-3s.dts
> new file mode 100644
> index 000000000000..ba7efd702050
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3566-tinker-3s.dts

This .dts file should be named "rk3566-tinker-board-3s.dts" instead,
because there's no need for shortening.

> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2025 Michael Opdenacker <michael.opdenacker@...tcommit.com>
> + */
> +
> +/dts-v1/;
> +
> +#include "rk3566-tinker-3.dtsi"
> +
> +/ {
> +	model = "Rockchip RK3566 Asus Tinker Board 3S";

For consistency and to avoid redundancy, the "Rockchip RK3566"
part should be removed.

> +	compatible = "asus,rk3566-tinker-3s", "asus,rk3566-tinker-3", "rockchip,rk3566";

The compatibles should be as below instead, for the same reasons
as already explained above.

  "asus,rk3566-tinker-board-3s", "asus,rk3566-tinker-board-3", "rockchip,rk3566"

Though, perhaps it would be better to not include the "asus,rk3566-
tinker-board-3" part, because I think it's pretty much redundant.

[snip]

Powered by blists - more mailing lists