Re: [PATCH net-next v2 4/4] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver

[Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>]

Hi Andrew,

On Thu, 2026-03-26 at 13:47 +0100, Andrew Lunn wrote:
> > +static int an8801r_led_blink_set(struct phy_device *phydev, u8
> > index,
> > +				 unsigned long *delay_on,
> > +				 unsigned long *delay_off)
> > +{
> 
> ...
> 
> > +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_ON_CTRL(index),
> > +			     LED_ON_EN, blink ? LED_ON_EN : 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Just
> 
> 
> 	return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> LED_ON_CTRL(index),
> 			     LED_ON_EN, blink ? LED_ON_EN : 0);
> 
> > +		if (!led_trigger)
> > +			continue;
> > +
> > +		ret = an8801r_led_hw_control_set(phydev, led_id,
> > led_trigger);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> 
> 
> Please take a look at all your functions. Can the last error check be
> removed and just use return ret, etc.
I'll fix this in the next version.

> 
> > +static int an8801r_of_init_leds(struct phy_device *phydev, u8
> > *led_cfg)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *leds;
> > +	u32 function_enum_idx;
> > +	int ret;
> > +
> > +	if (!np)
> > +		return 0;
> > +
> > +	/* If devicetree is present, leds configuration is
> > required */
> > +	leds = of_get_child_by_name(np, "leds");
> > +	if (!leds)
> > +		return 0;
> > +
> > +	for_each_available_child_of_node_scoped(leds, led) {
> > +		u32 led_idx;
> > +
> > +		ret = of_property_read_u32(led, "reg", &led_idx);
> > +		if (ret)
> > +			goto out;
> > +
> > +		if (led_idx >= AN8801R_NUM_LEDS) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		ret = of_property_read_u32(led, "function-
> > enumerator",
> > +					   &function_enum_idx);
> > +		if (ret)
> > +			function_enum_idx = AN8801R_LED_FN_NONE;
> > +
> 
> What is this doing? Is this documented in the binding?
The `function-enumerator` property is only documented in the led common
dt-binding file. The an8801 dt-bindings inherits this property from the
ethernet-phy dt-bindings.

We aimed to have this PHY have its led behaviour (how many to enable
and what their role shall be) configurable using devicetree and not to
rely on a default configuration, hard-coded in the driver (like the
air_en8811h driver did) and also make use of the led hardware
offloading (for functions like 100/1000, activity blinking, and others)
that this PHY is capable of.

From the available property list for the led node, this one seems to be
appropriate to distinguish between the possible LAN functions, that 
would mean that a specific LED has either a link or RX/Tx activity 
role. That is why we used it but we could be wrong.

The an8801 dt-bindings (in patch 1) misses the possible values and
should improved in that regard and I'll fix them in next version if
this implementation seems acceptable to you.
> 
> > +		if (function_enum_idx >= AN8801R_LED_FN_MAX) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		led_cfg[led_idx] = function_enum_idx;
> > +	}
> > +out:
> > +	of_node_put(leds);
> > +	return ret;
> > +}
> 
> > +static int an8801r_read_status(struct phy_device *phydev)
> > +{
> > +	int prev_speed, ret;
> > +	u32 val;
> > +
> > +	prev_speed = phydev->speed;
> > +
> > +	ret = genphy_read_status(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (phydev->link && prev_speed != phydev->speed) {
> > +		val = phydev->speed == SPEED_1000 ?
> > +		      AN8801_BPBUS_LINK_MODE_1000 : 0;
> > +
> > +		return an8801_buckpbus_reg_rmw(phydev,
> > +					      
> > AN8801_BPBUS_REG_LINK_MODE,
> > +					      
> > AN8801_BPBUS_LINK_MODE_1000,
> > +					       val);
> > +	};
> 
> This is unusual. What is it doing? Please add a comment.
This call is to ensure that the PHY switches to the expected 1Gbps 
speed when available. 
I'll confirm it and add a comment in v3.

Best regards,
Louis-Alexis
> 
> 	Andrew