Re: [PATCH resend] rockchip: rk3328: Add support for FriendlyARM NanoPi NEO3

[Robin Murphy <robin.murphy@arm.com>]

On 2020-12-28 20:28, Marty Jones wrote:
>  From 7fc1fbf4d223ddf911f04adc756e41683cbd2948 Mon Sep 17 00:00:00 2001
> From: Marty Jones <mj8263788@gmail.com>
> Date: Mon, 28 Dec 2020 13:01:28 -0500
> Subject: [PATCH] rockchip: rk3328: add support for FriendlyARM NanoPi NEO3
> 
> This patch adds support for FriendlyARM NanoPi NEO3
> 
> Soc:      RockChip RK3328
> RAM:      1GB/2GB DDR4
> LAN:      10/100/1000M Ethernet with unique MAC
> USB Host: 1x USB3.0 Type A and 2x USB2.0 on 2.54mm pin header
> MicroSD:  x 1 for system boot and storage
> LED:      Power LED x 1, System LED x 1
> Key:      User Button x 1
> Fan:      2 Pin JST ZH 1.5mm Connector for 5V Fan
> GPIO:     26 pin-header, include I2C, UART, SPI, I2S, GPIO
> Power:    5V/1A, via Type-C or GPIO
> 
> Signed-off-by: Marty Jones <mj8263788@gmail.com>
> ---
>   arch/arm64/boot/dts/rockchip/Makefile         |  1 +
>   .../boot/dts/rockchip/rk3328-nanopi-neo3.dts  | 35 +++++++++++++++++++
>   2 files changed, 36 insertions(+)
>   create mode 100644 arch/arm64/boot/dts/rockchip/rk3328-nanopi-neo3.dts
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index 1ab55a124a87..e2d8dcac7592 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -9,6 +9,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3318-a95x-z2.dtb
>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3326-odroid-go2.dtb
>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-a1.dtb
>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-neo3.dtb
>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-nanopi-neo3.dts b/arch/arm64/boot/dts/rockchip/rk3328-nanopi-neo3.dts
> new file mode 100644
> index 000000000000..b62b169bdb2e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-nanopi-neo3.dts
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2020 Marty Jones <mj8263788@gmail.com>
> + */
> +
> +/dts-v1/;
> +#include "rk3328-nanopi-r2s.dts"
> +
> +/ {
> +	model = "FriendlyElec NanoPi NEO3";
> +	compatible = "friendlyarm,nanopi-neo3", "rockchip,rk3328";
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-0 = <&lan_led_pin>,  <&sys_led_pin>, <&wan_led_pin>;
> +		pinctrl-names = "default";

No need to restate the compatible and pinctrl-names properties that 
don't need overriding from the base include. The pinctrl should only 
claim the one entry it actually needs though - for NEO3 those other two 
pins are on the user expansion header, so we don't want to needlessly 
get in the way of users doing whatever they want with them.

> +		lan_led: led-0 {
> +			gpios = <&gpio2 RK_PB7 GPIO_ACTIVE_HIGH>;
> +			label = "nanopi-neo3:green:lan";
> +			status = "disabled";
> +		};

It doesn't make much sense to do all this redefinition for something 
that doesn't even exist - easier to just use the /delete-node/ operator 
instead.

> +		sys_led: led-1 {
> +			gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>;
> +			label = "nanopi-neo3:green:sys";

According to the schematic this should probably be named either "status" 
or "stat".

> +		};
> +
> +		wan_led: led-2 {
> +			gpios = <&gpio2 RK_PC2 GPIO_ACTIVE_HIGH>;
> +			label = "nanopi-neo3:green:wan";
> +			status = "disabled";
> +		};

As above, this one doesn't exist either. TBH it might make the most 
sense to convert the base DTS to be the NEO3 one and the R2S the 
differences on top of that, especially since the latter may have future 
USB-ethernet-related additions as well. Personally I reckon they're 
still fundamentally close enough to get away with defining one in terms 
of the other for now rather than splitting out a common DTSI, but that's 
always there as the most thorough and bullet-proof option (and 
downstream does hint at there being more variants to come...)

Thanks,
Robin.

> +	};
> +};
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip