[PATCH] arm64: dts: rockchip: r4s: add i2c2 read eeprom for mac addr and fix voltage layout.
Chen-Yu Tsai
wens at kernel.org
Sun Jul 4 20:23:42 PDT 2021
On Mon, Jul 5, 2021 at 9:05 AM xiaobo <peterwillcn at gmail.com> wrote:
>
> 1. add i2c2 read eeprom for mac addr
> 2. fix voltage layout.
Why? The included .dtsi file already defines the regulators you
changed and hooks them up properly. You are simply copying them
over verbatim.
Also, You are changing a whole lot of things in one patch, some
of which you aren't even listing in the commit log.
Rule of thumb: one logical change per patch.
>
> Reviewed-by: Kever Yang <kever.yang at rock-chips.com>
> Signed-off-by: xiaobo <peterwillcn at gmail.com>
Please use your full name in a proper format, such as listed
on your passport.
> ---
> arch/arm64/boot/dts/rockchip/Makefile | 2 +-
> .../boot/dts/rockchip/rk3399-nanopi-r4s.dts | 138 ++++++++++--------
> 2 files changed, 78 insertions(+), 62 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index c3e00c0e2db7..6973141cb7a3 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -9,7 +9,6 @@ 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-r2s.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock-pi-e.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
> @@ -36,6 +35,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-m4b.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-nanopi-r2s.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> index fa5809887643..4dc6a80e8494 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi-r4s.dts
> @@ -1,15 +1,13 @@
> // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> /*
> - * FriendlyElec NanoPC-T4 board device tree source
> + * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
> *
> - * Copyright (c) 2020 FriendlyElec Computer Tech. Co., Ltd.
> + * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
> * (http://www.friendlyarm.com)
> *
> * Copyright (c) 2018 Collabora Ltd.
> - *
> - * Copyright (c) 2020 Jensen Huang <jensenhuang at friendlyarm.com>
> - * Copyright (c) 2020 Marty Jones <mj8263788 at gmail.com>
> - * Copyright (c) 2021 Tianling Shen <cnsztl at gmail.com>
> + * Copyright (c) 2019 Arm Ltd.
> + * Copyright (C) 2020 Xiaobo <peterwillcn at gmail.com>
You don't get to remove other people's copyright.
> */
>
> /dts-v1/;
> @@ -19,48 +17,59 @@ / {
> model = "FriendlyElec NanoPi R4S";
> compatible = "friendlyarm,nanopi-r4s", "rockchip,rk3399";
>
> - /delete-node/ display-subsystem;
> -
> - gpio-leds {
> - pinctrl-0 = <&lan_led_pin>, <&sys_led_pin>, <&wan_led_pin>;
> -
> - /delete-node/ led-0;
> -
> - lan_led: led-lan {
> - gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> - label = "green:lan";
> - };
> + chosen {
> + stdout-path = "serial2:1500000n8";
> + };
This is already defined in the underlying nanopi4.dtsi file.
>
> - sys_led: led-sys {
> - gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> - label = "red:sys";
> - default-state = "on";
> - };
> + /delete-node/ display-subsystem;
>
> - wan_led: led-wan {
> - gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> - label = "green:wan";
> - };
> + vcc1v8_s3: vcc1v8-s3 {
> + compatible = "regulator-fixed";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-name = "vcc1v8_s3";
> + vin-supply = <&vcc_1v8>;
As mentioned above, this is just copying stuff from the nanopi4.dtsi file
for no reason.
> };
>
> - gpio-keys {
> - pinctrl-0 = <&reset_button_pin>;
> + vcc3v0_sd: vcc3v0-sd {
> + compatible = "regulator-fixed";
> + enable-active-high;
> + gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdmmc0_pwr_h>;
> + regulator-always-on;
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-name = "vcc3v0_sd";
> + vin-supply = <&vcc3v3_sys>;
> + };
Same here.
>
> - /delete-node/ power;
> + vcc3v3_sys: vcc3v3-sys {
> + compatible = "regulator-fixed";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-name = "vcc3v3_sys";
> + };
And here.
>
> - reset {
> - debounce-interval = <50>;
> - gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
> - label = "reset";
> - linux,code = <KEY_RESTART>;
> - };
> + vcc5v0_sys: vcc5v0-sys {
> + compatible = "regulator-fixed";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + regulator-name = "vcc5v0_sys";
> + vin-supply = <&vdd_5v>;
And here.
> };
>
> vdd_5v: vdd-5v {
> compatible = "regulator-fixed";
> - regulator-name = "vdd_5v";
> regulator-always-on;
> regulator-boot-on;
> + regulator-name = "vdd_5v";
We _want_ the name to come first.
> };
> };
>
> @@ -68,40 +77,51 @@ &emmc_phy {
> status = "disabled";
> };
>
> +&fusb0 {
> + status = "disabled";
> +};
> +
> +&i2c2 {
> + eeprom at 51 {
> + compatible = "microchip,24c02", "atmel,24c02";
> + reg = <0x51>;
> + pagesize = <16>;
> + read-only;
> + };
> +};
> +
This is already covered by
https://lore.kernel.org/linux-rockchip/20210622034505.18824-1-cnsztl@gmail.com/
> &i2c4 {
> status = "disabled";
> };
>
> -&pcie0 {
> - max-link-speed = <1>;
> - num-lanes = <1>;
> - vpcie3v3-supply = <&vcc3v3_sys>;
> +&leds {
> + lan_led: led-1 {
> + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_HIGH>;
> + label = "green:lan";
> + };
> +
> + wan_led: led-2 {
> + gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>;
> + label = "green:wan";
> + };
This needs to be in a separate patch.
> };
>
> &pinctrl {
> gpio-leds {
> - /delete-node/ status-led-pin;
> -
> lan_led_pin: lan-led-pin {
> rockchip,pins = <1 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none>;
> };
>
> - sys_led_pin: sys-led-pin {
> - rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
> - };
> -
> wan_led_pin: wan-led-pin {
> rockchip,pins = <1 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> };
> };
> +};
>
> - rockchip-key {
> - /delete-node/ power-key;
> -
> - reset_button_pin: reset-button-pin {
> - rockchip,pins = <1 RK_PC6 RK_FUNC_GPIO &pcfg_pull_up>;
> - };
> - };
> +&pcie0 {
> + max-link-speed = <1>;
> + num-lanes = <1>;
> + vpcie3v3-supply = <&vcc3v3_sys>;
> };
Why are you moving this out of order? "pcie0" comes before "pinctrl" in
dictionary order.
>
> &sdhci {
> @@ -112,6 +132,10 @@ &sdio0 {
> status = "disabled";
> };
>
> +&sdmmc {
> + host-index-min = <1>;
This is not documented, nor is it supported.
> +};
> +
> &u2phy0_host {
> phy-supply = <&vdd_5v>;
> };
> @@ -120,14 +144,6 @@ &u2phy1_host {
> status = "disabled";
> };
>
> -&uart0 {
> - status = "disabled";
> -};
> -
Why? UART0 is not wired to anything, but it is enabled in the include dtsi
file. It absolutely should be disabled.
> &usbdrd_dwc3_0 {
> dr_mode = "host";
> };
> -
> -&vcc3v3_sys {
> - vin-supply = <&vcc5v0_sys>;
> -};
This whole patch feels like you took the dts file from some other patch,
copied it over, and failed to check for redundancies.
ChenYu
More information about the Linux-rockchip
mailing list