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-05-07 at 23:43 +0200, Andrew Lunn wrote:
> > > > +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.
> 
> What other drivers do is leave the configuration with its reset
> default. They are often sensible. When the netdev trigger loads, it
> should ask the LED how it is configured, and the values in sysfs will
> reflect it. After that you can change it, via udev rules, etc.
When you say "reset default", do you mean the default PHY register
values, the ones that may have been set by the bootloader or by the
driver with a default hardcoded functional config?
> 
> You have to be careful about what you put in DT. DT describes
> hardware, not configuration or policy. How the LED blinks is probably
> configuration, so it does not belong in DT.
I agree.
What I meant as configuration was only the leds node presence and the
led function properties, as the devicetree should describe what/how
many LED are connected to the PHY and they represent (or mean) for a
given board.
Parameters like off/on delay or trigger events are indeed not really
appropriate for devicetree and the AN8801 dt-bindings patch do not add
any such property.

And even, if technically the LEDs are reconfigurable with netdev or any
other LED trigger, the configuration is somehow hardware bound, because
the different colors of LEDs do kind-of bind a specific function to a
specific LED (amber vs green).

This is why the an8801r_of_init_leds function read those LED-related
properties (leds/led/function-enumerator) to set a default led config
behaviour (that can of course be overridden by an user with a led
trigger) rather than relying on bootloader or use a hardcoded led
register config in the driver.

What implementation would be preferred for this driver?

> 
> > > > +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. 
> 
> So this is an errata workaround? Please add this in a patch of its
> own, described the problem in the commit message, list the errata
> etc.
> 
OK, I'll add this in a separate patch in v3.

Best regards,
Louis-Alexis

>      Andrew