[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