Re: [PATCH net-next v7 10/11] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Unrelated functional DT change bundled into an "enable SGMII LAN
  port" patch: the hunk under `&mdio1` adds a `leds { led@1; led@2; }`…

--

arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

This enables GMAC0 on the Ariaboard Photonicat by adding the XPCS SGMII
plumbing (combphy2 mac-sel, xpcs/xpcs_mii0, gmac0 properties) and describing
the Motorcomm YT8521SC PHY on a new mdio0 node.  It also adds a PHY LED
description to the existing RGMII PHY on mdio1.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index 58c1052ba8ef3..fdaa4a2a4328b 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts

[ ... ]

> @@ -350,6 +391,25 @@ rgmii_phy: ethernet-phy@3 {
>  		reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
>  		rx-internal-delay-ps = <1500>;
>  		tx-internal-delay-ps = <1500>;
> +
> +		leds {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			led@1 {
> +				reg = <1>;
> +				color = <LED_COLOR_ID_AMBER>;
> +				function = LED_FUNCTION_WAN;
> +				default-state = "keep";
> +			};
> +
> +			led@2 {
> +				reg = <2>;
> +				color = <LED_COLOR_ID_GREEN>;
> +				function = LED_FUNCTION_WAN;
> +				default-state = "keep";
> +			};
> +		};
>  	};
>  };

[Severity: Low]

This isn't a bug, but could this hunk go into a separate patch?  It changes
the pre-existing RGMII WAN PHY (rgmii_phy under &mdio1), which is already
enabled and working before this patch:

arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts:
	&gmac1 {
		phy-handle = <&rgmii_phy>;
		phy-mode = "rgmii-id";
		status = "okay";
	};

None of the SGMII enablement in this patch (&combphy2
rockchip,sgmii-mac-sel, &gmac0, &mdio0/sgmii_phy, &xpcs, &xpcs_mii0)
depends on the mdio1 leds node.

It is also not purely descriptive - drivers/net/phy/motorcomm.c implements
the LED callbacks for this PHY (yt8521_led_hw_is_supported(),
yt8521_led_hw_control_set(), yt8521_led_hw_control_get()), so PHY LED class
devices get instantiated for the WAN PHY as a result of this change.

Split into its own patch, the WAN LED description and the LAN port
enablement can be bisected and reverted independently, and the subject line
would reflect that the WAN PHY description changed too.  The commit message
mentions it:

    Also add LED configuration for the existing RGMII PHY on mdio1 for
    consistency.

but the subject only mentions the SGMII LAN port.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204624.2427486-1-coiaprant%40gmail.com