Re: [PATCH 3/3] arm64: dts: amlogic: add T7 peripheral nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dt: i2c2 node is unconditionally enabled in SoC dtsi
- [Low] dt: ethernet-phy node name violates schema (uses underscore)
- [Low] dt: fixed regulators define enable-active-high without gpio
- [Low] dt: new UART nodes omit mandatory clocks properties
--

commit 6fa9620003d2e99adee0e1cd106450b9d0aabb2d
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

arm64: dts: amlogic: add T7 peripheral nodes

Add device tree descriptions for the peripherals on the Amlogic T7
SoC and the T7 A311D2 AN400 board.

Add support for the T7 Ethernet controller, SPI controllers, I2C
controllers, UARTs, pinctrl groups, regulators, and reserved memory.
Also enable the Ethernet controller on the AN400 board.

> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-an400.dts b/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-an400.dts
> index ca7536f772ff1..dfd82148179b0 100644
> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-an400.dts
> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-an400.dts

[ ... ]

> +	vddao_3v3: regulator-vddao-3v3 {
> +		vin-supply = <&dc_in>;
> +		compatible = "regulator-fixed";
> +		regulator-name = "VDDAO_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		startup-delay-us = <70000>;
> +		enable-active-high;

[Severity: Low]
Does this regulator actually need the enable-active-high property? Without
specifying a gpio, this property will not have a functional effect.

> +		regulator-boot-on;
> +		regulator-always-on;
> +	};

[ ... ]

> +	vdd_ddr: regulator-vdd-ddr {
> +		vin-supply = <&dc_in>;
> +		compatible = "regulator-fixed";
> +		regulator-name = "VDD_DDR";
> +		regulator-min-microvolt = <800000>;
> +		regulator-max-microvolt = <800000>;
> +		startup-delay-us = <70000>;
> +		enable-active-high;

[Severity: Low]
Similar to vddao_3v3 above, does this regulator need enable-active-high
without a gpio property?

> +		regulator-boot-on;
> +		regulator-always-on;
> +	};
>  };

[ ... ]

> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> index 8f3f1c0579a0f..a4ca3be358429 100644
> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi

[ ... ]

> +			uart_b: serial@7a000 {
> +				compatible = "amlogic,t7-uart", "amlogic,meson-s4-uart";
> +				reg = <0x0 0x7a000 0x0 0x18>;
> +				interrupts = <GIC_SPI 169 IRQ_TYPE_EDGE_RISING>;
> +				status = "disabled";
> +			};

[Severity: Low]
Is this node missing its clocks and clock-names properties? Omitting them here
might force board files to duplicate the clock configurations.

[ ... ]

> +				int_mdio: mdio@1 {
> +					reg = <1>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					internal_ephy: ethernet_phy@8 {

[Severity: Low]
Could this node be named ethernet-phy@8 instead? Using an underscore violates
the mdio.yaml schema, which expects names matching ^ethernet-phy(@[a-f0-9]+)?$.

> +						compatible = "ethernet-phy-id0180.3301",
> +							     "ethernet-phy-ieee802.3-c22";
> +						interrupts = <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>;
> +						reg = <8>;
> +						max-speed = <100>;
> +					};
> +				};
> +			};

[ ... ]

> +			i2c2: i2c@6a000 {
> +				compatible = "amlogic,t7-i2c", "amlogic,meson-axg-i2c";
> +				reg = <0x0 0x6a000 0x0 0x48>;
> +				interrupts = <GIC_SPI 162 IRQ_TYPE_EDGE_RISING>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&clkc_periphs CLKID_SYS_I2C_M_C>;
> +				status = "okay";

[Severity: Medium]
Should this node be set to status = "disabled" by default? Enabling it
unconditionally in the SoC dtsi might lead to probe failures or hardware
conflicts on boards that do not use this controller.

> +			};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-t7-node-new-v1-0-e6e0840c1b0b@amlogic.com?part=3

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