Re: [PATCH 3/4] arm64: dts: allwinner: h700: Add RG35XX-Plus DTS

[Andre Przywara <andre.przywara@xxxxxxx>]

On Sun, 14 Apr 2024 20:33:47 +1200
Ryan Walklin <ryan@xxxxxxxxxxxxx> wrote:

Hi,

> From: Ryan Walklin <ryan@xxxxxxxxxxxxx>
> 
> The RG35XX-Plus adds a RTL8221CS SDIO Wifi/BT chip to the RG35XX (2024).
> 
> Enabled in this DTS:
> - WiFi
> - Bluetooth
> - Supporting power sequence and GPIOs
> 
> Signed-off-by: Ryan Walklin <ryan@xxxxxxxxxxxxx>
> ---
>  .../sun50i-h700-anbernic-rg35xx-plus.dts      | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> new file mode 100644
> index 000000000000..b9a163ed5d0b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-plus.dts
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) 2024 Andre Przywara <andre.przywara@xxxxxxx>.

I don't remember writing anything in here explicitly, so please remove
this copyright line.

> + * Copyright (C) 2024 Ryan Walklin <ryan@xxxxxxxxxxxxx>.
> + * Copyright (C) 2024 Chris Morgan <macroalpha82@xxxxxxxxx>.
> + */
> +
> +/dts-v1/;

That line is already in the file included next, so please remove it
from here.

> +
> +#include "sun50i-h700-anbernic-rg35xx-2024.dts"
> +
> +/ {
> +    model = "Anbernic RG35XX Plus";
> +    compatible = "anbernic,rg35xx-plus", "allwinner,sun50i-h700";
> +
> +    wifi_pwrseq: wifi_pwrseq {
> +        compatible = "mmc-pwrseq-simple";
> +        clocks = <&rtc CLK_OSC32K_FANOUT>;
> +        clock-names = "ext_clock";
> +        pinctrl-0 = <&x32clk_fanout_pin>;
> +        pinctrl-names = "default";
> +        post-power-on-delay-ms = <200>;
> +        reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>; /* PG18 */
> +    };
> +};
> +
> +/* SDIO WiFi RTL8821CS, powered by CLDO4, regon=PG18, hostwake=PG15 */
> +&mmc1 {
> +    vmmc-supply = <&reg_cldo4>;
> +    vqmmc-supply = <&reg_vcc1v8>;

That regulator should then also provide the PIO's vcc-pg-supply. And
maybe it should be renamed.

> +    mmc-pwrseq = <&wifi_pwrseq>;
> +    bus-width = <4>;
> +    non-removable;
> +    status = "okay";
> +
> +    sdio_wifi: wifi@1 {
> +       reg = <1>;
> +       interrupt-parent = <&pio>;
> +       interrupts = <6 15 IRQ_TYPE_LEVEL_LOW>; /* PG15 */
> +       interrupt-names = "host-wake";
> +    };
> +};
> +
> +&uart0 {
> +    pinctrl-names = "default";
> +    pinctrl-0 = <&uart0_ph_pins>;
> +    status = "okay";
> +};

That would be redundant, as the node is already contained in the base
.dts?

> +
> +/* Bluetooth RTL8821CS, RST_N: PG19, BT_WAKE: PG17, BT_HOSTWAKE: PG16 */
> +&uart1 {
> +    pinctrl-names = "default";
> +    pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
> +    uart-has-rtscts;
> +    status = "okay";
> +
> +    bluetooth {
> +        compatible = "realtek,rtl8821cs-bt", "realtek,rtl8723bs-bt";
> +        device-wake-gpios = <&pio 6 17 GPIO_ACTIVE_HIGH>;
> +        enable-gpios = <&pio 6 19 GPIO_ACTIVE_HIGH>;
> +        host-wake-gpios = <&pio 6 16 GPIO_ACTIVE_HIGH>;

As usual, please add the GPIO names as a comment.

Cheers,
Andre

> +    };
> +};