Re: [PATCH v2 3/3] arm64: dts: rockchip: add dts for Ariaboard Photonicat RK3568

From: Junhao Xie <bigfoot@classfun.cn>
To: Chukun Pan <amadeus@jmu.edu.cn>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>, Jonas Karlman <jonas@kwiboo.se>,
	Chukun Pan <amadeus@jmu.edu.cn>, FUKAUMI Naoki <naoki@radxa.com>,
	Dragan Simic <dsimic@manjaro.org>,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	Junhao Xie <bigfoot@classfun.cn>
Subject: Re: [PATCH v2 3/3] arm64: dts: rockchip: add dts for Ariaboard Photonicat RK3568
Date: Fri, 6 Sep 2024 22:12:43 +0800	[thread overview]
Message-ID: <53784a02-087b-4912-94a9-aead49d7abea@classfun.cn> (raw)
In-Reply-To: <20240906081005.69334-1-amadeus@jmu.edu.cn>

On 2024/9/6 16:10, Chukun Pan wrote:
> Hi Junhao,
> 
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
>> @@ -0,0 +1,595 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include <dt-bindings/pinctrl/rockchip.h>
>> +#include <dt-bindings/soc/rockchip,vop2.h>
>> +#include <dt-bindings/soc/rockchip,boot-mode.h>
> 
> No need for input.h, leds/common.h and boot-mode.h.

I will remove them.

> 
>> +#include "rk3568.dtsi"
>> ...
>> +	vcc3v3_sd: regulator-3v3-vcc-sd {
>> +		pinctrl-0 = <&vcc_sd_h>;
> 
> schematics: sdmmc0_pwren

I will rename vcc_sd_h to sdmmc0_pwren.

> 
>> ...
>> +	vcc3v3_rf: regulator-3v3-vcc-rf {
> 
> schematics: VCC3V4_RF

I will change it to "vcc3v4_rf: regulator-3v4-vcc-rf {"

> VCCIN_5V -> VCC3V4_RF

Is vccin_5v a new regulator or is it actually vcc_sysin?

> 
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&vcc3v3_rf_pwren_en>;
> 
> schematics: RF_PWR_EN
> 
>> ...
>> +	vcc5v0_sysin: regulator-5v0-vcc-sysin {
> 
> schematics: VCC_SYSIN
> 
>> ...
>> +	vcc5v0_syson: regulator-5v0-vcc-syson {
> 
> schematics: VCC_SYSON

I will rename them.

> 
>> ...
>> +	vcc5v0_usb30_otg0: regulator-5v0-vcc-usb-host {
>> ...
>> +		vin-supply = <&vcc5v0_syson>;
> 
> VCCIN_5V -> VCC5V0_USB30_OTG0
> 
>> ...
>> +&gmac1 {
>> ...
>> +	tx_delay = <0x0>;
>> +	rx_delay = <0x0>;
> 
> Please remove the tx_delay and rx_delay, it's useless.
> I know there is an error log, but please ignore it first.

OK, I will remove them.

> 
>> ...
>> +&pinctrl {
>> ...
>> +		bt_reg_on_h: bt-enable-h {
>> +		pcie_pwren_h: pcie-enable-h {
>> +		wifi_reg_on_h: wifi-enable-h {
>> +		vcc3v3_rf_pwren_en: vcc5v0-modem-en {
>> +		usb_host_pwren_h: vcc5v0-host-en {
> 
> obviously (

Yes, I forgot to modify them, it looks better like this:
bt_reg_on_h: bt-reg-on-h {
pcie_pwren_h: pcie-pwren-h {
wifi_reg_on_h: wifi-reg-on-h {
sdmmc0_pwren: sdmmc0-pwren {
rf_pwren_en: rf-pwren-en {
usb_host_pwren_h: usb-host-pwren-h {

> 
>> +	wifi_pwrseq: wifi-pwrseq {
>> +		compatible = "mmc-pwrseq-simple";
>> ...
>> +&pinctrl {
>> ...
>> +	sdio-pwrseq {
> 
> I tend to write like this:

I will rename them.

> 
> ```
> &pinctrl {
> 	wifi {
> 		wifi_reg_on_h: wifi-reg-on-h {
> ```
> 
>> +	vcc_sd {
>> +		vcc_sd_h: vcc-sd-h {
> 
> Overwrite original to match `sdmmc0_pwren`
> 
> sdmmc0 {
> 	sdmmc0_pwren: sdmmc0-pwren {
> 
>> +			rockchip,pins = <0 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>> +		};
>> +	};
>> +
> 
> Extra blank lines.
> 
>> +};
>> ...
>> &sdmmc1 {
>> ...
>> 	max-frequency = <150000000>;
> 
> `max-frequency = <150000000>;` already defined in rk356x.dtsi

Yes, I will remove them.

> 
>> ...
>> +&usb_host0_ohci {
>> ...
>> +&usb_host0_ehci {
> 
> &usb_host0_ehci {
> &usb_host0_ohci {
> 
> Same for usb_host1
> 
>> ...
>> &usb2phy1_host {
>> 	phy-supply = <&vcc3v3_rf>;
>> 	status = "okay";
>> };
> 
> Is usb2phy1_host connected?

It looks like usb_host1_ehci, usb_host1_ohci, usb2phy1_host
are not used by any device.
I removed them and it still works fine.

> 
Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao

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