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
