Re: [PATCH 2/2] arm64: dts: rockchip: add Tinkerboard 3 and 3S device tree

[Michael Opdenacker]

Hi Dragan,

Thanks a lot for your review and feedback!

On 11/14/25 03:26, Dragan Simic wrote:
> Hello Michael,
>
> Thanks for this patch!  Please, see some comments below.
>
> On Tuesday, November 11, 2025 18:20 CET, michael.opdenacker@xxxxxxxxxxxxxx wrote:
> > From: Michael Opdenacker <michael.opdenacker@xxxxxxxxxxxxxx>
> >
> > 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/.

Fixed in all the patches.

> 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.

Done too. However, I used these names for consistency with what was used 
on arm(32) for the original Tinker Board:
arch/arm/boot/dts/rockchip/rk3288-tinker.dts
arch/arm/boot/dts/rockchip/rk3288-tinker-s.dts
arch/arm/boot/dts/rockchip/rk3288-tinker.dtsi

I guess it's fine to ignore what arm did right? It won't live as long as 
arm64 (I attend Arnd's talk about arm 32).

> >   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@xxxxxxxxxxxxxx>
> > + */
> > +
> > +/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.


Done.

> > +	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.


No problem to do it. However, here we have a slightly bigger problem: it 
would be inconsistent with the bindings for the original Tinker Board in 
the same rockchip.yaml file:

      - description: Asus Tinker board
        items:
          - const: asus,rk3288-tinker
          - const: rockchip,rk3288

      - description: Asus Tinker board S
        items:
          - const: asus,rk3288-tinker-s
          - const: rockchip,rk3288

What do you think? The discrepancy would be quite visiible.

> > +	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"

Yes, whatever is decided for the compatible strings.
> Though, perhaps it would be better to not include the "asus,rk3566-
> tinker-board-3" part, because I think it's pretty much redundant.


My reasoning was that Tinker Board 3S is a superset of Tinker Board 3 
(additional eMMC and headers).
If someday some code is associated to the compatible string for Tinker 
3, than Tinker 3S should use it too, right? Unless we want to have the 
possibility to ignore some Tinker3 code in Tinker 3S for some reason. 
Then, it's better indeed that 3S doesn't use the Tinker 3 compatible 
string. It seems we have more options with what you're suggesting.

What do you think?

Thanks again,
Cheers
Michael.

--
Michael Opdenacker
Root Commit
Embedded Linux Training and Consulting
https://rootcommit.com