Re: [PATCH net-next] net: phy: microchip_t1: Adds support for LAN887x phy

[Andrew Lunn]

> +	if (!IS_ENABLED(CONFIG_OF_MDIO)) {
> +		/* Configure default behavior of led to link and activity for any
> +		 * speed
> +		 */
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> +				     LAN887X_COMMON_LED3_LED2,
> +				     LAN887X_COMMON_LED2_MODE_SEL_MASK,
> +				     LAN887X_LED_LINK_ACT_ANY_SPEED);

This is unusual. What has OF_MDIO got to do with LEDs?

Since this is a new driver, you can default the LEDs to anything you
want. You however cannot change the default once merged. Ideally you
will follow up with some patches to add support for controlling the
LEDs via /sys/class/leds.

> +static void lan887x_get_strings(struct phy_device *phydev, u8 *data)
> +{
> +	for (int i = 0; i < ARRAY_SIZE(lan887x_hw_stats); i++) {
> +		strscpy(data + i * ETH_GSTRING_LEN,
> +			lan887x_hw_stats[i].string, ETH_GSTRING_LEN);
> +	}

There has been a general trend of replacing code like this with
ethtool_puts().

	Andrew